Most references to GetActiveEntry removed from chrome/browser/ui.

Most references to GetActiveEntry removed from chrome/browser/ui.

BUG=273710

[jww, 2013-11-08 22:55:48]

This is my first whack at removing a bunch of GetActiveEntry calls. I removed
all calls in chrome/browser/ui in order to prioritize autofill (except for
references in chrome/browser/ui/sync/tab_contents_synced_tab_delegate.h and
chrome/browser/ui/sync/tab_contents_synced_tab_delegate.cc because those require
changing some other references first).

I believe I was a bit more conservative in my use of GetVisibleEntry over
GetLastCommittedEntry than Nasko has been, so please feel free to scold me :-)

[nasko, 2013-11-08 23:52:02]

Great start on this work!
It will be great to start out conservatively with replacing using LastCommitted
where it is not clearly the Visible one.
Thanks for helping out!

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
chrome/browser/ui/browser_commands.cc:983: return
current_tab->GetController().GetVisibleEntry() != NULL;
For this and the next two instances, have you tried using LastCommitted? I'm not
sure what the right thing here is, but why not try to be more conservative? :)

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
chrome/browser/ui/browser_commands.cc:1058: // active entry in
view_source_contents will be the last committed entry.
nit: "active entry" -> "visible entry"

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
chrome/browser/ui/browser_commands.cc:1063:
view_source_contents->GetController().GetVisibleEntry();
If the comment above is correct, why not use LastCommitted?
Even if incorrect, it doesn't make sense to view source on something we haven't
yet loaded.

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/inst...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:64: const
content::NavigationEntry* entry = controller->GetVisibleEntry();
Shouldn't this be LastCommitted?

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/inst...
File chrome/browser/ui/search/instant_page_unittest.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_page_unittest.cc:153:
web_contents()->GetController().GetVisibleEntry();
If we are receiving an IPC from the renderer, doesn't that mean it should be
LastCommitted?

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/sear...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search_tab_helper.cc:307:
web_contents_->GetController().GetVisibleEntry();
DidNavigateMainFrame is the commit point. As such, it seems natural to use
LastCommitted here.

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/tabs/pinned...
File chrome/browser/ui/tabs/pinned_tab_codec.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/tabs/pinned...
chrome/browser/ui/tabs/pinned_tab_codec.cc:70: NavigationEntry* entry =
web_contents->GetController().GetVisibleEntry();
Why not LastCommitted here?

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/webui/ntp/m...
File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/webui/ntp/m...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:67:
web_ui()->GetWebContents()->GetController().GetVisibleEntry();
Seems like LastCommitted usage to me.

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/webui/ntp/s...
File chrome/browser/ui/webui/ntp/suggestions_page_handler.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/webui/ntp/s...
chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:52:
web_ui()->GetWebContents()->GetController().GetVisibleEntry();
Seems like another LastCommitted usage to me.

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/zoom/zoom_c...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/zoom/zoom_c...
chrome/browser/ui/zoom/zoom_controller.cc:77:
web_contents()->GetController().GetVisibleEntry();
It is sad that the bug above didn't have an automated test, but probably it is
easily reproducible by hand. I'm thinking LastCommitted makes more sense here,
but not sure whether the zoom indicator keys off of the URL bar or the contents
of the tab.

[jww, 2013-11-08 23:59:57]

On 2013/11/08 23:52:02, nasko wrote:
> Great start on this work!
> It will be great to start out conservatively with replacing using
LastCommitted
> where it is not clearly the Visible one.
I'm happy to. In fact, in my initial run, virtually all of these were
LastCommitted, but when I synced up with the Mega Patch, I actually changed a
bunch to GetVisible based on your patches. I guess in the Mega Patch you were
using GetVisible more than is necessary?

