PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

This CL ensures that the BeforeUnload event is always properly executed before
starting a renderer-initiated navigation when browser-side-navigation is
enabled.

BUG=608371, 475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/bfd6a8144791f47eefb45961819c6f3472035506
Cr-Commit-Position: refs/heads/master@{#396737}

[clamy, 2016-04-15 12:05:49]

Description was changed from

==========
PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

This CL ensures that the BeforeUnload event is always properly executed before
starting a renderer-initiated navigation when browser-side-navigation is
enabled.

BUG=576261,475027
==========

to

==========
PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

This CL ensures that the BeforeUnload event is always properly executed before
starting a renderer-initiated navigation when browser-side-navigation is
enabled.

BUG=576261,475027
==========

[clamy, 2016-04-15 12:05:49]

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

[clamy, 2016-04-15 12:06:05]

@nasko: PTAL.

[nasko, 2016-04-15 14:42:21]

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

[nasko, 2016-04-15 14:42:22]

Adding dcheng@ for blink review and a question about ordering of events.

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

https://codereview.chromium.org/1890493002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4866: // DO NOT ADD CODE AFTER THIS
FUNCTION. It executes the BeforeUnload event
nit: "AFTER THIS FUNCTION" < The function or function call?

https://codereview.chromium.org/1890493002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4870: : blink::WebNavigationPolicyIgnore;
Isn't the beforeunload event already invoked before getting to
decidePolicyForNavigation? It seems a bit strange to have content/ orchestrate
the beforeunload firing for renderer-initiated navigations. Let's have dcheng@
weigh in on this.

[clamy, 2016-04-15 14:45:54]

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

https://codereview.chromium.org/1890493002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4870: : blink::WebNavigationPolicyIgnore;
On 2016/04/15 14:42:22, nasko (very slow Apr 8-18) wrote:
> Isn't the beforeunload event already invoked before getting to
> decidePolicyForNavigation? It seems a bit strange to have content/ orchestrate
> the beforeunload firing for renderer-initiated navigations. Let's have dcheng@
> weigh in on this.

No it's executed right afterwards (hence why I have to do this).

[dcheng, 2016-04-15 17:21:43]

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

https://codereview.chromium.org/1890493002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4870: : blink::WebNavigationPolicyIgnore;
On 2016/04/15 at 14:45:54, clamy wrote:
> On 2016/04/15 14:42:22, nasko (very slow Apr 8-18) wrote:
> > Isn't the beforeunload event already invoked before getting to
> > decidePolicyForNavigation? It seems a bit strange to have content/
orchestrate
> > the beforeunload firing for renderer-initiated navigations. Let's have
dcheng@
> > weigh in on this.
> 
> No it's executed right afterwards (hence why I have to do this).

Hmm, we should try to figure out how to make Blink handle this correctly: after
all, it knows that the embedder is (probably) going to navigate it somewhere.
Similar to how we currently call progressStarted() today in the "handled by
client case" to make sure progress state is set up correctly, we should probably
enforce that beforeunload didn't cancel the load at this time.

https://codereview.chromium.org/1890493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1890493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1323: if
(!m_frame->host())
How do we get into a case where we call this on a detached frame?

[clamy, 2016-04-25 11:36:55]

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

https://codereview.chromium.org/1890493002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4870: : blink::WebNavigationPolicyIgnore;
On 2016/04/15 17:21:42, dcheng wrote:
> On 2016/04/15 at 14:45:54, clamy wrote:
> > On 2016/04/15 14:42:22, nasko (very slow Apr 8-18) wrote:
> > > Isn't the beforeunload event already invoked before getting to
> > > decidePolicyForNavigation? It seems a bit strange to have content/
> orchestrate
> > > the beforeunload firing for renderer-initiated navigations. Let's have
> dcheng@
> > > weigh in on this.
> > 
> > No it's executed right afterwards (hence why I have to do this).
> 
> Hmm, we should try to figure out how to make Blink handle this correctly:
after
> all, it knows that the embedder is (probably) going to navigate it somewhere.
> Similar to how we currently call progressStarted() today in the "handled by
> client case" to make sure progress state is set up correctly, we should
probably
> enforce that beforeunload didn't cancel the load at this time.

The problem is that the call to decidePolicyForNavigation also triggers the
navigation being sent to the browser. So if we execute BeforeUnload afterwards,
BeforeUnload can no longer cancel the navigation.

If we want to keep executing BeforeUnload in the FrameLoader, then we can no
longer have decidePolicyForNavigation send the navigation to the browser.
Instead, the FrameLoader has to ask the client whether to continue with the
navigation or not, get a HandledByClient (whith no side effect). Then execute
the BeforeUnload event, and when that's done send back the navigation to the
client if it got a NavigationPolicy::HandledByClient in the first place.

I'm not sure which is more understandable between having BeginNavigation ask for
BeforeUnload to execute, or having decidePolicyForNavigation return
WebNavigationPolicy::HandledByClient when it's not actually handling anything
until WebFrame calls another function to handle the navigation.

https://codereview.chromium.org/1890493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1890493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1323: if
(!m_frame->host())
On 2016/04/15 17:21:42, dcheng wrote:
> How do we get into a case where we call this on a detached frame?

There's a specific layout test checking for what happens when the BeforeUnload
event detaches the frame.

[dcheng, 2016-04-26 06:32:31]

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

https://codereview.chromium.org/1890493002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4870: : blink::WebNavigationPolicyIgnore;
On 2016/04/25 at 11:36:55, clamy wrote:
> On 2016/04/15 17:21:42, dcheng wrote:
> > On 2016/04/15 at 14:45:54, clamy wrote:
> > > On 2016/04/15 14:42:22, nasko (very slow Apr 8-18) wrote:
> > > > Isn't the beforeunload event already invoked before getting to
> > > > decidePolicyForNavigation? It seems a bit strange to have content/
> > orchestrate
> > > > the beforeunload firing for renderer-initiated navigations. Let's have
> > dcheng@
> > > > weigh in on this.
> > > 
> > > No it's executed right afterwards (hence why I have to do this).
> > 
> > Hmm, we should try to figure out how to make Blink handle this correctly:
after
> > all, it knows that the embedder is (probably) going to navigate it
somewhere.
> > Similar to how we currently call progressStarted() today in the "handled by
> > client case" to make sure progress state is set up correctly, we should
probably
> > enforce that beforeunload didn't cancel the load at this time.
> 
> The problem is that the call to decidePolicyForNavigation also triggers the
navigation being sent to the browser. So if we execute BeforeUnload afterwards,
BeforeUnload can no longer cancel the navigation.
> 
> If we want to keep executing BeforeUnload in the FrameLoader, then we can no
longer have decidePolicyForNavigation send the navigation to the browser.
Instead, the FrameLoader has to ask the client whether to continue with the
navigation or not, get a HandledByClient (whith no side effect). Then execute
the BeforeUnload event, and when that's done send back the navigation to the
client if it got a NavigationPolicy::HandledByClient in the first place.
> 
> I'm not sure which is more understandable between having BeginNavigation ask
for BeforeUnload to execute, or having decidePolicyForNavigation return
WebNavigationPolicy::HandledByClient when it's not actually handling anything
until WebFrame calls another function to handle the navigation.

Hmm, I see the problem, but I think it's awkward to have to add a bit on
WebURLRequest to track this: from what I can tell, WebURLRequest is used for a
lot of things other than frame loads.

Let me think about this tonight, sorry for the delay.

https://codereview.chromium.org/1890493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1890493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1323: if
(!m_frame->host())
On 2016/04/25 at 11:36:55, clamy wrote:
> On 2016/04/15 17:21:42, dcheng wrote:
> > How do we get into a case where we call this on a detached frame?
> 
> There's a specific layout test checking for what happens when the BeforeUnload
event detaches the frame.

Right, but why didn't we need to previously check for this?

[clamy, 2016-04-26 10:07:09]

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

https://codereview.chromium.org/1890493002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4870: : blink::WebNavigationPolicyIgnore;
On 2016/04/26 06:32:31, dcheng wrote:
> On 2016/04/25 at 11:36:55, clamy wrote:
> > On 2016/04/15 17:21:42, dcheng wrote:
> > > On 2016/04/15 at 14:45:54, clamy wrote:
> > > > On 2016/04/15 14:42:22, nasko (very slow Apr 8-18) wrote:
> > > > > Isn't the beforeunload event already invoked before getting to
> > > > > decidePolicyForNavigation? It seems a bit strange to have content/
> > > orchestrate
> > > > > the beforeunload firing for renderer-initiated navigations. Let's have
> > > dcheng@
> > > > > weigh in on this.
> > > > 
> > > > No it's executed right afterwards (hence why I have to do this).
> > > 
> > > Hmm, we should try to figure out how to make Blink handle this correctly:
> after
> > > all, it knows that the embedder is (probably) going to navigate it
> somewhere.
> > > Similar to how we currently call progressStarted() today in the "handled
by
> > > client case" to make sure progress state is set up correctly, we should
> probably
> > > enforce that beforeunload didn't cancel the load at this time.
> > 
> > The problem is that the call to decidePolicyForNavigation also triggers the
> navigation being sent to the browser. So if we execute BeforeUnload
afterwards,
> BeforeUnload can no longer cancel the navigation.
> > 
> > If we want to keep executing BeforeUnload in the FrameLoader, then we can no
> longer have decidePolicyForNavigation send the navigation to the browser.
> Instead, the FrameLoader has to ask the client whether to continue with the
> navigation or not, get a HandledByClient (whith no side effect). Then execute
> the BeforeUnload event, and when that's done send back the navigation to the
> client if it got a NavigationPolicy::HandledByClient in the first place.
> > 
> > I'm not sure which is more understandable between having BeginNavigation ask
> for BeforeUnload to execute, or having decidePolicyForNavigation return
> WebNavigationPolicy::HandledByClient when it's not actually handling anything
> until WebFrame calls another function to handle the navigation.
> 
> Hmm, I see the problem, but I think it's awkward to have to add a bit on
> WebURLRequest to track this: from what I can tell, WebURLRequest is used for a
> lot of things other than frame loads.
> 
> Let me think about this tonight, sorry for the delay.

The bit in WebURLRequest is not coming from this problem, it's there to not
execute BeforeUnload a second time when we commit the navigation. Even if I
change the patch so that BeforeUnload executes from the FrameLoader, I would
still need a way to skip it when we commit. Now since I already need to know
that the WebURLRequest corresponds to a navigation commit so that we don't send
it to the browser, I could probably just have a single boolean
isNavigationCommit that I can use to skip BeforeUnload on commits.

https://codereview.chromium.org/1890493002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (right):

https://codereview.chromium.org/1890493002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1323: if
(!m_frame->host())
On 2016/04/26 06:32:31, dcheng wrote:
> On 2016/04/25 at 11:36:55, clamy wrote:
> > On 2016/04/15 17:21:42, dcheng wrote:
> > > How do we get into a case where we call this on a detached frame?
> > 
> > There's a specific layout test checking for what happens when the
BeforeUnload
> event detaches the frame.
> 
> Right, but why didn't we need to previously check for this?

I think this i sbecause this function was only called for cross-process
navigations, and only for the main frame. I don't think it's possible for the
main frame to detach itself. However, the patch has it called on all
renderer-initiated navigations, including subframes when PlzNavigate is enabled.
And subframes can detach themselves.

[dcheng, 2016-04-27 09:21:29]

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

https://codereview.chromium.org/1890493002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4870: : blink::WebNavigationPolicyIgnore;
On 2016/04/26 at 10:07:09, clamy wrote:
> On 2016/04/26 06:32:31, dcheng wrote:
> > On 2016/04/25 at 11:36:55, clamy wrote:
> > > On 2016/04/15 17:21:42, dcheng wrote:
> > > > On 2016/04/15 at 14:45:54, clamy wrote:
> > > > > On 2016/04/15 14:42:22, nasko (very slow Apr 8-18) wrote:
> > > > > > Isn't the beforeunload event already invoked before getting to
> > > > > > decidePolicyForNavigation? It seems a bit strange to have content/
> > > > orchestrate
> > > > > > the beforeunload firing for renderer-initiated navigations. Let's
have
> > > > dcheng@
> > > > > > weigh in on this.
> > > > > 
> > > > > No it's executed right afterwards (hence why I have to do this).
> > > > 
> > > > Hmm, we should try to figure out how to make Blink handle this
correctly:
> > after
> > > > all, it knows that the embedder is (probably) going to navigate it
> > somewhere.
> > > > Similar to how we currently call progressStarted() today in the "handled
by
> > > > client case" to make sure progress state is set up correctly, we should
> > probably
> > > > enforce that beforeunload didn't cancel the load at this time.
> > > 
> > > The problem is that the call to decidePolicyForNavigation also triggers
the
> > navigation being sent to the browser. So if we execute BeforeUnload
afterwards,
> > BeforeUnload can no longer cancel the navigation.
> > > 
> > > If we want to keep executing BeforeUnload in the FrameLoader, then we can
no
> > longer have decidePolicyForNavigation send the navigation to the browser.
> > Instead, the FrameLoader has to ask the client whether to continue with the
> > navigation or not, get a HandledByClient (whith no side effect). Then
execute
> > the BeforeUnload event, and when that's done send back the navigation to the
> > client if it got a NavigationPolicy::HandledByClient in the first place.
> > > 
> > > I'm not sure which is more understandable between having BeginNavigation
ask
> > for BeforeUnload to execute, or having decidePolicyForNavigation return
> > WebNavigationPolicy::HandledByClient when it's not actually handling
anything
> > until WebFrame calls another function to handle the navigation.
> > 
> > Hmm, I see the problem, but I think it's awkward to have to add a bit on
> > WebURLRequest to track this: from what I can tell, WebURLRequest is used for
a
> > lot of things other than frame loads.
> > 
> > Let me think about this tonight, sorry for the delay.
> 
> The bit in WebURLRequest is not coming from this problem, it's there to not
execute BeforeUnload a second time when we commit the navigation. Even if I
change the patch so that BeforeUnload executes from the FrameLoader, I would
still need a way to skip it when we commit. Now since I already need to know
that the WebURLRequest corresponds to a navigation commit so that we don't send
it to the browser, I could probably just have a single boolean
isNavigationCommit that I can use to skip BeforeUnload on commits.

Part of me thinks that it's weird to expose internal Blink details like this:
ideally, I think I would prefer to structure it like this:
1) We revert to the original pre-PlzNavigate navigation policy.
2) Assuming it should continue, Blink continues on its typical loading path.
3) Before actually performing the navigation in Blink, give the embedder a
chance to intercept (e.g. today's HandledByClient enum, but as a separate
callback rather than using an enumerator value).

