Allow extensions to add, remove, or change their page action popup.

Allow extensions to add, remove, or change their page action popup.

A similar change will be made for browser action popups.

BUG=27526
TEST=Added unit tests.  Manual testing on linux.

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

[Aaron Boodman, 2010-01-15 03:11:43]

http://codereview.chromium.org/545068/diff/1/3
File chrome/browser/extensions/extension_page_actions_module.cc (right):

http://codereview.chromium.org/545068/diff/1/3#newcode204
chrome/browser/extensions/extension_page_actions_module.cc:204: // TODO:
Consider allowing null and undefined to mean the popup
Nit: We usually do TODO(skerner): ....

http://codereview.chromium.org/545068/diff/1/3#newcode213
chrome/browser/extensions/extension_page_actions_module.cc:213:
DCHECK(GetExtension());
This DCHECK seems unnecessary.

http://codereview.chromium.org/545068/diff/1/3#newcode216
chrome/browser/extensions/extension_page_actions_module.cc:216:
DCHECK(popup_string.empty() || page_action_->has_popup())
These also seem unnecessary.

http://codereview.chromium.org/545068/diff/1/5
File chrome/browser/extensions/page_action_apitest.cc (right):

http://codereview.chromium.org/545068/diff/1/5#newcode97
chrome/browser/extensions/page_action_apitest.cc:97: // Test that calling
chrome.pageAction.setPopup() can enable a popup.
You mean 'can remove a popup'

http://codereview.chromium.org/545068/diff/1/5#newcode99
chrome/browser/extensions/page_action_apitest.cc:99: // Load the extension,
which has no default popup.
You mean "which as a default popup... the background page will remove it
programmatically"

http://codereview.chromium.org/545068/diff/1/9
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/545068/diff/1/9#newcode443
chrome/common/extensions/extension.cc:443: // For backward compatibility, alias
old key "popup" to new key "default_popup".
line length

