[RDS] Reloads a page using the original request URL

[RDS] Reloads a page using the original request URL

When toggling between different versions of a website by changing user agents,
we often need to reload using an earlier URL in a redirect chain to avoid
redirects caused by user-agent detection.

* Adds a new type of reload that hooks into WebFrame::reloadWithOverrideURL,
added in https://bugs.webkit.org/show_bug.cgi?id=89788

* Changes the original request URL we were saving before to be the URL sent
back as part of WebKit's original request.  This URL often differs from
the URL stored at the beginning at the redirect chain.  It's sent back as
a new field when the renderer navigates.

Part of upstreaming Chrome for Android.
Internal BUG=6272286
BUG=112923
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=150003

[gone, 2012-06-29 20:25:38]

Hey guys,
Can you have a look at this when you get a chance?  It's upstreaming one of the
bits we need Chromium-side for Chrome for Android's Request Desktop Site
feature.

Thanks!

[Charlie Reis, 2012-06-29 21:43:25]

Can you add a unit test to navigation_controller_impl_unittest.cc?

https://chromiumcodereview.appspot.com/10704048/diff/1/content/common/view_me...
File content/common/view_message_enums.h (right):

https://chromiumcodereview.appspot.com/10704048/diff/1/content/common/view_me...
content/common/view_message_enums.h:39: RELOAD_ORIGINAL_REQUEST_URL,
Sanity check: These values aren't persisted to disk for session restore, are
they?  Just want to make sure it's safe to insert one in the middle.

[jam, 2012-06-29 21:46:17]

http://codereview.chromium.org/10704048/diff/1/content/public/browser/navigat...
File content/public/browser/navigation_controller.h (right):

http://codereview.chromium.org/10704048/diff/1/content/public/browser/navigat...
content/public/browser/navigation_controller.h:226: virtual void
ReloadOriginalRequestURL() = 0;
is this being called from code that's outside of content (but not in the
chromium repo)?

[gone, 2012-06-29 21:59:10]

Working on adding a unit test, now.

http://codereview.chromium.org/10704048/diff/1/content/common/view_message_en...
File content/common/view_message_enums.h (right):

http://codereview.chromium.org/10704048/diff/1/content/common/view_message_en...
content/common/view_message_enums.h:39: RELOAD_ORIGINAL_REQUEST_URL,
On 2012/06/29 21:43:25, creis wrote:
> Sanity check: These values aren't persisted to disk for session restore, are
> they?  Just want to make sure it's safe to insert one in the middle.

Hrm, will double check.

http://codereview.chromium.org/10704048/diff/1/content/public/browser/navigat...
File content/public/browser/navigation_controller.h (right):

http://codereview.chromium.org/10704048/diff/1/content/public/browser/navigat...
content/public/browser/navigation_controller.h:226: virtual void
ReloadOriginalRequestURL() = 0;
On 2012/06/29 21:46:17, John Abd-El-Malek wrote:
> is this being called from code that's outside of content (but not in the
> chromium repo)?

It hasn't been upstreamed, yet; the only place it's being called is under
content/browser/android/content_view_impl.cc
We're apparently doing some major refactoring here, though.

It pulls in the Controller from WebContentsImpl::GetController(), which returns
a regular NavigationController.  If we took in the NavigationControllerImpl
directly, we could probably avoid adding it to the public API, though the
ReloadType is likely going to have to stay.

[jam, 2012-06-29 22:58:58]

http://codereview.chromium.org/10704048/diff/1/content/public/browser/navigat...
File content/public/browser/navigation_controller.h (right):

http://codereview.chromium.org/10704048/diff/1/content/public/browser/navigat...
content/public/browser/navigation_controller.h:226: virtual void
ReloadOriginalRequestURL() = 0;
On 2012/06/29 21:59:10, dfalcantara wrote:
> On 2012/06/29 21:46:17, John Abd-El-Malek wrote:
> > is this being called from code that's outside of content (but not in the
> > chromium repo)?
> 
> It hasn't been upstreamed, yet; the only place it's being called is under
> content/browser/android/content_view_impl.cc
> We're apparently doing some major refactoring here, though.

