Remove the implicit wrench menu items for browser actions.

Remove the implicit wrench menu items for browser actions.

Also, allow browser actions with no initial icons, and add
some better tests.

BUG=24379, 24671
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=29095

[Erik does not do reviews, 2009-10-13 23:18:28]

http://codereview.chromium.org/276010/diff/44/3004
File chrome/browser/extensions/browser_action_apitest.cc (right):

http://codereview.chromium.org/276010/diff/44/3004#newcode35
Line 35: ExtensionActionState* action_state = extension->browser_action_state();
Isn't this going to be racy? The navigation finishes, and then the background
page gets the notification and then it sends a message back to the browser. 
Seems like you could hit this line before the last bits happen.  This is where
WaitForPassFail would be useful.

http://codereview.chromium.org/276010/diff/44/3004#newcode42
Line 42: // Simulate the event being clicked.
nit: event -> browser action

http://codereview.chromium.org/276010/diff/44/3004#newcode52
Line 52: TabContents* tab = browser()->GetSelectedTabContents();
same comment about races here

http://codereview.chromium.org/276010/diff/44/3004#newcode80
Line 80: ExtensionActionState* action_state = extension->browser_action_state();
race here too

[Erik does not do reviews, 2009-10-14 00:15:26]

Talked a bit about this offline.  Summary is that the racy behavior isn't what I
thought it would be.  I had originally written those comments I had thought the
test code triggered off of tab navigation events.  In reality the pages being
navigated to are the tests themselves.  The question then is whether we can
count on the JS executing before the renderer sends the FrameNavigate message
(which is sent when the first part of the response body has been received).  My
guess is that if it works once, then it will be reliable.  Perhaps if the
document was much larger, then this would change.  I don't know enough about
WebKit layout to answer that.

[Aaron Boodman, 2009-10-14 01:20:52]

On 2009/10/13 23:18:28, Erik Kay wrote:
> http://codereview.chromium.org/276010/diff/44/3004
> File chrome/browser/extensions/browser_action_apitest.cc (right):
> 
> http://codereview.chromium.org/276010/diff/44/3004#newcode35
> Line 35: ExtensionActionState* action_state =
extension->browser_action_state();
> Isn't this going to be racy? The navigation finishes, and then the background
> page gets the notification and then it sends a message back to the browser. 
> Seems like you could hit this line before the last bits happen.  This is where
> WaitForPassFail would be useful.

It definitely works locally for the reasons you describe below, but it feels
brittle. I pulled out a TestResultCatcher object that can be used in a more
flexible way than WaitForResult(), which should make it more robust.

> http://codereview.chromium.org/276010/diff/44/3004#newcode42
> Line 42: // Simulate the event being clicked.
> nit: event -> browser action

Done.

> http://codereview.chromium.org/276010/diff/44/3004#newcode52
> Line 52: TabContents* tab = browser()->GetSelectedTabContents();
> same comment about races here

There is no race here, the C++ always loses. That is why the it has that
setInterval() code in the JavaScript. It is a bit ghetto, but very reliable (it
has been enabled for awhile in the previous version of this test).

> http://codereview.chromium.org/276010/diff/44/3004#newcode80
> Line 80: ExtensionActionState* action_state =
extension->browser_action_state();
> race here too

Fixed with TestResultCatcher.

[Erik does not do reviews, 2009-10-14 23:11:02]

I really like your abstraction here.  Much cleaner than what I had!

LGTM

http://codereview.chromium.org/276010/diff/4003/2008
File chrome/browser/extensions/extension_apitest.cc (right):

http://codereview.chromium.org/276010/diff/4003/2008#newcode62
Line 62: DCHECK(false);
NOTREACHED

http://codereview.chromium.org/276010/diff/4003/2009
File chrome/browser/extensions/extension_apitest.h (right):

http://codereview.chromium.org/276010/diff/4003/2009#newcode25
Line 25: // WaitUntilComplete() method.
I don't see this method anywhere.

http://codereview.chromium.org/276010/diff/4003/2009#newcode26
Line 26: class TestResultCatcher : public NotificationObserver {
Maybe this should have a more Extension-specific name?