Anyway, I'll go through and change all these back to GetLastCommitted. Thanks!
> Thanks for helping out!
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
> File chrome/browser/ui/browser_commands.cc (right):
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
> chrome/browser/ui/browser_commands.cc:983: return
> current_tab->GetController().GetVisibleEntry() != NULL;
> For this and the next two instances, have you tried using LastCommitted? I'm
not
> sure what the right thing here is, but why not try to be more conservative? :)
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
> chrome/browser/ui/browser_commands.cc:1058: // active entry in
> view_source_contents will be the last committed entry.
> nit: "active entry" -> "visible entry"
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
> chrome/browser/ui/browser_commands.cc:1063:
> view_source_contents->GetController().GetVisibleEntry();
> If the comment above is correct, why not use LastCommitted?
> Even if incorrect, it doesn't make sense to view source on something we
haven't
> yet loaded.
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/inst...
> File chrome/browser/ui/search/instant_controller.cc (right):
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/inst...
> chrome/browser/ui/search/instant_controller.cc:64: const
> content::NavigationEntry* entry = controller->GetVisibleEntry();
> Shouldn't this be LastCommitted?
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/inst...
> File chrome/browser/ui/search/instant_page_unittest.cc (right):
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/inst...
> chrome/browser/ui/search/instant_page_unittest.cc:153:
> web_contents()->GetController().GetVisibleEntry();
> If we are receiving an IPC from the renderer, doesn't that mean it should be
> LastCommitted?
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/sear...
> File chrome/browser/ui/search/search_tab_helper.cc (right):
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/sear...
> chrome/browser/ui/search/search_tab_helper.cc:307:
> web_contents_->GetController().GetVisibleEntry();
> DidNavigateMainFrame is the commit point. As such, it seems natural to use
> LastCommitted here.
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/tabs/pinned...
> File chrome/browser/ui/tabs/pinned_tab_codec.cc (right):
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/tabs/pinned...
> chrome/browser/ui/tabs/pinned_tab_codec.cc:70: NavigationEntry* entry =
> web_contents->GetController().GetVisibleEntry();
> Why not LastCommitted here?
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/webui/ntp/m...
> File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/webui/ntp/m...
> chrome/browser/ui/webui/ntp/most_visited_handler.cc:67:
> web_ui()->GetWebContents()->GetController().GetVisibleEntry();
> Seems like LastCommitted usage to me.
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/webui/ntp/s...
> File chrome/browser/ui/webui/ntp/suggestions_page_handler.cc (right):
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/webui/ntp/s...
> chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:52:
> web_ui()->GetWebContents()->GetController().GetVisibleEntry();
> Seems like another LastCommitted usage to me.
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/zoom/zoom_c...
> File chrome/browser/ui/zoom/zoom_controller.cc (right):
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/zoom/zoom_c...
> chrome/browser/ui/zoom/zoom_controller.cc:77:
> web_contents()->GetController().GetVisibleEntry();
> It is sad that the bug above didn't have an automated test, but probably it is
> easily reproducible by hand. I'm thinking LastCommitted makes more sense here,
> but not sure whether the zoom indicator keys off of the URL bar or the
contents
> of the tab.

[nasko, 2013-11-09 00:01:54]

On 2013/11/08 23:59:57, jww wrote:
> On 2013/11/08 23:52:02, nasko wrote:
> > Great start on this work!
> > It will be great to start out conservatively with replacing using
> LastCommitted
> > where it is not clearly the Visible one.
> I'm happy to. In fact, in my initial run, virtually all of these were
> LastCommitted, but when I synced up with the Mega Patch, I actually changed a
> bunch to GetVisible based on your patches. I guess in the Mega Patch you were
> using GetVisible more than is necessary?
> 
> Anyway, I'll go through and change all these back to GetLastCommitted. Thanks!

My megapatch was mainly for discoverability than actually thinking through all
instances. I would take it as a guide to what needs to be modified, but not how
: ).

[jww, 2013-11-09 01:33:41]

On 2013/11/09 00:01:54, nasko wrote:
> On 2013/11/08 23:59:57, jww wrote:
> > On 2013/11/08 23:52:02, nasko wrote:
> > > Great start on this work!
> > > It will be great to start out conservatively with replacing using
> > LastCommitted
> > > where it is not clearly the Visible one.
> > I'm happy to. In fact, in my initial run, virtually all of these were
> > LastCommitted, but when I synced up with the Mega Patch, I actually changed
a
> > bunch to GetVisible based on your patches. I guess in the Mega Patch you
were
> > using GetVisible more than is necessary?
> > 
> > Anyway, I'll go through and change all these back to GetLastCommitted.
Thanks!
> 
> My megapatch was mainly for discoverability than actually thinking through all
> instances. I would take it as a guide to what needs to be modified, but not
how
> : ).

