Use LoadURLWithParams in ChromeWebContentsDelegateAndroid

Use LoadURLWithParams in ChromeWebContentsDelegateAndroid

Build a LoadURLParams object from the OpenURLParams and properly set all
parameters on that object when calling into NavigationController.  This makes
sure we set the correct state for the load.

BUG=352083
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273865

[David Trainor- moved to gerrit, 2014-05-07 21:17:29]

ptal.  I got most of this logic from Browser_navigator.cc: LoadURLInContents()
which does something similar from a NavigationState struct.

Ideally we'd use chrome::Navigate() for all of our navigations, but that needs
to be decoupled from Browser first :(.

[Charlie Reis, 2014-05-07 22:08:12]

Thanks for catching this!  Is there a way to test that GetVisibleEntry() does
the right thing now?

On 2014/05/07 21:17:29, David Trainor wrote:
> ptal.  I got most of this logic from Browser_navigator.cc: LoadURLInContents()
> which does something similar from a NavigationState struct.
> 
> Ideally we'd use chrome::Navigate() for all of our navigations, but that needs
> to be decoupled from Browser first :(.

+1 to finding a way to reuse that code (in a future CL).  I've never liked that
there was so much of it in chrome/ anyway.

https://chromiumcodereview.appspot.com/267253007/diff/20001/components/web_co...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/267253007/diff/20001/components/web_co...
components/web_contents_delegate_android/web_contents_delegate_android.cc:135:
source->GetController().LoadURLWithParams(load_params);
I'm guessing this is just a misnomer, but LoadURLInContents operates on the
target WebContents and not the source (e.g., in cases that a navigation opens in
a new tab).  Are we using the correct WebContents here?

[David Trainor- moved to gerrit, 2014-05-07 22:43:24]

On 2014/05/07 22:08:12, Charlie Reis wrote:
> Thanks for catching this!  Is there a way to test that GetVisibleEntry() does
> the right thing now?
> 

I can write a test given the repro html/javascript that can test this.  I'll add
something.

> On 2014/05/07 21:17:29, David Trainor wrote:
> > ptal.  I got most of this logic from Browser_navigator.cc:
LoadURLInContents()
> > which does something similar from a NavigationState struct.
> > 
> > Ideally we'd use chrome::Navigate() for all of our navigations, but that
needs
> > to be decoupled from Browser first :(.
> 
> +1 to finding a way to reuse that code (in a future CL).  I've never liked
that
> there was so much of it in chrome/ anyway.
> 
>
https://chromiumcodereview.appspot.com/267253007/diff/20001/components/web_co...
> File components/web_contents_delegate_android/web_contents_delegate_android.cc
> (right):
> 
>
https://chromiumcodereview.appspot.com/267253007/diff/20001/components/web_co...
> components/web_contents_delegate_android/web_contents_delegate_android.cc:135:
> source->GetController().LoadURLWithParams(load_params);
> I'm guessing this is just a misnomer, but LoadURLInContents operates on the
> target WebContents and not the source (e.g., in cases that a navigation opens
in
> a new tab).  Are we using the correct WebContents here?

[David Trainor- moved to gerrit, 2014-05-07 22:43:53]

https://chromiumcodereview.appspot.com/267253007/diff/20001/components/web_co...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/267253007/diff/20001/components/web_co...
components/web_contents_delegate_android/web_contents_delegate_android.cc:135:
source->GetController().LoadURLWithParams(load_params);
On 2014/05/07 22:08:12, Charlie Reis wrote:
> I'm guessing this is just a misnomer, but LoadURLInContents operates on the
> target WebContents and not the source (e.g., in cases that a navigation opens
in
> a new tab).  Are we using the correct WebContents here?

I can change it but I think we can only reach this if the disposition is
CURRENT_TAB.  I don't know if that means the web contents are the same or not. 
I can check/change this though.

[Charlie Reis, 2014-05-07 23:16:24]

https://chromiumcodereview.appspot.com/267253007/diff/20001/components/web_co...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/267253007/diff/20001/components/web_co...
components/web_contents_delegate_android/web_contents_delegate_android.cc:135:
source->GetController().LoadURLWithParams(load_params);
On 2014/05/07 22:43:54, David Trainor wrote:
> On 2014/05/07 22:08:12, Charlie Reis wrote:
> > I'm guessing this is just a misnomer, but LoadURLInContents operates on the
> > target WebContents and not the source (e.g., in cases that a navigation
opens
> in
> > a new tab).  Are we using the correct WebContents here?
> 
> I can change it but I think we can only reach this if the disposition is
> CURRENT_TAB.  I don't know if that means the web contents are the same or not.

> I can check/change this though.

Hmm, that's a good point: we don't use any of this code in the new tab cases. 
Won't that make a spoof possible on renderer-initiated loads in new tabs?

[David Trainor- moved to gerrit, 2014-05-12 20:52:45]

https://chromiumcodereview.appspot.com/267253007/diff/20001/components/web_co...
File components/web_contents_delegate_android/web_contents_delegate_android.cc
(right):

https://chromiumcodereview.appspot.com/267253007/diff/20001/components/web_co...
components/web_contents_delegate_android/web_contents_delegate_android.cc:135:
source->GetController().LoadURLWithParams(load_params);
On 2014/05/07 23:16:24, Charlie Reis (OOO May 17) wrote:
> On 2014/05/07 22:43:54, David Trainor wrote:
> > On 2014/05/07 22:08:12, Charlie Reis wrote:
> > > I'm guessing this is just a misnomer, but LoadURLInContents operates on
the
> > > target WebContents and not the source (e.g., in cases that a navigation
> opens
> > in
> > > a new tab).  Are we using the correct WebContents here?
> > 
> > I can change it but I think we can only reach this if the disposition is
> > CURRENT_TAB.  I don't know if that means the web contents are the same or
not.
> 
> > I can check/change this though.
> 
> Hmm, that's a good point: we don't use any of this code in the new tab cases. 
> Won't that make a spoof possible on renderer-initiated loads in new tabs?

Yeah I'm looking into fixing that as well.  It requires some upstream/downstream
changes so I'll have to have a 2-stage patch to fix it.  I'll update this one
with that fix.

[David Trainor- moved to gerrit, 2014-05-12 20:54:06]

Still working on how to test this upstream since we don't have a concept of
multiple tabs there yet.  Will see if it's feasible or not.

[David Trainor- moved to gerrit, 2014-05-15 00:50:23]

On 2014/05/12 20:54:06, David Trainor wrote:
> Still working on how to test this upstream since we don't have a concept of
> multiple tabs there yet.  Will see if it's feasible or not.

Okay updated a fix that should account for the other callsites.  Requires a
downstream change for other places too.

[Ted C, 2014-05-15 00:57:00]

lgtm

https://codereview.chromium.org/267253007/diff/40001/content/public/android/j...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://codereview.chromium.org/267253007/diff/40001/content/public/android/j...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:364:
* @param rendererInitiated Whether or not this load was initiated from a
renderer or not.
the trailing or not isn't needed here (or the following)...or not.

[Charlie Reis, 2014-05-15 01:07:17]

On 2014/05/15 00:50:23, David Trainor wrote:
> On 2014/05/12 20:54:06, David Trainor wrote:
> > Still working on how to test this upstream since we don't have a concept of
> > multiple tabs there yet.  Will see if it's feasible or not.
> 
> Okay updated a fix that should account for the other callsites.  Requires a
> downstream change for other places too.

Thanks for handling the different tab case!

Were you still planning to write a test for this?

[David Trainor- moved to gerrit, 2014-05-15 18:20:15]

On 2014/05/15 01:07:17, Charlie Reis(OOO as of May 17) wrote:
> On 2014/05/15 00:50:23, David Trainor wrote:
> > On 2014/05/12 20:54:06, David Trainor wrote:
> > > Still working on how to test this upstream since we don't have a concept
of
> > > multiple tabs there yet.  Will see if it's feasible or not.
> > 
> > Okay updated a fix that should account for the other callsites.  Requires a
> > downstream change for other places too.
> 
> Thanks for handling the different tab case!
> 
> Were you still planning to write a test for this?

I have one downstream.  I just put you on the CL.

[David Trainor- moved to gerrit, 2014-05-15 18:22:14]

https://chromiumcodereview.appspot.com/267253007/diff/40001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://chromiumcodereview.appspot.com/267253007/diff/40001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:364:
* @param rendererInitiated Whether or not this load was initiated from a
renderer or not.
On 2014/05/15 00:57:00, Ted C wrote:
> the trailing or not isn't needed here (or the following)...or not.

Done.

[Charlie Reis, 2014-05-15 18:37:17]

On 2014/05/15 18:20:15, David Trainor wrote:
> On 2014/05/15 01:07:17, Charlie Reis(OOO as of May 17) wrote:
> > On 2014/05/15 00:50:23, David Trainor wrote:
> > > On 2014/05/12 20:54:06, David Trainor wrote:
> > > > Still working on how to test this upstream since we don't have a concept
> of
> > > > multiple tabs there yet.  Will see if it's feasible or not.
> > > 
> > > Okay updated a fix that should account for the other callsites.  Requires
a
> > > downstream change for other places too.
> > 
> > Thanks for handling the different tab case!
> > 
> > Were you still planning to write a test for this?
> 
> I have one downstream.  I just put you on the CL.

Thanks!  I see it.  LGTM.

Note: In the future, how do you want to keep these copies of LoadURLInContents
up to date as browser_navigator.cc changes?  I'm expecting some changes to
happen in the next few months for various navigation-related refactors.

[David Trainor- moved to gerrit, 2014-05-15 18:41:00]

On 2014/05/15 18:37:17, Charlie Reis(OOO as of May 17) wrote:
> On 2014/05/15 18:20:15, David Trainor wrote:
> > On 2014/05/15 01:07:17, Charlie Reis(OOO as of May 17) wrote:
> > > On 2014/05/15 00:50:23, David Trainor wrote:
> > > > On 2014/05/12 20:54:06, David Trainor wrote:
> > > > > Still working on how to test this upstream since we don't have a
concept
> > of
> > > > > multiple tabs there yet.  Will see if it's feasible or not.
> > > > 
> > > > Okay updated a fix that should account for the other callsites. 
Requires
> a
> > > > downstream change for other places too.
> > > 
> > > Thanks for handling the different tab case!
> > > 
> > > Were you still planning to write a test for this?
> > 
> > I have one downstream.  I just put you on the CL.
> 
> Thanks!  I see it.  LGTM.
> 
> Note: In the future, how do you want to keep these copies of LoadURLInContents
> up to date as browser_navigator.cc changes?  I'm expecting some changes to
> happen in the next few months for various navigation-related refactors.

I want to move browser_navigator to not rely heavily on a browser instance by
default.  I don't have time to do that now.  I was also considering pulling out
the translation methods to utilities which I figured might be useful (we already
have FillNavigateParamsFromOpenURLParams).  The issue is LoadURLParams lives in
chrome and not content :(.  I'd need FillLoadURLParamsFromNavigateParams and
FillLoadURLParamsFromOpenURLParams.

Hmm do we even need all of these navigation structs?

[Charlie Reis, 2014-05-15 18:49:20]

On 2014/05/15 18:41:00, David Trainor wrote:
> On 2014/05/15 18:37:17, Charlie Reis(OOO as of May 17) wrote:
> > On 2014/05/15 18:20:15, David Trainor wrote:
> > > On 2014/05/15 01:07:17, Charlie Reis(OOO as of May 17) wrote:
> > > > On 2014/05/15 00:50:23, David Trainor wrote:
> > > > > On 2014/05/12 20:54:06, David Trainor wrote:
> > > > > > Still working on how to test this upstream since we don't have a
> concept
> > > of
> > > > > > multiple tabs there yet.  Will see if it's feasible or not.
> > > > > 
> > > > > Okay updated a fix that should account for the other callsites. 
> Requires
> > a
> > > > > downstream change for other places too.
> > > > 
> > > > Thanks for handling the different tab case!
> > > > 
> > > > Were you still planning to write a test for this?
> > > 
> > > I have one downstream.  I just put you on the CL.
> > 
> > Thanks!  I see it.  LGTM.
> > 
> > Note: In the future, how do you want to keep these copies of
LoadURLInContents
> > up to date as browser_navigator.cc changes?  I'm expecting some changes to
> > happen in the next few months for various navigation-related refactors.
> 
> I want to move browser_navigator to not rely heavily on a browser instance by
> default.  I don't have time to do that now.

Yes, addressing this would be separate from this CL.

>  I was also considering pulling out
> the translation methods to utilities which I figured might be useful (we
already
> have FillNavigateParamsFromOpenURLParams).  The issue is LoadURLParams lives
in
> chrome and not content :(.  I'd need FillLoadURLParamsFromNavigateParams and
> FillLoadURLParamsFromOpenURLParams.
> 
> Hmm do we even need all of these navigation structs?

I'd love to see fewer versions of those structs (and solely in content) if that
works.  When the time comes, it might make sense to ping folks who know about
chrome::NavigateParams (ben@?) and LoadURLParams (boliu@ from
http://crrev.com/151534?).

Thanks,
Charlie

[David Trainor- moved to gerrit, 2014-05-16 19:59:07]

Bo can I get an owners LGTM for android_webview?  Thanks!

[boliu, 2014-05-16 20:06:39]

One concern, but aw lgtm

https://chromiumcodereview.appspot.com/267253007/diff/60001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://chromiumcodereview.appspot.com/267253007/diff/60001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:366:
public void setIsRendererInitiated(boolean rendererInitiated) {
Nothing calls this?

LoadUrlParams was supposed to be passing parameters from embedder into content,
not the other way around. Now I'm confused where this fits in?

[David Trainor- moved to gerrit, 2014-05-19 18:12:59]

https://chromiumcodereview.appspot.com/267253007/diff/60001/content/public/an...
File
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java
(right):

https://chromiumcodereview.appspot.com/267253007/diff/60001/content/public/an...
content/public/android/java/src/org/chromium/content/browser/LoadUrlParams.java:366:
public void setIsRendererInitiated(boolean rendererInitiated) {
On 2014/05/16 20:06:39, boliu wrote:
> Nothing calls this?
> 
> LoadUrlParams was supposed to be passing parameters from embedder into
content,
> not the other way around. Now I'm confused where this fits in?

It's called the same way the other methods here are called downstream in
ChromeTab (openNewTab).  From the looks of it, we're sadly marshalling all of
the 'relevant' parameters, passing them to java, putting them into a java
LoadURLParams, then marshalling the 'relevant' parameters again back to native
and putting them into a native LoadURLParams...  I haven't looked into changing
any of that, as that isn't really in the scope of this bug fix.

Ideally I'd rather see us have a single place that builds a native/java
LoadURLParams from it's counterpart.  Or we do away with the struct completely
and rely on one of the two other ones that exist in native with practically the
same parameters.  IMO we have too many :(.

[David Trainor- moved to gerrit, 2014-05-20 17:42:08]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-05-20 17:43:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrainor@chromium.org/267253007/80001

[commit-bot: I haz the power, 2014-05-20 19:32:40]

FYI, CQ is re-trying this CL (attempt #1).
Please consider checking whether the failures are real,
and report flakes to chrome-troopers@google.com.
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...)

[commit-bot: I haz the power, 2014-05-20 20:34:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-20 20:34:47]

Try jobs failed on following builders:
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)

[David Trainor- moved to gerrit, 2014-05-29 20:55:04]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-05-29 20:57:29]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrainor@chromium.org/267253007/100001

[David Trainor- moved to gerrit, 2014-05-30 00:31:02]

The CQ bit was unchecked by dtrainor@chromium.org

[David Trainor- moved to gerrit, 2014-05-30 17:27:27]

The CQ bit was checked by dtrainor@chromium.org

[commit-bot: I haz the power, 2014-05-30 17:31:46]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/dtrainor@chromium.org/267253007/100001

[commit-bot: I haz the power, 2014-05-30 17:44:33]

Change committed as 273865