Set the ui::PAGE_TRANSITION_CLIENT_REDIRECT flag.

Set the ui::PAGE_TRANSITION_CLIENT_REDIRECT flag.

This patch sets ui::PAGE_TRANSITION_CLIENT_REDIRECT at the beginning of a request which allows it to be used for decisions when hooking into navigation via the components/navigation_interception API.

Committed: https://crrev.com/19893e6bce9005ef4a406fc61f0555c2e0f21d50
Cr-Commit-Position: refs/heads/master@{#304425}

[Fredrik Öhrn, 2014-10-07 09:37:09]

ohrn@opera.com changed reviewers:
+ jamesr@chromium.org

[Fredrik Öhrn, 2014-10-07 09:39:48]

The content API documents the PAGE_TRANSITION_CLIENT_REDIRECT flag but forgot to
set it.

It's useful to implementers of the components/navigation_interception API.

[mkosiba (inactive), 2014-10-07 11:38:48]

mkosiba@chromium.org changed reviewers:
+ mkosiba@chromium.org

[mkosiba (inactive), 2014-10-07 11:38:49]

https://codereview.chromium.org/626913004/diff/1/content/renderer/render_fram...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/626913004/diff/1/content/renderer/render_fram...
content/renderer/render_frame_impl.cc:2639: if (data_source->isClientRedirect())
{
This pretty much matches stuff we do in didCommitProvisionalLoad so it should be
fine I think:

https://code.google.com/p/chromium/codesearch#chromium/src/content/renderer/r...

[Fredrik Öhrn, 2014-11-07 13:25:56]

Ping?

[Mike West, 2014-11-07 13:38:07]

mkwst@chromium.org changed reviewers:
+ mkwst@chromium.org, nasko@chromium.org

[Mike West, 2014-11-07 13:38:07]

On 2014/11/07 13:25:56, Fredrik Öhrn wrote:
> Ping?

This LGTM. I'd suggest fleshing out the CL description a bit so that it's clear
why you're making the change.

I think James is a bit overloaded at the moment. Perhaps Nasko can take a look
to give it an OWNERS stamp?

[Fredrik Öhrn, 2014-11-07 13:45:27]

On 2014/11/07 13:38:07, Mike West wrote:
> On 2014/11/07 13:25:56, Fredrik Öhrn wrote:
> > Ping?
> 
> This LGTM. I'd suggest fleshing out the CL description a bit so that it's
clear
> why you're making the change.
> 

OK, I've added my initial comment to the description.

> I think James is a bit overloaded at the moment. Perhaps Nasko can take a look
> to give it an OWNERS stamp?

Please, with a cherry on top! :)

[nasko, 2014-11-07 16:50:05]

https://codereview.chromium.org/626913004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/626913004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2639: if (data_source->isClientRedirect())
{
This doesn't seem right, as it picks up the top-level frame's data source, which
should be irrelevant for subframes. Is there a test case that demonstrates what
behavior this CL is changing?

https://codereview.chromium.org/626913004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2641: transition_type |
ui::PAGE_TRANSITION_CLIENT_REDIRECT);
Why not use safer pattern that already exists in this file at line 3369?

[Fredrik Öhrn, 2014-11-10 12:51:58]

https://codereview.chromium.org/626913004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/626913004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2639: if (data_source->isClientRedirect())
{
On 2014/11/07 16:50:05, nasko wrote:
> This doesn't seem right, as it picks up the top-level frame's data source,
which
> should be irrelevant for subframes.

Fixed.

>Is there a test case that demonstrates what
> behavior this CL is changing?

I'm not aware of a test case for this.

https://codereview.chromium.org/626913004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2641: transition_type |
ui::PAGE_TRANSITION_CLIENT_REDIRECT);
On 2014/11/07 16:50:05, nasko wrote:
> Why not use safer pattern that already exists in this file at line 3369?

Because I didn't find PageTransitionFromInt to add anything of value. If it was
a straight up DCHECK it would be useful but the current implementation adds
bloat the compiler can't optimize away and if transition_type actually does
contain a bad value it'll silently discard the PAGE_TRANSITION_CLIENT_REDIRECT
bit.

I'd suggest putting in a DCHECK(PageTransitionIsValidType(transition_type));
instead. However if you really want me to use PageTransitionFromInt, sure I'll
add it.

[nasko, 2014-11-10 15:01:44]

Please include a test case for what you are changing to ensure we don't regress
it in the future.

https://codereview.chromium.org/626913004/diff/20001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/626913004/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2641: transition_type |
ui::PAGE_TRANSITION_CLIENT_REDIRECT);
On 2014/11/10 12:51:57, Fredrik Öhrn wrote:
> On 2014/11/07 16:50:05, nasko wrote:
> > Why not use safer pattern that already exists in this file at line 3369?
> 
> Because I didn't find PageTransitionFromInt to add anything of value. If it
was
> a straight up DCHECK it would be useful but the current implementation adds
> bloat the compiler can't optimize away and if transition_type actually does
> contain a bad value it'll silently discard the PAGE_TRANSITION_CLIENT_REDIRECT
> bit.
> 
> I'd suggest putting in a DCHECK(PageTransitionIsValidType(transition_type));
> instead. However if you really want me to use PageTransitionFromInt, sure I'll
> add it.