However, thinking about this more, it seems like this won't work well for OOPIF:
we won't run beforeunload events in cross-origin frames... so maybe we should
move to a model where the embedder *always* runs the beforeunload events in
behalf of Blink? WDYT?

[clamy, 2016-04-27 11:29:27]

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

https://codereview.chromium.org/1890493002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4870: : blink::WebNavigationPolicyIgnore;
On 2016/04/27 09:21:29, dcheng wrote:
> On 2016/04/26 at 10:07:09, clamy wrote:
> > On 2016/04/26 06:32:31, dcheng wrote:
> > > On 2016/04/25 at 11:36:55, clamy wrote:
> > > > On 2016/04/15 17:21:42, dcheng wrote:
> > > > > On 2016/04/15 at 14:45:54, clamy wrote:
> > > > > > On 2016/04/15 14:42:22, nasko (very slow Apr 8-18) wrote:
> > > > > > > Isn't the beforeunload event already invoked before getting to
> > > > > > > decidePolicyForNavigation? It seems a bit strange to have content/
> > > > > orchestrate
> > > > > > > the beforeunload firing for renderer-initiated navigations. Let's
> have
> > > > > dcheng@
> > > > > > > weigh in on this.
> > > > > > 
> > > > > > No it's executed right afterwards (hence why I have to do this).
> > > > > 
> > > > > Hmm, we should try to figure out how to make Blink handle this
> correctly:
> > > after
> > > > > all, it knows that the embedder is (probably) going to navigate it
> > > somewhere.
> > > > > Similar to how we currently call progressStarted() today in the
"handled
> by
> > > > > client case" to make sure progress state is set up correctly, we
should
> > > probably
> > > > > enforce that beforeunload didn't cancel the load at this time.
> > > > 
> > > > The problem is that the call to decidePolicyForNavigation also triggers
> the
> > > navigation being sent to the browser. So if we execute BeforeUnload
> afterwards,
> > > BeforeUnload can no longer cancel the navigation.
> > > > 
> > > > If we want to keep executing BeforeUnload in the FrameLoader, then we
can
> no
> > > longer have decidePolicyForNavigation send the navigation to the browser.
> > > Instead, the FrameLoader has to ask the client whether to continue with
the
> > > navigation or not, get a HandledByClient (whith no side effect). Then
> execute
> > > the BeforeUnload event, and when that's done send back the navigation to
the
> > > client if it got a NavigationPolicy::HandledByClient in the first place.
> > > > 
> > > > I'm not sure which is more understandable between having BeginNavigation
> ask
> > > for BeforeUnload to execute, or having decidePolicyForNavigation return
> > > WebNavigationPolicy::HandledByClient when it's not actually handling
> anything
> > > until WebFrame calls another function to handle the navigation.
> > > 
> > > Hmm, I see the problem, but I think it's awkward to have to add a bit on
> > > WebURLRequest to track this: from what I can tell, WebURLRequest is used
for
> a
> > > lot of things other than frame loads.
> > > 
> > > Let me think about this tonight, sorry for the delay.
> > 
> > The bit in WebURLRequest is not coming from this problem, it's there to not
> execute BeforeUnload a second time when we commit the navigation. Even if I
> change the patch so that BeforeUnload executes from the FrameLoader, I would
> still need a way to skip it when we commit. Now since I already need to know
> that the WebURLRequest corresponds to a navigation commit so that we don't
send
> it to the browser, I could probably just have a single boolean
> isNavigationCommit that I can use to skip BeforeUnload on commits.
> 
> Part of me thinks that it's weird to expose internal Blink details like this:
> ideally, I think I would prefer to structure it like this:
> 1) We revert to the original pre-PlzNavigate navigation policy.
> 2) Assuming it should continue, Blink continues on its typical loading path.
> 3) Before actually performing the navigation in Blink, give the embedder a
> chance to intercept (e.g. today's HandledByClient enum, but as a separate
> callback rather than using an enumerator value).
> 
> However, thinking about this more, it seems like this won't work well for
OOPIF:
> we won't run beforeunload events in cross-origin frames... so maybe we should
> move to a model where the embedder *always* runs the beforeunload events in
> behalf of Blink? WDYT?

