Integrate browser actions with the wrench menu. Browser actions

Integrate browser actions with the wrench menu. Browser 
actions always show up in a submenu of the wrench menu, and
if they have an icon, they also show up in the toolbar area.

BUG=23380, 22883
TEST=Added new automated tests for the command handling, but we need to test that the menu items show up manually. To do that, run with no extension installed, you should see "extensions" in the wrench menu. Add an extension that adds a browser action, you should now see an "extensions" submenu with "manage extensions" and the browser action(s) in the submenu.

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

[Finnur, 2009-09-29 23:58:38]

Overall, looks good. Some questions and comments below.
One general question: Should we add a browser test to see if the menu appears?
Hmm... Now I realize I think I forgot to add similar tests as I did with the
PageAction. :/

http://codereview.chromium.org/246037/diff/2001/3002
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/246037/diff/2001/3002#newcode760
Line 760: E&xtensions
Um... 'x' is already taken by the Exit command, no?
The only letter available from the word Extensions is 's'. :\
Press Alt+F to see.

http://codereview.chromium.org/246037/diff/2001/3003
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/246037/diff/2001/3003#newcode1267
Line 1267: AddTabWithURL(GURL("chrome://extensions/"), GURL(),
chrome://extensions/ is defined in url_constants.cc

http://codereview.chromium.org/246037/diff/2001/3003#newcode1519
Line 1519: DCHECK(service);  // no browser action command should have been
created
nit: Capitalize the 'n' at the start.

http://codereview.chromium.org/246037/diff/2001/3003#newcode1536
Line 1536: LOG(WARNING) << "Unknown browser action executed: " << id;
Shouldn't we DCHECK then? Or make this a NOTRECHED()?

http://codereview.chromium.org/246037/diff/2001/3005
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/246037/diff/2001/3005#newcode143
Line 143: DeleteBrowserActionViews();
We are now deleting the views all the time. Isn't it better to count the number
of browser_actions that have icons and compare that to the number of views
currently displayed?

http://codereview.chromium.org/246037/diff/2001/3006
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/246037/diff/2001/3006#newcode410
Line 410: command_id <= IDC_BROWSER_ACTION_LAST)
Why not do this inside browser.cc, such as in InitCommandState or similar
functions? Seems like it would be advisable to keep the menu enabling code all
together.

http://codereview.chromium.org/246037/diff/2001/3006#newcode1086
Line 1086: browser_->profile()->GetExtensionsService();
I'm curious... Have you've tested Incognito mode and the tests related to it?

http://codereview.chromium.org/246037/diff/2001/3006#newcode1102
Line 1102: DCHECK(false);
NOTREACHED() << "Too many browser actions";  ?

http://codereview.chromium.org/246037/diff/2001/3006#newcode1106
Line 1106: UTF8ToUTF16(browser_actions[i]->name()));
I'm used to seeing AsciiToWide as opposed to this function. Is this function
more appropriate in this context somehow?

http://codereview.chromium.org/246037/diff/2001/3006#newcode1106
Line 1106: UTF8ToUTF16(browser_actions[i]->name()));
Don't we need to start enforcing a maximum name length at this point? Or is that
handled by the menu system? We should document any max length.

http://codereview.chromium.org/246037/diff/2001/3008
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/246037/diff/2001/3008#newcode333
Line 333: }
What's the story here with showing the icon on the menu? It seems like since we
have a nicely sized icon we should show it in the menu. However, I can see
people wanting to have an icon in the menu yet not necessarily want to put the
icon in people's faces by adding it to the chrome. Aren't we forcing people who
just want a menu icon to also have it in the toolbar?

[Aaron Boodman, 2009-09-30 02:50:37]

Okie, responded to all comments, plus added two browser tests.

http://codereview.chromium.org/246037/diff/2001/3002
File chrome/app/generated_resources.grd (right):