ok, in that case, it should be on the impl only (see
http://www.chromium.org/developers/content-module/content-api)

you can safetly cast from NavigationController to NavigationControllerImpl in
the code that calls this.

> 
> It pulls in the Controller from WebContentsImpl::GetController(), which
returns
> a regular NavigationController.  If we took in the NavigationControllerImpl
> directly, we could probably avoid adding it to the public API, though the
> ReloadType is likely going to have to stay.

[gone, 2012-07-03 00:07:47]

Addressed the comments and added a unit test that checks to see if the original
request URL is used when the NavigationController navigates.

https://chromiumcodereview.appspot.com/10704048/diff/1/content/common/view_me...
File content/common/view_message_enums.h (right):

https://chromiumcodereview.appspot.com/10704048/diff/1/content/common/view_me...
content/common/view_message_enums.h:39: RELOAD_ORIGINAL_REQUEST_URL,
On 2012/06/29 21:59:10, dfalcantara wrote:
> On 2012/06/29 21:43:25, creis wrote:
> > Sanity check: These values aren't persisted to disk for session restore, are
> > they?  Just want to make sure it's safe to insert one in the middle.
> 
> Hrm, will double check.

Doesn't seem to be persisted, as far as I can tell.

https://chromiumcodereview.appspot.com/10704048/diff/1/content/public/browser...
File content/public/browser/navigation_controller.h (right):

https://chromiumcodereview.appspot.com/10704048/diff/1/content/public/browser...
content/public/browser/navigation_controller.h:226: virtual void
ReloadOriginalRequestURL() = 0;
On 2012/06/29 22:58:58, John Abd-El-Malek wrote:
> On 2012/06/29 21:59:10, dfalcantara wrote:
> > On 2012/06/29 21:46:17, John Abd-El-Malek wrote:
> > > is this being called from code that's outside of content (but not in the
> > > chromium repo)?
> > 
> > It hasn't been upstreamed, yet; the only place it's being called is under
> > content/browser/android/content_view_impl.cc
> > We're apparently doing some major refactoring here, though.
> 
> ok, in that case, it should be on the impl only (see
> http://www.chromium.org/developers/content-module/content-api)
> 
> you can safetly cast from NavigationController to NavigationControllerImpl in
> the code that calls this.
> 
> > 
> > It pulls in the Controller from WebContentsImpl::GetController(), which
> returns
> > a regular NavigationController.  If we took in the NavigationControllerImpl
> > directly, we could probably avoid adding it to the public API, though the
> > ReloadType is likely going to have to stay.
> 

Moved it over to the Impl.

[jam, 2012-07-03 05:50:12]

http://codereview.chromium.org/10704048/diff/8001/content/browser/renderer_ho...
File content/browser/renderer_host/test_render_view_host.cc (right):

http://codereview.chromium.org/10704048/diff/8001/content/browser/renderer_ho...
content/browser/renderer_host/test_render_view_host.cc:258: *last_params_ =
params;
i think there's a simpler way to get the url instead of making Navigate virtual
just for testing, and caching the params and having a getter for it on this
class.

the test can add a RenderViewHostObserver, and its Navigate function will get
called with the url

http://codereview.chromium.org/10704048/diff/8001/content/public/test/test_re...
File content/public/test/test_renderer_host.h (right):

http://codereview.chromium.org/10704048/diff/8001/content/public/test/test_re...
content/public/test/test_renderer_host.h:94: virtual void
SendNavigateWithOriginalRequestURL(
nit: only add methods to the public class if they're needed by chrome. since
these methods are only called by tests instead of content, just leave them on
TestRenderViewHost

[Charlie Reis, 2012-07-03 17:22:26]

https://chromiumcodereview.appspot.com/10704048/diff/8001/content/browser/web...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10704048/diff/8001/content/browser/web...
content/browser/web_contents/navigation_controller_impl_unittest.cc:640: // save
both the original URL and the final redirected URL.
Can you check this using GetURL() and GetOriginalRequestURL()?

https://chromiumcodereview.appspot.com/10704048/diff/8001/content/browser/web...
content/browser/web_contents/navigation_controller_impl_unittest.cc:659:
EXPECT_TRUE(controller.GetPendingEntry());
Why not check that the pending entry's URL is the original URL?  I'm not sure
why you need last_params().

[gone, 2012-07-03 18:24:39]

Retooled the test.

https://chromiumcodereview.appspot.com/10704048/diff/8001/content/browser/ren...
File content/browser/renderer_host/test_render_view_host.cc (right):

https://chromiumcodereview.appspot.com/10704048/diff/8001/content/browser/ren...
content/browser/renderer_host/test_render_view_host.cc:258: *last_params_ =
params;
On 2012/07/03 05:50:12, John Abd-El-Malek wrote:
> i think there's a simpler way to get the url instead of making Navigate
virtual
> just for testing, and caching the params and having a getter for it on this
> class.
> 
> the test can add a RenderViewHostObserver, and its Navigate function will get
> called with the url

Done.

https://chromiumcodereview.appspot.com/10704048/diff/8001/content/browser/web...
File content/browser/web_contents/navigation_controller_impl_unittest.cc
(right):

https://chromiumcodereview.appspot.com/10704048/diff/8001/content/browser/web...
content/browser/web_contents/navigation_controller_impl_unittest.cc:640: // save
both the original URL and the final redirected URL.
On 2012/07/03 17:22:26, creis wrote:
> Can you check this using GetURL() and GetOriginalRequestURL()?

Done.

https://chromiumcodereview.appspot.com/10704048/diff/8001/content/browser/web...
content/browser/web_contents/navigation_controller_impl_unittest.cc:659:
EXPECT_TRUE(controller.GetPendingEntry());
On 2012/07/03 17:22:26, creis wrote:
> Why not check that the pending entry's URL is the original URL?  I'm not sure
> why you need last_params().

We need to know where the NavigationController is navigating to after the
reload.  At this point, the PendingEntry has its URL equal to the redirected URL
instead of the original URL.

https://chromiumcodereview.appspot.com/10704048/diff/8001/content/public/test...
File content/public/test/test_renderer_host.h (right):

https://chromiumcodereview.appspot.com/10704048/diff/8001/content/public/test...
content/public/test/test_renderer_host.h:94: virtual void
SendNavigateWithOriginalRequestURL(
On 2012/07/03 05:50:12, John Abd-El-Malek wrote:
> nit: only add methods to the public class if they're needed by chrome. since
> these methods are only called by tests instead of content, just leave them on
> TestRenderViewHost

Moved over

[Charlie Reis, 2012-07-03 19:01:32]

Thanks, LGTM.

[jam, 2012-07-04 04:27:14]

lgtm, thanks

[commit-bot: I haz the power, 2012-08-04 00:58:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dfalcantara@chromium.org/10704048/22001

[commit-bot: I haz the power, 2012-08-04 02:38:24]

Change committed as 150003