Implement initial chrome.browserAction.openPopup API.

Implement chrome.browserAction.openPopup API.
https://docs.google.com/a/google.com/document/d/10NtNR0R77izXUd3kWWvrIzyqIJ2ZRP8jkkzuHw7mNUY/edit

BUG=305744
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=229777

[mark a. foltz, 2013-10-07 22:08:14]

A couple of comments

https://codereview.chromium.org/25305002/diff/1/chrome/browser/chrome_notific...
File chrome/browser/chrome_notification_types.h (right):

https://codereview.chromium.org/25305002/diff/1/chrome/browser/chrome_notific...
chrome/browser/chrome_notification_types.h:553:
NOTIFICATION_EXTENSION_COMMAND_BROWSER_ACTION_SHOW,
What are the source and details for the notification?  Seems like the source
could be the extension_id and the details an ExtensionAction*.

https://codereview.chromium.org/25305002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:813:
chrome::FindOrCreateTabbedBrowser(profile_, chrome::GetActiveDesktop());
Need to decide which window gets the popup.
  
- Would FindLastActiveWithProfile be more appropriate?
- Should we create a new browser window if one does not exist?
- Do we need any handling of incognito mode profiles?

https://codereview.chromium.org/25305002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:816:
content::NotificationService::current()->Notify(
It looks like only the Mac UI handles this notification... I wonder if we need
to port that code to the other platforms.

[justinlin, 2013-10-09 18:19:32]

There's implementations for all platforms now, but still some bugs to iron out,
namely none of them popup up anything if the extension button is hidden right
now.

Also, need to fill in the reply, currently the parameter is empty. It's a bit
tricky because I'm using an observer notification right now, so there's no good
way to get whether the popup was shown directly, but maybe there's another way..

https://codereview.chromium.org/25305002/diff/1/chrome/browser/chrome_notific...
File chrome/browser/chrome_notification_types.h (right):

https://codereview.chromium.org/25305002/diff/1/chrome/browser/chrome_notific...
chrome/browser/chrome_notification_types.h:553:
NOTIFICATION_EXTENSION_COMMAND_BROWSER_ACTION_SHOW,
On 2013/10/07 22:08:14, mark a. foltz wrote:
> What are the source and details for the notification?  Seems like the source
> could be the extension_id and the details an ExtensionAction*.

no longer needed.

https://codereview.chromium.org/25305002/diff/1/chrome/browser/extensions/api...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:813:
chrome::FindOrCreateTabbedBrowser(profile_, chrome::GetActiveDesktop());
On 2013/10/07 22:08:14, mark a. foltz wrote:
> Need to decide which window gets the popup.
>   
> - Would FindLastActiveWithProfile be more appropriate?

Yes, we should use the last active.

> - Should we create a new browser window if one does not exist?

I don't think so. Seems like if would be non-user-initiated anyway if we somehow
need to popup it up when there's no window.

> - Do we need any handling of incognito mode profiles?  

Depends on the platform, but yes we should pop it up in the incognito window if
that was the last one focused.

https://codereview.chromium.org/25305002/diff/1/chrome/browser/extensions/api...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:816:
content::NotificationService::current()->Notify(
On 2013/10/07 22:08:14, mark a. foltz wrote:
> It looks like only the Mac UI handles this notification... I wonder if we need
> to port that code to the other platforms.
> 

Using an observer notification instead now.

[justinlin, 2013-10-11 17:39:54]

hi matt, mark, ptal @ patch set #5 for a first review. there's a few TODO's that
I haven't resolved yet, but the basic functionality is here. will send for
review by UI code owners if this seems OK.

[mark a. foltz, 2013-10-11 20:25:20]

On 2013/10/11 17:39:54, justinlin wrote:
> hi matt, mark, ptal @ patch set #5 for a first review. there's a few TODO's
that
> I haven't resolved yet, but the basic functionality is here. will send for
> review by UI code owners if this seems OK.

Overall looks okay, a few comments inline.  Are there unit tests to update for
extension_toolbar_model and the ui/ classes?

[mark a. foltz, 2013-10-11 20:25:30]

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:7:
#include <set>
Did you mean to remove #include <string>?

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:66: const
char* whitelisted_extensions[] = {
kOpenPopupWhitelistedExtensions

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:822: if
(VersionInfo::GetChannel() != VersionInfo::CHANNEL_UNKNOWN &&
For clarity, suggest adding a private boolean method to check the whitelist

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:826:
std::set<std::string>(
Is this an overloaded ctor for std::set that takes a char*[]?  Not familiar with
it, but cool if it works.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:848:
SetResult(new base::DictionaryValue());
Just call this at the top of RunImpl?

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.cc:162: const Extension*
extension, Browser* browser) {
const Extension& instead?

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.cc:162: const Extension*
extension, Browser* browser) {
DCHECK(browser)

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.cc:170:
GetBrowserAction(*extension);
What if the extension has no browser action?

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/extension_toolbar_model.h (right):

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.h:53: const
extensions::Extension* extension) {}
unwrap line

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.h:66: void
ShowBrowserActionPopup(const extensions::Extension* extension);
How different from BrowserActionShowPopup?  Is this part of an API called by the
view to notify the model?

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:761: //
TODO(justinlin): This is not right.. position below hotdog menu.
When an extension is installed into the overflow bin it shows the popup bubble
below the hotdog menu ... I didn't see code in this class to handle that case
though.  Wonder where it lives - I couldn't find it.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/ui/gtk/bro...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/ui/gtk/bro...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:775: if
(ExtensionPopupGtk::get_current_extension_popup() != NULL ||
Does the toolbar model know if there is an active popup?  Seems like this check
should go there.

https://codereview.chromium.org/25305002/diff/30001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/browser_action/open_popup/open_popup_succeeds.js
(right):

https://codereview.chromium.org/25305002/diff/30001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/browser_action/open_popup/open_popup_succeeds.js:13:
chrome.test.succeed();
Can we assert that the popup actually exists (perhaps by sending a message from
the popup)?
Can we verify that calling close() on it destroys the popup?
Can we simulate a user click on the open popup and verify that the popup is
closed as well?

[Matt Perry, 2013-10-11 22:22:52]

Adding finnur to check the implementation. It's more complex than I thought. I
would've expected there to already be a ShowPopupForThisExtension function -
whatever we do when the user clicks on a browser action button.

Also, note that not all extensions have browser actions (though those that don't
will not have access to chrome.browserAction.*), and not all extensions with
browser actions have a popup. Make sure you handle the error cases for those.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:822: if
(VersionInfo::GetChannel() != VersionInfo::CHANNEL_UNKNOWN &&
We actually have a standard mechanism for this. Add browserAction.openPopup to
_api_features.json with this right restrictions/whitelists.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:836:
SetResult(new base::DictionaryValue());
Don't set result on errors. Use SetError instead, with a reasonable error
message (though this is just likely to be "internal error").

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_actions_container.cc:832: const
extensions::Extension* extension = button->extension();
Why add this?

https://codereview.chromium.org/25305002/diff/30001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/browser_action_custom_bindings.js
(right):

https://codereview.chromium.org/25305002/diff/30001/chrome/renderer/resources...
chrome/renderer/resources/extensions/browser_action_custom_bindings.js:25: //
before calling this.
The browser side will have to do that (add a notification observer to wait for
its ExtensionHost/RenderViewHost to load before calling the callback).

[Finnur, 2013-10-15 10:44:21]

This refers to patch set 6. I forgot to send this out yesterday (sorry) -- hope
it is not all irrelevant now.

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:702: 
nit: Extra line break.

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:7:
#include <set>
Where's std::set used?

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:28:
#include "chrome/common/extensions/features/simple_feature.h"
Don't need this?

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:36: using
chrome::VersionInfo;
Don't need this, right?

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:816:
return false;
Do you see these (service and model below) being NULL at some point?

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:819:
ExtensionToolbarModel *model = service->toolbar_model();
Foo* foo 
not
Foo *foo.

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.cc:475:
FOR_EACH_OBSERVER(Observer, observers_, BrowserActionShowPopup(extension));
So, if you have two windows, aren't both going to show the popup?

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/extension_toolbar_model.h (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.h:51: // The browser action
should show the popup now.
nit: The browser action for |extension| should...

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_actions_container.cc:691: // DO_NOT_REVIEW: This
is not completely correct yet.
Can you elaborate?

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:105: "contexts":
["blessed_extension", "unblessed_extension"]
I must admit I don't know what unblessed_extension means as I've never used it.
Why is it necessary to add this here?

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:107: "browserAction.showPopup":
[{
Please comment that you are restricting this to the dev channel and whitelisting
it on stable. Also that security concerns need to be addressed before making it
available to a wider audience (e.g. by removing the whitelist). I'm actually
wondering if just whitelisting for a few extensions is enough for all channels,
as I don't see the point of keeping this live on dev if we don't intend to
release it for everyone to stable.

What is the plan?

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/api/browser_action.json (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/browser_action.json:317: "description": "Opens the
extension popup window without a user action. The popup will be from the usual
location if the button is not hidden and anchored from the settings button
otherwise.",
nit: Is that "anchored" or "anchor ed"?

More importantly: We've discussed the problems with anchoring this off of the
hotdog menu. It seems that the API proposal said "this decision needs to be
revisited before this reaches beta/stable". Who is in charge of that and what's
the plan there?

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/browser_action.json:329: ]
Is it necessary to return the view here? Is this a problem if we go to a wider
audience with this API?

Matt, what do you think?

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature.cc (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:69: // TODO(justinlin):
Handle this correctly.
Um.... ? What is this about?

https://codereview.chromium.org/25305002/diff/44001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/browser_action_custom_bindings.js
(right):

https://codereview.chromium.org/25305002/diff/44001/chrome/renderer/resources...
chrome/renderer/resources/extensions/browser_action_custom_bindings.js:34: });
I've not been doing many custom bindings (that I can remember) so I don't feel
qualified to review this. I'm wondering if this can be clarified further or
perhaps Matt covered this already?

https://codereview.chromium.org/25305002/diff/44001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/browser_action/open_popup/manifest.json
(right):

https://codereview.chromium.org/25305002/diff/44001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/browser_action/open_popup/manifest.json:8:
"default_icon": "chromium.png",
If you can get away with not using an icon, then I would. Fewer files, less
space used and you can use the try servers for the whole change (don't need to
check in the image first).

https://codereview.chromium.org/25305002/diff/44001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/browser_action/open_popup/open_popup_succeeds.js
(right):

https://codereview.chromium.org/25305002/diff/44001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/browser_action/open_popup/open_popup_succeeds.js:7:
chrome.browserAction.openPopup(function(wind) {
s/wind/window/

[Matt Perry, 2013-10-15 21:29:41]

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/api/browser_action.json (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/browser_action.json:329: ]
On 2013/10/15 10:44:21, Finnur wrote:
> Is it necessary to return the view here? Is this a problem if we go to a wider
> audience with this API?
> 
> Matt, what do you think?

Should be fine. Extensions can already get the popup using
chrome.runtime.getViews().

https://codereview.chromium.org/25305002/diff/44001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/browser_action_custom_bindings.js
(right):

https://codereview.chromium.org/25305002/diff/44001/chrome/renderer/resources...
chrome/renderer/resources/extensions/browser_action_custom_bindings.js:34: });
On 2013/10/15 10:44:21, Finnur wrote:
> I've not been doing many custom bindings (that I can remember) so I don't feel
> qualified to review this. I'm wondering if this can be clarified further or
> perhaps Matt covered this already?

Yep, already commented on this. The custom bindings should be unnecessary.

[justinlin, 2013-10-16 07:06:48]

Thanks for the comments! Addressed them and fixed TODO's. Will add last few
missing browser tests shortly. PTAL

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:7:
#include <set>
On 2013/10/11 20:25:30, mark a. foltz wrote:
> Did you mean to remove #include <string>?

Yea, it's already in the .h, actually we don't need set here..

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:66: const
char* whitelisted_extensions[] = {
On 2013/10/11 20:25:30, mark a. foltz wrote:
> kOpenPopupWhitelistedExtensions

moved to _api_features.json

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:822: if
(VersionInfo::GetChannel() != VersionInfo::CHANNEL_UNKNOWN &&
On 2013/10/11 22:22:53, Matt Perry wrote:
> We actually have a standard mechanism for this. Add browserAction.openPopup to
> _api_features.json with this right restrictions/whitelists.

Done.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:822: if
(VersionInfo::GetChannel() != VersionInfo::CHANNEL_UNKNOWN &&
On 2013/10/11 20:25:30, mark a. foltz wrote:
> For clarity, suggest adding a private boolean method to check the whitelist

Just using _api_features.json now.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:836:
SetResult(new base::DictionaryValue());
On 2013/10/11 22:22:53, Matt Perry wrote:
> Don't set result on errors. Use SetError instead, with a reasonable error
> message (though this is just likely to be "internal error").

Done.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:848:
SetResult(new base::DictionaryValue());
On 2013/10/11 20:25:30, mark a. foltz wrote:
> Just call this at the top of RunImpl?

no longer needed.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.cc:162: const Extension*
extension, Browser* browser) {
On 2013/10/11 20:25:30, mark a. foltz wrote:
> DCHECK(browser)

Done.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.cc:162: const Extension*
extension, Browser* browser) {
On 2013/10/11 20:25:30, mark a. foltz wrote:
> const Extension& instead?

From what I've seen in code around here, I think we usually pass pointers to
Extension instances, I think because they can be NULL.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.cc:170:
GetBrowserAction(*extension);
On 2013/10/11 20:25:30, mark a. foltz wrote:
> What if the extension has no browser action?

Done.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
File chrome/browser/extensions/extension_toolbar_model.h (right):

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.h:53: const
extensions::Extension* extension) {}
On 2013/10/11 20:25:30, mark a. foltz wrote:
> unwrap line

Done.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.h:66: void
ShowBrowserActionPopup(const extensions::Extension* extension);
On 2013/10/11 20:25:30, mark a. foltz wrote:
> How different from BrowserActionShowPopup?  Is this part of an API called by
the
> view to notify the model?

It's a method on the model to send a notification to observers.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/ui/cocoa/e...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/ui/cocoa/e...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:761: //
TODO(justinlin): This is not right.. position below hotdog menu.
On 2013/10/11 20:25:30, mark a. foltz wrote:
> When an extension is installed into the overflow bin it shows the popup bubble
> below the hotdog menu ... I didn't see code in this class to handle that case
> though.  Wonder where it lives - I couldn't find it.

Right. It's a bit unclear how to plumb that in right now, so punting on that for
now. It should be seldom used anyway.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/ui/gtk/bro...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/ui/gtk/bro...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:775: if
(ExtensionPopupGtk::get_current_extension_popup() != NULL ||
On 2013/10/11 20:25:30, mark a. foltz wrote:
> Does the toolbar model know if there is an active popup?  Seems like this
check
> should go there.

No, the model doesn't know anything about whether native widgets are
showing/loaded.

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/30001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_actions_container.cc:832: const
extensions::Extension* extension = button->extension();
On 2013/10/11 22:22:53, Matt Perry wrote:
> Why add this?

Done.

https://codereview.chromium.org/25305002/diff/30001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/browser_action_custom_bindings.js
(right):

https://codereview.chromium.org/25305002/diff/30001/chrome/renderer/resources...
chrome/renderer/resources/extensions/browser_action_custom_bindings.js:25: //
before calling this.
On 2013/10/11 22:22:53, Matt Perry wrote:
> The browser side will have to do that (add a notification observer to wait for
> its ExtensionHost/RenderViewHost to load before calling the callback).

Done.

https://codereview.chromium.org/25305002/diff/30001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/browser_action/open_popup/open_popup_succeeds.js
(right):

https://codereview.chromium.org/25305002/diff/30001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/browser_action/open_popup/open_popup_succeeds.js:13:
chrome.test.succeed();
On 2013/10/11 20:25:30, mark a. foltz wrote:
> Can we assert that the popup actually exists (perhaps by sending a message
from
> the popup)?
> Can we verify that calling close() on it destroys the popup?
> Can we simulate a user click on the open popup and verify that the popup is
> closed as well?
> 

The C++ code asserts that the popup is actually loaded. I think there are
already hide popup tests that verify these things on popups.

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:702: 
On 2013/10/15 10:44:21, Finnur wrote:
> nit: Extra line break.

Done.

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:7:
#include <set>
On 2013/10/15 10:44:21, Finnur wrote:
> Where's std::set used?

Done.

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:28:
#include "chrome/common/extensions/features/simple_feature.h"
On 2013/10/15 10:44:21, Finnur wrote:
> Don't need this?

Done.

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:36: using
chrome::VersionInfo;
On 2013/10/15 10:44:21, Finnur wrote:
> Don't need this, right?

Done.

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:816:
return false;
On 2013/10/15 10:44:21, Finnur wrote:
> Do you see these (service and model below) being NULL at some point?

yea, I think extension_service() would not be NULL since there's a lot of
unprotected calls for that, but leaving the check for model

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:819:
ExtensionToolbarModel *model = service->toolbar_model();
On 2013/10/15 10:44:21, Finnur wrote:
> Foo* foo 
> not
> Foo *foo.

Done.

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.cc:475:
FOR_EACH_OBSERVER(Observer, observers_, BrowserActionShowPopup(extension));
On 2013/10/15 10:44:21, Finnur wrote:
> So, if you have two windows, aren't both going to show the popup?

Added comment clarifying that only the active window should handle this.

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
File chrome/browser/extensions/extension_toolbar_model.h (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.h:51: // The browser action
should show the popup now.
On 2013/10/15 10:44:21, Finnur wrote:
> nit: The browser action for |extension| should...

Done.

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_actions_container.cc:691: // DO_NOT_REVIEW: This
is not completely correct yet.
On 2013/10/15 10:44:21, Finnur wrote:
> Can you elaborate?

This is done now.

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:105: "contexts":
["blessed_extension", "unblessed_extension"]
On 2013/10/15 10:44:21, Finnur wrote:
> I must admit I don't know what unblessed_extension means as I've never used
it.
> Why is it necessary to add this here?

I don't quite know either. I think I added this with the intention of finding
out, but I haven't quite figured out yet and I don't think we need it. We can
add it if needed.

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:107: "browserAction.showPopup":
[{
On 2013/10/15 10:44:21, Finnur wrote:
> Please comment that you are restricting this to the dev channel and
whitelisting
> it on stable. Also that security concerns need to be addressed before making
it
> available to a wider audience (e.g. by removing the whitelist). I'm actually
> wondering if just whitelisting for a few extensions is enough for all
channels,
> as I don't see the point of keeping this live on dev if we don't intend to
> release it for everyone to stable.
> 
> What is the plan?

Done. I believe the plan is to first whitelist this API for a set of extensions,
but enable on dev as well while working through the issues. We can move it to
stable once it seems ready.

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/api/browser_action.json (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/browser_action.json:317: "description": "Opens the
extension popup window without a user action. The popup will be from the usual
location if the button is not hidden and anchored from the settings button
otherwise.",
On 2013/10/15 10:44:21, Finnur wrote:
> nit: Is that "anchored" or "anchor ed"?
> 
> More importantly: We've discussed the problems with anchoring this off of the
> hotdog menu. It seems that the API proposal said "this decision needs to be
> revisited before this reaches beta/stable". Who is in charge of that and
what's
> the plan there?

Ha, it is "anchored" in the code. Not sure why codereview decides to break the
line like that.

Updated the comment as well. Punting on the hotdog popup for now. Looking
through the code, it doesn't seem like a trivial change, so probably better to
do it separately.

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/features/complex_feature.cc (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/features/complex_feature.cc:69: // TODO(justinlin):
Handle this correctly.
On 2013/10/15 10:44:21, Finnur wrote:
> Um.... ? What is this about?

Clarified.. somewhat. It's mostly because we are enabling for both dev and a
whitelist, so it goes through ComplexFeature. There currently isn't much of a
way to return some complex IsInternal value, so just return the first item's
IsInternal result.

https://codereview.chromium.org/25305002/diff/44001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/browser_action_custom_bindings.js
(right):

https://codereview.chromium.org/25305002/diff/44001/chrome/renderer/resources...
chrome/renderer/resources/extensions/browser_action_custom_bindings.js:34: });
On 2013/10/15 21:29:41, Matt Perry wrote:
> On 2013/10/15 10:44:21, Finnur wrote:
> > I've not been doing many custom bindings (that I can remember) so I don't
feel
> > qualified to review this. I'm wondering if this can be clarified further or
> > perhaps Matt covered this already?
> 
> Yep, already commented on this. The custom bindings should be unnecessary.

I fixed the setTimeout hack. I think custom bindings are still necessary because
I don't see a good way to get the popup view form the browser side. All that
logic lives in the renderer from what I can tell.

https://codereview.chromium.org/25305002/diff/44001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/browser_action/open_popup/manifest.json
(right):

https://codereview.chromium.org/25305002/diff/44001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/browser_action/open_popup/manifest.json:8:
"default_icon": "chromium.png",
On 2013/10/15 10:44:21, Finnur wrote:
> If you can get away with not using an icon, then I would. Fewer files, less
> space used and you can use the try servers for the whole change (don't need to
> check in the image first).

Done. Good point, the other ones should probably be removed too. I think the
icon used to be required, but looks like not anymore.

https://codereview.chromium.org/25305002/diff/44001/chrome/test/data/extensio...
File
chrome/test/data/extensions/api_test/browser_action/open_popup/open_popup_succeeds.js
(right):

https://codereview.chromium.org/25305002/diff/44001/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/browser_action/open_popup/open_popup_succeeds.js:7:
chrome.browserAction.openPopup(function(wind) {
On 2013/10/15 10:44:21, Finnur wrote:
> s/wind/window/

isn't window a reserved keyword? changed to popupWindow

[justinlin, 2013-10-16 07:11:52]

Adding ui/* owners.

erg: ptal gtk
sky: ptal views
asvitkine: ptal cocoa

This mostly adds a method to show an extension popup that does NOT grant tab
permissions (new boolean added to the ExecuteBrowserAction function) +/- some
refactorings.

[sky, 2013-10-16 14:19:25]

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_actions_container.cc:694:
BrowserList::GetInstance(chrome::GetActiveDesktop())->GetLastActive() !=
browser_->window()->IsActive()?

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_actions_container.cc:703: button =
(*it)->button();
How about showing right here and returning?

[Finnur, 2013-10-16 15:01:26]

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:105: "contexts":
["blessed_extension", "unblessed_extension"]
Um... can you remove it then? :)

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:107: "browserAction.showPopup":
[{
I'd like to make this comment more explicit, for example:

This API is whitelisted on stable and should not be enabled for a wider audience
without resolving security issues raised in API proposal and review
(https://codereview.chromium.org/25305002).

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/api/browser_action.json (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/browser_action.json:317: "description": "Opens the
extension popup window without a user action. The popup will be from the usual
location if the button is not hidden and anchored from the settings button
otherwise.",
I asked not to prompt you to do it in this changelist but to see how you see it
pan out. Are you going to be tackling that? I'm not too bothered if only Cast is
using this API, but if others get a chance to use it then this perhaps becomes a
bigger issue.

But, just to be sure: Does your CL work if you run showPopup against a browser
action that is in the overflow bucket?

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/browser_action.json:329: ]
Yeah. Minor short-circuit in my brain. For a minute I thought an extension could
use this to get at *other* extensions' popups. But that's not the case here.

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:838:
SendResponse(new base::ListValue());
It seems like we're jumping through hoops just to execute a DVLOG command, which
doesn't seem useful. You might as well just call ShowBrowserActionPopup and call
it a day. I don't see the caller benefiting from all this extra complexity.

Actually, wait. Is this because of the view returned in custom bindings? You
might want to document that that's whats going on behind the scenes. But this
seems brittle though... 1 sec, will that suffice in tests under memory bots
(which slow everything down)?

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:848:
return;
Should be enough to DCHECK at the top that type == HOST_DID_STOP_LOADING.

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
File chrome/browser/extensions/extension_toolbar_model.h (right):

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.h:76: bool should_grant);
These kinds of optional params (should_grant) are against the style guide. You
should add this param to the function below and update all the callers.

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.h:98: // Tells observers to
display a popup without granting tab permissions.
nit: Line break before this line.

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_actions_container.cc:879: // granted tab
permissions, then it was not by a user click.
nit: double negative. Can you reword?

[Elliot Glaysher, 2013-10-16 17:21:14]

gtk lgtm

[mark a. foltz, 2013-10-16 19:00:52]

lgtm

https://codereview.chromium.org/25305002/diff/95001/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/95001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:856:
SendResponse(new base::ListValue());
Is this what you intended?  The signature for ExtensionFunction::SendResponse
is:

virtual void SendResponse(bool success) = 0;

https://codereview.chromium.org/25305002/diff/95001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:857:
response_sent_ = true;
You could also check for NULL-ness of results_, but that may be adding a
dependency on an implementation detail of ExtensionFunction you don't want.

https://codereview.chromium.org/25305002/diff/95001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:858: //
Don't need to de-register notification listener since the last ref for this
Would it hurt to de-register it anyway?

[justinlin, 2013-10-16 19:31:28]

thanks! ptal.

finner@: We can discuss offline if we want to enable for dev or not. I don't
feel strongly either way, but I don't see any very strong reasons against.

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/api/_api_features.json (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:105: "contexts":
["blessed_extension", "unblessed_extension"]
On 2013/10/16 15:01:26, Finnur wrote:
> Um... can you remove it then? :)

Thought I did. Oh, I accidentally added it to the above. Oops, removed.

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/_api_features.json:107: "browserAction.showPopup":
[{
On 2013/10/16 15:01:26, Finnur wrote:
> I'd like to make this comment more explicit, for example:
> 
> This API is whitelisted on stable and should not be enabled for a wider
audience
> without resolving security issues raised in API proposal and review
> (https://codereview.chromium.org/25305002).

Done. OK, that wording seems better.

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
File chrome/common/extensions/api/browser_action.json (right):

https://codereview.chromium.org/25305002/diff/44001/chrome/common/extensions/...
chrome/common/extensions/api/browser_action.json:317: "description": "Opens the
extension popup window without a user action. The popup will be from the usual
location if the button is not hidden and anchored from the settings button
otherwise.",
On 2013/10/16 15:01:26, Finnur wrote:
> I asked not to prompt you to do it in this changelist but to see how you see
it
> pan out. Are you going to be tackling that? I'm not too bothered if only Cast
is
> using this API, but if others get a chance to use it then this perhaps becomes
a
> bigger issue.
> 
> But, just to be sure: Does your CL work if you run showPopup against a browser
> action that is in the overflow bucket?

Yes, I can tackle them or maybe someone on my team if we decide to move forward
to stable. I believe there's a launch bug for it with needed approvals. Yup, it
works if it's in the overflow bucket. Adding test for that.

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:838:
SendResponse(new base::ListValue());
On 2013/10/16 15:01:26, Finnur wrote:
> It seems like we're jumping through hoops just to execute a DVLOG command,
which
> doesn't seem useful. You might as well just call ShowBrowserActionPopup and
call
> it a day. I don't see the caller benefiting from all this extra complexity.
> 
> Actually, wait. Is this because of the view returned in custom bindings? You
> might want to document that that's whats going on behind the scenes. But this
> seems brittle though... 1 sec, will that suffice in tests under memory bots
> (which slow everything down)?

Hmm, that's a good point and right it's for the custom bindings. We can't just
send a response right away because we won't have a popup view to return yet.
This either waits for the loaded notification (which I'm hoping won't take over
a second on memory bots) or a second (where the popup might already be loaded by
the time we send the response). Clarified the comment.

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:848:
return;
On 2013/10/16 15:01:26, Finnur wrote:
> Should be enough to DCHECK at the top that type == HOST_DID_STOP_LOADING. 

Done.

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
File chrome/browser/extensions/extension_toolbar_model.h (right):

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.h:76: bool should_grant);
On 2013/10/16 15:01:26, Finnur wrote:
> These kinds of optional params (should_grant) are against the style guide. You
> should add this param to the function below and update all the callers.

Hmm.. according to this, this should be the preferred way to add an argument
that can be overriden, but is seldom used:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Defaul...

I believe SimpleFeature does something similar as well. I can change all callers
too, but I think it makes this call more confusing since almost everytime except
through this new extension API you'll want the granting behavior. WDYT?

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.h:98: // Tells observers to
display a popup without granting tab permissions.
On 2013/10/16 15:01:26, Finnur wrote:
> nit: Line break before this line.

Done.

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/ui/views/b...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_actions_container.cc:694:
BrowserList::GetInstance(chrome::GetActiveDesktop())->GetLastActive() !=
On 2013/10/16 14:19:25, sky wrote:
> browser_->window()->IsActive()?

Done.

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_actions_container.cc:703: button =
(*it)->button();
On 2013/10/16 14:19:25, sky wrote:
> How about showing right here and returning?

Done.

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/ui/views/b...
chrome/browser/ui/views/browser_actions_container.cc:879: // granted tab
permissions, then it was not by a user click.
On 2013/10/16 15:01:26, Finnur wrote:
> nit: double negative. Can you reword?

Done.

https://codereview.chromium.org/25305002/diff/95001/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/95001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:856:
SendResponse(new base::ListValue());
On 2013/10/16 19:00:53, mark a. foltz wrote:
> Is this what you intended?  The signature for ExtensionFunction::SendResponse
> is:
> 
> virtual void SendResponse(bool success) = 0;

Oh good catch. So, apparently base::Value* can be implicitly cast to bool..
that's somewhat unsettling.

https://codereview.chromium.org/25305002/diff/95001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:857:
response_sent_ = true;
On 2013/10/16 19:00:53, mark a. foltz wrote:
> You could also check for NULL-ness of results_, but that may be adding a
> dependency on an implementation detail of ExtensionFunction you don't want.

I think it's okay not to set result here since we don't need or use it anyway.

https://codereview.chromium.org/25305002/diff/95001/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:858: //
Don't need to de-register notification listener since the last ref for this
On 2013/10/16 19:00:53, mark a. foltz wrote:
> Would it hurt to de-register it anyway?

Done.

[justinlin, 2013-10-16 20:23:03]

finnur*, sorry

[Matt Perry, 2013-10-16 20:34:35]

Almost there.

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:830:
base::TimeDelta::FromSeconds(1));
Why add a timeout? The popup can take a while to load. DID_STOP_LOADING will
wait until the page is fully loaded, so a second may not be enough.

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:853: if
(host->extension()->id() != extension_->id())
You should also check that the host type is popup.

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.h (right):

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.h:36: // MVC.
With the new method, this class is no longer really an "observer". Probably not
worth renaming, but the comment could use changing.

[justinlin, 2013-10-16 21:16:22]

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:830:
base::TimeDelta::FromSeconds(1));
On 2013/10/16 20:34:35, Matt Perry wrote:
> Why add a timeout? The popup can take a while to load. DID_STOP_LOADING will
> wait until the page is fully loaded, so a second may not be enough.

The timeout is mostly needed because there isn't a good way to get notified if
the popup did not show for some reason, so this is to handle when opening a
popup fails. It's not super ideal, but it should be correct most of the time. It
may fire before the NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING notification,
but 1 second should be enough for the popup to be created already.

Oh, also the PostTask is also needed to keep a ref to this function, otherwise
it gets deleted before we can observe anything, added that comment.

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:853: if
(host->extension()->id() != extension_->id())
On 2013/10/16 20:34:35, Matt Perry wrote:
> You should also check that the host type is popup.

Done.

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.h (right):

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.h:36: // MVC.
On 2013/10/16 20:34:35, Matt Perry wrote:
> With the new method, this class is no longer really an "observer". Probably
not
> worth renaming, but the comment could use changing.

Done.

[Matt Perry, 2013-10-16 21:32:49]

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:830:
base::TimeDelta::FromSeconds(1));
On 2013/10/16 21:16:23, justinlin wrote:
> On 2013/10/16 20:34:35, Matt Perry wrote:
> > Why add a timeout? The popup can take a while to load. DID_STOP_LOADING will
> > wait until the page is fully loaded, so a second may not be enough.
> 
> The timeout is mostly needed because there isn't a good way to get notified if
> the popup did not show for some reason, so this is to handle when opening a
> popup fails. It's not super ideal, but it should be correct most of the time.
It
> may fire before the NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING notification,
> but 1 second should be enough for the popup to be created already.
> 
> Oh, also the PostTask is also needed to keep a ref to this function, otherwise
> it gets deleted before we can observe anything, added that comment.

I believe you're guaranteed to get a DID_STOP_LOADING message even if the load
fails. Did you find that not to be the case?

You can always manually AddRef/Release to keep the ExtensionFunction alive.

[justinlin, 2013-10-16 21:42:01]

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:830:
base::TimeDelta::FromSeconds(1));
On 2013/10/16 21:32:50, Matt Perry wrote:
> On 2013/10/16 21:16:23, justinlin wrote:
> > On 2013/10/16 20:34:35, Matt Perry wrote:
> > > Why add a timeout? The popup can take a while to load. DID_STOP_LOADING
will
> > > wait until the page is fully loaded, so a second may not be enough.
> > 
> > The timeout is mostly needed because there isn't a good way to get notified
if
> > the popup did not show for some reason, so this is to handle when opening a
> > popup fails. It's not super ideal, but it should be correct most of the
time.
> It
> > may fire before the NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING
notification,
> > but 1 second should be enough for the popup to be created already.
> > 
> > Oh, also the PostTask is also needed to keep a ref to this function,
otherwise
> > it gets deleted before we can observe anything, added that comment.
> 
> I believe you're guaranteed to get a DID_STOP_LOADING message even if the load
> fails. Did you find that not to be the case?
> 
> You can always manually AddRef/Release to keep the ExtensionFunction alive.

I mean if the extension doesn't try loading at all because of any of the logic
in the platform-specific code (i.e. if there was already a popup or the
extension had no button).

[Matt Perry, 2013-10-16 21:45:21]

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:830:
base::TimeDelta::FromSeconds(1));
On 2013/10/16 21:42:01, justinlin wrote:
> On 2013/10/16 21:32:50, Matt Perry wrote:
> > On 2013/10/16 21:16:23, justinlin wrote:
> > > On 2013/10/16 20:34:35, Matt Perry wrote:
> > > > Why add a timeout? The popup can take a while to load. DID_STOP_LOADING
> will
> > > > wait until the page is fully loaded, so a second may not be enough.
> > > 
> > > The timeout is mostly needed because there isn't a good way to get
notified
> if
> > > the popup did not show for some reason, so this is to handle when opening
a
> > > popup fails. It's not super ideal, but it should be correct most of the
> time.
> > It
> > > may fire before the NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING
> notification,
> > > but 1 second should be enough for the popup to be created already.
> > > 
> > > Oh, also the PostTask is also needed to keep a ref to this function,
> otherwise
> > > it gets deleted before we can observe anything, added that comment.
> > 
> > I believe you're guaranteed to get a DID_STOP_LOADING message even if the
load
> > fails. Did you find that not to be the case?
> > 
> > You can always manually AddRef/Release to keep the ExtensionFunction alive.
> 
> I mean if the extension doesn't try loading at all because of any of the logic
> in the platform-specific code (i.e. if there was already a popup or the
> extension had no button).

Can you add logic to detect that instead? Maybe ShowBrowserActionPopup should
return a bool indicating whether it was successful.

[sky, 2013-10-16 22:26:06]

https://codereview.chromium.org/25305002/diff/109001/chrome/browser/ui/views/...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/109001/chrome/browser/ui/views/...
chrome/browser/ui/views/browser_actions_container.cc:695: BrowserActionButton*
button = NULL;
Did you try this? I think you want to nuke button, line 699 and have ShowPopup()
use (*it)->button(). I think.

[justinlin, 2013-10-17 04:14:51]

thanks! ptal

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/100001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:830:
base::TimeDelta::FromSeconds(1));
Yea, I considered it, but it seemed a bit weird to iterate over the observer
list. It's not that bad afterall, and this definitely makes a better user
experience. I think it's still possible that the popup should be shown, but it
for some reason does not end up showing (create the view host, but something
makes it decide not to load afterall), hence we may still need the timeout, but
it's set to a bit longer now.

https://codereview.chromium.org/25305002/diff/109001/chrome/browser/ui/views/...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/109001/chrome/browser/ui/views/...
chrome/browser/ui/views/browser_actions_container.cc:695: BrowserActionButton*
button = NULL;
On 2013/10/16 22:26:07, sky wrote:
> Did you try this? I think you want to nuke button, line 699 and have
ShowPopup()
> use (*it)->button(). I think.

err, right. Was *just* about to compile it and test. I thought the change was
simple enough.. guess not.

Found a bug though, it's kind of weird how on Windows/views (and not on linux or
mac) it creates a "BrowserActionsContainer" even for windows that don't display
browser actions such as an extension's background page. Needed to add an extra
check that the window had a toolbar too. Not sure if it's worth filing a bug for
that or expected..

[Finnur, 2013-10-17 14:58:33]

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
File chrome/browser/extensions/extension_toolbar_model.h (right):

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.h:76: bool should_grant);
The public style guide seems to be down (my coworker also tried).

I looked up Default Argument on the internal style guide and I don't see it as
the preferred way.

You have two options: 
1) Add the param to the original function and update all the callers.
2) Add a second function with a different (but similar name) that redirects.

1 is cleaner, I believe, but if we have a ton of callers that all pass in false
except one (which I don't think is the case) then I guess 2 is preferred. Having
said that, you'd probably want ExecuteBrowserAction and
ExecuteBrowserActionAndGrant...etc, so you'd end up updating all the call sites
anyway.

https://codereview.chromium.org/25305002/diff/109001/chrome/browser/ui/views/...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/109001/chrome/browser/ui/views/...
chrome/browser/ui/views/browser_actions_container.cc:695: BrowserActionButton*
button = NULL;
Wait? A window is created for extension's background pages? Those have no UI so
that sounds wrong... Please file a bug.

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:784: //
enabled for incognito.
Line 806 says the popup opened. Is the test incorrect or does the comment and
name of the test need to be changed?

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:817:
MAYBE_TestOpenPopupDoesNotCloseOtherPopups) {
I thought the design doc said only the latest popup would stay on screen
(last-to-be-opened)?

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:841:
EXPECT_TRUE(GetBrowserActionsBar().HasPopup());
It is not clear from casual reading of this that this ensures the correct one
opened...

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:857:
DCHECK_EQ(type, chrome::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING);
DCHECK_EQ(expected, actual)
not 
DCHECK_EQ(actual, expected)

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/ui/gtk/br...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/ui/gtk/br...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:260: bool
Activate(GtkWidget* widget, bool allow_grant) {
Same here (avoid forking the function just to add a param).

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/ui/views/...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/ui/views/...
chrome/browser/ui/views/browser_actions_container.cc:693: // must also have a
toolbar, otherwise it should not be showing popups.
Document that you are checking for toolbar visibility due to a bug.

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/ui/views/...
File chrome/browser/ui/views/browser_actions_container.h (right):

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/ui/views/...
chrome/browser/ui/views/browser_actions_container.h:317: bool should_grant);
Same argument here (one function or two similar functions, not identically
named).

https://codereview.chromium.org/25305002/diff/132001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/browser_action_custom_bindings.js
(right):

https://codereview.chromium.org/25305002/diff/132001/chrome/renderer/resource...
chrome/renderer/resources/extensions/browser_action_custom_bindings.js:24: }
Braces here but not below. Should be consistent, no?
I'd skip them.

https://codereview.chromium.org/25305002/diff/132001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/browser_action/open_popup/open_popup_fails.js
(right):

https://codereview.chromium.org/25305002/diff/132001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/browser_action/open_popup/open_popup_fails.js:7:
// extension hides any open popups. After a popup is opened, we can assert that
> because loading an extension hides any open popups

Do you know why that is?

[justinlin, 2013-10-17 18:32:52]

thanks! ptal

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
File chrome/browser/extensions/extension_toolbar_model.h (right):

https://codereview.chromium.org/25305002/diff/73001/chrome/browser/extensions...
chrome/browser/extensions/extension_toolbar_model.h:76: bool should_grant);
On 2013/10/17 14:58:34, Finnur wrote:
> The public style guide seems to be down (my coworker also tried).
> 
> I looked up Default Argument on the internal style guide and I don't see it as
> the preferred way.
> 
> You have two options: 
> 1) Add the param to the original function and update all the callers.
> 2) Add a second function with a different (but similar name) that redirects.
> 
> 1 is cleaner, I believe, but if we have a ton of callers that all pass in
false
> except one (which I don't think is the case) then I guess 2 is preferred.
Having
> said that, you'd probably want ExecuteBrowserAction and
> ExecuteBrowserActionAndGrant...etc, so you'd end up updating all the call
sites
> anyway.

Done.

https://codereview.chromium.org/25305002/diff/109001/chrome/browser/ui/views/...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/109001/chrome/browser/ui/views/...
chrome/browser/ui/views/browser_actions_container.cc:695: BrowserActionButton*
button = NULL;
On 2013/10/17 14:58:34, Finnur wrote:
> Wait? A window is created for extension's background pages? Those have no UI
so
> that sounds wrong... Please file a bug.

I mean when opening the console view of the background page. I think the window
makes sense, but I don't think a BrowserActionsContainer should be created for
the window. Filed bug 308645

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:817:
MAYBE_TestOpenPopupDoesNotCloseOtherPopups) {
On 2013/10/17 14:58:34, Finnur wrote:
> I thought the design doc said only the latest popup would stay on screen
> (last-to-be-opened)?

Right. Also, this should not overwrite any existing opened popups (i.e. it
should only display if there isn't already a popup showing).

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:841:
EXPECT_TRUE(GetBrowserActionsBar().HasPopup());
On 2013/10/17 14:58:34, Finnur wrote:
> It is not clear from casual reading of this that this ensures the correct one
> opened...

Done.

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/extension_action_api.cc
(right):

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/extension_action_api.cc:857:
DCHECK_EQ(type, chrome::NOTIFICATION_EXTENSION_HOST_DID_STOP_LOADING);
On 2013/10/17 14:58:34, Finnur wrote:
> DCHECK_EQ(expected, actual)
> not 
> DCHECK_EQ(actual, expected)

Done.

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/ui/views/...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/ui/views/...
chrome/browser/ui/views/browser_actions_container.cc:693: // must also have a
toolbar, otherwise it should not be showing popups.
On 2013/10/17 14:58:34, Finnur wrote:
> Document that you are checking for toolbar visibility due to a bug.

Done.

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/ui/views/...
File chrome/browser/ui/views/browser_actions_container.h (right):

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/ui/views/...
chrome/browser/ui/views/browser_actions_container.h:317: bool should_grant);
On 2013/10/17 14:58:34, Finnur wrote:
> Same argument here (one function or two similar functions, not identically
> named).

Done.

https://codereview.chromium.org/25305002/diff/132001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/browser_action_custom_bindings.js
(right):

https://codereview.chromium.org/25305002/diff/132001/chrome/renderer/resource...
chrome/renderer/resources/extensions/browser_action_custom_bindings.js:24: }
On 2013/10/17 14:58:34, Finnur wrote:
> Braces here but not below. Should be consistent, no?
> I'd skip them.

Done.

https://codereview.chromium.org/25305002/diff/132001/chrome/test/data/extensi...
File
chrome/test/data/extensions/api_test/browser_action/open_popup/open_popup_fails.js
(right):

https://codereview.chromium.org/25305002/diff/132001/chrome/test/data/extensi...
chrome/test/data/extensions/api_test/browser_action/open_popup/open_popup_fails.js:7:
// extension hides any open popups. After a popup is opened, we can assert that
On 2013/10/17 14:58:34, Finnur wrote:
> > because loading an extension hides any open popups
> 
> Do you know why that is?

Oh, well I haven't dug into this, but it seems like reasonable behavior. I'm
assuming it's because it needs to update the browser action icons after an
extension install, so it will close any popups that are there first.

So, in this case, I need to load the first extension, notify pass, then load the
other extension, then test that this doesn't open an popup.

[Alexei Svitkine (slow), 2013-10-17 20:39:55]

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:142: -
(bool)browserActionClicked:(BrowserActionButton*)button
Can you add a comment here and explain |shouldGrand|.

Also, -browserActionClicked:andGrant: isn't a good Obj-C name (when there's an
"and", it's usually to make the method name readable like a sentence, e.g. "with
foo and bar", but in this case it doesn't make sense).

Just name it: -browserActionClicked:shouldGrant:

[justinlin, 2013-10-17 20:49:46]

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:142: -
(bool)browserActionClicked:(BrowserActionButton*)button
On 2013/10/17 20:39:55, Alexei Svitkine wrote:
> Can you add a comment here and explain |shouldGrand|.
> 
> Also, -browserActionClicked:andGrant: isn't a good Obj-C name (when there's an
> "and", it's usually to make the method name readable like a sentence, e.g.
"with
> foo and bar", but in this case it doesn't make sense).
> 
> Just name it: -browserActionClicked:shouldGrant:

Done.

[Alexei Svitkine (slow), 2013-10-17 20:56:39]

Cocoa LGTM with nits.

https://codereview.chromium.org/25305002/diff/155001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/25305002/diff/155001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:141: // it
should grant tab permissions. API-simulated clicks should not grant.
Nit: Remove extra 'it'

https://codereview.chromium.org/25305002/diff/155001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:252:
shouldGrant:false];
Nit: This is an Obj-C call, so change false -> NO here.

https://codereview.chromium.org/25305002/diff/155001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:756: -
(bool)browserActionClicked:(BrowserActionButton*)button
Nit: Change return value to BOOL and return YES/NO per Obj-C style.

[justinlin, 2013-10-17 21:33:58]

thanks

mpcomplete, finner@: ptal for extension stuff
sky@: ptal for views/

https://codereview.chromium.org/25305002/diff/155001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/25305002/diff/155001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:141: // it
should grant tab permissions. API-simulated clicks should not grant.
On 2013/10/17 20:56:40, Alexei Svitkine wrote:
> Nit: Remove extra 'it'

Done.

https://codereview.chromium.org/25305002/diff/155001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:252:
shouldGrant:false];
On 2013/10/17 20:56:40, Alexei Svitkine wrote:
> Nit: This is an Obj-C call, so change false -> NO here.

Done.

https://codereview.chromium.org/25305002/diff/155001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:756: -
(bool)browserActionClicked:(BrowserActionButton*)button
On 2013/10/17 20:56:40, Alexei Svitkine wrote:
> Nit: Change return value to BOOL and return YES/NO per Obj-C style.

Done.

[Matt Perry, 2013-10-17 21:49:56]

lgtm

[sky, 2013-10-18 00:41:20]

LGTM

[Finnur, 2013-10-18 10:31:00]

Your new tests are failing on some of the bots. See for example:
http://build.chromium.org/p/tryserver.chromium/builders/linux_aura/builds/87913

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:784: //
enabled for incognito.
I don't see a reply on this question...

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:817:
MAYBE_TestOpenPopupDoesNotCloseOtherPopups) {
That's fine. Does the design doc need to be updated? Or am I mis-remembering?

On 2013/10/17 18:32:53, justinlin wrote:
> On 2013/10/17 14:58:34, Finnur wrote:
> > I thought the design doc said only the latest popup would stay on screen
> > (last-to-be-opened)?
> 
> Right. Also, this should not overwrite any existing opened popups (i.e. it
> should only display if there isn't already a popup showing).

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:715:
browser()->window()->Show();
I thought this was implicit in the test framework. Why are these call needed
everywhere? Makes me wonder if there's a race condition in there...

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:825:
const Extension* extension = GetSingleLoadedExtension();
Maybe ASSERT the extension pointer?

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:142: -
(bool)browserActionClicked:(BrowserActionButton*)button
I think these should be consolidated into one function, like in the other
implementation files.

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/gtk/br...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/gtk/br...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:256: bool
Activate(GtkWidget* widget, bool should_grant) {
I would copy the explanation of should_grant (in the previous comment) to this
location also.

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/views/...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/views/...
chrome/browser/ui/views/browser_actions_container.cc:694: // Can remove toolbar
check when http://crbug.com/308645 is fixed.
TODO(justinlin): Remove toolbar check when ...

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/views/...
File chrome/browser/ui/views/browser_actions_container.h (right):

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/views/...
chrome/browser/ui/views/browser_actions_container.h:317: bool should_grant);
Same here -- document should_grant. And while you are at it, maybe document
show_action too.

https://codereview.chromium.org/25305002/diff/159001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.h (right):

https://codereview.chromium.org/25305002/diff/159001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.h:70: // the browser action
should grant the page tab permissions, which is the case
Suggest rewording: ... controls whether the extension should be granted page tab
permissions, which is what happens when the user clicks the browser action, but
not, for example, when the showPopup API is called.

[justinlin, 2013-10-19 06:01:48]

thanks, I think a few of those were already addressed in the latest patch.
fixing tests..

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:784: //
enabled for incognito.
On 2013/10/18 10:31:01, Finnur wrote:
> I don't see a reply on this question...

Clarified, sorry missed this and added check that it's the popup in the regular
browser that's opened.

https://codereview.chromium.org/25305002/diff/132001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:817:
MAYBE_TestOpenPopupDoesNotCloseOtherPopups) {
On 2013/10/18 10:31:01, Finnur wrote:
> That's fine. Does the design doc need to be updated? Or am I mis-remembering?
> 
> On 2013/10/17 18:32:53, justinlin wrote:
> > On 2013/10/17 14:58:34, Finnur wrote:
> > > I thought the design doc said only the latest popup would stay on screen
> > > (last-to-be-opened)?
> > 
> > Right. Also, this should not overwrite any existing opened popups (i.e. it
> > should only display if there isn't already a popup showing).
> 

I thought it was. Guess not. I think it might have been discussed in the design
review as mfoltz@ had mentioned this as a constraint. Added to the design doc
"The popup should not preempt any existing popup that was already opened."

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:715:
browser()->window()->Show();
On 2013/10/18 10:31:01, Finnur wrote:
> I thought this was implicit in the test framework. Why are these call needed
> everywhere? Makes me wonder if there's a race condition in there...

OK, thought they might be needed, maybe it's not required, will fix tests..

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:825:
const Extension* extension = GetSingleLoadedExtension();
On 2013/10/18 10:31:01, Finnur wrote:
> Maybe ASSERT the extension pointer?

Done. Doesn't really seem needed though, since if that's NULL, the test will
just crash/fail..

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/cocoa/...
File chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm (right):

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/cocoa/...
chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm:142: -
(bool)browserActionClicked:(BrowserActionButton*)button
On 2013/10/18 10:31:01, Finnur wrote:
> I think these should be consolidated into one function, like in the other
> implementation files.

Yea, so the problem is this function is used with @selector below, which will do
partial binding to the button that's clicked, so I'm not sure you can first bind
the 2nd argument before passing to the @selector. Also, these two functions are
named differently.

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/gtk/br...
File chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc (right):

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/gtk/br...
chrome/browser/ui/gtk/browser_actions_toolbar_gtk.cc:256: bool
Activate(GtkWidget* widget, bool should_grant) {
On 2013/10/18 10:31:01, Finnur wrote:
> I would copy the explanation of should_grant (in the previous comment) to this
> location also.

Hmm, this was already done, must be an older patch set.

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/views/...
File chrome/browser/ui/views/browser_actions_container.cc (right):

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/views/...
chrome/browser/ui/views/browser_actions_container.cc:694: // Can remove toolbar
check when http://crbug.com/308645 is fixed.
On 2013/10/18 10:31:01, Finnur wrote:
> TODO(justinlin): Remove toolbar check when ...

Done.

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/views/...
File chrome/browser/ui/views/browser_actions_container.h (right):

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/ui/views/...
chrome/browser/ui/views/browser_actions_container.h:317: bool should_grant);
On 2013/10/18 10:31:01, Finnur wrote:
> Same here -- document should_grant. And while you are at it, maybe document
> show_action too.

Old patch set as well. Hmm.. I don't completely understand what show_action does
(SHOW vs SHOW_AND_INSPECT), so I'd rather leave that undocumented..

https://codereview.chromium.org/25305002/diff/159001/chrome/browser/extension...
File chrome/browser/extensions/extension_toolbar_model.h (right):

https://codereview.chromium.org/25305002/diff/159001/chrome/browser/extension...
chrome/browser/extensions/extension_toolbar_model.h:70: // the browser action
should grant the page tab permissions, which is the case
On 2013/10/18 10:31:01, Finnur wrote:
> Suggest rewording: ... controls whether the extension should be granted page
tab
> permissions, which is what happens when the user clicks the browser action,
but
> not, for example, when the showPopup API is called.

Done.

[justinlin, 2013-10-21 06:57:10]

OK, ptal. Moved the tests to interactive_ui_tests. Now they should all pass.
They needed to be moved because they deal with window focus, and that's not
going to work as a browser_test since those can run concurrently.

I had to disable the tests for Mac for now though because it doesn't seem to be
possible to make the window foreground automatically on mac. I only found one
test that kind of does that, but it just fullscreens the browser window (but
that's not enough here because some tests need multiple windows). If I just
remove the constraint that the browser needs to be active, and run tests
locally, then they pass.

[Finnur, 2013-10-21 08:12:11]

LGTM

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/extension...
File chrome/browser/extensions/api/extension_action/browser_action_apitest.cc
(right):

https://codereview.chromium.org/25305002/diff/145001/chrome/browser/extension...
chrome/browser/extensions/api/extension_action/browser_action_apitest.cc:825:
const Extension* extension = GetSingleLoadedExtension();
If it fails, it means the env is to blame (not your test) and you don't have to
go read all the code below to figure out what happened. I find it helpful to
have these kinds of checks up front because then you know immediately if your
test/code is to blame or if the problem lies in the environment (another good
example: asserting that the web server started up properly).

[commit-bot: I haz the power, 2013-10-21 08:31:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/justinlin@chromium.org/25305002/382001

[commit-bot: I haz the power, 2013-10-21 11:24:58]

Change committed as 229777

[not at google - send to devlin, 2013-10-23 23:18:40]

https://codereview.chromium.org/25305002/diff/382001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/browser_action_custom_bindings.js
(right):

https://codereview.chromium.org/25305002/diff/382001/chrome/renderer/resource...
chrome/renderer/resources/extensions/browser_action_custom_bindings.js:23: throw
new Error(chrome.runtime.lastError.message);
don't throw an error here. chrome.runtime.lastError should just be set for the
callback. the code for this should look something like

if (!request.callback)
  return;

if (chrome.runtime.lastError) {
  request.callback();
  return;
}

[justinlin, 2013-10-24 00:01:37]

https://codereview.chromium.org/25305002/diff/382001/chrome/renderer/resource...
File chrome/renderer/resources/extensions/browser_action_custom_bindings.js
(right):

https://codereview.chromium.org/25305002/diff/382001/chrome/renderer/resource...
chrome/renderer/resources/extensions/browser_action_custom_bindings.js:23: throw
new Error(chrome.runtime.lastError.message);
Ah thanks. https://codereview.chromium.org/32283005/. I thought API's can throw
an exception as well on error, but I think that's only for before the call when
things like input parameters are the wrong type.