Navigation transitions: Pass is_transition_navigation flag up to the embedder

Navigation transitions: Initiate transition navigations.

A is_transition_navigation flag now gets passed up from Blink (through decidePolicyForNavigation and OpenURL) to the navigator, which then calls didStartNavigationTransitionForFrame on the ContentViewCore of the embedder if it's set.

R=nasko,jam
BUG=370696
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=286672

[oystein (OOO til 10th of July), 2014-07-01 12:06:03]

This flag is what kicks off the whole navigation transition process. Blink sets
it if it detects the presence of a valid transition metatag, and the embedder
uses it to set up the transition navigation state machine and delay the
navigation.

ptal :).

[jam, 2014-07-01 16:14:15]

just looked at public for now

https://codereview.chromium.org/358973005/diff/20001/content/public/browser/n...
File content/public/browser/navigation_controller.h (right):

https://codereview.chromium.org/358973005/diff/20001/content/public/browser/n...
content/public/browser/navigation_controller.h:172: bool
is_transition_navigation;
ditto

https://codereview.chromium.org/358973005/diff/20001/content/public/browser/n...
File content/public/browser/navigation_entry.h (right):

https://codereview.chromium.org/358973005/diff/20001/content/public/browser/n...
content/public/browser/navigation_entry.h:221: virtual void
SetIsTransitionNavigation(bool is_transition_navigation) = 0;
every other method in this file is documented...

https://codereview.chromium.org/358973005/diff/20001/content/public/browser/p...
File content/public/browser/page_navigator.h (right):

https://codereview.chromium.org/358973005/diff/20001/content/public/browser/p...
content/public/browser/page_navigator.h:86: bool is_transition_navigation;
ditto

[nasko, 2014-07-03 09:39:31]

I'm not quite clear when transitions will be performed. If we have an entry in
session history that is stamped with "is transition", will going back to it
perform a transition?
Also, how would transitions behave when I type an URL in the omnibox and the
currently displayed document is specifying transitions?

https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_entry_impl.h:92: virtual void
SetIsTransitionNavigation(bool is_transition_navigation)
Does this mean that both forward and backwards navigations will have to use
transitions?

https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_entry_impl.h:348: bool
is_transition_navigation_;
Just like other places, this needs a comment.

https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:195:
delegate_->DidStartNavigationTransition(render_frame_host);
Why add a full new delegate method when you are passing the bit as part of
DidStartProvisionalLoad?

https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/render_frame_host_manager.cc (right):

https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
content/browser/frame_host/render_frame_host_manager.cc:428: true, false);
Why is this hardcoded to false? It seems like it will break your feature for
cross-process navigations.

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

https://codereview.chromium.org/358973005/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.h:478: bool is_transition_navigation);
Why not collapse all of these also to a NavigationPolicyInfo?

[oystein (OOO til 10th of July), 2014-07-17 17:52:49]

https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigation_entry_impl.h (right):

https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigation_entry_impl.h:92: virtual void
SetIsTransitionNavigation(bool is_transition_navigation)
On 2014/07/03 09:39:30, nasko (in Munich) wrote:
> Does this mean that both forward and backwards navigations will have to use
> transitions?

Hm, ideally not. The reason for this flag being in NavigationEntry was to
preserve it for cross-process navigations (so
NavigatorImpl::DidStartProvisionalLoad can pick it up again); is there a better
place to store this? I could clear it once it's "used", but that doesn't feel
too clean.

[nasko, 2014-07-21 14:31:13]

On 2014/07/17 17:52:49, Oystein wrote:
>
https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
> File content/browser/frame_host/navigation_entry_impl.h (right):
> 
>
https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
> content/browser/frame_host/navigation_entry_impl.h:92: virtual void
> SetIsTransitionNavigation(bool is_transition_navigation)
> On 2014/07/03 09:39:30, nasko (in Munich) wrote:
> > Does this mean that both forward and backwards navigations will have to use
> > transitions?
> 
> Hm, ideally not. The reason for this flag being in NavigationEntry was to
> preserve it for cross-process navigations (so
> NavigatorImpl::DidStartProvisionalLoad can pick it up again); is there a
better
> place to store this? I could clear it once it's "used", but that doesn't feel
> too clean.

There are other changes in this CL that seem to prevent transitions in
cross-process navigations. Can you look over the rest of the comments?

[oystein (OOO til 10th of July), 2014-07-21 21:08:36]

On 2014/07/21 14:31:13, nasko (in Munich) wrote:
> On 2014/07/17 17:52:49, Oystein wrote:
> >
>
https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
> > File content/browser/frame_host/navigation_entry_impl.h (right):
> > 
> >
>
https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
> > content/browser/frame_host/navigation_entry_impl.h:92: virtual void
> > SetIsTransitionNavigation(bool is_transition_navigation)
> > On 2014/07/03 09:39:30, nasko (in Munich) wrote:
> > > Does this mean that both forward and backwards navigations will have to
use
> > > transitions?
> > 
> > Hm, ideally not. The reason for this flag being in NavigationEntry was to
> > preserve it for cross-process navigations (so
> > NavigatorImpl::DidStartProvisionalLoad can pick it up again); is there a
> better
> > place to store this? I could clear it once it's "used", but that doesn't
feel
> > too clean.
> 
> There are other changes in this CL that seem to prevent transitions in
> cross-process navigations. Can you look over the rest of the comments?