Got it.

[jww, 2013-11-11 19:18:50]

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
chrome/browser/ui/browser_commands.cc:983: return
current_tab->GetController().GetVisibleEntry() != NULL;
On 2013/11/08 23:52:03, nasko wrote:
> For this and the next two instances, have you tried using LastCommitted? I'm
not
> sure what the right thing here is, but why not try to be more conservative? :)

Done.

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
chrome/browser/ui/browser_commands.cc:1058: // active entry in
view_source_contents will be the last committed entry.
On 2013/11/08 23:52:03, nasko wrote:
> nit: "active entry" -> "visible entry"

Refactored the comment a bit more to align with the change below.

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
chrome/browser/ui/browser_commands.cc:1060:
DCHECK(view_source_contents->GetController().CanPruneAllButVisible());
On a side note, this function seems woefully misnamed now. Looking at it's
comments, it seems like it prunes everything down to the last committed entry,
*not* the visible entry :-P Should I put a TODO in that function to change the
name of it?

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
chrome/browser/ui/browser_commands.cc:1063:
view_source_contents->GetController().GetVisibleEntry();
On 2013/11/08 23:52:03, nasko wrote:
> If the comment above is correct, why not use LastCommitted?
> Even if incorrect, it doesn't make sense to view source on something we
haven't
> yet loaded.

Done.

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/inst...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_controller.cc:64: const
content::NavigationEntry* entry = controller->GetVisibleEntry();
On 2013/11/08 23:52:03, nasko wrote:
> Shouldn't this be LastCommitted?

Done.

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/inst...
File chrome/browser/ui/search/instant_page_unittest.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/inst...
chrome/browser/ui/search/instant_page_unittest.cc:153:
web_contents()->GetController().GetVisibleEntry();
On 2013/11/08 23:52:03, nasko wrote:
> If we are receiving an IPC from the renderer, doesn't that mean it should be
> LastCommitted?

Done.

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/sear...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/search/sear...
chrome/browser/ui/search/search_tab_helper.cc:307:
web_contents_->GetController().GetVisibleEntry();
On 2013/11/08 23:52:03, nasko wrote:
> DidNavigateMainFrame is the commit point. As such, it seems natural to use
> LastCommitted here.

Done.

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/tabs/pinned...
File chrome/browser/ui/tabs/pinned_tab_codec.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/tabs/pinned...
chrome/browser/ui/tabs/pinned_tab_codec.cc:70: NavigationEntry* entry =
web_contents->GetController().GetVisibleEntry();
On 2013/11/08 23:52:03, nasko wrote:
> Why not LastCommitted here?
In this case, my thinking was that a user is trying to pin the URL that they see
in the omnibox, not the content, but I suppose that's debatable. What do you
think? Probably needs confirmation from an owner either way on this one.

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/webui/ntp/m...
File chrome/browser/ui/webui/ntp/most_visited_handler.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/webui/ntp/m...
chrome/browser/ui/webui/ntp/most_visited_handler.cc:67:
web_ui()->GetWebContents()->GetController().GetVisibleEntry();
On 2013/11/08 23:52:03, nasko wrote:
> Seems like LastCommitted usage to me.

Done.

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/webui/ntp/s...
File chrome/browser/ui/webui/ntp/suggestions_page_handler.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/webui/ntp/s...
chrome/browser/ui/webui/ntp/suggestions_page_handler.cc:52:
web_ui()->GetWebContents()->GetController().GetVisibleEntry();
On 2013/11/08 23:52:03, nasko wrote:
> Seems like another LastCommitted usage to me.

Done.

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/zoom/zoom_c...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/zoom/zoom_c...
chrome/browser/ui/zoom/zoom_controller.cc:77:
web_contents()->GetController().GetVisibleEntry();
On 2013/11/08 23:52:03, nasko wrote:
> It is sad that the bug above didn't have an automated test, but probably it is
> easily reproducible by hand. I'm thinking LastCommitted makes more sense here,
> but not sure whether the zoom indicator keys off of the URL bar or the
contents
> of the tab.

Done.

[Charlie Reis, 2013-11-11 22:58:23]