http://codereview.chromium.org/246037/diff/2001/3002#newcode760
Line 760: E&xtensions
On 2009/09/29 23:58:38, Finnur wrote:
> Um... 'x' is already taken by the Exit command, no?
> The only letter available from the word Extensions is 's'. :\
> Press Alt+F to see.

Thanks for the Alt+F tip, I didn't know about that. I don't think that it
matters to have them overlap, it just means you have to press the letter
multiple times to cycle through the options.

Since I think exit is probably more frequently used than help, I'll change it to
E to avoid breaking people's muscle memory.

http://codereview.chromium.org/246037/diff/2001/3003
File chrome/browser/browser.cc (right):

http://codereview.chromium.org/246037/diff/2001/3003#newcode1267
Line 1267: AddTabWithURL(GURL("chrome://extensions/"), GURL(),
On 2009/09/29 23:58:38, Finnur wrote:
> chrome://extensions/ is defined in url_constants.cc

Done.

http://codereview.chromium.org/246037/diff/2001/3003#newcode1519
Line 1519: DCHECK(service);  // no browser action command should have been
created
On 2009/09/29 23:58:38, Finnur wrote:
> nit: Capitalize the 'n' at the start.

Done.

http://codereview.chromium.org/246037/diff/2001/3003#newcode1536
Line 1536: LOG(WARNING) << "Unknown browser action executed: " << id;
On 2009/09/29 23:58:38, Finnur wrote:
> Shouldn't we DCHECK then? Or make this a NOTRECHED()?

No the comment is intended to communicate that this really can happen, but in
the real world, it would be exceptionally rare to hit it.

We can't make it a NOTREACHED since really, it could happen.

http://codereview.chromium.org/246037/diff/2001/3005
File chrome/browser/views/browser_actions_container.cc (right):

http://codereview.chromium.org/246037/diff/2001/3005#newcode143
Line 143: DeleteBrowserActionViews();
On 2009/09/29 23:58:38, Finnur wrote:
> We are now deleting the views all the time. Isn't it better to count the
number
> of browser_actions that have icons and compare that to the number of views
> currently displayed? 

It isn't correct to count the number of browser actions since the set of browser
actions (or their details) could change without changing the number.

I think that we will need to modify this to be more fine-grained about updating
the particular browser actions that have changed, since as it is right now,
deleting and re-creating will cause flicker.

But since that is not really related to what I'm working on right now, I thought
this was an OK stopgap.

http://codereview.chromium.org/246037/diff/2001/3006
File chrome/browser/views/toolbar_view.cc (right):

http://codereview.chromium.org/246037/diff/2001/3006#newcode410
Line 410: command_id <= IDC_BROWSER_ACTION_LAST)
On 2009/09/29 23:58:38, Finnur wrote:
> Why not do this inside browser.cc, such as in InitCommandState or similar
> functions? Seems like it would be advisable to keep the menu enabling code all
> together.

Good point about keeping things together. We cannot do it in ::InitCommandState
since it is called at startup but the set of browser actions can change anytime.
I've move the code to enable and disable browser actions to the ::Observe()
method of Browser, so that it can be updated as extensions are loaded and
unloaded.

http://codereview.chromium.org/246037/diff/2001/3006#newcode1086
Line 1086: browser_->profile()->GetExtensionsService();
On 2009/09/29 23:58:38, Finnur wrote:
> I'm curious... Have you've tested Incognito mode and the tests related to it?

Yes, it seems to work.

http://codereview.chromium.org/246037/diff/2001/3006#newcode1106
Line 1106: UTF8ToUTF16(browser_actions[i]->name()));
On 2009/09/29 23:58:38, Finnur wrote:
> I'm used to seeing AsciiToWide as opposed to this function. Is this function
> more appropriate in this context somehow?

AddItem() wants string16, not wstring. I'm not sure why we have string16 and
wstring. It looks to me from the code like wstring is preferred, but I still
find string16 from time to time.

http://codereview.chromium.org/246037/diff/2001/3006#newcode1106
Line 1106: UTF8ToUTF16(browser_actions[i]->name()));
On 2009/09/29 23:58:38, Finnur wrote:
> Don't we need to start enforcing a maximum name length at this point? Or is
that
> handled by the menu system? We should document any max length.

