Rename WebContentsObserver::NavigateToPendingEntry to DidStartNavigationToPendingEntry.

Rename WebContentsObserver::NavigateToPendingEntry to DidStartNavigationToPendingEntry.

NavigateToPending entry is not quite properly named, as it is an observer method. Renaming it to reflect the actual event of creating a pending navigation entry.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=236983

[nasko, 2013-11-22 20:45:54]

Hey Charlie,
Can you review this rename CL for me?
Thanks,
Nasko

[Charlie Reis, 2013-11-22 22:09:25]

https://codereview.chromium.org/84063003/diff/1/content/public/browser/web_co...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/84063003/diff/1/content/public/browser/web_co...
content/public/browser/web_contents_observer.h:83: virtual void
DidCreatePendingEntry(
Sorry to possibly create more work for you, but I want to be sure we end up with
a good name.  As I think about it, there's a lot of things to consider here.

1) Looking at the call sites, it would be nice keep something about "navigation"
in the name, since it's not always obvious from context (e.g.,
FavIconTabHelper::DidCreatePendingEntry).  DidStartNavigation is one option, but
that's admittedly confusing next to DidStartProvisionalLoadForFrame.  Maybe
DidCreatePendingNavigationEntry, as you suggested.

2) Not every navigation goes through this path, and I'm not sure whether that
matters to overriders.  For example, most renderer-initiated navigations won't,
unless they punt to the browser via OpenURL.  Also, some will get here twice
(e.g., cross-process client-redirect).

3) I bet overriders don't realize that subframe navigations will soon pass
through here.  That's more likely to be a problem than (2), but we can ignore it
until we add the cross-process subframe logic.

4) DidCreatePendingEntry accurately conveys that it's for navigations that have
pending entries (which isn't all navigations, and may include subframes). 
However, the event is not fired immediately after creating the pending entry,
but rather after we've told the renderer to go there (possibly confusing).  I
suppose it's still part of the same task, though I wish the name conveyed that
we'd already kicked off the navigation.

5) There could be some confusion around the fact that we've started creating
pending entries for renderer-initiated navigations (in DidStartProvisionalLoad),
but we're not calling this in those cases.  Someone might look at this method
and wonder why we aren't calling it in that case.  That's actually a reasonable
concern, and it applies to the "DidCreatePendingNavigationEntry" name as well.

Other names that don't seem to fit those guidelines: DidStartPendingNavigation,
DidCreatePendingEntryForNavigation, ...

I wish we could just say DidNavigateToPendingEntry.  That's probably the most
accurate, though the "DidNavigate" part of it sounds a lot like
WebContents::DidNavigate, which means commit.  (Should we rename DidNavigate to
something like DidCommit when it moves to Navigator?)

Do you see any better suggestions?  I'm not sure which one to recommend so far.

[nasko, 2013-11-22 22:10:36]

John, can you also review the chrome/ side of this CL?
Thanks!

[Charlie Reis, 2013-11-22 22:15:28]

On 2013/11/22 22:10:36, nasko wrote:
> John, can you also review the chrome/ side of this CL?
> Thanks!

John, you can probably hold off until we arrive at the name, unless you have
ideas for the discussion!  :)

Charlie

[nasko, 2013-11-22 22:46:33]

It seems rather long, but it conveys the right message and fits most of the
items you listed: DidStartNavigationToPendingEntry.

Let me know if this looks good.

[Charlie Reis, 2013-11-22 23:29:10]

Thanks!  LGTM with the extra comment.

https://codereview.chromium.org/84063003/diff/120001/content/public/browser/w...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/84063003/diff/120001/content/public/browser/w...
content/public/browser/web_contents_observer.h:83: virtual void
DidStartNavigationToPendingEntry(
Much better, thanks.  Let's try to capture some of the reasoning for the name in
the comment, with something like:

This method is invoked after the browser process starts a navigation to a
pending NavigationEntry.  It is not called for renderer-initiated navigations
unless they are sent to the browser process via OpenURL.  It may be called
multiple times for a given navigation, such as a typed URL followed by a
cross-process client or server redirect.

[nasko, 2013-11-23 00:17:24]

Added comment.

John, can you review it for chrome/ OWNERS?
Thanks!

https://codereview.chromium.org/84063003/diff/120001/content/public/browser/w...
File content/public/browser/web_contents_observer.h (right):

https://codereview.chromium.org/84063003/diff/120001/content/public/browser/w...
content/public/browser/web_contents_observer.h:83: virtual void
DidStartNavigationToPendingEntry(
On 2013/11/22 23:29:10, creis wrote:
> Much better, thanks.  Let's try to capture some of the reasoning for the name
in
> the comment, with something like:
> 
> This method is invoked after the browser process starts a navigation to a
> pending NavigationEntry.  It is not called for renderer-initiated navigations
> unless they are sent to the browser process via OpenURL.  It may be called
> multiple times for a given navigation, such as a typed URL followed by a
> cross-process client or server redirect.

Done.

[jam, 2013-11-23 01:58:08]

lgtm

[commit-bot: I haz the power, 2013-11-23 04:39:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nasko@chromium.org/84063003/170001

[commit-bot: I haz the power, 2013-11-23 21:22:43]

Change committed as 236983