Better distinguish didFinishLoad and didStopLoading

Better distinguish didFinishLoad and didStopLoading

Currently, they mean roughly the same thing. didFinishLoad
fires when a navigation successfully completes and no other
navigations are in progress. didStopLoading does the same
thing, but fires whether or not the navigation was successful.

This changes didFinishLoad to mean "a navigation successfully
completed", while didStopLoading still means "there are now
no navigations in progress in this frame". For each
didStartProvisionalLoad, there will be exactly one of:
didFailProvisionalLoad, didFailLoad, didFinishLoad. Prior to
this change, there would be at most one, but it could be that
a load would complete without a matching notification.

This also reverses the order of didFinishLoad and
didStopLoading. didStartLoading and didStopLoading will now
bracket zero to many individual navigation notifications,
which may be interleaved.

This requires 4 changes to consumers of these notifications:
1. http/tests/loading/progress-finished-callback.html
   asserted the old finish/stop ordering, and did nothing
   else. Remove it and its test harness logic.
2. web_navigation_api.cc assumed that didFinishLoad would not
   be sent if a new provisional navigation began before the
   finish notification. Change a DCHECK to an early exit.
3. password_autofill_agent.cc depends on stop firing before
   finish for determining whether all loading is done. Give
   it the ability to determine loading state for itself
   inside DidFinishLoad.
4. chrome/android/ incorrectly listens to the browser process
   equivalents of didStartProvisionaLoad and didFinishLoad to
   determine whether loading is in progress and start/stop
   the progress bar. Switch it to the equivalent of
   didStartLoading and didStopLoading.

BUG=539952
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect

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

[Nate Chapin, 2015-10-06 18:35:35]

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

[Nate Chapin, 2015-10-06 18:35:36]