Yeah I saw the issue you pointed out in RenderFrameHostManager::SwappedOut.
Cross-process navigations still seem to work with this hardcoded to false, but
there's presumably a specific condition I'm not forcing which would trigger this
particular codepath.

In either case though, I guess it's the same question: Given that the
is_transition_navigation bit needs to somehow be set somewhere in browser on the
original navigation, so that when the NavigatorImpl::DidStartProvisionalLoad
call comes up from the new fresh renderer process we can treat it as a
transition navigation, where should we store this state? NavigationEntry is out,
if that means it'll be preserved in the history I guess. I could check if
there's pending data in the TransitionRequestManager, if I have the render
process id and frame_id, but that may be fragile. Or this could be stored in the
pending RenderFrameHost, but that's not too clean either.

The only other alternative I see is to pass the bit all the way up in the
OpenURL call to the embedder, then all the way down to the new renderer process,
so that the FrameLoader in Blink can then pass it to StartProvisionalLoad again.
That seems pretty roundabout though, hopefully there's an easier way.

[oystein (OOO til 10th of July), 2014-07-22 00:02:43]

In the meantime: I've uploaded a reduced version of this patch which removes all
the code for dealing with cross-platform navigations. Maybe we can start with
this, and deal with the cross-platform stuff in a companion CL once we figure
that one out. PTAL :)

https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
File content/browser/frame_host/navigator_impl.cc (right):

https://codereview.chromium.org/358973005/diff/20001/content/browser/frame_ho...
content/browser/frame_host/navigator_impl.cc:195:
delegate_->DidStartNavigationTransition(render_frame_host);
On 2014/07/03 09:39:30, nasko (in Munich) wrote:
> Why add a full new delegate method when you are passing the bit as part of
> DidStartProvisionalLoad?

I could pass it along to WebContentsObserver::DidStartProvionalLoadForFrame and
from there up to the Android embedder, but virtual is overriden in a million
places. This seemed cleaner (or at least less of an intrusive change), but I can
add the bool if that's preferred.

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

https://codereview.chromium.org/358973005/diff/20001/content/renderer/render_...
content/renderer/render_frame_impl.h:478: bool is_transition_navigation);
On 2014/07/03 09:39:31, nasko (in Munich) wrote:
> Why not collapse all of these also to a NavigationPolicyInfo?

Done.

[oystein (OOO til 10th of July), 2014-07-24 22:29:29]

nasko: friendly ping :) (Are you doing reviews while in Munich?)

[nasko, 2014-07-25 09:28:12]

On 2014/07/24 22:29:29, Oystein wrote:
> nasko: friendly ping :) (Are you doing reviews while in Munich?)

Yes, I am :). You will have the review by Monday morning PST.

[nasko, 2014-07-25 09:50:57]

This now being a plumbing only CL is much simpler. LGTM

[oystein (OOO til 10th of July), 2014-07-25 16:19:31]

On 2014/07/25 09:50:57, nasko (in Munich) wrote:
> This now being a plumbing only CL is much simpler. LGTM

Thanks! :)

[oystein (OOO til 10th of July), 2014-07-25 16:19:37]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2014-07-25 16:20:31]

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

[commit-bot: I haz the power, 2014-07-25 16:34:32]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/21...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/33368)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/38405)

[commit-bot: I haz the power, 2014-07-25 16:36:08]

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

[commit-bot: I haz the power, 2014-07-25 16:36:09]

Try jobs failed on following builders:
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/33375)

[oystein (OOO til 10th of July), 2014-07-25 18:12:35]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2014-07-25 18:13:04]

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

[commit-bot: I haz the power, 2014-07-25 18:26:15]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_chromium_gn_compile_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_chromium_gn_c...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/21...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_gn_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_gn_rel...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/33427)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/38460)

[oystein (OOO til 10th of July), 2014-07-25 18:28:01]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2014-07-25 18:28:39]

Try jobs failed on following builders:
  ios_dbg_simulator on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
  ios_rel_device on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device/builds...)
  ios_rel_device_ninja on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/33435)

[commit-bot: I haz the power, 2014-07-25 18:29:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oysteine@chromium.org/358973005/120001

[commit-bot: I haz the power, 2014-07-25 19:42:43]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  android_aosp on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/9...)
  android_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_clang_dbg/bui...)
  android_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/android_dbg/builds/21...)
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)
  linux_chromium_chromeos_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_chromeos_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
  linux_chromium_clang_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_clang_...)
  linux_chromium_rel_swarming on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel_sw...)
  mac_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_compile_...)
  mac_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...)
  win8_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win8_chromium_rel/bui...)
  win_chromium_compile_dbg on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_compile_...)
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)
  win_chromium_x64_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/33439)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/38473)