Why not improve the code then? I would go even a step further, why use DCHECK
instead of CHECK? If we ever have code that is using invalid value, this is bad
enough to kill the renderer IMHO.

[Fredrik Öhrn, 2014-11-10 16:32:39]

On 2014/11/10 15:01:44, nasko wrote:
> Please include a test case for what you are changing to ensure we don't
regress
> it in the future.
> 

Of course, however, I'm unsure how to add a test for this as it wouldn't be a
simple unit test but more of a functional test that loads a web page with a meta
refresh tag or JavaScript on it. Perhaps you can point me to some documentation
on how to do this or to an exiting test of a similar nature.


>
https://codereview.chromium.org/626913004/diff/20001/content/renderer/render_...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/626913004/diff/20001/content/renderer/render_...
> content/renderer/render_frame_impl.cc:2641: transition_type |
> ui::PAGE_TRANSITION_CLIENT_REDIRECT);
> On 2014/11/10 12:51:57, Fredrik Öhrn wrote:
> > On 2014/11/07 16:50:05, nasko wrote:
> > > Why not use safer pattern that already exists in this file at line 3369?
> > 
> > Because I didn't find PageTransitionFromInt to add anything of value. If it
> was
> > a straight up DCHECK it would be useful but the current implementation adds
> > bloat the compiler can't optimize away and if transition_type actually does
> > contain a bad value it'll silently discard the
PAGE_TRANSITION_CLIENT_REDIRECT
> > bit.
> > 
> > I'd suggest putting in a DCHECK(PageTransitionIsValidType(transition_type));
> > instead. However if you really want me to use PageTransitionFromInt, sure
I'll
> > add it.
> 
> Why not improve the code then? I would go even a step further, why use DCHECK
> instead of CHECK? If we ever have code that is using invalid value, this is
bad
> enough to kill the renderer IMHO.

Well, I guess we all dream of type safe bit-flags guaranteed to be verified
correct at compile time, until that happens the war will be over how much
runtime checking should be done versus finding all bit-bugs using debug asserts
and unit tests. In my opinion a DCHECK ought to be enough, but I've put in a
CHECK now.

[nasko, 2014-11-10 17:05:57]

>Of course, however, I'm unsure how to add a test for this as it wouldn't be a
>simple unit test but more of a functional test that loads a web page with a
meta
>refresh tag or JavaScript on it. Perhaps you can point me to some documentation
>on how to do this or to an exiting test of a similar nature.

There are browser tests, which drive the whole browser and you can do anything
in general. 
In this case, though, there is render_view_browsertest.cc, which contains a
bunch of tests which don't bring up the full browser (despite the naming), but
setup enough infrastructure for you to write tests in the renderer.

https://codereview.chromium.org/626913004/diff/60001/content/renderer/render_...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/626913004/diff/60001/content/renderer/render_...
content/renderer/render_frame_impl.cc:2642:
CHECK(ui::PageTransitionIsValidType(transition_type));
My point was that if you believe that PageTransitionFromInt could be better, you
should improve it.

[Fredrik Öhrn, 2014-11-11 17:12:27]

On 2014/11/10 17:05:57, nasko wrote:
> >Of course, however, I'm unsure how to add a test for this as it wouldn't be a
> >simple unit test but more of a functional test that loads a web page with a
> meta
> >refresh tag or JavaScript on it. Perhaps you can point me to some
documentation
> >on how to do this or to an exiting test of a similar nature.
> 
> There are browser tests, which drive the whole browser and you can do anything
> in general. 
> In this case, though, there is render_view_browsertest.cc, which contains a
> bunch of tests which don't bring up the full browser (despite the naming), but
> setup enough infrastructure for you to write tests in the renderer.
> 