I'm not really sure how OOPIF works here in the current architecture so not
comenting on this.

For PlzNavigate, what you suggest can work, but we would still have to know in
Blink when we are committing a navigation so that we don't execute the
BeforeUnload event again, and we don't send the navigation to the browser.

Note that when PlzNavigate lands, I have plans to do more changes in the
FrameLoader. In particular, have two separated methods, one that handles the
renderer-initiated load until it is sent to the browser. The other would do the
navigation commit.

[dcheng, 2016-04-27 13:42:04]

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

https://codereview.chromium.org/1890493002/diff/60001/content/renderer/render...
content/renderer/render_frame_impl.cc:4870: : blink::WebNavigationPolicyIgnore;
On 2016/04/27 at 11:29:27, clamy wrote:
> On 2016/04/27 09:21:29, dcheng wrote:
> > On 2016/04/26 at 10:07:09, clamy wrote:
> > > On 2016/04/26 06:32:31, dcheng wrote:
> > > > On 2016/04/25 at 11:36:55, clamy wrote:
> > > > > On 2016/04/15 17:21:42, dcheng wrote:
> > > > > > On 2016/04/15 at 14:45:54, clamy wrote:
> > > > > > > On 2016/04/15 14:42:22, nasko (very slow Apr 8-18) wrote:
> > > > > > > > Isn't the beforeunload event already invoked before getting to
> > > > > > > > decidePolicyForNavigation? It seems a bit strange to have
content/
> > > > > > orchestrate
> > > > > > > > the beforeunload firing for renderer-initiated navigations.
Let's
> > have
> > > > > > dcheng@
> > > > > > > > weigh in on this.
> > > > > > > 
> > > > > > > No it's executed right afterwards (hence why I have to do this).
> > > > > > 
> > > > > > Hmm, we should try to figure out how to make Blink handle this
> > correctly:
> > > > after
> > > > > > all, it knows that the embedder is (probably) going to navigate it
> > > > somewhere.
> > > > > > Similar to how we currently call progressStarted() today in the
"handled
> > by
> > > > > > client case" to make sure progress state is set up correctly, we
should
> > > > probably
> > > > > > enforce that beforeunload didn't cancel the load at this time.
> > > > > 
> > > > > The problem is that the call to decidePolicyForNavigation also
triggers
> > the
> > > > navigation being sent to the browser. So if we execute BeforeUnload
> > afterwards,
> > > > BeforeUnload can no longer cancel the navigation.
> > > > > 
> > > > > If we want to keep executing BeforeUnload in the FrameLoader, then we
can
> > no
> > > > longer have decidePolicyForNavigation send the navigation to the
browser.
> > > > Instead, the FrameLoader has to ask the client whether to continue with
the
> > > > navigation or not, get a HandledByClient (whith no side effect). Then
> > execute
> > > > the BeforeUnload event, and when that's done send back the navigation to
the
> > > > client if it got a NavigationPolicy::HandledByClient in the first place.
> > > > > 
> > > > > I'm not sure which is more understandable between having
BeginNavigation
> > ask
> > > > for BeforeUnload to execute, or having decidePolicyForNavigation return
> > > > WebNavigationPolicy::HandledByClient when it's not actually handling
> > anything
> > > > until WebFrame calls another function to handle the navigation.
> > > > 
> > > > Hmm, I see the problem, but I think it's awkward to have to add a bit on
> > > > WebURLRequest to track this: from what I can tell, WebURLRequest is used
for
> > a
> > > > lot of things other than frame loads.
> > > > 
> > > > Let me think about this tonight, sorry for the delay.
> > > 
> > > The bit in WebURLRequest is not coming from this problem, it's there to
not
> > execute BeforeUnload a second time when we commit the navigation. Even if I
> > change the patch so that BeforeUnload executes from the FrameLoader, I would
> > still need a way to skip it when we commit. Now since I already need to know
> > that the WebURLRequest corresponds to a navigation commit so that we don't
send
> > it to the browser, I could probably just have a single boolean
> > isNavigationCommit that I can use to skip BeforeUnload on commits.
> > 
> > Part of me thinks that it's weird to expose internal Blink details like
this:
> > ideally, I think I would prefer to structure it like this:
> > 1) We revert to the original pre-PlzNavigate navigation policy.
> > 2) Assuming it should continue, Blink continues on its typical loading path.
> > 3) Before actually performing the navigation in Blink, give the embedder a
> > chance to intercept (e.g. today's HandledByClient enum, but as a separate
> > callback rather than using an enumerator value).
> > 
> > However, thinking about this more, it seems like this won't work well for
OOPIF:
> > we won't run beforeunload events in cross-origin frames... so maybe we
should
> > move to a model where the embedder *always* runs the beforeunload events in
> > behalf of Blink? WDYT?
> 
> I'm not really sure how OOPIF works here in the current architecture so not
comenting on this.
> 
> For PlzNavigate, what you suggest can work, but we would still have to know in
Blink when we are committing a navigation so that we don't execute the
BeforeUnload event again, and we don't send the navigation to the browser.

