Disable encoding menu item for chrome://bookmarks

Disable encoding menu item for chrome://bookmarks

Use |NavigationEntry::virtual_url()| to account for URL rewriting, which happens for chrome://bookmarks.

BUG=88387
TEST=Go to chrome://bookmarks. View -> Encoding submenu items should be disabled.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=91647

[Peter Kasting, 2011-07-04 22:36:01]

Rubber-stamp LGTM, but I don't really know what "virtual_url()" is.  You might
want to check with someone who knows that (e.g. whoever added it).

http://codereview.chromium.org/7309006/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7309006/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:3981: // Instead of using GetURL here, we use url()
(which is the "real" url of the
This comment is now out-of-date, no?

[Alexei Svitkine (slow), 2011-07-05 14:18:13]

http://codereview.chromium.org/7309006/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7309006/diff/1/chrome/browser/ui/browser.cc#ne...
chrome/browser/ui/browser.cc:3981: // Instead of using GetURL here, we use url()
(which is the "real" url of the
On 2011/07/04 22:36:02, Peter Kasting wrote:
> This comment is now out-of-date, no?

You're right - I removed the out of date comment.

Indeed, it also seems my previous simple change caused view-source:chrome://
URLs to get the encoding menu enabled. I've updated the code to handle those
specifically.

The virtual URL is the one that is actually displayed in the address bar (which
may have gotten rewritten by the URL rewriter i.e. in the case of
chrome://bookmarks from the chrome-extension:// URL, or in the case of
view-source: to add that prefix).

Can you review my updated CL?

[Peter Kasting, 2011-07-06 03:15:56]

If there is this much complexity involved, I'm not willing to rubber-stamp this
with my zero knowledge.  Please ask someone more familiar with this stuff.  msw
(for chrome: scheme stuff) and brettw (for navigation-related stuff) might be
good people to start with.

[Alexei Svitkine (slow), 2011-07-06 13:39:06]

Fair enough, Peter.

Mike, Brett - can you review this change? Thanks.

[brettw, 2011-07-06 16:55:23]

http://codereview.chromium.org/7309006/diff/5003/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

http://codereview.chromium.org/7309006/diff/5003/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:3951: bool Browser::HasInternalURL(const
NavigationEntry* entry) const {
When you move this above, please be sure to write a comment for this

http://codereview.chromium.org/7309006/diff/5003/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/7309006/diff/5003/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:933: bool HasInternalURL(const NavigationEntry
*entry) const;
It looks like this doesn't actually have to be a member. Can you make it
function in an anon namespace in browser.cc to reduce the scope (browser.h is
too large).

[Alexei Svitkine (slow), 2011-07-06 19:00:05]

http://codereview.chromium.org/7309006/diff/5003/chrome/browser/ui/browser.h
File chrome/browser/ui/browser.h (right):

http://codereview.chromium.org/7309006/diff/5003/chrome/browser/ui/browser.h#...
chrome/browser/ui/browser.h:933: bool HasInternalURL(const NavigationEntry
*entry) const;
On 2011/07/06 16:55:24, brettw wrote:
> It looks like this doesn't actually have to be a member. Can you make it
> function in an anon namespace in browser.cc to reduce the scope (browser.h is
> too large).

Done.

[msw, 2011-07-06 19:06:56]

LGTM.

[brettw, 2011-07-06 22:20:20]

LGTM

[commit-bot: I haz the power, 2011-07-07 01:07:22]

Change committed as 91647