Turns out ResourceDispatcherHost::Get() returns NULL when adding a test to
render_view_browsertest.cc and I didn't manage to figure out how to fix that.

Instead I found resource_dispatcher_host_browsertest.cc where it was possible to
install the required delegate. Unfortunately the handy LoadHTML function wasn't
available there.

If there is a better way to implement this test I'm all ears.


>
https://codereview.chromium.org/626913004/diff/60001/content/renderer/render_...
> File content/renderer/render_frame_impl.cc (right):
> 
>
https://codereview.chromium.org/626913004/diff/60001/content/renderer/render_...
> content/renderer/render_frame_impl.cc:2642:
> CHECK(ui::PageTransitionIsValidType(transition_type));
> My point was that if you believe that PageTransitionFromInt could be better,
you
> should improve it.

I got that, but my improvement would have been to turn that function into
essentially a DCHECK and I didn't expect you to like that.

Perhaps a new function would be in order instead, how about something like

PageTransitionSetFlag(PageTransition& target, PageTransition flag) {
  DCHECK(!(flag & PAGE_TRANSITION_CORE_MASK));
  target = static_cast<PageTransition>(target | flag);
}

That would improve type safety and catch bad flags, or for the really paranoid
make it a CHECK.

[nasko, 2014-11-13 00:09:32]

On 2014/11/11 17:12:27, Fredrik Öhrn wrote:
> On 2014/11/10 17:05:57, nasko wrote:
> > >Of course, however, I'm unsure how to add a test for this as it wouldn't be
a
> > >simple unit test but more of a functional test that loads a web page with a
> > meta
> > >refresh tag or JavaScript on it. Perhaps you can point me to some
> documentation
> > >on how to do this or to an exiting test of a similar nature.
> > 
> > There are browser tests, which drive the whole browser and you can do
anything
> > in general. 
> > In this case, though, there is render_view_browsertest.cc, which contains a
> > bunch of tests which don't bring up the full browser (despite the naming),
but
> > setup enough infrastructure for you to write tests in the renderer.
> > 
> 
> Turns out ResourceDispatcherHost::Get() returns NULL when adding a test to
> render_view_browsertest.cc and I didn't manage to figure out how to fix that.

Yes, render_view_browsertest doesn't really have a full stack for loading from
the network.

> Instead I found resource_dispatcher_host_browsertest.cc where it was possible
to
> install the required delegate. Unfortunately the handy LoadHTML function
wasn't
> available there.
> 
> If there is a better way to implement this test I'm all ears.

I think this is reasonable, but I'm not sure if it tests exactly this method.
Adding creis@ for other ideas.

> >
>
https://codereview.chromium.org/626913004/diff/60001/content/renderer/render_...
> > File content/renderer/render_frame_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/626913004/diff/60001/content/renderer/render_...
> > content/renderer/render_frame_impl.cc:2642:
> > CHECK(ui::PageTransitionIsValidType(transition_type));
> > My point was that if you believe that PageTransitionFromInt could be better,
> you
> > should improve it.
> 
> I got that, but my improvement would have been to turn that function into
> essentially a DCHECK and I didn't expect you to like that.
> 
> Perhaps a new function would be in order instead, how about something like
> 
> PageTransitionSetFlag(PageTransition& target, PageTransition flag) {
>   DCHECK(!(flag & PAGE_TRANSITION_CORE_MASK));
>   target = static_cast<PageTransition>(target | flag);
> }
> 
> That would improve type safety and catch bad flags, or for the really paranoid
> make it a CHECK.

If it is renderer side only, I'd make it a CHECK : ).

[nasko, 2014-11-13 00:09:57]

nasko@chromium.org changed reviewers:
+ creis@chromium.org

[nasko, 2014-11-13 00:09:58]

Really adding creis@ now.

[Charlie Reis, 2014-11-13 00:49:35]

Test LGTM with nits.  

Can you update the CL description now that the flag is in the ui:: namespace and
not content?  Also, it appears that this flag is already set in
RenderFrameImpl::SendDidCommitProvisionalLoad-- this CL is about setting it
earlier for WillSendRequest, which is certainly worthwhile.