I think there should only be one place that runs the beforeunload events. I
believe that will be the case with either suggestion.

If we keep it wholly inside Blink, we'll have add a hook for PlzNavigate after
running the beforeunload events so content won't need to run it again even with
PlzNavigate on.

Alternatively, if we move the trigger location for beforeunload into the
embedder, then we should only have the embedder trigger it, even if PlzNavigate
is off. This will be a bit trickier, since we'll need to be careful not to touch
the RenderFrame after the beforeunload events run.

(My personal hope is that we could take advantage of approach #2 in the long run
to clean up some things for OOPIF, but I haven't thought through all the
implications yet.)

> 
> Note that when PlzNavigate lands, I have plans to do more changes in the
FrameLoader. In particular, have two separated methods, one that handles the
renderer-initiated load until it is sent to the browser. The other would do the
navigation commit.

[clamy, 2016-04-29 12:18:24]

Thanks! I have uploaded a new version where the embedder triggers BeforeUnload
in decidePolicyForNavigation, whether PlzNavigate is enabled or not.

[clamy, 2016-05-02 15:53:41]

Description was changed from

==========
PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

This CL ensures that the BeforeUnload event is always properly executed before
starting a renderer-initiated navigation when browser-side-navigation is
enabled.

BUG=576261,475027
==========