https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1102: if (frame
!= render_frame()->GetWebFrame() && frame->isLoading()) {
This is kind of gross, and necessary because didStopLoading() is now called
after didFinishLoad(). I guess the alternative is to add this bit as a parameter
in RenderFrameObserver::DidFinishLoad, since RenderFrameImpl can probably
determine it in O(1)?

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (left):

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:303: if (m_mainResource
&& m_frame->document()->hasAppCacheManifest())
This code should have been removed when we blinked, it's irrelevant because we
never store main resources in MemoryCache.

[dcheng, 2015-10-08 05:34:35]

https://codereview.chromium.org/1381003004/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://codereview.chromium.org/1381003004/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:404: if
(navigation_state_.GetUrl(render_frame_host) != validated_url &&
This first check seems obviously related to the comment, but I'm having a hard
time understanding how the other two relate. Would it be possible to clarify the
comment here?

https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1102: if (frame
!= render_frame()->GetWebFrame() && frame->isLoading()) {
On 2015/10/06 at 18:35:36, Nate Chapin wrote:
> This is kind of gross, and necessary because didStopLoading() is now called
after didFinishLoad(). I guess the alternative is to add this bit as a parameter
in RenderFrameObserver::DidFinishLoad, since RenderFrameImpl can probably
determine it in O(1)?

"didStopLoading does the same thing [no other navigations are in progress], but
fires whether or not the navigation was successful."

With the previous semantics, was it checking for navigations across all frames
in the page, or just navigations in this frame and its descendants?

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/loading/progress-finished-callback-expected.txt
(left):

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/progress-finished-callback-expected.txt:7:
This is a test of progress finished callback ordering in relation to frame
loader callbacks. It is only useful inside the regression test tool.
It might be nice to keep this and just update the expected result: it seems nice
to have this order 'enforced' in a test somewhere.

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:296: if
(!m_mainDocumentError.isNull() || !m_frame)
How can m_frame be null now?

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:542: // The
readystatechanged or load event may have disconnected this frame.
Move this comment to 578 in the new file.

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

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:587: // Finishing the load
can detach the frame when running layout tests.
Can we fix this, maybe in a followup patch? This is the layout test runner doing
something crazy in didFinishLoad(), right?

[dcheng, 2015-10-08 05:37:41]

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

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:542: // We might have
declined to run the load event due to an imminent content-initiated navigation.
This is probably a dumb question... content-initiated: is this referring to the
embedder, e.g. //content?

[Nate Chapin, 2015-10-08 21:03:59]

https://codereview.chromium.org/1381003004/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://codereview.chromium.org/1381003004/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:404: if
(navigation_state_.GetUrl(render_frame_host) != validated_url &&
On 2015/10/08 05:34:35, dcheng wrote:
> This first check seems obviously related to the comment, but I'm having a hard
> time understanding how the other two relate. Would it be possible to clarify
the
> comment here?

Done.

https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1102: if (frame
!= render_frame()->GetWebFrame() && frame->isLoading()) {
On 2015/10/08 05:34:35, dcheng wrote:
> On 2015/10/06 at 18:35:36, Nate Chapin wrote:
> > This is kind of gross, and necessary because didStopLoading() is now called
> after didFinishLoad(). I guess the alternative is to add this bit as a
parameter
> in RenderFrameObserver::DidFinishLoad, since RenderFrameImpl can probably
> determine it in O(1)?
> 
> "didStopLoading does the same thing [no other navigations are in progress],
but
> fires whether or not the navigation was successful."
> 
> With the previous semantics, was it checking for navigations across all frames
> in the page, or just navigations in this frame and its descendants?

It was checking all frames. The message blink sends out is per-frame, but
PasswordAutofillAgent was listening to RenderViewObserver::DidStopLoading, which
is per-page.

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/http/tests/loading/progress-finished-callback-expected.txt
(left):

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/http/tests/loading/progress-finished-callback-expected.txt:7:
This is a test of progress finished callback ordering in relation to frame
loader callbacks. It is only useful inside the regression test tool.
On 2015/10/08 05:34:35, dcheng wrote:
> It might be nice to keep this and just update the expected result: it seems
nice
> to have this order 'enforced' in a test somewhere.

Translated to WebFrameTest.

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/DocumentLoader.cpp (right):

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/DocumentLoader.cpp:296: if
(!m_mainDocumentError.isNull() || !m_frame)
On 2015/10/08 05:34:35, dcheng wrote:
> How can m_frame be null now?

It could be null when the DocumentLoader is synchronously detached inside
endWriting() before this change, too. It's m_mainDocumentError.isNull() that
changed.

Before:
* iframe completes, sends didFinishLoad, fires parent's load event.
* Parent's load event detached iframe, which calls DocumentLoader::stopLoading()
* DocumentLoader::finishedLoading() hasn't yet set m_state to MainResourceDone,
so it cancels the main resource load, which makes m_mainDocumentError non-null
as well as nulling m_frame and m_applicationCacheHost.
* Successful early exit.

After:
* iframe completes, sends didFinishLoad, ***sets DocumentLoader::m_state to
SentDidFinishLoad***, fires parent's load event.
* Parent's load event detached iframe, which calls DocumentLoader::stopLoading()
* DocumentLoader::isLoading() returns false because m_state is not in the right
range, main resource is not cancelled, m_mainDocumentError remains null.
* m_applicationCacheHost and m_frame were still both nulled, so we crash when we
unconditionally derefernce m_applicationCacheHost.

Moving m_applicationCacheHost->finishedLoadingMainResource() before
endWriting(), so that it is guaranteed non-null. The rest of the function is
benign when detached.

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameLoader.cpp (left):

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:542: // The
readystatechanged or load event may have disconnected this frame.
On 2015/10/08 05:34:35, dcheng wrote:
> Move this comment to 578 in the new file.

Done.

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

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:542: // We might have
declined to run the load event due to an imminent content-initiated navigation.
On 2015/10/08 05:37:40, dcheng wrote:
> This is probably a dumb question... content-initiated: is this referring to
the
> embedder, e.g. //content?

In this case, it means blink. I think it's the same terminology as in
RenderFrameImpl: browser-initiated is from the browser process,
content-initiated is from blink. That naming is unfortunate.

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:587: // Finishing the load
can detach the frame when running layout tests.
On 2015/10/08 05:34:35, dcheng wrote:
> Can we fix this, maybe in a followup patch? This is the layout test runner
doing
> something crazy in didFinishLoad(), right?

I think the craziness is tearing down the test. I guess that could/should be
done async?

[dcheng, 2015-10-09 18:34:01]

https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1102: if (frame
!= render_frame()->GetWebFrame() && frame->isLoading()) {
On 2015/10/08 at 21:03:59, Nate Chapin wrote:
> On 2015/10/08 05:34:35, dcheng wrote:
> > On 2015/10/06 at 18:35:36, Nate Chapin wrote:
> > > This is kind of gross, and necessary because didStopLoading() is now
called
> > after didFinishLoad(). I guess the alternative is to add this bit as a
parameter
> > in RenderFrameObserver::DidFinishLoad, since RenderFrameImpl can probably
> > determine it in O(1)?
> > 
> > "didStopLoading does the same thing [no other navigations are in progress],
but
> > fires whether or not the navigation was successful."
> > 
> > With the previous semantics, was it checking for navigations across all
frames
> > in the page, or just navigations in this frame and its descendants?
> 
> It was checking all frames. The message blink sends out is per-frame, but
PasswordAutofillAgent was listening to RenderViewObserver::DidStopLoading, which
is per-page.

Hmm... but when this is called, the entire page might not be finished loading
right? So this callback might never run in that case.

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

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:587: // Finishing the load
can detach the frame when running layout tests.
On 2015/10/08 at 21:03:59, Nate Chapin wrote:
> On 2015/10/08 05:34:35, dcheng wrote:
> > Can we fix this, maybe in a followup patch? This is the layout test runner
doing
> > something crazy in didFinishLoad(), right?
> 
> I think the craziness is tearing down the test. I guess that could/should be
done async?

Yeah, I think it'd make sense to do it async... when possible, I think we should
move in the direction of allowing less insanity in the WebFrameClient callbacks.

[Nate Chapin, 2015-10-09 18:39:44]

https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1102: if (frame
!= render_frame()->GetWebFrame() && frame->isLoading()) {
On 2015/10/09 18:34:01, dcheng wrote:
> On 2015/10/08 at 21:03:59, Nate Chapin wrote:
> > On 2015/10/08 05:34:35, dcheng wrote:
> > > On 2015/10/06 at 18:35:36, Nate Chapin wrote:
> > > > This is kind of gross, and necessary because didStopLoading() is now
> called
> > > after didFinishLoad(). I guess the alternative is to add this bit as a
> parameter
> > > in RenderFrameObserver::DidFinishLoad, since RenderFrameImpl can probably
> > > determine it in O(1)?
> > > 
> > > "didStopLoading does the same thing [no other navigations are in
progress],
> but
> > > fires whether or not the navigation was successful."
> > > 
> > > With the previous semantics, was it checking for navigations across all
> frames
> > > in the page, or just navigations in this frame and its descendants?
> > 
> > It was checking all frames. The message blink sends out is per-frame, but
> PasswordAutofillAgent was listening to RenderViewObserver::DidStopLoading,
which
> is per-page.
> 
> Hmm... but when this is called, the entire page might not be finished loading
> right? So this callback might never run in that case.

This callback is called for each DidFinishLoad. is_last_load should be true if
this will be the last DidFinishLoad in this page (i.e., if this frame is the
only one where isLoading() is true. So it will only not get called if the last
frame fails, which is no change from the status quo.

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

https://codereview.chromium.org/1381003004/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameLoader.cpp:587: // Finishing the load
can detach the frame when running layout tests.
On 2015/10/09 18:34:01, dcheng wrote:
> On 2015/10/08 at 21:03:59, Nate Chapin wrote:
> > On 2015/10/08 05:34:35, dcheng wrote:
> > > Can we fix this, maybe in a followup patch? This is the layout test runner
> doing
> > > something crazy in didFinishLoad(), right?
> > 
> > I think the craziness is tearing down the test. I guess that could/should be
> done async?
> 
> Yeah, I think it'd make sense to do it async... when possible, I think we
should
> move in the direction of allowing less insanity in the WebFrameClient
callbacks.

Agreed.

[dcheng, 2015-10-09 18:45:26]

https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
File components/autofill/content/renderer/password_autofill_agent.cc (right):

https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
components/autofill/content/renderer/password_autofill_agent.cc:1102: if (frame
!= render_frame()->GetWebFrame() && frame->isLoading()) {
On 2015/10/09 at 18:39:44, Nate Chapin wrote:
> On 2015/10/09 18:34:01, dcheng wrote:
> > On 2015/10/08 at 21:03:59, Nate Chapin wrote:
> > > On 2015/10/08 05:34:35, dcheng wrote:
> > > > On 2015/10/06 at 18:35:36, Nate Chapin wrote:
> > > > > This is kind of gross, and necessary because didStopLoading() is now
> > called
> > > > after didFinishLoad(). I guess the alternative is to add this bit as a
> > parameter
> > > > in RenderFrameObserver::DidFinishLoad, since RenderFrameImpl can
probably
> > > > determine it in O(1)?
> > > > 
> > > > "didStopLoading does the same thing [no other navigations are in
progress],
> > but
> > > > fires whether or not the navigation was successful."
> > > > 
> > > > With the previous semantics, was it checking for navigations across all
> > frames
> > > > in the page, or just navigations in this frame and its descendants?
> > > 
> > > It was checking all frames. The message blink sends out is per-frame, but
> > PasswordAutofillAgent was listening to RenderViewObserver::DidStopLoading,
which
> > is per-page.
> > 
> > Hmm... but when this is called, the entire page might not be finished
loading
> > right? So this callback might never run in that case.
> 
> This callback is called for each DidFinishLoad. is_last_load should be true if
this will be the last DidFinishLoad in this page (i.e., if this frame is the
only one where isLoading() is true. So it will only not get called if the last
frame fails, which is no change from the status quo.

Hmm. I'm thinking of the OOPI case: subframe renderers will never send the IPC
message in that case, right?

[Nate Chapin, 2015-10-09 18:52:32]

On 2015/10/09 18:45:26, dcheng wrote:
>
https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
> File components/autofill/content/renderer/password_autofill_agent.cc (right):
> 
>
https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
> components/autofill/content/renderer/password_autofill_agent.cc:1102: if
(frame
> != render_frame()->GetWebFrame() && frame->isLoading()) {
> On 2015/10/09 at 18:39:44, Nate Chapin wrote:
> > On 2015/10/09 18:34:01, dcheng wrote:
> > > On 2015/10/08 at 21:03:59, Nate Chapin wrote:
> > > > On 2015/10/08 05:34:35, dcheng wrote:
> > > > > On 2015/10/06 at 18:35:36, Nate Chapin wrote:
> > > > > > This is kind of gross, and necessary because didStopLoading() is now
> > > called
> > > > > after didFinishLoad(). I guess the alternative is to add this bit as a
> > > parameter
> > > > > in RenderFrameObserver::DidFinishLoad, since RenderFrameImpl can
> probably
> > > > > determine it in O(1)?
> > > > > 
> > > > > "didStopLoading does the same thing [no other navigations are in
> progress],
> > > but
> > > > > fires whether or not the navigation was successful."
> > > > > 
> > > > > With the previous semantics, was it checking for navigations across
all
> > > frames
> > > > > in the page, or just navigations in this frame and its descendants?
> > > > 
> > > > It was checking all frames. The message blink sends out is per-frame,
but
> > > PasswordAutofillAgent was listening to RenderViewObserver::DidStopLoading,
> which
> > > is per-page.
> > > 
> > > Hmm... but when this is called, the entire page might not be finished
> loading
> > > right? So this callback might never run in that case.
> > 
> > This callback is called for each DidFinishLoad. is_last_load should be true
if
> this will be the last DidFinishLoad in this page (i.e., if this frame is the
> only one where isLoading() is true. So it will only not get called if the last
> frame fails, which is no change from the status quo.
> 
> Hmm. I'm thinking of the OOPI case: subframe renderers will never send the IPC
> message in that case, right?

It might be racy, depending on when the Frame::isLoading() bit gets propagated
to RemotFrames? The longer term solution is to track this browser-side, but the
current layering makes that non-trivial.

[dcheng, 2015-10-09 18:54:18]

On 2015/10/09 at 18:52:32, japhet wrote:
> On 2015/10/09 18:45:26, dcheng wrote:
> >
https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
> > File components/autofill/content/renderer/password_autofill_agent.cc
(right):
> > 
> >
https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
> > components/autofill/content/renderer/password_autofill_agent.cc:1102: if
(frame
> > != render_frame()->GetWebFrame() && frame->isLoading()) {
> > On 2015/10/09 at 18:39:44, Nate Chapin wrote:
> > > On 2015/10/09 18:34:01, dcheng wrote:
> > > > On 2015/10/08 at 21:03:59, Nate Chapin wrote:
> > > > > On 2015/10/08 05:34:35, dcheng wrote:
> > > > > > On 2015/10/06 at 18:35:36, Nate Chapin wrote:
> > > > > > > This is kind of gross, and necessary because didStopLoading() is
now
> > > > called
> > > > > > after didFinishLoad(). I guess the alternative is to add this bit as
a
> > > > parameter
> > > > > > in RenderFrameObserver::DidFinishLoad, since RenderFrameImpl can
> > probably
> > > > > > determine it in O(1)?
> > > > > > 
> > > > > > "didStopLoading does the same thing [no other navigations are in
> > progress],
> > > > but
> > > > > > fires whether or not the navigation was successful."
> > > > > > 
> > > > > > With the previous semantics, was it checking for navigations across
all
> > > > frames
> > > > > > in the page, or just navigations in this frame and its descendants?
> > > > > 
> > > > > It was checking all frames. The message blink sends out is per-frame,
but
> > > > PasswordAutofillAgent was listening to
RenderViewObserver::DidStopLoading,
> > which
> > > > is per-page.
> > > > 
> > > > Hmm... but when this is called, the entire page might not be finished
> > loading
> > > > right? So this callback might never run in that case.
> > > 
> > > This callback is called for each DidFinishLoad. is_last_load should be
true if
> > this will be the last DidFinishLoad in this page (i.e., if this frame is the
> > only one where isLoading() is true. So it will only not get called if the
last
> > frame fails, which is no change from the status quo.
> > 
> > Hmm. I'm thinking of the OOPI case: subframe renderers will never send the
IPC
> > message in that case, right?
> 
> It might be racy, depending on when the Frame::isLoading() bit gets propagated
to RemotFrames? The longer term solution is to track this browser-side, but the
current layering makes that non-trivial.

OK. This is pretty horrendous, but hopefully updating this feature for OOPI will
make it less so.

LGTM

[Nate Chapin, 2015-10-09 18:56:51]

On 2015/10/09 18:54:18, dcheng wrote:
> On 2015/10/09 at 18:52:32, japhet wrote:
> > On 2015/10/09 18:45:26, dcheng wrote:
> > >
>
https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
> > > File components/autofill/content/renderer/password_autofill_agent.cc
> (right):
> > > 
> > >
>
https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
> > > components/autofill/content/renderer/password_autofill_agent.cc:1102: if
> (frame
> > > != render_frame()->GetWebFrame() && frame->isLoading()) {
> > > On 2015/10/09 at 18:39:44, Nate Chapin wrote:
> > > > On 2015/10/09 18:34:01, dcheng wrote:
> > > > > On 2015/10/08 at 21:03:59, Nate Chapin wrote:
> > > > > > On 2015/10/08 05:34:35, dcheng wrote:
> > > > > > > On 2015/10/06 at 18:35:36, Nate Chapin wrote:
> > > > > > > > This is kind of gross, and necessary because didStopLoading() is
> now
> > > > > called
> > > > > > > after didFinishLoad(). I guess the alternative is to add this bit
as
> a
> > > > > parameter
> > > > > > > in RenderFrameObserver::DidFinishLoad, since RenderFrameImpl can
> > > probably
> > > > > > > determine it in O(1)?
> > > > > > > 
> > > > > > > "didStopLoading does the same thing [no other navigations are in
> > > progress],
> > > > > but
> > > > > > > fires whether or not the navigation was successful."
> > > > > > > 
> > > > > > > With the previous semantics, was it checking for navigations
across
> all
> > > > > frames
> > > > > > > in the page, or just navigations in this frame and its
descendants?
> > > > > > 
> > > > > > It was checking all frames. The message blink sends out is
per-frame,
> but
> > > > > PasswordAutofillAgent was listening to
> RenderViewObserver::DidStopLoading,
> > > which
> > > > > is per-page.
> > > > > 
> > > > > Hmm... but when this is called, the entire page might not be finished
> > > loading
> > > > > right? So this callback might never run in that case.
> > > > 
> > > > This callback is called for each DidFinishLoad. is_last_load should be
> true if
> > > this will be the last DidFinishLoad in this page (i.e., if this frame is
the
> > > only one where isLoading() is true. So it will only not get called if the
> last
> > > frame fails, which is no change from the status quo.
> > > 
> > > Hmm. I'm thinking of the OOPI case: subframe renderers will never send the
> IPC
> > > message in that case, right?
> > 
> > It might be racy, depending on when the Frame::isLoading() bit gets
propagated
> to RemotFrames? The longer term solution is to track this browser-side, but
the
> current layering makes that non-trivial.
> 
> OK. This is pretty horrendous, but hopefully updating this feature for OOPI
will
> make it less so.
> 
> LGTM

Would it feel less horrendous if RenderFrameObserver::DidFinishLoad included the
bit as parameter, or is it just the OOPI-antagonism of the current
implementation?

[dcheng, 2015-10-09 18:58:10]

On 2015/10/09 at 18:56:51, japhet wrote:
> On 2015/10/09 18:54:18, dcheng wrote:
> > On 2015/10/09 at 18:52:32, japhet wrote:
> > > On 2015/10/09 18:45:26, dcheng wrote:
> > > >
> >
https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
> > > > File components/autofill/content/renderer/password_autofill_agent.cc
> > (right):
> > > > 
> > > >
> >
https://codereview.chromium.org/1381003004/diff/80001/components/autofill/con...
> > > > components/autofill/content/renderer/password_autofill_agent.cc:1102: if
> > (frame
> > > > != render_frame()->GetWebFrame() && frame->isLoading()) {
> > > > On 2015/10/09 at 18:39:44, Nate Chapin wrote:
> > > > > On 2015/10/09 18:34:01, dcheng wrote:
> > > > > > On 2015/10/08 at 21:03:59, Nate Chapin wrote:
> > > > > > > On 2015/10/08 05:34:35, dcheng wrote:
> > > > > > > > On 2015/10/06 at 18:35:36, Nate Chapin wrote:
> > > > > > > > > This is kind of gross, and necessary because didStopLoading()
is
> > now
> > > > > > called
> > > > > > > > after didFinishLoad(). I guess the alternative is to add this
bit as
> > a
> > > > > > parameter
> > > > > > > > in RenderFrameObserver::DidFinishLoad, since RenderFrameImpl can
> > > > probably
> > > > > > > > determine it in O(1)?
> > > > > > > > 
> > > > > > > > "didStopLoading does the same thing [no other navigations are in
> > > > progress],
> > > > > > but
> > > > > > > > fires whether or not the navigation was successful."
> > > > > > > > 
> > > > > > > > With the previous semantics, was it checking for navigations
across
> > all
> > > > > > frames
> > > > > > > > in the page, or just navigations in this frame and its
descendants?
> > > > > > > 
> > > > > > > It was checking all frames. The message blink sends out is
per-frame,
> > but
> > > > > > PasswordAutofillAgent was listening to
> > RenderViewObserver::DidStopLoading,
> > > > which
> > > > > > is per-page.
> > > > > > 
> > > > > > Hmm... but when this is called, the entire page might not be
finished
> > > > loading
> > > > > > right? So this callback might never run in that case.
> > > > > 
> > > > > This callback is called for each DidFinishLoad. is_last_load should be
> > true if
> > > > this will be the last DidFinishLoad in this page (i.e., if this frame is
the
> > > > only one where isLoading() is true. So it will only not get called if
the
> > last
> > > > frame fails, which is no change from the status quo.
> > > > 
> > > > Hmm. I'm thinking of the OOPI case: subframe renderers will never send
the
> > IPC
> > > > message in that case, right?
> > > 
> > > It might be racy, depending on when the Frame::isLoading() bit gets
propagated
> > to RemotFrames? The longer term solution is to track this browser-side, but
the
> > current layering makes that non-trivial.
> > 
> > OK. This is pretty horrendous, but hopefully updating this feature for OOPI
will
> > make it less so.
> > 
> > LGTM
> 
> Would it feel less horrendous if RenderFrameObserver::DidFinishLoad included
the bit as parameter, or is it just the OOPI-antagonism of the current
implementation?

Probably not =)

[Nate Chapin, 2015-10-09 19:39:45]

The CQ bit was checked by japhet@chromium.org

[commit-bot: I haz the power, 2015-10-09 19:41:27]

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

[commit-bot: I haz the power, 2015-10-09 19:50:30]

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

[commit-bot: I haz the power, 2015-10-09 19:50:31]

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

[Nate Chapin, 2015-10-09 21:01:32]

japhet@chromium.org changed reviewers:
+ dpranke@chromium.org, isherman@chromium.org, nasko@chromium.org

[Nate Chapin, 2015-10-09 21:01:32]

dpranke: Please review components/test_runner/ changes

isherman: Please review components/autofill/ changes

nasko: Please review chrome/browser/extensions/api/web_navigation/ changes

[Dirk Pranke, 2015-10-09 21:06:05]

lgtm

[Ilya Sherman, 2015-10-10 02:40:48]

autofill lgtm, thanks.

[nasko, 2015-10-12 23:48:24]

https://codereview.chromium.org/1381003004/diff/120001/chrome/browser/extensi...
File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc (right):

https://codereview.chromium.org/1381003004/diff/120001/chrome/browser/extensi...
chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:409: return;
Does your change imply that a DidFailLoad will be called if we start a new
provisional load before DidFinishLoad is called? Or would we call DidFailLoad
only when the provisional load is ready for commit?
I'm trying to understand the exact sequence of events that was hitting this
CHECK and how it will behave with your changes.

[Nate Chapin, 2015-10-13 18:30:36]

On 2015/10/12 23:48:24, nasko (slow to review) wrote:
>
https://codereview.chromium.org/1381003004/diff/120001/chrome/browser/extensi...
> File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
(right):
> 
>
https://codereview.chromium.org/1381003004/diff/120001/chrome/browser/extensi...
> chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:409:
return;
> Does your change imply that a DidFailLoad will be called if we start a new
> provisional load before DidFinishLoad is called? Or would we call DidFailLoad
> only when the provisional load is ready for commit?
> I'm trying to understand the exact sequence of events that was hitting this
> CHECK and how it will behave with your changes.

The latter.

If a provisional load commits and the previous committed navigation is not yet
done, then the previous committed navigation will be cancelled and send a
DidFailLoad. Nothing should change at provisional load start time.

[nasko, 2015-10-13 20:59:38]

On 2015/10/13 18:30:36, Nate Chapin wrote:
> On 2015/10/12 23:48:24, nasko (slow to review) wrote:
> >
>
https://codereview.chromium.org/1381003004/diff/120001/chrome/browser/extensi...
> > File chrome/browser/extensions/api/web_navigation/web_navigation_api.cc
> (right):
> > 
> >
>
https://codereview.chromium.org/1381003004/diff/120001/chrome/browser/extensi...
> > chrome/browser/extensions/api/web_navigation/web_navigation_api.cc:409:
> return;
> > Does your change imply that a DidFailLoad will be called if we start a new
> > provisional load before DidFinishLoad is called? Or would we call
DidFailLoad
> > only when the provisional load is ready for commit?
> > I'm trying to understand the exact sequence of events that was hitting this
> > CHECK and how it will behave with your changes.
> 
> The latter.
> 
> If a provisional load commits and the previous committed navigation is not yet
> done, then the previous committed navigation will be cancelled and send a
> DidFailLoad. Nothing should change at provisional load start time.

Awesome! LGTM

[Nate Chapin, 2015-10-13 21:07:19]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-10-13 21:07:20]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, isherman@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1381003004/#ps120001
(title: "Fix WebFrameTest.CallbackOrdering race")

[commit-bot: I haz the power, 2015-10-13 21:11:34]

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

[commit-bot: I haz the power, 2015-10-14 00:00:05]

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

[commit-bot: I haz the power, 2015-10-14 00:00:06]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[Nate Chapin, 2015-10-22 20:12:55]

Description was changed from

==========
Better distinguish didFinishLoad and didStopLoading

Currently, they mean roughly the same thing. didFinishLoad
fires when a navigation successfully completes and no other
navigations are in progress. didStopLoading does the same
thing, but fires whether or not the navigation was successful.

This changes didFinishLoad to mean "a navigation successfully
completed", while didStopLoading still means "there are now
no navigations in progress in this frame". For each
didStartProvisionalLoad, there will be exactly one of:
didFailProvisionalLoad, didFailLoad, didFinishLoad. Prior to
this change, there would be at most one, but it could be that
a load would complete without a matching notification.

This also reverses the order of didFinishLoad and
didStopLoading. didStartLoading and didStopLoading will now
bracket zero to many individual navigation notifications,
which may be interleaved.

This requires 3 changes to consumers of these notifications:
1. http/tests/loading/progress-finished-callback.html
   asserted the old finish/stop ordering, and did nothing
   else. Remove it and its test harness logic.
2. web_navigation_api.cc assumed that didFinishLoad would not
   be sent if a new provisional navigation began before the
   finish notification. Change a DCHECK to an early exit.
3. password_autofill_agent.cc depends on stop firing before
   finish for determining whether all loading is done. Give
   it the ability to determine loading state for itself
   inside DidFinishLoad.

BUG=539952
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect
==========

to

==========
Better distinguish didFinishLoad and didStopLoading

Currently, they mean roughly the same thing. didFinishLoad
fires when a navigation successfully completes and no other
navigations are in progress. didStopLoading does the same
thing, but fires whether or not the navigation was successful.

This changes didFinishLoad to mean "a navigation successfully
completed", while didStopLoading still means "there are now
no navigations in progress in this frame". For each
didStartProvisionalLoad, there will be exactly one of:
didFailProvisionalLoad, didFailLoad, didFinishLoad. Prior to
this change, there would be at most one, but it could be that
a load would complete without a matching notification.

This also reverses the order of didFinishLoad and
didStopLoading. didStartLoading and didStopLoading will now
bracket zero to many individual navigation notifications,
which may be interleaved.

This requires 4 changes to consumers of these notifications:
1. http/tests/loading/progress-finished-callback.html
   asserted the old finish/stop ordering, and did nothing
   else. Remove it and its test harness logic.
2. web_navigation_api.cc assumed that didFinishLoad would not
   be sent if a new provisional navigation began before the
   finish notification. Change a DCHECK to an early exit.
3. password_autofill_agent.cc depends on stop firing before
   finish for determining whether all loading is done. Give
   it the ability to determine loading state for itself
   inside DidFinishLoad.
4. chrome/android/ incorrectly listens to the browser process
   equivalents of didStartProvisionaLoad and didFinishLoad to
   determine whether loading is in progress and start/stop
   the progress bar. Switch it to the equivalent of
   didStartLoading and didStopLoading.

BUG=539952
CQ_EXTRA_TRYBOTS=tryserver.chromium.perf:linux_perf_bisect;tryserver.chromium.perf:mac_10_10_perf_bisect;tryserver.chromium.perf:win_perf_bisect;tryserver.chromium.perf:android_nexus5_perf_bisect
==========

[Nate Chapin, 2015-10-22 21:21:25]

japhet@chromium.org changed reviewers:
+ nednguyen@google.com

[Nate Chapin, 2015-10-22 21:21:26]

nednguyen: Please review the logging addition in tools/perf/measurements

tedchoc: Please review the chrome/android/ changes. I added context to the patch
description, so hopefully it makes sense :)

[Nate Chapin, 2015-10-22 21:21:50]

japhet@chromium.org changed reviewers:
+ tedchoc@chromium.org

[Nate Chapin, 2015-10-22 21:21:50]

tedchoc: Please review the chrome/android/ changes. I added context to the patch
description, so hopefully it makes sense :)

[nednguyen, 2015-10-22 21:27:32]

On 2015/10/22 21:21:50, Nate Chapin wrote:
> tedchoc: Please review the chrome/android/ changes. I added context to the
patch
> description, so hopefully it makes sense :)

lgtm, sorry for the trouble in figuring out the root cause & thanks for adding
the log

[Nate Chapin, 2015-10-27 22:37:19]

tedchoc: friendly ping :)

[Ted C, 2015-10-27 23:07:20]

https://codereview.chromium.org/1381003004/diff/140001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1381003004/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1382: protected
void onLoadStarted() {
javadoc non-private methods

https://codereview.chromium.org/1381003004/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/1381003004/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:313:
updateButtonStatus();
Based on your comments on the change, I think all of this should
go into onLoadStarted.

Also, reading the description in TabObserver:

"""
Called when the WebContents starts loading. Different from
{@link #onPageLoadStarted(Tab, String)}, if the user is navigated to a different
url while
staying in the same html document, {@link #onLoadStarted(Tab)} will be called,
while
{@link #onPageLoadStarted(Tab, String)} will not.
@param tab The notifying {@link Tab}.
"""

So "if" that comment is still valid, then onLoadStarted will be
called for in page navigations while onPageLoadStarted will not.
So that will likely result in undesired flashing of the progress bar.

The progress bar logic is different from the throbber on desktop as
we want it shown at most one time per page load.

For clarity (and my own sanity), the current tab observer methods that
are being listened to for showing the progress bar:

TabObserver#onPageLoadStarted == DidStartProvisionalLoadForFrame
    (where isMainFrame == true)

TabObserver#onPageLoadFinished == DidFinishLoad
    (where isMainFrame == true)

TabObserver#onPageLoadFailed == DidFailProvisionalLoad || DidFailLoad
    (where isMainFrame == true)

So, I'm curious how onLoadStarted and onLoadStopped correspond, as they
both seem to be driven off of WebContentsDelegate::LoadingStateChanged.
But it looks like we do not propagate the to_different_document bool up
to java which might get the behaviors closer together.

[Nate Chapin, 2015-10-28 20:57:39]

https://codereview.chromium.org/1381003004/diff/140001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1381003004/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1382: protected
void onLoadStarted() {
On 2015/10/27 23:07:19, Ted C wrote:
> javadoc non-private methods

Done.

https://codereview.chromium.org/1381003004/diff/140001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/1381003004/diff/140001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:313:
updateButtonStatus();
On 2015/10/27 23:07:20, Ted C wrote:
> Based on your comments on the change, I think all of this should
> go into onLoadStarted.
> 
> Also, reading the description in TabObserver:
> 
> """
> Called when the WebContents starts loading. Different from
> {@link #onPageLoadStarted(Tab, String)}, if the user is navigated to a
different
> url while
> staying in the same html document, {@link #onLoadStarted(Tab)} will be called,
> while
> {@link #onPageLoadStarted(Tab, String)} will not.
> @param tab The notifying {@link Tab}.
> """
> 
> So "if" that comment is still valid, then onLoadStarted will be
> called for in page navigations while onPageLoadStarted will not.
> So that will likely result in undesired flashing of the progress bar.
> 
> The progress bar logic is different from the throbber on desktop as
> we want it shown at most one time per page load.
> 
> For clarity (and my own sanity), the current tab observer methods that
> are being listened to for showing the progress bar:
> 
> TabObserver#onPageLoadStarted == DidStartProvisionalLoadForFrame
>     (where isMainFrame == true)
> 
> TabObserver#onPageLoadFinished == DidFinishLoad
>     (where isMainFrame == true)
> 
> TabObserver#onPageLoadFailed == DidFailProvisionalLoad || DidFailLoad
>     (where isMainFrame == true)
> 
> So, I'm curious how onLoadStarted and onLoadStopped correspond, as they
> both seem to be driven off of WebContentsDelegate::LoadingStateChanged.
> But it looks like we do not propagate the to_different_document bool up
> to java which might get the behaviors closer together.

Done, toDifferentDocument propagated.

[Ted C, 2015-10-28 23:41:02]

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
(right):

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:137:
if (toDifferentDocument) mProgressObserver.onProgressBarStarted();
this wasn't guarded before by toDifferentDocument, so I'm hesitant to say you
should here.

And if we do guard on it, I think we need to do the same in onLoadStopped below.
 Otherwise you'll get mismatched calls.

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChrome.java
(right):

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChrome.java:258:
if (toDifferentDocument) tabLoadStarted(tab.getId(), tab.isIncognito());
same comment

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1402:
mIsLoading = true;
To keep the logic the same as before, I think you want to guard mIsLoading with
toDifferentDocument (and the same in onLoadStopped).

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:353:
public void onLoadStopped(Tab tab) {
I think you'll want the same early exit here with access to toDifferentDocument

[Nate Chapin, 2015-10-30 20:41:34]

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java
(right):

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/bottombar/OverlayPanelContent.java:137:
if (toDifferentDocument) mProgressObserver.onProgressBarStarted();
On 2015/10/28 23:41:02, Ted C wrote:
> this wasn't guarded before by toDifferentDocument, so I'm hesitant to say you
> should here.
> 
> And if we do guard on it, I think we need to do the same in onLoadStopped
below.
>  Otherwise you'll get mismatched calls.

Yeah, I saw "onProgressBarStarted" and went into auto-pilot. if() dropped.

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChrome.java
(right):

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/compositor/layouts/LayoutManagerChrome.java:258:
if (toDifferentDocument) tabLoadStarted(tab.getId(), tab.isIncognito());
On 2015/10/28 23:41:02, Ted C wrote:
> same comment

Done.

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1402:
mIsLoading = true;
On 2015/10/28 23:41:02, Ted C wrote:
> To keep the logic the same as before, I think you want to guard mIsLoading
with
> toDifferentDocument (and the same in onLoadStopped).

Guarded here. I don't think onLoadStopped needs the guard though, since if this
is a same-document navigation, mIsLoading should be guaranteed false when
onLoadStopped is called. Moved mIsLoading = false in onLoadStopped to after the
observer calls, so that ToolbarManager can see the difference between same and
different document navigations.

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
File
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java
(right):

https://codereview.chromium.org/1381003004/diff/160001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/toolbar/ToolbarManager.java:353:
public void onLoadStopped(Tab tab) {
On 2015/10/28 23:41:02, Ted C wrote:
> I think you'll want the same early exit here with access to
toDifferentDocument

toDifferentDocument isn't actually plumbed from the renderer process. The value
on LoadingStateChanged is always true for load stops. Checking tab.isLoading()
instead.

[Ted C, 2015-11-02 21:08:47]

https://codereview.chromium.org/1381003004/diff/200001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1381003004/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1410:
mIsLoading = false;
I think you'll have to synthesize the toDifferentDocument here (i.e.
mIsLoading).

There are places that check Tab#isLoading() as a result of these onLoadStopped
calls.  i.e. ToolbarManager#updateButtonStatus checks isLoading() and that will
now be incorrect.

[Nate Chapin, 2015-11-02 21:49:25]

The CQ bit was checked by japhet@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-11-02 21:49:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1381003004/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1381003004/220001

[commit-bot: I haz the power, 2015-11-02 22:11:16]

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

[commit-bot: I haz the power, 2015-11-02 22:11:17]

Dry run: Try jobs failed on following builders:
  android_chromium_gn_compile_dbg on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)

[Nate Chapin, 2015-11-02 23:13:19]

https://codereview.chromium.org/1381003004/diff/200001/chrome/android/java/sr...
File chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java (right):

https://codereview.chromium.org/1381003004/diff/200001/chrome/android/java/sr...
chrome/android/java/src/org/chromium/chrome/browser/tab/Tab.java:1410:
mIsLoading = false;
On 2015/11/02 21:08:47, Ted C wrote:
> I think you'll have to synthesize the toDifferentDocument here (i.e.
> mIsLoading).
> 
> There are places that check Tab#isLoading() as a result of these onLoadStopped
> calls.  i.e. ToolbarManager#updateButtonStatus checks isLoading() and that
will
> now be incorrect.

Done. PTAL.

[Ted C, 2015-11-02 23:18:29]

lgtm

[Nate Chapin, 2015-11-02 23:39:04]

The CQ bit was checked by japhet@chromium.org

[Nate Chapin, 2015-11-02 23:39:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, isherman@chromium.org, dcheng@chromium.org,
nasko@chromium.org, nednguyen@google.com
Link to the patchset: https://codereview.chromium.org/1381003004/#ps240001
(title: " ")

[commit-bot: I haz the power, 2015-11-02 23:40:21]

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

[commit-bot: I haz the power, 2015-11-03 02:41:33]

Committed patchset #13 (id:240001)

[commit-bot: I haz the power, 2015-11-03 02:42:49]

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