On 2014/11/13 00:09:32, nasko wrote:
> On 2014/11/11 17:12:27, Fredrik Öhrn wrote:
> > On 2014/11/10 17:05:57, nasko wrote:
> > > >Of course, however, I'm unsure how to add a test for this as it wouldn't
be
> a
> > > >simple unit test but more of a functional test that loads a web page with
a
> > > meta
> > > >refresh tag or JavaScript on it. Perhaps you can point me to some
> > documentation
> > > >on how to do this or to an exiting test of a similar nature.
> > > 
> > > There are browser tests, which drive the whole browser and you can do
> anything
> > > in general. 
> > > In this case, though, there is render_view_browsertest.cc, which contains
a
> > > bunch of tests which don't bring up the full browser (despite the naming),
> but
> > > setup enough infrastructure for you to write tests in the renderer.
> > > 
> > 
> > Turns out ResourceDispatcherHost::Get() returns NULL when adding a test to
> > render_view_browsertest.cc and I didn't manage to figure out how to fix
that.
> 
> Yes, render_view_browsertest doesn't really have a full stack for loading from
> the network.
> 
> > Instead I found resource_dispatcher_host_browsertest.cc where it was
possible
> to
> > install the required delegate. Unfortunately the handy LoadHTML function
> wasn't
> > available there.
> > 
> > If there is a better way to implement this test I'm all ears.
> 
> I think this is reasonable, but I'm not sure if it tests exactly this method.
> Adding creis@ for other ideas.

Seems correct to me.  (It fails without your change, right?)  Thanks for adding
the test.