to

==========
PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

This CL ensures that the BeforeUnload event is always properly executed before
starting a renderer-initiated navigation when browser-side-navigation is
enabled.

BUG=608371,475027
==========

[dcheng, 2016-05-03 06:51:59]

Sorry for the delay on the review… almost caught up now…

https://codereview.chromium.org/1890493002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/1890493002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1391: if
(!shouldClose(navigationType == NavigationTypeReload))
I think we need to find a way to plumb through this bool: we currently use it to
distinguish between leaving/reloading the page in the beforeunload dialog.

https://codereview.chromium.org/1890493002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1890493002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1843: if (!frame()) {
Do you mind pointing me at the layout test that triggers this condition?

https://codereview.chromium.org/1890493002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebFrame.h (left):

https://codereview.chromium.org/1890493002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebFrame.h:259: virtual bool
dispatchBeforeUnloadEvent() = 0;
Thanks for moving this

https://codereview.chromium.org/1890493002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/1890493002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:92: virtual bool
dispatchBeforeUnloadEvent(bool* proceed) = 0;
Rather than using a bool out param, I'd suggest just combining the meanings into
the return val.

return false => don't navigate, bail out immediately, frame might be destroyed
return true => ok to proceed

Alternatively, we can express the state with an enum, which might make the code
clearer but isn't really needed atm.

[clamy, 2016-05-03 13:17:09]

Thanks!

https://codereview.chromium.org/1890493002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/1890493002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:1391: if
(!shouldClose(navigationType == NavigationTypeReload))
On 2016/05/03 06:51:58, dcheng wrote:
> I think we need to find a way to plumb through this bool: we currently use it
to
> distinguish between leaving/reloading the page in the beforeunload dialog.

Done. We have access to the web version of the navigationType in
RenderFrameImpl::decidePolicyForNavigation.

https://codereview.chromium.org/1890493002/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/web/WebLocalFrameImpl.cpp (right):

https://codereview.chromium.org/1890493002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/web/WebLocalFrameImpl.cpp:1843: if (!frame()) {
On 2016/05/03 06:51:58, dcheng wrote:
> Do you mind pointing me at the layout test that triggers this condition?

It's third_party/WebKit/LayoutTests/fast/events/before-unload-crash.html.

https://codereview.chromium.org/1890493002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/1890493002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:92: virtual bool
dispatchBeforeUnloadEvent(bool* proceed) = 0;
On 2016/05/03 06:51:59, dcheng wrote:
> Rather than using a bool out param, I'd suggest just combining the meanings
into
> the return val.
> 
> return false => don't navigate, bail out immediately, frame might be destroyed
> return true => ok to proceed
> 
> Alternatively, we can express the state with an enum, which might make the
code
> clearer but isn't really needed atm.

There is one case where we need to distinguish between the two: cross-site
navigations where the browser expect an IPC back with the return value of
BeforeUnload. So I went with the enum option.

[dcheng, 2016-05-03 21:34:31]

https://codereview.chromium.org/1890493002/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1890493002/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1518: // TODO(clamy): Pass the right value
for is_reload here.
Let's fix this in this CL as well: since this is somewhat web-visible, we should
try not to break it. It shouldn't be too hard to add this parameter to
FrameMsg_BeforeUnload.