http://codereview.chromium.org/545068/diff/1/9#newcode458
chrome/common/extensions/extension.cc:458: if
(extension_action->HasKey(popup_key) &&
At this point you already know extension_action has key popup_key.

[Erik does not do reviews, 2010-01-15 17:20:38]

http://codereview.chromium.org/545068/diff/1/3
File chrome/browser/extensions/extension_page_actions_module.cc (right):

http://codereview.chromium.org/545068/diff/1/3#newcode210
chrome/browser/extensions/extension_page_actions_module.cc:210: if
(!popup_string.empty()) {
nit: remove {} around one line bodies

http://codereview.chromium.org/545068/diff/1/3#newcode214
chrome/browser/extensions/extension_page_actions_module.cc:214:
page_action_->set_popup_url(popup_url);
You also need to fix extension_action to take tab_id as an argument and then to
be dynamic based on that.

http://codereview.chromium.org/545068/diff/1/4
File chrome/browser/extensions/extension_page_actions_module.h (right):

http://codereview.chromium.org/545068/diff/1/4#newcode61
chrome/browser/extensions/extension_page_actions_module.h:61: class
PageActionSetPopupFunction : public PageActionFunction {
It looks like in this file, we haven't followed style requirements to have a
comment above every class.  I know it's not your fault, but while you're in
here, could you add simple comments to these classes?

http://codereview.chromium.org/545068/diff/1/5
File chrome/browser/extensions/page_action_apitest.cc (right):

http://codereview.chromium.org/545068/diff/1/5#newcode98
chrome/browser/extensions/page_action_apitest.cc:98:
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PageActionRemovePopup) {
the bummer about this test as written is that it doesn't verify that there was a
popup before it was removed.  it would be nice if you could test that.

http://codereview.chromium.org/545068/diff/1/9
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/545068/diff/1/9#newcode440
chrome/common/extensions/extension.cc:440: if
(extension_action->HasKey(keys::kPageActionDefaultPopup)) {
nit: remove {}

http://codereview.chromium.org/545068/diff/1/9#newcode471
chrome/common/extensions/extension.cc:471: GURL url = GetResourceURL(url_str);
this code is copied and pasted below.  Just end the if block before this line
and share the code.

http://codereview.chromium.org/545068/diff/1/18
File
chrome/test/data/extensions/api_test/page_action/remove_popup/background.html
(right):

http://codereview.chromium.org/545068/diff/1/18#newcode12
chrome/test/data/extensions/api_test/page_action/remove_popup/background.html:12:
chrome.pageAction.onClicked.addListener(function(tab) {
remove unused code

[Sam Kerner (Chrome), 2010-01-19 19:30:06]

http://codereview.chromium.org/545068/diff/1/3
File chrome/browser/extensions/extension_page_actions_module.cc (right):

http://codereview.chromium.org/545068/diff/1/3#newcode204
chrome/browser/extensions/extension_page_actions_module.cc:204: // TODO:
Consider allowing null and undefined to mean the popup
On 2010/01/15 03:11:43, Aaron Boodman wrote:
> Nit: We usually do TODO(skerner): ....

Done.

http://codereview.chromium.org/545068/diff/1/3#newcode210
chrome/browser/extensions/extension_page_actions_module.cc:210: if
(!popup_string.empty()) {
On 2010/01/15 17:20:38, Erik Kay wrote:
> nit: remove {} around one line bodies

Done.

http://codereview.chromium.org/545068/diff/1/3#newcode213
chrome/browser/extensions/extension_page_actions_module.cc:213:
DCHECK(GetExtension());
On 2010/01/15 03:11:43, Aaron Boodman wrote:
> This DCHECK seems unnecessary.

Yes, it is a bit late to check this...  Removed.

http://codereview.chromium.org/545068/diff/1/3#newcode216
chrome/browser/extensions/extension_page_actions_module.cc:216:
DCHECK(popup_string.empty() || page_action_->has_popup())
On 2010/01/15 03:11:43, Aaron Boodman wrote:
> These also seem unnecessary.

Done.

http://codereview.chromium.org/545068/diff/1/4
File chrome/browser/extensions/extension_page_actions_module.h (right):

http://codereview.chromium.org/545068/diff/1/4#newcode61
chrome/browser/extensions/extension_page_actions_module.h:61: class
PageActionSetPopupFunction : public PageActionFunction {
On 2010/01/15 17:20:38, Erik Kay wrote:
> It looks like in this file, we haven't followed style requirements to have a
> comment above every class.  I know it's not your fault, but while you're in
> here, could you add simple comments to these classes?

Done.

http://codereview.chromium.org/545068/diff/1/5
File chrome/browser/extensions/page_action_apitest.cc (right):

http://codereview.chromium.org/545068/diff/1/5#newcode97
chrome/browser/extensions/page_action_apitest.cc:97: // Test that calling
chrome.pageAction.setPopup() can enable a popup.
On 2010/01/15 03:11:43, Aaron Boodman wrote:
> You mean 'can remove a popup'

Done.

http://codereview.chromium.org/545068/diff/1/5#newcode98
chrome/browser/extensions/page_action_apitest.cc:98:
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PageActionRemovePopup) {
On 2010/01/15 17:20:38, Erik Kay wrote:
> the bummer about this test as written is that it doesn't verify that there was
a
> popup before it was removed.  it would be nice if you could test that.

Done.

http://codereview.chromium.org/545068/diff/1/5#newcode99
chrome/browser/extensions/page_action_apitest.cc:99: // Load the extension,
which has no default popup.
On 2010/01/15 03:11:43, Aaron Boodman wrote:
> You mean "which as a default popup... the background page will remove it
> programmatically"

Done.

http://codereview.chromium.org/545068/diff/1/9
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/545068/diff/1/9#newcode440
chrome/common/extensions/extension.cc:440: if
(extension_action->HasKey(keys::kPageActionDefaultPopup)) {
On 2010/01/15 17:20:38, Erik Kay wrote:
> nit: remove {}

Done.

http://codereview.chromium.org/545068/diff/1/9#newcode443
chrome/common/extensions/extension.cc:443: // For backward compatibility, alias
old key "popup" to new key "default_popup".
On 2010/01/15 03:11:43, Aaron Boodman wrote:
> line length

Done.

http://codereview.chromium.org/545068/diff/1/9#newcode458
chrome/common/extensions/extension.cc:458: if
(extension_action->HasKey(popup_key) &&
On 2010/01/15 03:11:43, Aaron Boodman wrote:
> At this point you already know extension_action has key popup_key.

Done.

http://codereview.chromium.org/545068/diff/1/9#newcode471
chrome/common/extensions/extension.cc:471: GURL url = GetResourceURL(url_str);
On 2010/01/15 17:20:38, Erik Kay wrote:
> this code is copied and pasted below.  Just end the if block before this line
> and share the code.

Done.

http://codereview.chromium.org/545068/diff/1/18
File
chrome/test/data/extensions/api_test/page_action/remove_popup/background.html
(right):

http://codereview.chromium.org/545068/diff/1/18#newcode12
chrome/test/data/extensions/api_test/page_action/remove_popup/background.html:12:
chrome.pageAction.onClicked.addListener(function(tab) {
On 2010/01/15 17:20:38, Erik Kay wrote:
> remove unused code

Done.

[Sam Kerner (Chrome), 2010-01-19 19:42:57]

http://codereview.chromium.org/545068/diff/1/3
File chrome/browser/extensions/extension_page_actions_module.cc (right):

http://codereview.chromium.org/545068/diff/1/3#newcode204
chrome/browser/extensions/extension_page_actions_module.cc:204: // TODO:
Consider allowing null and undefined to mean the popup
On 2010/01/15 03:11:43, Aaron Boodman wrote:
> Nit: We usually do TODO(skerner): ....

What do you think of using {popup: undefined} or {popup: null} to mean there
should be no popup?  I expect many developers will try {popup: undefined}, but I
hate to create three different constants ('', null,  undefined) that do the same
thing.

http://codereview.chromium.org/545068/diff/1/3#newcode214
chrome/browser/extensions/extension_page_actions_module.cc:214:
page_action_->set_popup_url(popup_url);
On 2010/01/15 17:20:38, Erik Kay wrote:
> You also need to fix extension_action to take tab_id as an argument and then
to
> be dynamic based on that.

Still working on this....

[Erik does not do reviews, 2010-01-19 20:37:33]

http://codereview.chromium.org/545068/diff/3002/3010
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/545068/diff/3002/3010#newcode475
chrome/common/extensions/extension.cc:475: // An empty string is treated as
having no popup.
doesn't this mean that there should be an else clause where the popup is
cleared?

[Finnur, 2010-01-19 21:31:35]

http://codereview.chromium.org/545068/diff/3002/3004
File chrome/browser/extensions/extension_page_actions_module.cc (right):

http://codereview.chromium.org/545068/diff/3002/3004#newcode214
chrome/browser/extensions/extension_page_actions_module.cc:214: 
nit: If you want to be consistent with other functions, then no linebreak. :)

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

http://codereview.chromium.org/545068/diff/3002/3006#newcode94
chrome/browser/extensions/page_action_apitest.cc:94:
ASSERT_STREQ("/a_popup.html", page_action->popup_url().path().c_str());
With this and below, you've covered:
None -> Popup A
Popup B -> None.
Why not test Popup A -> Popup B also?

http://codereview.chromium.org/545068/diff/3002/3006#newcode105
chrome/browser/extensions/page_action_apitest.cc:105: ASSERT_TRUE(extension);
Wondering: This now become the fifth function in this file to do the above 5
lines of code. Should we consolidate this into one function that takes the
extension test name as a param?

http://codereview.chromium.org/545068/diff/3002/3007
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/545068/diff/3002/3007#newcode1009
chrome/common/extensions/api/extension_api.json:1009: "description": "Sets the
html document to be opened when the user clicks on the page action's icon.",
nit: Can you say: "Sets the html document (popup) to be opened..."?

http://codereview.chromium.org/545068/diff/3002/3007#newcode1029
chrome/common/extensions/api/extension_api.json:1029: "description": "Fired when
a page action icon is clicked.  However, this event will not fire if a popup is
set.",
nit: Would it be clearer to change this to:
Fired when a page action icon is clicked if the page action doesn't show a
popup.
?

http://codereview.chromium.org/545068/diff/3002/3010
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/545068/diff/3002/3010#newcode468
chrome/common/extensions/extension.cc:468: 
What does this mean? Will we not have a popup? Should we return an error? At a
minimum this should be documented.

Also, nit: Don't need this linebreak.

http://codereview.chromium.org/545068/diff/3002/3010#newcode475
chrome/common/extensions/extension.cc:475: // An empty string is treated as
having no popup.
Erik: This is creating a new object so this string will be empty to begin with.

On 2010/01/19 20:37:33, Erik Kay wrote:
> doesn't this mean that there should be an else clause where the popup is
> cleared?

http://codereview.chromium.org/545068/diff/3002/3011
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/545068/diff/3002/3011#newcode145
chrome/common/extensions/extension_constants.cc:145: "Can\'t use keys \"*\" and
\"*\" at the same time.";
Should we suggest using default_popup and removing popup, instead of leaving it
up to the developer to figure out what the recommended practice is?

http://codereview.chromium.org/545068/diff/3002/3013
File chrome/common/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/545068/diff/3002/3013#newcode511
chrome/common/extensions/extension_unittest.cc:511:
ASSERT_FALSE(action->has_popup());
Don't you want to test that the popup path is blank?

http://codereview.chromium.org/545068/diff/3002/3013#newcode519
chrome/common/extensions/extension_unittest.cc:519:
ASSERT_FALSE(action->has_popup());
Ditto.

[Finnur, 2010-01-19 21:32:53]

Also: PageAction browser tests are failing on Linux, it seems.

[Erik does not do reviews, 2010-01-19 22:22:11]

Sam, based on your other comment, I'm assuming the per-tab behavior is still
coming in this review, so I'm still waiting on that for the lg.

http://codereview.chromium.org/545068/diff/3002/3010
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/545068/diff/3002/3010#newcode475
chrome/common/extensions/extension.cc:475: // An empty string is treated as
having no popup.
On 2010/01/19 21:31:35, Finnur wrote:
> Erik: This is creating a new object so this string will be empty to begin
with.
> 
> On 2010/01/19 20:37:33, Erik Kay wrote:
> > doesn't this mean that there should be an else clause where the popup is
> > cleared?
> 
> 

Ahh, good point.  Thanks.

[Sam Kerner (Chrome), 2010-01-20 22:21:56]

http://codereview.chromium.org/545068/diff/3002/3004
File chrome/browser/extensions/extension_page_actions_module.cc (right):

http://codereview.chromium.org/545068/diff/3002/3004#newcode214
chrome/browser/extensions/extension_page_actions_module.cc:214: 
On 2010/01/19 21:31:35, Finnur wrote:
> nit: If you want to be consistent with other functions, then no linebreak. :)

Done.

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

http://codereview.chromium.org/545068/diff/3002/3006#newcode94
chrome/browser/extensions/page_action_apitest.cc:94:
ASSERT_STREQ("/a_popup.html", page_action->popup_url().path().c_str());
On 2010/01/19 21:31:35, Finnur wrote:
> With this and below, you've covered:
> None -> Popup A
> Popup B -> None.
> Why not test Popup A -> Popup B also?

Added it to this test.

http://codereview.chromium.org/545068/diff/3002/3006#newcode105
chrome/browser/extensions/page_action_apitest.cc:105: ASSERT_TRUE(extension);
On 2010/01/19 21:31:35, Finnur wrote:
> Wondering: This now become the fifth function in this file to do the above 5
> lines of code. Should we consolidate this into one function that takes the
> extension test name as a param?

The name for such a function would be long.  Something like
RunExtensionAndGetSingleExtensionPtr().  How about a new method that does what
lines 102-105 do, like this:

IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PageActionRemovePopup) {
  ASSERT_TRUE(RunExtensionTest("page_action/remove_popup")) << message_;
  Extension* extension;
  ASSERT_TRUE(GetSingleExtension(&extension)) << message_;
  ...
}

Mind if I add a TODO, and fix this as a separate change?  If it helps other
tests, I might end up touching many files.

http://codereview.chromium.org/545068/diff/3002/3007
File chrome/common/extensions/api/extension_api.json (right):

http://codereview.chromium.org/545068/diff/3002/3007#newcode1009
chrome/common/extensions/api/extension_api.json:1009: "description": "Sets the
html document to be opened when the user clicks on the page action's icon.",
On 2010/01/19 21:31:35, Finnur wrote:
> nit: Can you say: "Sets the html document (popup) to be opened..."?

I went with: "Sets the html document to be opened as a popup when the user ...".
 Will reconsider if people object.

http://codereview.chromium.org/545068/diff/3002/3007#newcode1029
chrome/common/extensions/api/extension_api.json:1029: "description": "Fired when
a page action icon is clicked.  However, this event will not fire if a popup is
set.",
On 2010/01/19 21:31:35, Finnur wrote:
> nit: Would it be clearer to change this to:
> Fired when a page action icon is clicked if the page action doesn't show a
> popup.
> ?

Done.  That is definitely better.

http://codereview.chromium.org/545068/diff/3002/3010
File chrome/common/extensions/extension.cc (right):

http://codereview.chromium.org/545068/diff/3002/3010#newcode468
chrome/common/extensions/extension.cc:468: 
On 2010/01/19 21:31:35, Finnur wrote:
> What does this mean? Will we not have a popup? Should we return an error? At a
> minimum this should be documented.

I was trying to reproduce and clarify the comment on line 448 (before my
change), but I said the opposite of what the old comment said.  At some point,
popups were specified like this:

{
  page_action: {
    popup: {
      path: "popup.html"
    }
  }
}

Comment fixed.


> Also, nit: Don't need this linebreak.

Done.

http://codereview.chromium.org/545068/diff/3002/3010#newcode475
chrome/common/extensions/extension.cc:475: // An empty string is treated as
having no popup.
On 2010/01/19 22:22:11, Erik Kay wrote:
> On 2010/01/19 21:31:35, Finnur wrote:
> > Erik: This is creating a new object so this string will be empty to begin
> with.
> > 
> > On 2010/01/19 20:37:33, Erik Kay wrote:
> > > doesn't this mean that there should be an else clause where the popup is
> > > cleared?
> > 
> > 
> 
> Ahh, good point.  Thanks.
> 

Fortune favors the paranoid.  Added a DCHECK.

http://codereview.chromium.org/545068/diff/3002/3011
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/545068/diff/3002/3011#newcode145
chrome/common/extensions/extension_constants.cc:145: "Can\'t use keys \"*\" and
\"*\" at the same time.";
On 2010/01/19 21:31:35, Finnur wrote:
> Should we suggest using default_popup and removing popup, instead of leaving
it
> up to the developer to figure out what the recommended practice is?

Done.

http://codereview.chromium.org/545068/diff/3002/3013
File chrome/common/extensions/extension_unittest.cc (right):

http://codereview.chromium.org/545068/diff/3002/3013#newcode511
chrome/common/extensions/extension_unittest.cc:511:
ASSERT_FALSE(action->has_popup());
On 2010/01/19 21:31:35, Finnur wrote:
> Don't you want to test that the popup path is blank?

Done.

http://codereview.chromium.org/545068/diff/3002/3013#newcode519
chrome/common/extensions/extension_unittest.cc:519:
ASSERT_FALSE(action->has_popup());
On 2010/01/19 21:31:35, Finnur wrote:
> Ditto.

Done.

[Finnur, 2010-01-21 01:14:51]

LGTM, with nits

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

http://codereview.chromium.org/545068/diff/3002/3006#newcode105
chrome/browser/extensions/page_action_apitest.cc:105: ASSERT_TRUE(extension);
That's fine.

http://codereview.chromium.org/545068/diff/8002/2012#newcode98
chrome/browser/extensions/page_action_apitest.cc:98:
IN_PROC_BROWSER_TEST_F(ExtensionApiTest, PageActionRemovePopup) {
Second line a copy-paste error?

http://codereview.chromium.org/545068/diff/8002/2017
File chrome/common/extensions/extension_constants.cc (right):

http://codereview.chromium.org/545068/diff/8002/2017#newcode139
chrome/common/extensions/extension_constants.cc:139: "Key \"*\" is deprecated. 
Key \"*\" has the same meaning.  You can not use both.";
Instead of "has the same meaning" use "is preferred"?

[skerner, 2010-01-21 04:04:42]

On Tue, Jan 19, 2010 at 5:22 PM,  <erikkay@chromium.org> wrote:
> Sam, based on your other comment, I'm assuming the per-tab behavior is still
> coming in this review, so I'm still waiting on that for the lg.

Erik,
   I will be sending out a new CL which adds per-tab behavior, and I
will submit it before this change.  Making popups per-tab touches a
few more files, and this change is large enough already.

Sam

[Erik does not do reviews, 2010-01-21 15:57:42]

OK.  LGTM.

Erik


On Wed, Jan 20, 2010 at 8:04 PM, Sam Kerner <skerner@google.com> wrote:

> On Tue, Jan 19, 2010 at 5:22 PM,  <erikkay@chromium.org> wrote:
> > Sam, based on your other comment, I'm assuming the per-tab behavior is
> still
> > coming in this review, so I'm still waiting on that for the lg.
>
> Erik,
>   I will be sending out a new CL which adds per-tab behavior, and I
> will submit it before this change.  Making popups per-tab touches a
> few more files, and this change is large enough already.
>
> Sam
>