> 
> > >
> >
>
https://codereview.chromium.org/626913004/diff/60001/content/renderer/render_...
> > > File content/renderer/render_frame_impl.cc (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/626913004/diff/60001/content/renderer/render_...
> > > content/renderer/render_frame_impl.cc:2642:
> > > CHECK(ui::PageTransitionIsValidType(transition_type));
> > > My point was that if you believe that PageTransitionFromInt could be
better,
> > you
> > > should improve it.
> > 
> > I got that, but my improvement would have been to turn that function into
> > essentially a DCHECK and I didn't expect you to like that.
> > 
> > Perhaps a new function would be in order instead, how about something like
> > 
> > PageTransitionSetFlag(PageTransition& target, PageTransition flag) {
> >   DCHECK(!(flag & PAGE_TRANSITION_CORE_MASK));
> >   target = static_cast<PageTransition>(target | flag);
> > }
> > 
> > That would improve type safety and catch bad flags, or for the really
paranoid
> > make it a CHECK.
> 
> If it is renderer side only, I'd make it a CHECK : ).

It wouldn't be renderer-only, though.  It would be in ui/base, so a DCHECK is
probably better IMO.

https://codereview.chromium.org/626913004/diff/80001/content/browser/loader/r...
File content/browser/loader/resource_dispatcher_host_browsertest.cc (right):

https://codereview.chromium.org/626913004/diff/80001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_browsertest.cc:484: 
nit: No blank line.

https://codereview.chromium.org/626913004/diff/80001/content/browser/loader/r...
content/browser/loader/resource_dispatcher_host_browsertest.cc:500: GURL watch_;
nit: watch_url_

[Fredrik Öhrn, 2014-11-13 12:57:08]

On 2014/11/13 00:09:32, nasko wrote:
> On 2014/11/11 17:12:27, Fredrik Öhrn wrote:
> > 
> > Perhaps a new function would be in order instead, how about something like
> > 
> > PageTransitionSetFlag(PageTransition& target, PageTransition flag) {
> >   DCHECK(!(flag & PAGE_TRANSITION_CORE_MASK));
> >   target = static_cast<PageTransition>(target | flag);
> > }
> > 
> > That would improve type safety and catch bad flags, or for the really
paranoid
> > make it a CHECK.
> 
> If it is renderer side only, I'd make it a CHECK : ).

With a quick grep over content I found 7-8 places that could be changed from
PageTransitionFromInt to a new PageTransitionSetFlag function. How about I do
this in a new CL instead of piling on a somewhat unrelated change here?

I changed this patch to use PageTransitionFromInt for now.

[Fredrik Öhrn, 2014-11-13 12:59:26]

On 2014/11/13 00:49:35, Charlie Reis wrote:
> Test LGTM with nits.  
> 
> Can you update the CL description now that the flag is in the ui:: namespace
and
> not content?  Also, it appears that this flag is already set in
> RenderFrameImpl::SendDidCommitProvisionalLoad-- this CL is about setting it
> earlier for WillSendRequest, which is certainly worthwhile.
> 

Correct, this patch is about setting the flag early enough to be useful when
intercepting navigation. I've updated the CL description to make this more
clear.

> 
>
https://codereview.chromium.org/626913004/diff/80001/content/browser/loader/r...
> File content/browser/loader/resource_dispatcher_host_browsertest.cc (right):
> 
>
https://codereview.chromium.org/626913004/diff/80001/content/browser/loader/r...
> content/browser/loader/resource_dispatcher_host_browsertest.cc:484: 
> nit: No blank line.
> 
>
https://codereview.chromium.org/626913004/diff/80001/content/browser/loader/r...
> content/browser/loader/resource_dispatcher_host_browsertest.cc:500: GURL
watch_;
> nit: watch_url_

Nits fixed.

[nasko, 2014-11-13 15:20:33]

LGTM

[Fredrik Öhrn, 2014-11-13 15:32:35]

The CQ bit was checked by ohrn@opera.com

[commit-bot: I haz the power, 2014-11-13 15:33:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/626913004/100001

[commit-bot: I haz the power, 2014-11-13 15:36:09]

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

[commit-bot: I haz the power, 2014-11-13 15:36:11]

Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  ios_rel_device on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device/bu...)

[Fredrik Öhrn, 2014-11-13 15:55:12]

The CQ bit was checked by ohrn@opera.com

[commit-bot: I haz the power, 2014-11-13 15:56:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/626913004/120001

[commit-bot: I haz the power, 2014-11-13 16:20:45]

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

[commit-bot: I haz the power, 2014-11-13 16:20:47]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fredrik Öhrn, 2014-11-13 17:10:29]

The CQ bit was checked by ohrn@opera.com

[commit-bot: I haz the power, 2014-11-13 17:11:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/626913004/140001

[commit-bot: I haz the power, 2014-11-13 18:03:22]

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

[commit-bot: I haz the power, 2014-11-13 18:03:23]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Fredrik Öhrn, 2014-11-14 12:59:52]

On 2014/11/13 18:03:23, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   linux_chromium_rel_ng on tryserver.chromium.linux
>
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

Turns out the test case is flaky in release builds, worked fine for me in debug,
thats why I didn't notice.

It needs to wait until the redirect has happened before testing the page
transition value. Can anyone suggest how to do this?

[Charlie Reis, 2014-11-14 18:05:54]

https://codereview.chromium.org/626913004/diff/140001/content/browser/loader/...
File content/browser/loader/resource_dispatcher_host_browsertest.cc (right):

https://codereview.chromium.org/626913004/diff/140001/content/browser/loader/...
content/browser/loader/resource_dispatcher_host_browsertest.cc:514:
NavigateToURL(shell(),
A client redirect will commit twice, right?  Maybe use
NavigateToURLBlockUntilNavigationsComplete with a final argument of 2?

[Fredrik Öhrn, 2014-11-17 11:42:44]

On 2014/11/14 18:05:54, Charlie Reis wrote:
>
https://codereview.chromium.org/626913004/diff/140001/content/browser/loader/...
> File content/browser/loader/resource_dispatcher_host_browsertest.cc (right):
> 
>
https://codereview.chromium.org/626913004/diff/140001/content/browser/loader/...
> content/browser/loader/resource_dispatcher_host_browsertest.cc:514:
> NavigateToURL(shell(),
> A client redirect will commit twice, right?  Maybe use
> NavigateToURLBlockUntilNavigationsComplete with a final argument of 2?

That did the trick, thanks a lot! New patch uploaded.

I also changed the redirect to an existing page to avoid a 404 warning that
appeared in the output with this fix.

Maybe you can give it a quick look over again before I push the commit checkbox.

[nasko, 2014-11-17 14:44:16]

That looks better. LGTM

[Fredrik Öhrn, 2014-11-17 14:49:33]

The CQ bit was checked by ohrn@opera.com

[commit-bot: I haz the power, 2014-11-17 14:49:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/626913004/160001

[commit-bot: I haz the power, 2014-11-17 16:00:09]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-11-17 16:00:50]

Patchset 9 (id:??) landed as
https://crrev.com/19893e6bce9005ef4a406fc61f0555c2e0f21d50
Cr-Commit-Position: refs/heads/master@{#304425}

[Fredrik Öhrn, 2014-11-17 16:06:12]

Thanks everyone for the help and your patience, this review turned out noisier
than I expected.

[nasko, 2014-11-17 16:46:57]

On 2014/11/17 16:06:12, Fredrik Öhrn wrote:
> Thanks everyone for the help and your patience, this review turned out noisier
> than I expected.

No problem. CLs aren't always as easy as they seem initially :) Keep the patches
coming though!