Quick comment on the PruneAllButVisible case.  I can take a look at the rest as
well later today, since Nasko is OOO for a bit.

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
chrome/browser/ui/browser_commands.cc:1060:
DCHECK(view_source_contents->GetController().CanPruneAllButVisible());
On 2013/11/11 19:18:51, jww wrote:
> On a side note, this function seems woefully misnamed now. Looking at it's
> comments, it seems like it prunes everything down to the last committed entry,
> *not* the visible entry :-P Should I put a TODO in that function to change the
> name of it?

Wow, that is poorly named.  I'll take the blame for that one.  I changed it from
CanPruneAllButActive when we were starting to get rid of the Active concept but
perhaps hadn't quite settled on the meaning of Visible.  (In this case,
"visible" meant which page was visible, rather than which URL was visible. 
That's entirely non-obvious now that we have the LastCommitted vs Visible
distinction for entries.)

Yes, let's change it to PruneAllButLastCommitted and CanPrunAllButLastCommitted.
 Thanks for catching it!

[jww, 2013-11-11 23:05:54]

On 2013/11/11 22:58:23, creis wrote:
> Quick comment on the PruneAllButVisible case.  I can take a look at the rest
as
> well later today, since Nasko is OOO for a bit.
Thanks, I appreciate it!
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
> File chrome/browser/ui/browser_commands.cc (right):
> 
>
https://codereview.chromium.org/66993003/diff/1/chrome/browser/ui/browser_com...
> chrome/browser/ui/browser_commands.cc:1060:
> DCHECK(view_source_contents->GetController().CanPruneAllButVisible());
> On 2013/11/11 19:18:51, jww wrote:
> > On a side note, this function seems woefully misnamed now. Looking at it's
> > comments, it seems like it prunes everything down to the last committed
entry,
> > *not* the visible entry :-P Should I put a TODO in that function to change
the
> > name of it?
> 
> Wow, that is poorly named.  I'll take the blame for that one.  I changed it
from
> CanPruneAllButActive when we were starting to get rid of the Active concept
but
> perhaps hadn't quite settled on the meaning of Visible.  (In this case,
> "visible" meant which page was visible, rather than which URL was visible. 
> That's entirely non-obvious now that we have the LastCommitted vs Visible
> distinction for entries.)
> 
> Yes, let's change it to PruneAllButLastCommitted and
CanPrunAllButLastCommitted.
>  Thanks for catching it!
Great. I'll actually make a separate CL to do that.

[Charlie Reis, 2013-11-12 01:09:28]

I'm adding in a lot of reviewers to answer specific questions about code they're
likely familiar with.

@sschmitz, @benwells, @pinkerton, @samarth, @sreeram, @sky: Please look for your
name below.  Thanks!

Joel, you may also want to consider breaking this up into multiple CLs to land
in pieces, once we find out the answers to these questions.  This all-at-once
approach is likely to get reverted if even one of the call sites leads to
something unexpected.  (Nasko and I hit that problem in his changes to content/,
when one of the changes caused a perf regression.)

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/androi...
File chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc
(right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/androi...
chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc:195:
const GURL& active_url = contents_->GetLastCommittedURL();
nit: active_url -> current_url

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:594: const GURL&
active_url = web_contents()->GetLastCommittedURL();
nit: current_url

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands.cc:261: entry ? entry->GetURL() : GURL()) ||
nit: Should fit on the previous line now.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands.cc:983: return
current_tab->GetController().GetLastCommittedEntry() != NULL;
Hmm, I can't tell if this is correct or not.