[commit-bot: I haz the power, 2014-07-25 19:47:31]

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

[commit-bot: I haz the power, 2014-07-25 19:47:33]

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

[oystein (OOO til 10th of July), 2014-07-26 00:01:13]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2014-07-26 00:02:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oysteine@chromium.org/358973005/200001

[oystein (OOO til 10th of July), 2014-07-26 00:06:54]

jam: I somewhat belatedly realized that nasko only owns some of these files.
Mind taking a look at the rest of this?

[commit-bot: I haz the power, 2014-07-26 17:09:44]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-07-26 18:10:18]

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

[commit-bot: I haz the power, 2014-07-26 18:10:20]

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

[oystein (OOO til 10th of July), 2014-07-29 21:29:34]

+creis

[Charlie Reis, 2014-07-30 00:00:52]

I don't see why the DecidePolicyForNavigation change was needed, since it
doesn't appear to mention transitions.

LGTM with nits apart from that.

https://codereview.chromium.org/358973005/diff/200001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/358973005/diff/200001/content/common/frame_me...
content/common/frame_messages.h:433: // Sent when the renderer starts a
provisional load for a frame.
Please document the |is_transition_navigation| member, since it's not
immediately obvious what that means.

https://codereview.chromium.org/358973005/diff/200001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/358973005/diff/200001/content/renderer/render...
content/renderer/render_frame_impl.cc:3164: const NavigationPolicyInfo& info ) {
nit: No space after info

[Charlie Reis, 2014-07-30 16:56:51]

On 2014/07/30 00:00:52, Charlie Reis wrote:
> I don't see why the DecidePolicyForNavigation change was needed, since it
> doesn't appear to mention transitions.

Ah, I suppose it was used in a previous version of the CL, and now it's not
necessarily but already baked into the WebFrameClient API.  I suppose we may as
well move forward with NavigationPolicyInfo.

LGTM with the nits I mentioned.

[Charlie Reis, 2014-07-30 16:57:00]

On 2014/07/30 00:00:52, Charlie Reis wrote:
> I don't see why the DecidePolicyForNavigation change was needed, since it
> doesn't appear to mention transitions.

Ah, I suppose it was used in a previous version of the CL, and now it's not
necessary but already baked into the WebFrameClient API.  I suppose we may as
well move forward with NavigationPolicyInfo.

LGTM with the nits I mentioned.

[oystein (OOO til 10th of July), 2014-07-30 17:30:45]

Great, thanks! :)

On 2014/07/30 16:57:00, Charlie Reis wrote:
> On 2014/07/30 00:00:52, Charlie Reis wrote:
> > I don't see why the DecidePolicyForNavigation change was needed, since it
> > doesn't appear to mention transitions.
> 
> Ah, I suppose it was used in a previous version of the CL, and now it's not
> necessary but already baked into the WebFrameClient API.  I suppose we may as
> well move forward with NavigationPolicyInfo.
> 


Yeah that's exactly right. The earlier version of the CL also included passing
the flag around for cross-process navigations (including via
decidePolicyForNavigation to OpenURL), but we need to do that somewhat
differently (storing it in the NavigationEntry like we were doesn't make sense).
This'll make it easy to make use of the flag again or remove it if we won't need
it though.

https://codereview.chromium.org/358973005/diff/200001/content/common/frame_me...
File content/common/frame_messages.h (right):

https://codereview.chromium.org/358973005/diff/200001/content/common/frame_me...
content/common/frame_messages.h:433: // Sent when the renderer starts a
provisional load for a frame.
On 2014/07/30 00:00:51, Charlie Reis wrote:
> Please document the |is_transition_navigation| member, since it's not
> immediately obvious what that means.

Done.

https://codereview.chromium.org/358973005/diff/200001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/358973005/diff/200001/content/renderer/render...
content/renderer/render_frame_impl.cc:3164: const NavigationPolicyInfo& info ) {
On 2014/07/30 00:00:51, Charlie Reis wrote:
> nit: No space after info

Done.

[oystein (OOO til 10th of July), 2014-07-30 18:21:14]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2014-07-30 18:23:40]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oysteine@chromium.org/358973005/220001

[commit-bot: I haz the power, 2014-07-30 20:21:26]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_chromium_chromeos_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel/...)
  win_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel/...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[commit-bot: I haz the power, 2014-07-30 21:08:42]

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

[commit-bot: I haz the power, 2014-07-30 21:08:43]

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

[oystein (OOO til 10th of July), 2014-07-30 21:30:57]

The CQ bit was checked by oysteine@chromium.org

[commit-bot: I haz the power, 2014-07-30 21:31:51]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/oysteine@chromium.org/358973005/240001

[commit-bot: I haz the power, 2014-07-31 01:29:13]

Change committed as 286672