https://codereview.chromium.org/1890493002/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1522:
blink::WebLocalFrame::BeforeUnloadReturnValue::FrameDestroyed) {
Let's just use weak_factory_.GetWeakPtr() to detect this condition. Then we
won't need the enum at all, and the code will look simpler. Some other things
that run script hold a |weak_self| pointer to check if |this| is still live.

https://codereview.chromium.org/1890493002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/1890493002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:97: // BeforeUnload led to the
destruction of the frame, true otherwise.
Also, this comment should be updated.

[clamy, 2016-05-04 13:49:28]

Description was changed from

==========
PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

This CL ensures that the BeforeUnload event is always properly executed before
starting a renderer-initiated navigation when browser-side-navigation is
enabled.

BUG=608371,475027
==========

to

==========
PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

This CL ensures that the BeforeUnload event is always properly executed before
starting a renderer-initiated navigation when browser-side-navigation is
enabled.

BUG=608371,475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-05-04 13:50:16]

Thanks!

https://codereview.chromium.org/1890493002/diff/160001/content/renderer/rende...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/1890493002/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1518: // TODO(clamy): Pass the right value
for is_reload here.
On 2016/05/03 21:34:30, dcheng wrote:
> Let's fix this in this CL as well: since this is somewhat web-visible, we
should
> try not to break it. It shouldn't be too hard to add this parameter to
> FrameMsg_BeforeUnload.

Done.

https://codereview.chromium.org/1890493002/diff/160001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1522:
blink::WebLocalFrame::BeforeUnloadReturnValue::FrameDestroyed) {
On 2016/05/03 21:34:30, dcheng wrote:
> Let's just use weak_factory_.GetWeakPtr() to detect this condition. Then we
> won't need the enum at all, and the code will look simpler. Some other things
> that run script hold a |weak_self| pointer to check if |this| is still live.

Done.

https://codereview.chromium.org/1890493002/diff/160001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebLocalFrame.h (right):

https://codereview.chromium.org/1890493002/diff/160001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebLocalFrame.h:97: // BeforeUnload led to the
destruction of the frame, true otherwise.
On 2016/05/03 21:34:30, dcheng wrote:
> Also, this comment should be updated.

Done.

[dcheng, 2016-05-04 18:41:35]

//third_party/WebKit LGTM

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

https://codereview.chromium.org/1890493002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1523: return;
It's probably slightly safer (and consistent with current behavior) to not check
weak_self here.

[clamy, 2016-05-09 08:26:55]

@nasko: can you PTAL at the changes in content?
Note: I'll be addressing dcheng's comment in the next patchset.

[nasko, 2016-05-09 21:56:03]

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

[nasko, 2016-05-09 21:56:04]

Mostly nits, but the test_runner change doesn't look safe. Adding lukasza@ for
guidance on that one.

https://codereview.chromium.org/1890493002/diff/200001/components/test_runner...
File components/test_runner/test_runner_for_specific_view.cc (right):

https://codereview.chromium.org/1890493002/diff/200001/components/test_runner...
components/test_runner/test_runner_for_specific_view.cc:562: return
web_view()->mainFrame()->toWebLocalFrame()->dispatchBeforeUnloadEvent(
I don't think you can make this assumption here. It is better off being an if
statement to check if the frame is local or remote. You can ping lukasza@, who
has been working on making the test runner work properly in OOPIF mode.

https://codereview.chromium.org/1890493002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1890493002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:401: // indicates whether
the navigation is a reload of the page or a different
nit: s/or a different kind of navigation/or not/

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

https://codereview.chromium.org/1890493002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1518: // Keep a WeakPtr to this
RenderFrameHost to detect if executing the
RenderFrame. The RFH is browser side.

https://codereview.chromium.org/1890493002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1519: // BeforeUnload event destriyed this
frame.
destroyed

https://codereview.chromium.org/1890493002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4867:
info.urlRequest.checkForBrowserSideNavigation())) {
I'm slightly puzzled by this check. If PlzNavagaite is not enabled, why does it
expect checkForBrowserSideNavigation() to be true?

https://codereview.chromium.org/1890493002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:4869: // BeforeUnload event destriyed this
frame.
This comment is a copy of the above, so fixes of the original comment should be
applied here too.

[Łukasz Anforowicz, 2016-05-09 22:43:23]

https://codereview.chromium.org/1890493002/diff/200001/components/test_runner...
File components/test_runner/test_runner_for_specific_view.cc (right):

https://codereview.chromium.org/1890493002/diff/200001/components/test_runner...
components/test_runner/test_runner_for_specific_view.cc:562: return
web_view()->mainFrame()->toWebLocalFrame()->dispatchBeforeUnloadEvent(
On 2016/05/09 21:56:04, nasko (slow) wrote:
> I don't think you can make this assumption here. It is better off being an if
> statement to check if the frame is local or remote. You can ping lukasza@, who
> has been working on making the test runner work properly in OOPIF mode.

nasko@ - you are correct that web_view()->mainFrame() might be a WebRemoteFrame
here.  At the same time:

1) It seems that testRunner.callShouldCloseOnWebView() is called only from a
single layout test (fast/events/onbeforeunload-focused-iframe.html) and this
particular test only involves single-origin (file:) frames.

2) There is already one existing binding (i.e. testRunner.findString aka
TestRunnerForSpecificView::FindString) that assumes that main frame is a
WebLocalFrame.

3) I don't see an easy way to allow calling these bindings from any frame
(including frames in another renderer from the main frame) - allowing this would
probably require some IPCs to forward the call to the right renderer (and this
seems like an overkill for future, hypothetical test needs)

Given above, I think the changes in the CL-under-review are almost okay - I
would just add an explicit check if web_view()->mainFrame()->isWebLocalFrame()
and do something smart if it isn't (I guess violently failing a test is best to
avoid surprising future maintainers - a CHECK(false) + a comment might
accomplish this).

