Implement page action popups.

Implement page action popups.

A random side effect of this change is that it changes page action hover from bubble to tool tip.

BUG=24645
TEST=ExtensionApiTest.PageActionPopup

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

[Matt Perry, 2009-10-30 20:34:42]

http://codereview.chromium.org/347016/diff/3002/6003
File chrome/browser/extensions/page_action_apitest.cc (right):

http://codereview.chromium.org/347016/diff/3002/6003#newcode95
Line 95: IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PageActionPopup) {
looks like this test should be ifdefd to windows only.

http://codereview.chromium.org/347016/diff/3002/6003#newcode98
Line 98: ExtensionsService* service =
browser()->profile()->GetExtensionsService();
indent

http://codereview.chromium.org/347016/diff/3002/6008
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/347016/diff/3002/6008#newcode1126
Line 1126: return;
test code?

http://codereview.chromium.org/347016/diff/3002/6008#newcode1445
Line 1445: int current = 0;
nit: use size_t

http://codereview.chromium.org/347016/diff/3002/6008#newcode1462
Line 1462: int current = 0;
nit: use size_t

http://codereview.chromium.org/347016/diff/3002/6010
File chrome/test/data/extensions/api_test/page_action_popup/background.html
(right):

http://codereview.chromium.org/347016/diff/3002/6010#newcode8
Line 8: window.setTimeout(chrome.test.notifyPass, 300);
Is this to give time for the page action to show? This is almost guaranteed to
cause flakiness on the bots. Maybe there's some way you can listen for some
notification in the C++ to wait for the event you want.

[Erik does not do reviews, 2009-10-30 22:55:31]

http://codereview.chromium.org/347016/diff/3002/6003
File chrome/browser/extensions/page_action_apitest.cc (right):

http://codereview.chromium.org/347016/diff/3002/6003#newcode95
Line 95: IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PageActionPopup) {
On 2009/10/30 20:34:42, Matt Perry wrote:
> looks like this test should be ifdefd to windows only.

Done.

http://codereview.chromium.org/347016/diff/3002/6003#newcode98
Line 98: ExtensionsService* service =
browser()->profile()->GetExtensionsService();
On 2009/10/30 20:34:42, Matt Perry wrote:
> indent

Done.

http://codereview.chromium.org/347016/diff/3002/6008
File chrome/browser/views/location_bar_view.cc (right):

http://codereview.chromium.org/347016/diff/3002/6008#newcode1126
Line 1126: return;
On 2009/10/30 20:34:42, Matt Perry wrote:
> test code?

good catch.  This was the trivial/temporary fix to switch from bubbles to
tooltips for page actions, but it also breaks the SSL icon.  I have the right
fix uploaded now.

http://codereview.chromium.org/347016/diff/3002/6008#newcode1445
Line 1445: int current = 0;
On 2009/10/30 20:34:42, Matt Perry wrote:
> nit: use size_t

Done.

http://codereview.chromium.org/347016/diff/3002/6008#newcode1462
Line 1462: int current = 0;
On 2009/10/30 20:34:42, Matt Perry wrote:
> nit: use size_t

Done.

http://codereview.chromium.org/347016/diff/3002/6010
File chrome/test/data/extensions/api_test/page_action_popup/background.html
(right):

http://codereview.chromium.org/347016/diff/3002/6010#newcode8
Line 8: window.setTimeout(chrome.test.notifyPass, 300);
On 2009/10/30 20:34:42, Matt Perry wrote:
> Is this to give time for the page action to show? This is almost guaranteed to
> cause flakiness on the bots. Maybe there's some way you can listen for some
> notification in the C++ to wait for the event you want.

Yes.  This is what I was complaining about on IRC, that without the callback
notifying me when it actually became visible, it's hard to test this.  Based on
our conversation, I wound up adding a notification and handling this on the C++
side.

[Matt Perry, 2009-10-30 23:11:42]

LGTM

http://codereview.chromium.org/347016/diff/4011/5006
File chrome/browser/extensions/page_action_apitest.cc (right):

http://codereview.chromium.org/347016/diff/4011/5006#newcode157
Line 157: ASSERT_TRUE(last_visibility_);
nit: use EXPECT_EQ for failure modes that don't invalidate the rest of the test.

http://codereview.chromium.org/347016/diff/4011/5014
File chrome/test/data/extensions/api_test/page_action_popup/background.html
(right):

http://codereview.chromium.org/347016/diff/4011/5014#newcode3
Line 3: chrome.test.notifyPass();
seems strange that this is first. I guess it doesn't matter what order it
appears in?

[Erik does not do reviews, 2009-10-30 23:30:01]

http://codereview.chromium.org/347016/diff/4011/5006
File chrome/browser/extensions/page_action_apitest.cc (right):

http://codereview.chromium.org/347016/diff/4011/5006#newcode157
Line 157: ASSERT_TRUE(last_visibility_);
On 2009/10/30 23:11:42, Matt Perry wrote:
> nit: use EXPECT_EQ for failure modes that don't invalidate the rest of the
test.

Done.

http://codereview.chromium.org/347016/diff/4011/5014
File chrome/test/data/extensions/api_test/page_action_popup/background.html
(right):

http://codereview.chromium.org/347016/diff/4011/5014#newcode3
Line 3: chrome.test.notifyPass();
On 2009/10/30 23:11:42, Matt Perry wrote:
> seems strange that this is first. I guess it doesn't matter what order it
> appears in?

Yeah, agree that it looks weird and that it shouldn't matter.  I guess I was
being overly cautious given that the order on the C++ side is to first wait for
the extension to say that it succeeded loading (notifyPass) and then to wait for
the page action to show.  However, even if somehow the notifications came in in
the opposite order, the code should still work.