Remove WebContentsObserver::DidCommitNavigation

Remove WebContentsObserver::DidCommitNavigation

This CL removes the WebContentsObserver::DidCommitNavigation. On navigation
commit, the WebContentsObserver::DidFinishNavigation will be called, making
clear that the NavigationHandle tracks a navigation from start to finish. Note
that if the navigation committed, the document load will still be ongoing after
DidFinishNavigation is called.

BUG=504347
Committed: https://crrev.com/b876f0a7041ac13322c2c8e8f53589871d4e8b63
Cr-Commit-Position: refs/heads/master@{#350637}

[clamy, 2015-09-17 23:58:38]

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

[clamy, 2015-09-18 00:00:34]

clamy@chromium.org changed reviewers:
+ carlosk@chromium.org, fdegans@chromium.org

[clamy, 2015-09-18 00:00:35]

@nasko, creis: PTAL

This is the change of interface in NavigationHandle that we discussed.

@carlosk, fdegans: FYI

[nasko, 2015-09-18 16:42:16]

Awesome! Few nits here and there.

https://codereview.chromium.org/1350673003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1350673003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:207:
render_frame_host->navigation_handle()->ReadyToCommitNavigation(
Why here instead of right before CommitNavigation call in OnResponseStarted? It
isn't immediately clear why this is correct.

https://codereview.chromium.org/1350673003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/1350673003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:40: // Called when a document
load resulting from the navigation stopped. Note
Let's update the comment. We don't track document load anymore.

https://codereview.chromium.org/1350673003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (left):

https://codereview.chromium.org/1350673003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:741:
delegate_->ReadyToCommitNavigation(render_frame_host->navigation_handle());
I think having this explicitly called before commit makes it more clear.
Otherwise we have to ensure that TransferNavigationHandleOwnership always
implies ready to commit and ideally enforce this.

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:149: // observers should
implement DidFinishNavigation instead.
Let's use a bit stronger language, something like "If not absolutely certain
this is the correct method to use, implement DidFinishNavigation only.".
Charlie can probably help wordsmith this a bit better too.

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:153: // navigation is either
committed, aborted or replaced by a new one. Note
nit: drop "either"

[nasko, 2015-09-18 16:48:44]

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:160: 
Let's also organize the document loading notifications in a separate part of the
file with an explicit header, so people can find them a bit more easily.

[Charlie Reis, 2015-09-18 17:05:21]

I like the simpler API.  Hopefully it proves easier for others to use correctly.
 A few thoughts below.

https://codereview.chromium.org/1350673003/diff/20001/chrome/browser/captive_...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

https://codereview.chromium.org/1350673003/diff/20001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:92:
navigation_handle->HasCommittedErrorPage();
Seems like lots of places will have to use this pattern, which is a bit awkward.
 Would it make sense to have HasCommitted() (true for either a normal page or
error page) and IsErrorPage() (to distinguish between the two) instead?

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:147: // structures/state before
the RenderFrame is navigated, should use this
nit: Drop comma.

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:148: // method as opposed to
DidCommitNavigation, which is after the fact. Other
DidCommitNavigation is gone now.

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:149: // observers should
implement DidFinishNavigation instead.
On 2015/09/18 16:42:16, nasko (slow to review) wrote:
> Let's use a bit stronger language, something like "If not absolutely certain
> this is the correct method to use, implement DidFinishNavigation only.".
> Charlie can probably help wordsmith this a bit better too.

I'd reorder the whole thing to emphasize this point.  Something like:

"Called when the navigation is ready to be committed in a renderer. Most
observers should use DidFinishNavigation instead, which happens right after the
navigation commits. This method is for observers that want to initialize
renderer-side state just before the RenderFrame commits the navigation. 

This is the first point in time where a RenderFrameHost is associated with the
navigation."

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:156: // If this called because
the navigation committed, then the document load
nit: this is

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:158: // |navigation_handle|.
Add something like: "Use DidStopLoading and related methods to listen for
continued events from this RenderFrameHost."

[clamy, 2015-09-18 20:37:34]

Thanks!

https://codereview.chromium.org/1350673003/diff/20001/chrome/browser/captive_...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

https://codereview.chromium.org/1350673003/diff/20001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:92:
navigation_handle->HasCommittedErrorPage();
On 2015/09/18 17:05:20, Charlie Reis wrote:
> Seems like lots of places will have to use this pattern, which is a bit
awkward.
>  Would it make sense to have HasCommitted() (true for either a normal page or
> error page) and IsErrorPage() (to distinguish between the two) instead?

Done.

https://codereview.chromium.org/1350673003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigation_request.cc (right):

https://codereview.chromium.org/1350673003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigation_request.cc:207:
render_frame_host->navigation_handle()->ReadyToCommitNavigation(
On 2015/09/18 16:42:16, nasko (slow to review) wrote:
> Why here instead of right before CommitNavigation call in OnResponseStarted?
It
> isn't immediately clear why this is correct.

Because we don't have a renderer prior to the call to CommitNavigation. And
after the call this request is destroyed. The handle is transfered before any
IPC is sent.

https://codereview.chromium.org/1350673003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_delegate.h (right):

https://codereview.chromium.org/1350673003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_delegate.h:40: // Called when a document
load resulting from the navigation stopped. Note
On 2015/09/18 16:42:16, nasko (slow to review) wrote:
> Let's update the comment. We don't track document load anymore.

Done.

https://codereview.chromium.org/1350673003/diff/20001/content/browser/frame_h...
File content/browser/frame_host/navigator_impl.cc (left):

https://codereview.chromium.org/1350673003/diff/20001/content/browser/frame_h...
content/browser/frame_host/navigator_impl.cc:741:
delegate_->ReadyToCommitNavigation(render_frame_host->navigation_handle());
On 2015/09/18 16:42:16, nasko (slow to review) wrote:
> I think having this explicitly called before commit makes it more clear.
> Otherwise we have to ensure that TransferNavigationHandleOwnership always
> implies ready to commit and ideally enforce this.

Well that's what TransferNavigationHandleOwnership is meant to do: transfer the
NavigationHandle ownership to a RFH when the navigation is ready to commit (see
the description in navigation_handle.h). I can still move it out of the function
if you want.

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:147: // structures/state before
the RenderFrame is navigated, should use this
On 2015/09/18 17:05:21, Charlie Reis wrote:
> nit: Drop comma.

Done.

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:148: // method as opposed to
DidCommitNavigation, which is after the fact. Other
On 2015/09/18 17:05:21, Charlie Reis wrote:
> DidCommitNavigation is gone now.

Done.

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:149: // observers should
implement DidFinishNavigation instead.
On 2015/09/18 17:05:21, Charlie Reis wrote:
> On 2015/09/18 16:42:16, nasko (slow to review) wrote:
> > Let's use a bit stronger language, something like "If not absolutely certain
> > this is the correct method to use, implement DidFinishNavigation only.".
> > Charlie can probably help wordsmith this a bit better too.
> 
> I'd reorder the whole thing to emphasize this point.  Something like:
> 
> "Called when the navigation is ready to be committed in a renderer. Most
> observers should use DidFinishNavigation instead, which happens right after
the
> navigation commits. This method is for observers that want to initialize
> renderer-side state just before the RenderFrame commits the navigation. 
> 
> This is the first point in time where a RenderFrameHost is associated with the
> navigation."

Done.

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:153: // navigation is either
committed, aborted or replaced by a new one. Note
On 2015/09/18 16:42:16, nasko (slow to review) wrote:
> nit: drop "either"

Done.

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:156: // If this called because
the navigation committed, then the document load
On 2015/09/18 17:05:21, Charlie Reis wrote:
> nit: this is

Done.

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:158: // |navigation_handle|.
On 2015/09/18 17:05:21, Charlie Reis wrote:
> Add something like: "Use DidStopLoading and related methods to listen for
> continued events from this RenderFrameHost."

Done.

https://codereview.chromium.org/1350673003/diff/20001/content/public/browser/...
content/public/browser/web_contents_observer.h:160: 
On 2015/09/18 16:48:44, nasko (slow to review) wrote:
> Let's also organize the document loading notifications in a separate part of
the
> file with an explicit header, so people can find them a bit more easily.

Done. I did not include the DidNavigateMainFrame and DidNavigateAnyFrame since
they seem more navigation related, and we may want to remove them in the future.

[nasko, 2015-09-18 23:37:40]

LGTM

[Charlie Reis, 2015-09-21 16:40:42]

Thanks.  LGTM with nits.

https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
content/public/browser/navigation_handle.h:36: // Whether the navigation has
committed.
Please also mention that this returns true for either successful commits or
error pages that replace the previous page (distinguished by |IsErrorPage|), and
false for errors that leave the user on the previous page.

(This is an important point for how to use the API correctly.)

https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
content/public/browser/web_contents_observer.h:155: // navigation is committed,
aborted or replaced by a new one. Note that
Please mention NavigationHandle's HasCommitted and IsErrorPage methods as a way
to tell the difference between these cases, since almost all users will need to
check them.

https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
content/public/browser/web_contents_observer.h:158: // If this is called because
the navigation committed, then the document load
nit: Add an empty comment line above this paragraph, similar to line 149.

https://codereview.chromium.org/1350673003/diff/40001/content/test/web_conten...
File content/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/1350673003/diff/40001/content/test/web_conten...
content/test/web_contents_observer_sanity_checker.cc:129:
CHECK(!navigation_handle->HasCommitted());
We can check !IsErrorPage() as well, right?  (Same below?)

https://codereview.chromium.org/1350673003/diff/40001/content/test/web_conten...
content/test/web_contents_observer_sanity_checker.cc:161:
CHECK_IMPLIES(navigation_handle->HasCommitted(),
Should this be CHECK_EQ?  (In other words, does the implication go both ways,
where a null RFH implies the navigation hasn't committed?)

[clamy, 2015-09-21 17:36:33]

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

[clamy, 2015-09-21 17:36:34]

Thanks! @mmenke: PTAL at the changes in captive_portal/.

[mmenke, 2015-09-21 17:37:45]

On 2015/09/21 17:36:34, clamy wrote:
> Thanks! @mmenke: PTAL at the changes in captive_portal/.

May not get to this until tomorrow.  Still finishing up perf (I'm slow...).

[mmenke, 2015-09-21 20:28:24]

https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:91: if
(!navigation_handle->HasCommitted())
Why the change in behavior here?  Worth a comment?

https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:99:
tab_reloader_->OnAbort();
nit:  Use braces with else if (Style here varies by committer, but I always use
them, and am the only owner, so best to be consistent)

https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:104: void
CaptivePortalTabHelper::DidStopLoading() {
Why the change here?

[clamy, 2015-09-22 00:38:04]

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

[clamy, 2015-09-22 00:38:05]

Thanks!

@csharrison: PTAL at the changes in components/page_load_metrics/

https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):

https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:91: if
(!navigation_handle->HasCommitted())
On 2015/09/21 20:28:24, mmenke wrote:
> Why the change in behavior here?  Worth a comment?

Done.

https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:99:
tab_reloader_->OnAbort();
On 2015/09/21 20:28:24, mmenke wrote:
> nit:  Use braces with else if (Style here varies by committer, but I always
use
> them, and am the only owner, so best to be consistent)

Done.

https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
chrome/browser/captive_portal/captive_portal_tab_helper.cc:104: void
CaptivePortalTabHelper::DidStopLoading() {
On 2015/09/21 20:28:24, mmenke wrote:
> Why the change here?

We changed the moment DidFinishNavigation will be called: in a normal navigation
it is now called at commit time. Since login_detector_->OnStoppedLoading() needs
to be called when the document stops loading, I moved back to the
WebContentsObserver method which signals that (it was the initial implementation
in the code).

https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
File content/public/browser/navigation_handle.h (right):

https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
content/public/browser/navigation_handle.h:36: // Whether the navigation has
committed.
On 2015/09/21 16:40:42, Charlie Reis wrote:
> Please also mention that this returns true for either successful commits or
> error pages that replace the previous page (distinguished by |IsErrorPage|),
and
> false for errors that leave the user on the previous page.
> 
> (This is an important point for how to use the API correctly.)

Done.

https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
content/public/browser/web_contents_observer.h:155: // navigation is committed,
aborted or replaced by a new one. Note that
On 2015/09/21 16:40:42, Charlie Reis wrote:
> Please mention NavigationHandle's HasCommitted and IsErrorPage methods as a
way
> to tell the difference between these cases, since almost all users will need
to
> check them.

Done.

https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
content/public/browser/web_contents_observer.h:158: // If this is called because
the navigation committed, then the document load
On 2015/09/21 16:40:42, Charlie Reis wrote:
> nit: Add an empty comment line above this paragraph, similar to line 149.

Done.

https://codereview.chromium.org/1350673003/diff/40001/content/test/web_conten...
File content/test/web_contents_observer_sanity_checker.cc (right):

https://codereview.chromium.org/1350673003/diff/40001/content/test/web_conten...
content/test/web_contents_observer_sanity_checker.cc:129:
CHECK(!navigation_handle->HasCommitted());
On 2015/09/21 16:40:42, Charlie Reis wrote:
> We can check !IsErrorPage() as well, right?  (Same below?)

Done.

https://codereview.chromium.org/1350673003/diff/40001/content/test/web_conten...
content/test/web_contents_observer_sanity_checker.cc:161:
CHECK_IMPLIES(navigation_handle->HasCommitted(),
On 2015/09/21 16:40:42, Charlie Reis wrote:
> Should this be CHECK_EQ?  (In other words, does the implication go both ways,
> where a null RFH implies the navigation hasn't committed?)

No this is an implication because I've put a CHECK in the accessor that will
break if called before the navigation is ready to commit.

[Charlie Harrison, 2015-09-22 01:37:51]

On 2015/09/22 00:38:05, clamy wrote:
> Thanks!
> 
> @csharrison: PTAL at the changes in components/page_load_metrics/
> 
>
https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
> File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
> chrome/browser/captive_portal/captive_portal_tab_helper.cc:91: if
> (!navigation_handle->HasCommitted())
> On 2015/09/21 20:28:24, mmenke wrote:
> > Why the change in behavior here?  Worth a comment?
> 
> Done.
> 
>
https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
> chrome/browser/captive_portal/captive_portal_tab_helper.cc:99:
> tab_reloader_->OnAbort();
> On 2015/09/21 20:28:24, mmenke wrote:
> > nit:  Use braces with else if (Style here varies by committer, but I always
> use
> > them, and am the only owner, so best to be consistent)
> 
> Done.
> 
>
https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
> chrome/browser/captive_portal/captive_portal_tab_helper.cc:104: void
> CaptivePortalTabHelper::DidStopLoading() {
> On 2015/09/21 20:28:24, mmenke wrote:
> > Why the change here?
> 
> We changed the moment DidFinishNavigation will be called: in a normal
navigation
> it is now called at commit time. Since login_detector_->OnStoppedLoading()
needs
> to be called when the document stops loading, I moved back to the
> WebContentsObserver method which signals that (it was the initial
implementation
> in the code).
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
> File content/public/browser/navigation_handle.h (right):
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
> content/public/browser/navigation_handle.h:36: // Whether the navigation has
> committed.
> On 2015/09/21 16:40:42, Charlie Reis wrote:
> > Please also mention that this returns true for either successful commits or
> > error pages that replace the previous page (distinguished by |IsErrorPage|),
> and
> > false for errors that leave the user on the previous page.
> > 
> > (This is an important point for how to use the API correctly.)
> 
> Done.
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
> File content/public/browser/web_contents_observer.h (right):
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
> content/public/browser/web_contents_observer.h:155: // navigation is
committed,
> aborted or replaced by a new one. Note that
> On 2015/09/21 16:40:42, Charlie Reis wrote:
> > Please mention NavigationHandle's HasCommitted and IsErrorPage methods as a
> way
> > to tell the difference between these cases, since almost all users will need
> to
> > check them.
> 
> Done.
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
> content/public/browser/web_contents_observer.h:158: // If this is called
because
> the navigation committed, then the document load
> On 2015/09/21 16:40:42, Charlie Reis wrote:
> > nit: Add an empty comment line above this paragraph, similar to line 149.
> 
> Done.
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/test/web_conten...
> File content/test/web_contents_observer_sanity_checker.cc (right):
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/test/web_conten...
> content/test/web_contents_observer_sanity_checker.cc:129:
> CHECK(!navigation_handle->HasCommitted());
> On 2015/09/21 16:40:42, Charlie Reis wrote:
> > We can check !IsErrorPage() as well, right?  (Same below?)
> 
> Done.
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/test/web_conten...
> content/test/web_contents_observer_sanity_checker.cc:161:
> CHECK_IMPLIES(navigation_handle->HasCommitted(),
> On 2015/09/21 16:40:42, Charlie Reis wrote:
> > Should this be CHECK_EQ?  (In other words, does the implication go both
ways,
> > where a null RFH implies the navigation hasn't committed?)
> 
> No this is an implication because I've put a CHECK in the accessor that will
> break if called before the navigation is ready to commit.

page_load_metrics/ lgtm. You might need to get a rubber stamp from rdsmith or
ttuttle, because I'm not a committer yet. Thanks for this change!

[Charlie Harrison, 2015-09-22 01:37:52]

On 2015/09/22 00:38:05, clamy wrote:
> Thanks!
> 
> @csharrison: PTAL at the changes in components/page_load_metrics/
> 
>
https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
> File chrome/browser/captive_portal/captive_portal_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
> chrome/browser/captive_portal/captive_portal_tab_helper.cc:91: if
> (!navigation_handle->HasCommitted())
> On 2015/09/21 20:28:24, mmenke wrote:
> > Why the change in behavior here?  Worth a comment?
> 
> Done.
> 
>
https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
> chrome/browser/captive_portal/captive_portal_tab_helper.cc:99:
> tab_reloader_->OnAbort();
> On 2015/09/21 20:28:24, mmenke wrote:
> > nit:  Use braces with else if (Style here varies by committer, but I always
> use
> > them, and am the only owner, so best to be consistent)
> 
> Done.
> 
>
https://codereview.chromium.org/1350673003/diff/40001/chrome/browser/captive_...
> chrome/browser/captive_portal/captive_portal_tab_helper.cc:104: void
> CaptivePortalTabHelper::DidStopLoading() {
> On 2015/09/21 20:28:24, mmenke wrote:
> > Why the change here?
> 
> We changed the moment DidFinishNavigation will be called: in a normal
navigation
> it is now called at commit time. Since login_detector_->OnStoppedLoading()
needs
> to be called when the document stops loading, I moved back to the
> WebContentsObserver method which signals that (it was the initial
implementation
> in the code).
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
> File content/public/browser/navigation_handle.h (right):
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
> content/public/browser/navigation_handle.h:36: // Whether the navigation has
> committed.
> On 2015/09/21 16:40:42, Charlie Reis wrote:
> > Please also mention that this returns true for either successful commits or
> > error pages that replace the previous page (distinguished by |IsErrorPage|),
> and
> > false for errors that leave the user on the previous page.
> > 
> > (This is an important point for how to use the API correctly.)
> 
> Done.
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
> File content/public/browser/web_contents_observer.h (right):
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
> content/public/browser/web_contents_observer.h:155: // navigation is
committed,
> aborted or replaced by a new one. Note that
> On 2015/09/21 16:40:42, Charlie Reis wrote:
> > Please mention NavigationHandle's HasCommitted and IsErrorPage methods as a
> way
> > to tell the difference between these cases, since almost all users will need
> to
> > check them.
> 
> Done.
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/public/browser/...
> content/public/browser/web_contents_observer.h:158: // If this is called
because
> the navigation committed, then the document load
> On 2015/09/21 16:40:42, Charlie Reis wrote:
> > nit: Add an empty comment line above this paragraph, similar to line 149.
> 
> Done.
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/test/web_conten...
> File content/test/web_contents_observer_sanity_checker.cc (right):
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/test/web_conten...
> content/test/web_contents_observer_sanity_checker.cc:129:
> CHECK(!navigation_handle->HasCommitted());
> On 2015/09/21 16:40:42, Charlie Reis wrote:
> > We can check !IsErrorPage() as well, right?  (Same below?)
> 
> Done.
> 
>
https://codereview.chromium.org/1350673003/diff/40001/content/test/web_conten...
> content/test/web_contents_observer_sanity_checker.cc:161:
> CHECK_IMPLIES(navigation_handle->HasCommitted(),
> On 2015/09/21 16:40:42, Charlie Reis wrote:
> > Should this be CHECK_EQ?  (In other words, does the implication go both
ways,
> > where a null RFH implies the navigation hasn't committed?)
> 
> No this is an implication because I've put a CHECK in the accessor that will
> break if called before the navigation is ready to commit.

page_load_metrics/ lgtm. You might need to get a rubber stamp from rdsmith or
ttuttle, because I'm not a committer yet. Thanks for this change!

[mmenke, 2015-09-22 14:47:52]

captive_portal LGTM

[Charlie Harrison, 2015-09-22 19:39:45]

https://codereview.chromium.org/1350673003/diff/80001/components/page_load_me...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1350673003/diff/80001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:76: if
(!navigation_handle->HasCommitted())
Will this ever be called while the previous navigation is ongoing? If not, could
you remove this early return condition? We want to log the previous page load
timing even if the subsequent navigation results in an error.

[clamy, 2015-09-22 23:04:22]

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

[clamy, 2015-09-22 23:04:45]

@rdsmith: pTAL at the changes in components/page_load_metrics

https://codereview.chromium.org/1350673003/diff/80001/components/page_load_me...
File components/page_load_metrics/browser/metrics_web_contents_observer.cc
(right):

https://codereview.chromium.org/1350673003/diff/80001/components/page_load_me...
components/page_load_metrics/browser/metrics_web_contents_observer.cc:76: if
(!navigation_handle->HasCommitted())
On 2015/09/22 19:39:45, csharrison wrote:
> Will this ever be called while the previous navigation is ongoing? If not,
could
> you remove this early return condition? We want to log the previous page load
> timing even if the subsequent navigation results in an error.

Yes it can be called when a previous navigation is ongoing. If the navigation
results in an error that commits an error page HasCommitted will be true. If the
navigation results in an error that does not commit an error page (ie it gets
aborted/canceled) then HasCommitted will be false. The early return here is
equivalent to only implementing DidCommitNavigation before its removal: if
handle->HasCommitted is false, then DidCommitNavigation would not have been
called (you would only get DidFinishNavigation).

[Charlie Harrison, 2015-09-22 23:11:25]

On 2015/09/22 23:04:45, clamy wrote:
> @rdsmith: pTAL at the changes in components/page_load_metrics
> 
>
https://codereview.chromium.org/1350673003/diff/80001/components/page_load_me...
> File components/page_load_metrics/browser/metrics_web_contents_observer.cc
> (right):
> 
>
https://codereview.chromium.org/1350673003/diff/80001/components/page_load_me...
> components/page_load_metrics/browser/metrics_web_contents_observer.cc:76: if
> (!navigation_handle->HasCommitted())
> On 2015/09/22 19:39:45, csharrison wrote:
> > Will this ever be called while the previous navigation is ongoing? If not,
> could
> > you remove this early return condition? We want to log the previous page
load
> > timing even if the subsequent navigation results in an error.
> 
> Yes it can be called when a previous navigation is ongoing. If the navigation
> results in an error that commits an error page HasCommitted will be true. If
the
> navigation results in an error that does not commit an error page (ie it gets
> aborted/canceled) then HasCommitted will be false. The early return here is
> equivalent to only implementing DidCommitNavigation before its removal: if
> handle->HasCommitted is false, then DidCommitNavigation would not have been
> called (you would only get DidFinishNavigation).

Thanks for the clarification, we are currently dealing with aborts/cancellation
in another CL so this is fine.

[clamy, 2015-09-24 18:32:25]

@rdsmith: friendly ping :)

[clamy, 2015-09-24 18:40:46]

clamy@chromium.org changed reviewers:
+ ttuttle@chromium.org
- rdsmith@chromium.org

[clamy, 2015-09-24 18:41:14]

@ttuttle: PTAL at the changes components/page_load_metrics/

[Deprecated (see juliatuttle), 2015-09-24 18:45:12]

On 2015/09/24 18:41:14, clamy wrote:
> @ttuttle: PTAL at the changes components/page_load_metrics/

components/page_load_metrics/* lgtm

[clamy, 2015-09-24 18:46:58]

The CQ bit was checked by clamy@chromium.org

[clamy, 2015-09-24 18:46:58]

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

[commit-bot: I haz the power, 2015-09-24 18:48:42]

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

[commit-bot: I haz the power, 2015-09-24 20:01:29]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-09-24 20:02:36]

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