[clamy, 2016-05-10 00:39:56]

Thanks!

https://codereview.chromium.org/1890493002/diff/200001/components/test_runner...
File components/test_runner/test_runner_for_specific_view.cc (right):

https://codereview.chromium.org/1890493002/diff/200001/components/test_runner...
components/test_runner/test_runner_for_specific_view.cc:562: return
web_view()->mainFrame()->toWebLocalFrame()->dispatchBeforeUnloadEvent(
On 2016/05/09 22:43:23, Łukasz Anforowicz wrote:
> On 2016/05/09 21:56:04, nasko (slow) wrote:
> > I don't think you can make this assumption here. It is better off being an
if
> > statement to check if the frame is local or remote. You can ping lukasza@,
who
> > has been working on making the test runner work properly in OOPIF mode.
> 
> nasko@ - you are correct that web_view()->mainFrame() might be a
WebRemoteFrame
> here.  At the same time:
> 
> 1) It seems that testRunner.callShouldCloseOnWebView() is called only from a
> single layout test (fast/events/onbeforeunload-focused-iframe.html) and this
> particular test only involves single-origin (file:) frames.
> 
> 2) There is already one existing binding (i.e. testRunner.findString aka
> TestRunnerForSpecificView::FindString) that assumes that main frame is a
> WebLocalFrame.
> 
> 3) I don't see an easy way to allow calling these bindings from any frame
> (including frames in another renderer from the main frame) - allowing this
would
> probably require some IPCs to forward the call to the right renderer (and this
> seems like an overkill for future, hypothetical test needs)
> 
> Given above, I think the changes in the CL-under-review are almost okay - I
> would just add an explicit check if web_view()->mainFrame()->isWebLocalFrame()
> and do something smart if it isn't (I guess violently failing a test is best
to
> avoid surprising future maintainers - a CHECK(false) + a comment might
> accomplish this).

With the current architecture, you would get a NOTREACHED() if we tried to call
dispatchBeforeUnloadEvent on a WebRemoteFrame. We could either have that or a
CHECK(false) << "This function should only be called when the main frame is a
WebLocalFrame.".

[clamy, 2016-05-10 01:58:09]

https://codereview.chromium.org/1890493002/diff/200001/components/test_runner...
File components/test_runner/test_runner_for_specific_view.cc (right):

https://codereview.chromium.org/1890493002/diff/200001/components/test_runner...
components/test_runner/test_runner_for_specific_view.cc:562: return
web_view()->mainFrame()->toWebLocalFrame()->dispatchBeforeUnloadEvent(
On 2016/05/10 00:39:56, clamy wrote:
> On 2016/05/09 22:43:23, Łukasz Anforowicz wrote:
> > On 2016/05/09 21:56:04, nasko (slow) wrote:
> > > I don't think you can make this assumption here. It is better off being an
> if
> > > statement to check if the frame is local or remote. You can ping lukasza@,
> who
> > > has been working on making the test runner work properly in OOPIF mode.
> > 
> > nasko@ - you are correct that web_view()->mainFrame() might be a
> WebRemoteFrame
> > here.  At the same time:
> > 
> > 1) It seems that testRunner.callShouldCloseOnWebView() is called only from a
> > single layout test (fast/events/onbeforeunload-focused-iframe.html) and this
> > particular test only involves single-origin (file:) frames.
> > 
> > 2) There is already one existing binding (i.e. testRunner.findString aka
> > TestRunnerForSpecificView::FindString) that assumes that main frame is a
> > WebLocalFrame.
> > 
> > 3) I don't see an easy way to allow calling these bindings from any frame
> > (including frames in another renderer from the main frame) - allowing this
> would
> > probably require some IPCs to forward the call to the right renderer (and
this
> > seems like an overkill for future, hypothetical test needs)
> > 
> > Given above, I think the changes in the CL-under-review are almost okay - I
> > would just add an explicit check if
web_view()->mainFrame()->isWebLocalFrame()
> > and do something smart if it isn't (I guess violently failing a test is best
> to
> > avoid surprising future maintainers - a CHECK(false) + a comment might
> > accomplish this).
> 
> With the current architecture, you would get a NOTREACHED() if we tried to
call
> dispatchBeforeUnloadEvent on a WebRemoteFrame. We could either have that or a
> CHECK(false) << "This function should only be called when the main frame is a
> WebLocalFrame.".

Went with CHECK(false).

https://codereview.chromium.org/1890493002/diff/200001/content/browser/frame_...
File content/browser/frame_host/render_frame_host_impl.h (right):

https://codereview.chromium.org/1890493002/diff/200001/content/browser/frame_...
content/browser/frame_host/render_frame_host_impl.h:401: // indicates whether
the navigation is a reload of the page or a different
On 2016/05/09 21:56:04, nasko (slow) wrote:
> nit: s/or a different kind of navigation/or not/

Done.

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

https://codereview.chromium.org/1890493002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1518: // Keep a WeakPtr to this
RenderFrameHost to detect if executing the
On 2016/05/09 21:56:04, nasko (slow) wrote:
> RenderFrame. The RFH is browser side.

Removed this part as per dcheng's comment.

https://codereview.chromium.org/1890493002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1519: // BeforeUnload event destriyed this
frame.
On 2016/05/09 21:56:04, nasko (slow) wrote:
> destroyed

Same.

https://codereview.chromium.org/1890493002/diff/200001/content/renderer/rende...
content/renderer/render_frame_impl.cc:1523: return;
On 2016/05/04 18:41:35, dcheng wrote:
> It's probably slightly safer (and consistent with current behavior) to not
check
> weak_self here.

Done.

[dcheng, 2016-05-10 04:55:04]

(Sorry to make your life complicated =)