I tried it with a supa long name. It seems to be automatically clipped by the
menu system (either Windows or something in our code lower-level).

http://codereview.chromium.org/246037/diff/2001/3008
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/246037/diff/2001/3008#newcode333
Line 333: }
On 2009/09/29 23:58:38, Finnur wrote:
> What's the story here with showing the icon on the menu? It seems like since
we
> have a nicely sized icon we should show it in the menu. However, I can see
> people wanting to have an icon in the menu yet not necessarily want to put the
> icon in people's faces by adding it to the chrome. Aren't we forcing people
who
> just want a menu icon to also have it in the toolbar?

I think that icons in menus tend to look pretty awful. I think that we should
just not show the icons in the menu and use the presence of an icon as a signal
as to whether to show up in the toolbar area.

[Finnur, 2009-09-30 03:10:49]

You might want to add a TEST= line, even just to call attention to the fact that
you added automated tests for this (thanks for that, btw).

http://codereview.chromium.org/246037/diff/4002/7006
File chrome/browser/extensions/browser_action_test.cc (right):

http://codereview.chromium.org/246037/diff/4002/7006#newcode7
Line 7: #include "chrome/browser/profile.h"
nit: This is not in the right alphabetical order.

http://codereview.chromium.org/246037/diff/4002/7006#newcode33
Line 33: &result);
I worry what happens to this test when this fails. It will hang the browser for
a while, no? Why not send false when the bgColor is not red?

[Aaron Boodman, 2009-09-30 03:29:04]

On 2009/09/30 03:10:49, Finnur wrote:
> You might want to add a TEST= line, even just to call attention to the fact
that
> you added automated tests for this (thanks for that, btw).

Good point. Done.

> http://codereview.chromium.org/246037/diff/4002/7006
> File chrome/browser/extensions/browser_action_test.cc (right):
> 
> http://codereview.chromium.org/246037/diff/4002/7006#newcode7
> Line 7: #include "chrome/browser/profile.h"
> nit: This is not in the right alphabetical order.

Done.

> http://codereview.chromium.org/246037/diff/4002/7006#newcode33
> Line 33: &result);
> I worry what happens to this test when this fails. It will hang the browser
for
> a while, no? Why not send false when the bgColor is not red?

The test needs to wait for the page to turn red since the code to do the check
runs before the code that changes the color (because the extension is in another
process). I prefer to do tests that have to wait like this without any timeout
to avoid flakiness. It is better (IMO) that a failure shows up as a timeout
after 5 minutes or whatever, rather than having a flakey test because the
in-code timeout was too tight.

[Finnur, 2009-09-30 04:02:20]

OK, LG.

On 2009/09/30 03:29:04, Aaron Boodman wrote:
> On 2009/09/30 03:10:49, Finnur wrote:
> > You might want to add a TEST= line, even just to call attention to the fact
> that
> > you added automated tests for this (thanks for that, btw).
> 
> Good point. Done.
> 
> > http://codereview.chromium.org/246037/diff/4002/7006
> > File chrome/browser/extensions/browser_action_test.cc (right):
> > 
> > http://codereview.chromium.org/246037/diff/4002/7006#newcode7
> > Line 7: #include "chrome/browser/profile.h"
> > nit: This is not in the right alphabetical order.
> 
> Done.
> 
> > http://codereview.chromium.org/246037/diff/4002/7006#newcode33
> > Line 33: &result);
> > I worry what happens to this test when this fails. It will hang the browser
> for
> > a while, no? Why not send false when the bgColor is not red?
> 
> The test needs to wait for the page to turn red since the code to do the check
> runs before the code that changes the color (because the extension is in
another
> process). I prefer to do tests that have to wait like this without any timeout
> to avoid flakiness. It is better (IMO) that a failure shows up as a timeout
> after 5 minutes or whatever, rather than having a flakey test because the
> in-code timeout was too tight.