@sschmitz: Looks like you added the RequestTabletSite stuff in
https://chromiumcodereview.appspot.com/10827146.  If the user looking at page A
and has started a navigation to a slow page B that hasn't committed yet, should
the RequestTabletSite feature affect page A (last committed) or page B (visible
URL / pending entry)?

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands.cc:1139: const GURL& url =
contents->GetController().GetLastCommittedEntry()->GetURL();
@benwells: Do you know how ConvertTabToAppWindow is used?  If the user is on
page A with a slow pending navigation to page B, should we be using page A (last
committed) or page B (visible URL / pending entry) here?

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/applescript/tab_applescript.mm (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:117:
webContents_->GetController().GetLastCommittedEntry();
@pinkerton:
My Cocoa skills are a bit rusty (or nonexistent), but this looks like it's in a
method to get "the URL" of a tab from AppleScript.  That would be the pending
URL if a slow navigation were in progress, correct?  That means we should use
GetVisibleEntry here.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:141:
webContents_->GetController().GetLastCommittedEntry();
This looks correct, since we're trying to get the previousURL from it.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:156:
webContents_->GetController().GetLastCommittedEntry();
This should almost certainly be GetVisibleEntry, since it's the title that the
user sees.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:64: const
content::NavigationEntry* entry = controller->GetLastCommittedEntry();
@samarth: I'm not sure which entry you're looking for here.  Suppose there's a
slow pending entry, which may or may not have its URL visible to the user (e.g.,
depending on some checks for URL spoofs).  Does this code need the URL from the
omnibox, the URL of the last committed page, or the URL of the actual pending
entry, regardless of whether it's visible in the omnibox or not?

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.cc:72:
contents->GetController().GetLastCommittedEntry();
@sreeram: I see you specifically used GetVisibleEntry below in
http://crrev.com/182381.  Should this be using it as well?

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/tabs/p...
File chrome/browser/ui/tabs/pinned_tab_codec.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/tabs/p...
chrome/browser/ui/tabs/pinned_tab_codec.cc:70: NavigationEntry* entry =
web_contents->GetController().GetVisibleEntry();
@sky: Is the "pinned tab encoding" expecting to get pending URLs if a navigation
is in progress?

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (left):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:630:
initiator->GetController().GetActiveEntry();
Aha!  That's a real bug in Chrome today.  If you have a slow URL pending and
print your current page, the (wrong) pending URL will be listed at the bottom. 
Once again, I'm glad we're doing this cleanup.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller.cc:74: // Use the active navigation
entry's URL instead of the WebContents' so
This comment is very confusing since they meant to call attention to "navigation
entry's URL" rather than "active".  Let's remove the word "active."

[benwells, 2013-11-12 05:32:46]

ConvertTabToAppWindow lgtm

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands.cc:1139: const GURL& url =
contents->GetController().GetLastCommittedEntry()->GetURL();
On 2013/11/12 01:09:29, creis wrote:
> @benwells: Do you know how ConvertTabToAppWindow is used?  If the user is on
> page A with a slow pending navigation to page B, should we be using page A
(last
> committed) or page B (visible URL / pending entry) here?

I'd say A (last committed), but TBH I don't think it really matters.

We're hoping to remove this function in the near future.

[sreeram, 2013-11-12 09:01:24]

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.cc:72:
contents->GetController().GetLastCommittedEntry();
On 2013/11/12 01:09:29, creis wrote:
> @sreeram: I see you specifically used GetVisibleEntry below in
> http://crrev.com/182381.  Should this be using it as well?

Based on the call sites below (i.e., where IsCacheableNTP() is called), it seems
to me that GetLastCommittedEntry() is correct. No need for GetVisibleEntry()
here.

[pink (ping after 24hrs), 2013-11-12 15:27:31]

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/applescript/tab_applescript.mm (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:117:
webContents_->GetController().GetLastCommittedEntry();
On 2013/11/12 01:09:29, creis wrote:
> @pinkerton:
> My Cocoa skills are a bit rusty (or nonexistent), but this looks like it's in
a
> method to get "the URL" of a tab from AppleScript.  That would be the pending
> URL if a slow navigation were in progress, correct?  That means we should use
> GetVisibleEntry here.

Sounds right, though my memory of NavigationController is fleeting at best.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:141:
webContents_->GetController().GetLastCommittedEntry();
On 2013/11/12 01:09:29, creis wrote:
> This looks correct, since we're trying to get the previousURL from it.

agreed.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:156:
webContents_->GetController().GetLastCommittedEntry();
On 2013/11/12 01:09:29, creis wrote:
> This should almost certainly be GetVisibleEntry, since it's the title that the
> user sees.

Sounds right, same caveat as above.

[samarth, 2013-11-12 15:55:06]

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:64: const
content::NavigationEntry* entry = controller->GetLastCommittedEntry();
On 2013/11/12 01:09:29, creis wrote:
> @samarth: I'm not sure which entry you're looking for here.  Suppose there's a
> slow pending entry, which may or may not have its URL visible to the user
(e.g.,
> depending on some checks for URL spoofs).  Does this code need the URL from
the
> omnibox, the URL of the last committed page, or the URL of the actual pending
> entry, regardless of whether it's visible in the omnibox or not?

What's important here is the fact that we set the search terms on the transient
entry using SetExtraData, not the actual URL.   Given that, using the last
committed entry seems safer (since if InstantTab is set, then that should be a
search url).

Note to self: I should revisit this function to make sure this is actually still
necessary.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.cc:307:
web_contents_->GetController().GetLastCommittedEntry();
Just to confirm, this will be the entry that "triggered" the call to
DidNavigateMainFrame?  The same question applies for the usage in
IsCacheableNTP.

[jww, 2013-11-12 22:32:49]

creis, I'll split this into separate CLs after LGTMs from the various reviewers.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/androi...
File chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc
(right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/androi...
chrome/browser/ui/android/autofill/autofill_dialog_controller_android.cc:195:
const GURL& active_url = contents_->GetLastCommittedURL();
On 2013/11/12 01:09:29, creis wrote:
> nit: active_url -> current_url

Done.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/autofi...
File chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/autofi...
chrome/browser/ui/autofill/autofill_dialog_controller_impl.cc:594: const GURL&
active_url = web_contents()->GetLastCommittedURL();
On 2013/11/12 01:09:29, creis wrote:
> nit: current_url

Done.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands.cc:261: entry ? entry->GetURL() : GURL()) ||
On 2013/11/12 01:09:29, creis wrote:
> nit: Should fit on the previous line now.

Done.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands.cc:1139: const GURL& url =
contents->GetController().GetLastCommittedEntry()->GetURL();
On 2013/11/12 05:32:47, benwells wrote:
> On 2013/11/12 01:09:29, creis wrote:
> > @benwells: Do you know how ConvertTabToAppWindow is used?  If the user is on
> > page A with a slow pending navigation to page B, should we be using page A
> (last
> > committed) or page B (visible URL / pending entry) here?
> 
> I'd say A (last committed), but TBH I don't think it really matters.
> 
> We're hoping to remove this function in the near future.

Okay, so leaving as-is.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/applescript/tab_applescript.mm (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:117:
webContents_->GetController().GetLastCommittedEntry();
On 2013/11/12 15:27:32, pink wrote:
> On 2013/11/12 01:09:29, creis wrote:
> > @pinkerton:
> > My Cocoa skills are a bit rusty (or nonexistent), but this looks like it's
in
> a
> > method to get "the URL" of a tab from AppleScript.  That would be the
pending
> > URL if a slow navigation were in progress, correct?  That means we should
use
> > GetVisibleEntry here.
> 
> Sounds right, though my memory of NavigationController is fleeting at best.

Done.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:141:
webContents_->GetController().GetLastCommittedEntry();
On 2013/11/12 15:27:32, pink wrote:
> On 2013/11/12 01:09:29, creis wrote:
> > This looks correct, since we're trying to get the previousURL from it.
> 
> agreed.

Done.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/applescript/tab_applescript.mm:156:
webContents_->GetController().GetLastCommittedEntry();
On 2013/11/12 15:27:32, pink wrote:
> On 2013/11/12 01:09:29, creis wrote:
> > This should almost certainly be GetVisibleEntry, since it's the title that
the
> > user sees.
> 
> Sounds right, same caveat as above.

Done.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
File chrome/browser/ui/search/instant_controller.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
chrome/browser/ui/search/instant_controller.cc:64: const
content::NavigationEntry* entry = controller->GetLastCommittedEntry();
On 2013/11/12 15:55:07, samarth wrote:
> On 2013/11/12 01:09:29, creis wrote:
> > @samarth: I'm not sure which entry you're looking for here.  Suppose there's
a
> > slow pending entry, which may or may not have its URL visible to the user
> (e.g.,
> > depending on some checks for URL spoofs).  Does this code need the URL from
> the
> > omnibox, the URL of the last committed page, or the URL of the actual
pending
> > entry, regardless of whether it's visible in the omnibox or not?
> 
> What's important here is the fact that we set the search terms on the
transient
> entry using SetExtraData, not the actual URL.   Given that, using the last
> committed entry seems safer (since if InstantTab is set, then that should be a
> search url).
> 
> Note to self: I should revisit this function to make sure this is actually
still
> necessary.

Done.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.cc:72:
contents->GetController().GetLastCommittedEntry();
On 2013/11/12 09:01:26, sreeram wrote:
> On 2013/11/12 01:09:29, creis wrote:
> > @sreeram: I see you specifically used GetVisibleEntry below in
> > http://crrev.com/182381.  Should this be using it as well?
> 
> Based on the call sites below (i.e., where IsCacheableNTP() is called), it
seems
> to me that GetLastCommittedEntry() is correct. No need for GetVisibleEntry()
> here.

Done.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.cc:307:
web_contents_->GetController().GetLastCommittedEntry();
On 2013/11/12 15:55:07, samarth wrote:
> Just to confirm, this will be the entry that "triggered" the call to
> DidNavigateMainFrame?  The same question applies for the usage in
> IsCacheableNTP.

creis should confirm, but yes.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/webui/...
File chrome/browser/ui/webui/print_preview/print_preview_handler.cc (left):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/webui/...
chrome/browser/ui/webui/print_preview/print_preview_handler.cc:630:
initiator->GetController().GetActiveEntry();
On 2013/11/12 01:09:29, creis wrote:
> Aha!  That's a real bug in Chrome today.  If you have a slow URL pending and
> print your current page, the (wrong) pending URL will be listed at the bottom.

> Once again, I'm glad we're doing this cleanup.

Awesome!

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/zoom/z...
File chrome/browser/ui/zoom/zoom_controller.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/zoom/z...
chrome/browser/ui/zoom/zoom_controller.cc:74: // Use the active navigation
entry's URL instead of the WebContents' so
On 2013/11/12 01:09:29, creis wrote:
> This comment is very confusing since they meant to call attention to
"navigation
> entry's URL" rather than "active".  Let's remove the word "active."

Done.

[Charlie Reis, 2013-11-13 18:40:02]

Thanks all for the quick reviews!

@sschmitz: Can you look at my comment in browser_commands.cc?
@sky: Can you look at my comment in pinned_tab_codec.cc?

Joel: LGTM once those questions are answered.

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
File chrome/browser/ui/search/search_tab_helper.cc (right):

https://codereview.chromium.org/66993003/diff/140001/chrome/browser/ui/search...
chrome/browser/ui/search/search_tab_helper.cc:307:
web_contents_->GetController().GetLastCommittedEntry();
On 2013/11/12 22:32:50, jww wrote:
> On 2013/11/12 15:55:07, samarth wrote:
> > Just to confirm, this will be the entry that "triggered" the call to
> > DidNavigateMainFrame?  The same question applies for the usage in
> > IsCacheableNTP.
> 
> creis should confirm, but yes.

Yes, this is the entry that just committed as part of the DidNavigateMainFrame. 
There shouldn't be any way for it to change before we get here.

[samarth, 2013-11-13 19:59:44]

lgtm

[sky, 2013-11-13 22:03:40]

pinned_tab_codec.cc LGTM

[jww, 2013-11-15 23:10:49]

@sky, would you mind taking a look at one more comment? Thanks a lot!

https://codereview.chromium.org/66993003/diff/590001/chrome/browser/ui/browse...
File chrome/browser/ui/browser_commands.cc (right):

https://codereview.chromium.org/66993003/diff/590001/chrome/browser/ui/browse...
chrome/browser/ui/browser_commands.cc:982: return
current_tab->GetController().GetLastCommittedEntry() != NULL;
@sky: I know you gave the LGTM already, but can you just double check this as
well since you reviewed https://chromiumcodereview.appspot.com/10827146? creis
asked:

"If the user looking at page A and has started a navigation to a slow page B
that hasn't committed yet, should the RequestTabletSite feature affect page A
(last committed) or page B (visible URL / pending entry)?"

Thanks!

[sky, 2013-11-16 03:04:55]

SLGTM

[jww, 2013-11-16 05:04:21]

On 2013/11/16 03:04:55, sky wrote:
> SLGTM

Just a heads up to everyone that I've broken this down into several smaller
chunks to be committed separately:
https://codereview.chromium.org/74563003/
https://codereview.chromium.org/68713009/
https://codereview.chromium.org/59603009/
https://codereview.chromium.org/68033004/
https://codereview.chromium.org/58963004/