https://codereview.chromium.org/1890493002/diff/240001/components/test_runner...
File components/test_runner/test_runner_for_specific_view.cc (right):

https://codereview.chromium.org/1890493002/diff/240001/components/test_runner...
components/test_runner/test_runner_for_specific_view.cc:570: CHECK(false) <<
"This function cannot be called if the main frame is not a "
Why can't we just let toWebLocalFrame() explode? In all test configurations we
care about, NOTREACHED() will trigger a DCHECK(false) anyway.

[dcheng, 2016-05-10 04:59:35]

On 2016/05/10 at 04:55:04, dcheng wrote:
> (Sorry to make your life complicated =)
> 
>
https://codereview.chromium.org/1890493002/diff/240001/components/test_runner...
> File components/test_runner/test_runner_for_specific_view.cc (right):
> 
>
https://codereview.chromium.org/1890493002/diff/240001/components/test_runner...
> components/test_runner/test_runner_for_specific_view.cc:570: CHECK(false) <<
"This function cannot be called if the main frame is not a "
> Why can't we just let toWebLocalFrame() explode? In all test configurations we
care about, NOTREACHED() will trigger a DCHECK(false) anyway.

(I would also support making toWebLocalFrame() assert when called on
WebRemoteFrame and vice versa. Basically, this check should happen at a higher
level then one function in the test runner implementation)

[clamy, 2016-05-25 16:51:21]

@nasko: ping now that you're back.

[nasko, 2016-05-25 17:24:02]

content/ LGTM, though you likely need to rebase the patch.

[clamy, 2016-05-26 09:43:27]

Description was changed from

==========
PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

This CL ensures that the BeforeUnload event is always properly executed before
starting a renderer-initiated navigation when browser-side-navigation is
enabled.

BUG=608371,475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

This CL ensures that the BeforeUnload event is always properly executed before
starting a renderer-initiated navigation when browser-side-navigation is
enabled.

BUG=608371,475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[clamy, 2016-05-26 09:43:27]

clamy@chromium.org changed reviewers:
+ blundell@chromium.org

[clamy, 2016-05-26 09:43:54]

Thanks!
@blundell: PTAL at the changes in components/

https://codereview.chromium.org/1890493002/diff/240001/components/test_runner...
File components/test_runner/test_runner_for_specific_view.cc (right):

https://codereview.chromium.org/1890493002/diff/240001/components/test_runner...
components/test_runner/test_runner_for_specific_view.cc:570: CHECK(false) <<
"This function cannot be called if the main frame is not a "
On 2016/05/10 04:55:03, dcheng wrote:
> Why can't we just let toWebLocalFrame() explode? In all test configurations we
> care about, NOTREACHED() will trigger a DCHECK(false) anyway.

I think the CHECK(false) here makes it clearer to the developper what's
happening here.

[blundell, 2016-05-26 09:50:02]

An OWNER of //components/test_runner would be a better reviewer here (in
general, prefer sending reviews for individual components to OWNERS of those
components rather than top-level //components OWNERS).

[clamy, 2016-05-26 09:56:00]

clamy@chromium.org changed reviewers:
+ rbyers@chromium.org

[clamy, 2016-05-26 09:56:25]

@rbyers: PTAL at the changes in components/test_runner

[Rick Byers, 2016-05-26 13:32:57]

On 2016/05/26 09:56:25, clamy wrote:
> @rbyers: PTAL at the changes in components/test_runner

components/test_runner/ LGTM

[clamy, 2016-05-26 13:36:33]

Thanks!

[clamy, 2016-05-26 13:36:41]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-05-26 13:36:42]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
nasko@chromium.org
Link to the patchset: https://codereview.chromium.org/1890493002/#ps260001
(title: "Rebase")

[commit-bot: I haz the power, 2016-05-26 13:36:52]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1890493002/260001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1890493002/260001

[commit-bot: I haz the power, 2016-05-26 15:14:08]

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

[commit-bot: I haz the power, 2016-05-26 15:14:10]

Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[clamy, 2016-05-30 14:58:50]

The CQ bit was checked by clamy@chromium.org

[clamy, 2016-05-30 14:58:51]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org,
nasko@chromium.org, rbyers@chromium.org
Link to the patchset: https://codereview.chromium.org/1890493002/#ps280001
(title: "Rebase + fixed issue with layout test")

[commit-bot: I haz the power, 2016-05-30 14:59:08]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1890493002/280001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1890493002/280001

[commit-bot: I haz the power, 2016-05-30 16:25:31]

Description was changed from

==========
PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

This CL ensures that the BeforeUnload event is always properly executed before
starting a renderer-initiated navigation when browser-side-navigation is
enabled.

BUG=608371,475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

This CL ensures that the BeforeUnload event is always properly executed before
starting a renderer-initiated navigation when browser-side-navigation is
enabled.

BUG=608371,475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[commit-bot: I haz the power, 2016-05-30 16:25:33]

Committed patchset #15 (id:280001)

[commit-bot: I haz the power, 2016-05-30 16:27:12]

Description was changed from

==========
PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

This CL ensures that the BeforeUnload event is always properly executed before
starting a renderer-initiated navigation when browser-side-navigation is
enabled.

BUG=608371,475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
PlzNavigate: properly execute BeforeUnload on renderer initiated navigations

This CL ensures that the BeforeUnload event is always properly executed before
starting a renderer-initiated navigation when browser-side-navigation is
enabled.

BUG=608371,475027
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/bfd6a8144791f47eefb45961819c6f3472035506
Cr-Commit-Position: refs/heads/master@{#396737}
==========

[commit-bot: I haz the power, 2016-05-30 16:27:14]

Patchset 15 (id:??) landed as
https://crrev.com/bfd6a8144791f47eefb45961819c6f3472035506
Cr-Commit-Position: refs/heads/master@{#396737}