Initial skeleton for System Indicator API

Initial skeleton for System Indicator API

Setting up the IDL and the appropriate boilerplate.  Integrates with the existing extension action framework.

The new API is currently restricted to dev channel.

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

[dewittj, 2012-11-10 01:36:04]

Would you guys give this a once-over?  There's no real meat to it yet, that will
be a later CL.

[jianli, 2012-11-12 23:00:55]

Don't forget to run try job.

https://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.cc
(right):

https://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:8: 
nit: extra empty line not needed

https://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:11:
namespace extensions {
nit: empty line before this line

https://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:20:
return true;
nit: space alignment

https://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.h
(right):

https://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_API_SYSTEM_INDICATOR_SYSTEM_INDICATOR_API_H__
nit: only one trailing underscore is needed.

https://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:54:
private:
nit: not needed

https://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:58: #endif
 // CHROME_BROWSER_EXTENSIONS_API_SYSTEM_INDICATOR_SYSTEM_INDICATOR_API_H__
nit: empty line before this line.

https://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:58: #endif
 // CHROME_BROWSER_EXTENSIONS_API_SYSTEM_INDICATOR_SYSTEM_INDICATOR_API_H__
nit: inconsistent naming in the comment

https://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions...
File chrome/browser/extensions/api/system_indicator/system_indicator_apitest.cc
(right):

https://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_apitest.cc:11:
switches::kEnableExperimentalExtensionApis);
nit: space alignment

https://codereview.chromium.org/11361189/diff/10002/chrome/common/extensions/...
File chrome/common/extensions/api/experimental_system_indicator.idl (right):

https://codereview.chromium.org/11361189/diff/10002/chrome/common/extensions/...
chrome/common/extensions/api/experimental_system_indicator.idl:22: // fired when
a click does not result in a menu being shown.
nit: Capital

[dcheng, 2012-11-13 00:52:19]

Drive by.

https://codereview.chromium.org/11361189/diff/12003/chrome/browser/extensions...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.cc
(right):

https://codereview.chromium.org/11361189/diff/12003/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:9: class
ExtensionService;
This is probably unneeded.

https://codereview.chromium.org/11361189/diff/12003/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/system_indicator/test.js (right):

https://codereview.chromium.org/11361189/diff/12003/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/system_indicator/test.js:5: // Idle api
test for Chrome.
This comment should probably be updated =)

https://codereview.chromium.org/11361189/diff/12003/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/system_indicator/test.js:28: path =
'128.png';
Nit: I'd probably just say var path = here as well. It's much more common to see
one variable declaration per statement.

[dewittj, 2012-11-13 06:58:53]

Trying to get API skeleton into experimental section.  Includes just the IDL and
boilerplate plus an extension test.

Had issues with trybots and binaries, most recent reds were just due to that
issue.  Took out 128.png and altered the tests so that the dependency is gone
now.

http://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.cc
(right):

http://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:8: 
On 2012/11/12 23:00:55, jianli wrote:
> nit: extra empty line not needed
Fixed

http://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:11:
namespace extensions {
On 2012/11/12 23:00:55, jianli wrote:
> nit: empty line before this line

Fixed

http://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:20:
return true;
On 2012/11/12 23:00:55, jianli wrote:
> nit: space alignment

Fixed

http://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.h
(right):

http://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:5: #ifndef
CHROME_BROWSER_EXTENSIONS_API_SYSTEM_INDICATOR_SYSTEM_INDICATOR_API_H__
On 2012/11/12 23:00:55, jianli wrote:
> nit: only one trailing underscore is needed.
Fixed

http://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:54:
private:
On 2012/11/12 23:00:55, jianli wrote:
> nit: not needed
Fixed

http://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:58: #endif
 // CHROME_BROWSER_EXTENSIONS_API_SYSTEM_INDICATOR_SYSTEM_INDICATOR_API_H__
On 2012/11/12 23:00:55, jianli wrote:
> nit: empty line before this line.

Fixed

http://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:58: #endif
 // CHROME_BROWSER_EXTENSIONS_API_SYSTEM_INDICATOR_SYSTEM_INDICATOR_API_H__
On 2012/11/12 23:00:55, jianli wrote:
> nit: empty line before this line.

Fixed

http://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_indicator/system_indicator_apitest.cc
(right):

http://codereview.chromium.org/11361189/diff/10002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_indicator/system_indicator_apitest.cc:11:
switches::kEnableExperimentalExtensionApis);
On 2012/11/12 23:00:55, jianli wrote:
> nit: space alignment

Fixed

http://codereview.chromium.org/11361189/diff/10002/chrome/common/extensions/a...
File chrome/common/extensions/api/experimental_system_indicator.idl (right):

http://codereview.chromium.org/11361189/diff/10002/chrome/common/extensions/a...
chrome/common/extensions/api/experimental_system_indicator.idl:22: // fired when
a click does not result in a menu being shown.
On 2012/11/12 23:00:55, jianli wrote:
> nit: Capital

Fixed

http://codereview.chromium.org/11361189/diff/12003/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.cc
(right):

http://codereview.chromium.org/11361189/diff/12003/chrome/browser/extensions/...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:9: class
ExtensionService;
On 2012/11/13 00:52:19, dcheng wrote:
> This is probably unneeded.

Removed.

http://codereview.chromium.org/11361189/diff/12003/chrome/test/data/extension...
File chrome/test/data/extensions/api_test/system_indicator/test.js (right):

http://codereview.chromium.org/11361189/diff/12003/chrome/test/data/extension...
chrome/test/data/extensions/api_test/system_indicator/test.js:5: // Idle api
test for Chrome.
On 2012/11/13 00:52:19, dcheng wrote:
> This comment should probably be updated =)

Done

http://codereview.chromium.org/11361189/diff/12003/chrome/test/data/extension...
chrome/test/data/extensions/api_test/system_indicator/test.js:28: path =
'128.png';
On 2012/11/13 00:52:19, dcheng wrote:
> Nit: I'd probably just say var path = here as well. It's much more common to
see
> one variable declaration per statement.

I've found that in JS, the "single var statement at the top of each scope" idiom
is prevalent, since block scoping doesn't exist.  I'm removing the dependency on
128.png anyway, so no issue here.

[Pete Williamson, 2012-11-13 18:01:40]

http://codereview.chromium.org/11361189/diff/5005/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_system_indicator.idl (right):

http://codereview.chromium.org/11361189/diff/5005/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_system_indicator.idl:25: };
I don't see any faults with the code we have, but there is functionality that I
would have expected to find that I don't see here. Specifically, it would be
great if an app had a way to populate a right click menu with several options,
and to get notified when an option was chosen.

Also, if an app sets a system tray icon, what happens if the app exits, or if
chrome exits?  (I realize that this is not the right place for the question, but
I thought of it while looking at this code.)

As I mentioned elsewhere, if we want to minimize the risk of spoofing, we should
demand that the URL is local or relative instead of allowing any site on the
internet, and modify the comments to say so.

http://codereview.chromium.org/11361189/diff/1019/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.cc
(right):

http://codereview.chromium.org/11361189/diff/1019/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:5:
#include "chrome/browser/extensions/api/system_indicator/system_indicator_api.h"
As I recall, coding standards say that this should be first, which it is, but
there is normally a blank line after the first header and before the others.

http://codereview.chromium.org/11361189/diff/1019/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.h
(right):

http://codereview.chromium.org/11361189/diff/1019/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:12: class
SystemIndicatorSetURLFunction : public SyncExtensionFunction {
Why Sync instead of Async?  (It's not necessarily wrong, I just want to
understand the intent better)

http://codereview.chromium.org/11361189/diff/1019/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/system_indicator/test.js (right):

http://codereview.chromium.org/11361189/diff/1019/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/system_indicator/test.js:8:
chrome.test.runTests([
can you take the verification in these tests a bit further, and verify that
something actually shows?  Better still if we can verify that what shows is what
we expect.

[Ben Goodger (Google), 2012-11-13 18:03:26]

gyp lgtm, but you can tbr that kind of thing to me.

[dewittj, 2012-11-14 00:25:07]

Addressed all reveiwers' notes.

http://codereview.chromium.org/11361189/diff/5005/chrome/common/extensions/ap...
File chrome/common/extensions/api/experimental_system_indicator.idl (right):

http://codereview.chromium.org/11361189/diff/5005/chrome/common/extensions/ap...
chrome/common/extensions/api/experimental_system_indicator.idl:25: };
On 2012/11/13 18:01:41, Pete Williamson wrote:
> I don't see any faults with the code we have, but there is functionality that
I
> would have expected to find that I don't see here. Specifically, it would be
> great if an app had a way to populate a right click menu with several options,
> and to get notified when an option was chosen.
> 
> Also, if an app sets a system tray icon, what happens if the app exits, or if
> chrome exits?  (I realize that this is not the right place for the question,
but
> I thought of it while looking at this code.)
> 
> As I mentioned elsewhere, if we want to minimize the risk of spoofing, we
should
> demand that the URL is local or relative instead of allowing any site on the
> internet, and modify the comments to say so.

Ultimately agreed that a separate menu API is appropriate so it is now in. 
Added an IDL comment about local icon resources.

http://codereview.chromium.org/11361189/diff/1019/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.cc
(right):

http://codereview.chromium.org/11361189/diff/1019/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:5:
#include "chrome/browser/extensions/api/system_indicator/system_indicator_api.h"
On 2012/11/13 18:01:41, Pete Williamson wrote:
> As I recall, coding standards say that this should be first, which it is, but
> there is normally a blank line after the first header and before the others.

Good catch, fixed

http://codereview.chromium.org/11361189/diff/1019/chrome/browser/extensions/a...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.h
(right):

http://codereview.chromium.org/11361189/diff/1019/chrome/browser/extensions/a...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:12: class
SystemIndicatorSetURLFunction : public SyncExtensionFunction {
On 2012/11/13 18:01:41, Pete Williamson wrote:
> Why Sync instead of Async?  (It's not necessarily wrong, I just want to
> understand the intent better)

I expect the actual loading of the image from the URL will be done in a custom
JS binding, so the backend work to actually set the data will be synchronous.

http://codereview.chromium.org/11361189/diff/1019/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/system_indicator/test.js (right):

http://codereview.chromium.org/11361189/diff/1019/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/system_indicator/test.js:8:
chrome.test.runTests([
On 2012/11/13 18:01:41, Pete Williamson wrote:
> can you take the verification in these tests a bit further, and verify that
> something actually shows?  Better still if we can verify that what shows is
what
> we expect.

Once I have some c++ code in there, I intend to add more testing around the
actual behavior.  Unfortunately the JS can't currently determine what actually
is in the system tray.

[jianli, 2012-11-14 19:27:41]

lgtm

https://codereview.chromium.org/11361189/diff/26/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.cc
(right):

https://codereview.chromium.org/11361189/diff/26/chrome/browser/extensions/ap...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:27: /*
Validation is tricky since instanceOf can't be used for ImageData */
nit: We usually start the comment with "//".

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
File chrome/common/extensions/api/experimental_system_indicator.idl (right):

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
chrome/common/extensions/api/experimental_system_indicator.idl:46: // fired only
when a click on the icon does not result in a menu being
nit: capitalize "fired"

https://codereview.chromium.org/11361189/diff/26/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/system_indicator/test.js (right):

https://codereview.chromium.org/11361189/diff/26/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/system_indicator/test.js:10: // Hide before
show, just in case
nit: end with period

https://codereview.chromium.org/11361189/diff/26/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/system_indicator/test.js:22: // create a
canvas, then set the icon using it.
nit: capitalize "create"

https://codereview.chromium.org/11361189/diff/26/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/system_indicator/test.js:36: // set empty
menu
nit: ditto

[dewittj, 2012-11-14 19:51:09]

Just need an OWNERS review for the extensions directory.

[not at google - send to devlin, 2012-11-14 19:53:20]

https://codereview.chromium.org/11361189/diff/26/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.cc
(right):

https://codereview.chromium.org/11361189/diff/26/chrome/browser/extensions/ap...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:40: 
nit: remove blank line

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
File chrome/common/extensions/api/experimental_system_indicator.idl (right):

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
chrome/common/extensions/api/experimental_system_indicator.idl:7: namespace
experimental.systemIndicator {
We should avoid making APIs experimental, it complicates the permission model
(particularly with respect to the comment below). Instead, while it's in
development, restrict it to trunk and in this patch set up the required
permissions/manifest/etc.

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
chrome/common/extensions/api/experimental_system_indicator.idl:11: MenuItemNode
[] nodes;
For better or for worse, we already have an API for constructing and updating
context menus. Have you considered re-using its idioms? We may even be able to
re-use parts of the API and its implementation.

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
chrome/common/extensions/api/experimental_system_indicator.idl:24: interface
Functions {
This API looks very similar to the extension action APIs (pageAction and
browserAction). I'd like to see that here too -

The system indicator is setup via a manifest property, like pageAction and
browserAction. Like pageAction it's hidden by default but the app/extension can
choose to show it.

It seems to me like a lot of the methods on page actions are valid here -
hide/show/setTitle/setIcon (which neatly combines both the URL and Data
aspects).

The one difference would be the event model, and the menu, but that's fine.

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
chrome/common/extensions/api/experimental_system_indicator.idl:28: static void
setIconURL(DOMString path);
separating each of these methods with blank lines (like one usually does in
header files) would make this easier to read.

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
chrome/common/extensions/api/experimental_system_indicator.idl:47: // shown.
It's confusing to me that we have onClicked meaning that the menu *wasn't*
clicked, where for the context menu API we have onClicked meaning that it *was*
clicked.

From the user's perspective - if I was to click an icon and then *not* choose a
menu item, I would expect that to be a no-op, and not give the app/extension a
chance to react to it.

My proposal would be for there to be a single onClicked event, to be called with
a null item if something in the menu wasn't clicked - but to only fire this
event if a menu hasn't been specified.

[not at google - send to devlin, 2012-11-14 19:55:22]

P.S. why "system indicator" rather than "system tray"?

[dewittj, 2012-11-14 23:29:37]

On 2012/11/14 19:55:22, kalman wrote:
> P.S. why "system indicator" rather than "system tray"?

Just because the terminology is not the same across all platforms, mostly OSX.

Big change to come due to ripping out the custom menu API.

[dewittj, 2012-11-16 00:56:28]

PTAL

I revamped the API based on all the suggestions from kalman's review and from
the extension review meeting.

* No longer experimental
* Restricted to dev channel
* Removed menuing APIs in favor of adding a scope to chrome.contextMenus API
* Enabled for extensions, packaged apps, and platform apps.

https://codereview.chromium.org/11361189/diff/26/chrome/browser/extensions/ap...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.cc
(right):

https://codereview.chromium.org/11361189/diff/26/chrome/browser/extensions/ap...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:27: /*
Validation is tricky since instanceOf can't be used for ImageData */
On 2012/11/14 19:27:41, jianli wrote:
> nit: We usually start the comment with "//".

Removed comment since I found a way to get params to create successfully.

https://codereview.chromium.org/11361189/diff/26/chrome/browser/extensions/ap...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:40: 
On 2012/11/14 19:53:20, kalman wrote:
> nit: remove blank line

Done

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
File chrome/common/extensions/api/experimental_system_indicator.idl (right):

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
chrome/common/extensions/api/experimental_system_indicator.idl:7: namespace
experimental.systemIndicator {
On 2012/11/14 19:53:20, kalman wrote:
> We should avoid making APIs experimental, it complicates the permission model
> (particularly with respect to the comment below). Instead, while it's in
> development, restrict it to trunk and in this patch set up the required
> permissions/manifest/etc.

Fixed, now restricted to dev channel, but not experimental.

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
chrome/common/extensions/api/experimental_system_indicator.idl:11: MenuItemNode
[] nodes;
On 2012/11/14 19:53:20, kalman wrote:
> For better or for worse, we already have an API for constructing and updating
> context menus. Have you considered re-using its idioms? We may even be able to
> re-use parts of the API and its implementation.

I removed the menuing APIs here and will add access to this menu to the
contextMenus API.

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
chrome/common/extensions/api/experimental_system_indicator.idl:24: interface
Functions {
On 2012/11/14 19:53:20, kalman wrote:
> This API looks very similar to the extension action APIs (pageAction and
> browserAction). I'd like to see that here too -
> 
> The system indicator is setup via a manifest property, like pageAction and
> browserAction. Like pageAction it's hidden by default but the app/extension
can
> choose to show it.
> 
> It seems to me like a lot of the methods on page actions are valid here -
> hide/show/setTitle/setIcon (which neatly combines both the URL and Data
> aspects).
> 
> The one difference would be the event model, and the menu, but that's fine.

I've changed the names and parameters to more closely match the extension action
APIs.  I haven't added a setTitle method since I was planning to require the
title = the app name for attribution purposes.  Spoofing another app would be
made a little harder in that case.

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
chrome/common/extensions/api/experimental_system_indicator.idl:28: static void
setIconURL(DOMString path);
On 2012/11/14 19:53:20, kalman wrote:
> separating each of these methods with blank lines (like one usually does in
> header files) would make this easier to read.

New IDL has spaces

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
chrome/common/extensions/api/experimental_system_indicator.idl:46: // fired only
when a click on the icon does not result in a menu being
On 2012/11/14 19:27:41, jianli wrote:
> nit: capitalize "fired"

Fixed.

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
chrome/common/extensions/api/experimental_system_indicator.idl:47: // shown.
On 2012/11/14 19:53:20, kalman wrote:
> It's confusing to me that we have onClicked meaning that the menu *wasn't*
> clicked, where for the context menu API we have onClicked meaning that it
*was*
> clicked.
> 
> From the user's perspective - if I was to click an icon and then *not* choose
a
> menu item, I would expect that to be a no-op, and not give the app/extension a
> chance to react to it.
> 
> My proposal would be for there to be a single onClicked event, to be called
with
> a null item if something in the menu wasn't clicked - but to only fire this
> event if a menu hasn't been specified.

I think that the confusion is actually in when this event fires.  (I'm removing
onMenuClicked in favor of contextMenus.onClicked)  The systemIndicator.onClicked
event will fire if the system's default behavior for that click is not to open a
menu at all.

For example, on Windows the behavior that is most familiar to users is
right-click opens menu, left-click does either nothing or some app-specific
behavior like focusing the app's main window.  In that case, the onClicked event
will fire on left click, and will not fire if the user right-clicks but does not
select any menu item.

Alternatively, on Mac the standard is for either button to open a menu.  On
these systems, the onClicked event will never fire.

https://codereview.chromium.org/11361189/diff/26/chrome/test/data/extensions/...
File chrome/test/data/extensions/api_test/system_indicator/test.js (right):

https://codereview.chromium.org/11361189/diff/26/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/system_indicator/test.js:10: // Hide before
show, just in case
On 2012/11/14 19:27:41, jianli wrote:
> nit: end with period

Fixed

https://codereview.chromium.org/11361189/diff/26/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/system_indicator/test.js:22: // create a
canvas, then set the icon using it.
On 2012/11/14 19:27:41, jianli wrote:
> nit: capitalize "create"

Fixed

https://codereview.chromium.org/11361189/diff/26/chrome/test/data/extensions/...
chrome/test/data/extensions/api_test/system_indicator/test.js:36: // set empty
menu
On 2012/11/14 19:27:41, jianli wrote:
> nit: ditto

Fixed.

[not at google - send to devlin, 2012-11-16 17:42:04]

Sorry to keep bouncing you around like this.

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
File chrome/common/extensions/api/experimental_system_indicator.idl (right):

https://codereview.chromium.org/11361189/diff/26/chrome/common/extensions/api...
chrome/common/extensions/api/experimental_system_indicator.idl:47: // shown.
On 2012/11/16 00:56:28, dewittj wrote:
> On 2012/11/14 19:53:20, kalman wrote:
> > It's confusing to me that we have onClicked meaning that the menu *wasn't*
> > clicked, where for the context menu API we have onClicked meaning that it
> *was*
> > clicked.
> > 
> > From the user's perspective - if I was to click an icon and then *not*
choose
> a
> > menu item, I would expect that to be a no-op, and not give the app/extension
a
> > chance to react to it.
> > 
> > My proposal would be for there to be a single onClicked event, to be called
> with
> > a null item if something in the menu wasn't clicked - but to only fire this
> > event if a menu hasn't been specified.
> 
> I think that the confusion is actually in when this event fires.  (I'm
removing
> onMenuClicked in favor of contextMenus.onClicked)  The
systemIndicator.onClicked
> event will fire if the system's default behavior for that click is not to open
a
> menu at all.
> 
> For example, on Windows the behavior that is most familiar to users is
> right-click opens menu, left-click does either nothing or some app-specific
> behavior like focusing the app's main window.  In that case, the onClicked
event
> will fire on left click, and will not fire if the user right-clicks but does
not
> select any menu item.
> 
> Alternatively, on Mac the standard is for either button to open a menu.  On
> these systems, the onClicked event will never fire.

Yep that's exactly what I meant.

https://codereview.chromium.org/11361189/diff/22002/chrome/browser/extensions...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.h
(right):

https://codereview.chromium.org/11361189/diff/22002/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:11: 
It seems to me that the eventual implementation for this feature will be to add
an ExtensionAction for the system indicator and hang it off Extension (in the
way that browser/page actions and script badges do).

Which means that we can get most of the implementation for free by making these
functions point to their ExtensionAction equivalents (ditto), just being
selective about what functions we expose to it (ditto).

Advantages to using ExtensionAction: consistency, icon handling for free, simple
implementation, you can add better tests for it now, and probably more.

I am not sure how nicely this will play into using IDL for this feature. It
might be tricky. But JSON isn't that bad (and it will be even better once
crbug.com/141318 is fixed but I guess that's moot).

https://codereview.chromium.org/11361189/diff/22002/chrome/common/extensions/...
File chrome/common/extensions/permissions/api_permission.h (right):

https://codereview.chromium.org/11361189/diff/22002/chrome/common/extensions/...
chrome/common/extensions/permissions/api_permission.h:97: kSystemIndicator,
The permission for the system indicator API should come from the
system_indicator manifest property (requiring both that and a permission would
be redundant).

Implementation-wise that means adding it to the kNonPermissionFunctionNames
array in chrome/common/extensions/permissions/permission_set.cc.

[dewittj, 2012-11-16 19:13:57]

reply to kalman's comments

http://codereview.chromium.org/11361189/diff/22002/chrome/browser/extensions/...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.h
(right):

http://codereview.chromium.org/11361189/diff/22002/chrome/browser/extensions/...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:11: 
On 2012/11/16 17:42:04, kalman wrote:
> It seems to me that the eventual implementation for this feature will be to
add
> an ExtensionAction for the system indicator and hang it off Extension (in the
> way that browser/page actions and script badges do).
> 
> Which means that we can get most of the implementation for free by making
these
> functions point to their ExtensionAction equivalents (ditto), just being
> selective about what functions we expose to it (ditto).
> 
> Advantages to using ExtensionAction: consistency, icon handling for free,
simple
> implementation, you can add better tests for it now, and probably more.
> 
> I am not sure how nicely this will play into using IDL for this feature. It
> might be tricky. But JSON isn't that bad (and it will be even better once
> crbug.com/141318 is fixed but I guess that's moot).

In fact I have already written part of a solution in which this API is just
another extensionaction.  There are some mismatches (ubiquitous tab_ids for
example) but overall it's a pretty good fit.

Do you mind if I go forward with IDL until I hit an insurmountable barrier?

http://codereview.chromium.org/11361189/diff/22002/chrome/common/extensions/p...
File chrome/common/extensions/permissions/api_permission.h (right):

http://codereview.chromium.org/11361189/diff/22002/chrome/common/extensions/p...
chrome/common/extensions/permissions/api_permission.h:97: kSystemIndicator,
On 2012/11/16 17:42:04, kalman wrote:
> The permission for the system indicator API should come from the
> system_indicator manifest property (requiring both that and a permission would
> be redundant).
> 
> Implementation-wise that means adding it to the kNonPermissionFunctionNames
> array in chrome/common/extensions/permissions/permission_set.cc.

I'm not sure all system indicator apps are going to have the manifest section,
for example they might dynamically generate their icons from a canvas.  Is this
primarily for parity with browseraction?

[not at google - send to devlin, 2012-11-16 19:21:09]

https://codereview.chromium.org/11361189/diff/22002/chrome/browser/extensions...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.h
(right):

https://codereview.chromium.org/11361189/diff/22002/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:11: 
On 2012/11/16 19:13:57, dewittj wrote:
> On 2012/11/16 17:42:04, kalman wrote:
> > It seems to me that the eventual implementation for this feature will be to
> add
> > an ExtensionAction for the system indicator and hang it off Extension (in
the
> > way that browser/page actions and script badges do).
> > 
> > Which means that we can get most of the implementation for free by making
> these
> > functions point to their ExtensionAction equivalents (ditto), just being
> > selective about what functions we expose to it (ditto).
> > 
> > Advantages to using ExtensionAction: consistency, icon handling for free,
> simple
> > implementation, you can add better tests for it now, and probably more.
> > 
> > I am not sure how nicely this will play into using IDL for this feature. It
> > might be tricky. But JSON isn't that bad (and it will be even better once
> > crbug.com/141318 is fixed but I guess that's moot).
> 
> In fact I have already written part of a solution in which this API is just
> another extensionaction.  There are some mismatches (ubiquitous tab_ids for
> example) but overall it's a pretty good fit.

Yes the tab ID stuff is a bit problematic, particularly since it doesn't make
sense for apps, but I think it will shake out ok if you just leave it out of the
schema.

> 
> Do you mind if I go forward with IDL until I hit an insurmountable barrier?

Sounds fine.

https://codereview.chromium.org/11361189/diff/22002/chrome/common/extensions/...
File chrome/common/extensions/permissions/api_permission.h (right):

https://codereview.chromium.org/11361189/diff/22002/chrome/common/extensions/...
chrome/common/extensions/permissions/api_permission.h:97: kSystemIndicator,
On 2012/11/16 19:13:57, dewittj wrote:
> On 2012/11/16 17:42:04, kalman wrote:
> > The permission for the system indicator API should come from the
> > system_indicator manifest property (requiring both that and a permission
would
> > be redundant).
> > 
> > Implementation-wise that means adding it to the kNonPermissionFunctionNames
> > array in chrome/common/extensions/permissions/permission_set.cc.
> 
> I'm not sure all system indicator apps are going to have the manifest section,
> for example they might dynamically generate their icons from a canvas.  Is
this
> primarily for parity with browseraction?

The same could be said for browser actions. Generally speaking, we should
encourage apps/extensions to declare their icons up-front since it's likely that
mostly that's what they'll want to do, and it saves extensions/apps needing to
start themselves in order to set an icon (and would save on UI jank).

Browser actions (and page actions) are perfectly happy to just have a blank
manifest section though, forcing them to set it themselves.

It'll be a bit weird only supporting the "default icons" property of it, but
perhaps we'll want to add more features to it later anyway (like
"default_enabled" perhaps), so I think that's fine.

[dewittj, 2012-11-20 16:48:00]

On 2012/11/16 19:21:09, kalman wrote:
>
https://codereview.chromium.org/11361189/diff/22002/chrome/browser/extensions...
> File chrome/browser/extensions/api/system_indicator/system_indicator_api.h
> (right):
> 
>
https://codereview.chromium.org/11361189/diff/22002/chrome/browser/extensions...
> chrome/browser/extensions/api/system_indicator/system_indicator_api.h:11: 
> On 2012/11/16 19:13:57, dewittj wrote:
> > On 2012/11/16 17:42:04, kalman wrote:
> > > It seems to me that the eventual implementation for this feature will be
to
> > add
> > > an ExtensionAction for the system indicator and hang it off Extension (in
> the
> > > way that browser/page actions and script badges do).
> > > 
> > > Which means that we can get most of the implementation for free by making
> > these
> > > functions point to their ExtensionAction equivalents (ditto), just being
> > > selective about what functions we expose to it (ditto).
> > > 
> > > Advantages to using ExtensionAction: consistency, icon handling for free,
> > simple
> > > implementation, you can add better tests for it now, and probably more.
> > > 
> > > I am not sure how nicely this will play into using IDL for this feature.
It
> > > might be tricky. But JSON isn't that bad (and it will be even better once
> > > crbug.com/141318 is fixed but I guess that's moot).
> > 
> > In fact I have already written part of a solution in which this API is just
> > another extensionaction.  There are some mismatches (ubiquitous tab_ids for
> > example) but overall it's a pretty good fit.
> 
> Yes the tab ID stuff is a bit problematic, particularly since it doesn't make
> sense for apps, but I think it will shake out ok if you just leave it out of
the
> schema.
> 
> > 
> > Do you mind if I go forward with IDL until I hit an insurmountable barrier?
> 
> Sounds fine.
> 
>
https://codereview.chromium.org/11361189/diff/22002/chrome/common/extensions/...
> File chrome/common/extensions/permissions/api_permission.h (right):
> 
>
https://codereview.chromium.org/11361189/diff/22002/chrome/common/extensions/...
> chrome/common/extensions/permissions/api_permission.h:97: kSystemIndicator,
> On 2012/11/16 19:13:57, dewittj wrote:
> > On 2012/11/16 17:42:04, kalman wrote:
> > > The permission for the system indicator API should come from the
> > > system_indicator manifest property (requiring both that and a permission
> would
> > > be redundant).
> > > 
> > > Implementation-wise that means adding it to the
kNonPermissionFunctionNames
> > > array in chrome/common/extensions/permissions/permission_set.cc.
> > 
> > I'm not sure all system indicator apps are going to have the manifest
section,
> > for example they might dynamically generate their icons from a canvas.  Is
> this
> > primarily for parity with browseraction?
> 
> The same could be said for browser actions. Generally speaking, we should
> encourage apps/extensions to declare their icons up-front since it's likely
that
> mostly that's what they'll want to do, and it saves extensions/apps needing to
> start themselves in order to set an icon (and would save on UI jank).
> 
> Browser actions (and page actions) are perfectly happy to just have a blank
> manifest section though, forcing them to set it themselves.
> 
> It'll be a bit weird only supporting the "default icons" property of it, but
> perhaps we'll want to add more features to it later anyway (like
> "default_enabled" perhaps), so I think that's fine.

So in light of all this I'm a little unsure of the correct action to take.  We
want this API to be only on dev channel to start - but if we're following the
manifest entry <=> permission model, then there isn't a way to restrict the
actual API bindings to the dev channel, they will be exposed to all apps on all
channels and it will be up to my code to throw an error if it's not the right
spot.  I'm tempted to keep the redundant permission and manifest entry until I
can move out of dev-only land.

[not at google - send to devlin, 2012-11-20 16:56:09]

> 
> So in light of all this I'm a little unsure of the correct action to take.  We
> want this API to be only on dev channel to start - but if we're following the
> manifest entry <=> permission model, then there isn't a way to restrict the
> actual API bindings to the dev channel, they will be exposed to all apps on
all
> channels and it will be up to my code to throw an error if it's not the right
> spot.  I'm tempted to keep the redundant permission and manifest entry until I
> can move out of dev-only land.

The script badge API only allows extensions to specify the "script_badge"
manifest key on trunk. Is it enough to do the same for this API, only specify
"dev"?

[dewittj, 2012-11-20 18:37:04]

On 2012/11/20 16:56:09, kalman wrote:
> > 
> > So in light of all this I'm a little unsure of the correct action to take. 
We
> > want this API to be only on dev channel to start - but if we're following
the
> > manifest entry <=> permission model, then there isn't a way to restrict the
> > actual API bindings to the dev channel, they will be exposed to all apps on
> all
> > channels and it will be up to my code to throw an error if it's not the
right
> > spot.  I'm tempted to keep the redundant permission and manifest entry until
I
> > can move out of dev-only land.
> 
> The script badge API only allows extensions to specify the "script_badge"
> manifest key on trunk. Is it enough to do the same for this API, only specify
> "dev"?

That is not really the problem - the issue is that I want both the API and the
manifest entry to be disabled on stable and beta.  I'm happy to add the proper
magic to _manifest_features.json - but if I remove the systemIndicator
permission and add the module chrome.systemIndicator to the whitelist in
kNonPermissionModuleNames, the API will then be whitelisted on all channels.  Am
I missing something?

[not at google - send to devlin, 2012-11-20 18:49:54]

On 2012/11/20 18:37:04, dewittj wrote:
> On 2012/11/20 16:56:09, kalman wrote:
> > > 
> > > So in light of all this I'm a little unsure of the correct action to take.

> We
> > > want this API to be only on dev channel to start - but if we're following
> the
> > > manifest entry <=> permission model, then there isn't a way to restrict
the
> > > actual API bindings to the dev channel, they will be exposed to all apps
on
> > all
> > > channels and it will be up to my code to throw an error if it's not the
> right
> > > spot.  I'm tempted to keep the redundant permission and manifest entry
until
> I
> > > can move out of dev-only land.
> > 
> > The script badge API only allows extensions to specify the "script_badge"
> > manifest key on trunk. Is it enough to do the same for this API, only
specify
> > "dev"?
> 
> That is not really the problem - the issue is that I want both the API and the
> manifest entry to be disabled on stable and beta.  I'm happy to add the proper
> magic to _manifest_features.json - but if I remove the systemIndicator
> permission and add the module chrome.systemIndicator to the whitelist in
> kNonPermissionModuleNames, the API will then be whitelisted on all channels. 
Am
> I missing something?

If you do as you just said and
- add to _manifest_features.json restricted to dev channel
- add to kNonPermissionModuleNames

Then for an extension on dev:
- if it specifies a system_indicator then it will get one. API calls will work.
- if it doesn't, API calls will reach the browser, but the ExtensionAction API
bindings will fail calls because the extension won't have the required action.

Then likewise, for an extension on stable or beta, if it specifies a
system_indicator it will just be ignored (and show a warning in the UI) that
it's not supported, and it will be as though it never specified one. So the API
calls will just fail.

Note that we could put in a bit more effort and actually only insert the
browser-action/page-action/script-badge/system-indicator permissions if the
manifest supports it (this wouldn't be too hard), just nobody has done it yet.

[dewittj, 2012-11-20 23:16:02]

I've written a patch that adds the systemIndicator permission when a
system_indicator manifest section is successfully parsed.  This is done in
extension.cc within the LoadExtensionFeatures routine.

If this idiom seems good then I could extend the
Extension::LoadExtensionActionInfoHelper function so all the extension action
apis do the same thing.

[not at google - send to devlin, 2012-11-20 23:58:08]

Thanks.

I think we should just wire this up now. It's not that much more effort and it's
better to not leave the code in an interim state.

https://codereview.chromium.org/11361189/diff/24007/chrome/browser/extensions...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.h
(right):

https://codereview.chromium.org/11361189/diff/24007/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:12: class
SystemIndicatorSetIconFunction : public SyncExtensionFunction {
Why not just exactly copy the way this is done from the browser action API? With
minimal effort you'd be able to get everything wired up, since
extension_actions_api.cc does most of the work for you (you'd need to add a bit
of code there to support system indicator).

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2461:
api_permissions.insert(APIPermission::kSystemIndicator);
I like this idea. We should make it happen for all extension actions (except for
script badges - they should always get the permission - but that's another
story).

Note that we were going to build this into the Features system but that didn't
quite happen.

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2464: return true;
Why not just implement it now?

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/...
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/...
chrome/common/extensions/extension.h:913: bool
LoadExtensionFeatures(APIPermissionSet& api_permissions,
Non const pointers bad. If this isn't const anymore, make it a pointer.

[not at google - send to devlin, 2012-11-21 00:00:07]

By "wire this up" I mean to a system_indicator_info property on Extension and
GetSystemIndicator on ExtensionActionManager.

The chrome OS stuff that reads it is something else.

[dewittj, 2012-11-21 22:09:41]

Main changes revolve around integrating with extension action.

https://codereview.chromium.org/11361189/diff/24007/chrome/browser/extensions...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.h
(right):

https://codereview.chromium.org/11361189/diff/24007/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_api.h:12: class
SystemIndicatorSetIconFunction : public SyncExtensionFunction {
On 2012/11/20 23:58:08, kalman wrote:
> Why not just exactly copy the way this is done from the browser action API?
With
> minimal effort you'd be able to get everything wired up, since
> extension_actions_api.cc does most of the work for you (you'd need to add a
bit
> of code there to support system indicator).

Was going to do this in a later CL, but added it here.

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2461:
api_permissions.insert(APIPermission::kSystemIndicator);
On 2012/11/20 23:58:08, kalman wrote:
> I like this idea. We should make it happen for all extension actions (except
for
> script badges - they should always get the permission - but that's another
> story).
> 
> Note that we were going to build this into the Features system but that didn't
> quite happen.

Will add to the rest of the _actions in a separate CL.  What is the state of
Features, is it still being developed?

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2464: return true;
On 2012/11/20 23:58:08, kalman wrote:
> Why not just implement it now?

I still have doubts about what the final format will look like, since
system_indicator will need to support more resolutions than just 19x19@1x and
19x19@2x.  But for now, I have just reused the manifest parser from the other
extension_actions and will update once I get a strategy together for that.

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/...
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/...
chrome/common/extensions/extension.h:913: bool
LoadExtensionFeatures(APIPermissionSet& api_permissions,
On 2012/11/20 23:58:08, kalman wrote:
> Non const pointers bad. If this isn't const anymore, make it a pointer.

I've been revealed as a C++ n00b.  Fixed.

[not at google - send to devlin, 2012-11-21 23:39:23]

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2461:
api_permissions.insert(APIPermission::kSystemIndicator);
On 2012/11/21 22:09:41, dewittj wrote:
> On 2012/11/20 23:58:08, kalman wrote:
> > I like this idea. We should make it happen for all extension actions (except
> for
> > script badges - they should always get the permission - but that's another
> > story).
> > 
> > Note that we were going to build this into the Features system but that
didn't
> > quite happen.
> 
> Will add to the rest of the _actions in a separate CL.  What is the state of
> Features, is it still being developed?

Ok.

The status of the Feature system is partially in this change:
https://codereview.chromium.org/10025007/ but mostly in Aaron's brain.

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2464: return true;
On 2012/11/21 22:09:41, dewittj wrote:
> On 2012/11/20 23:58:08, kalman wrote:
> > Why not just implement it now?
> 
> I still have doubts about what the final format will look like, since
> system_indicator will need to support more resolutions than just 19x19@1x and
> 19x19@2x.  But for now, I have just reused the manifest parser from the other
> extension_actions and will update once I get a strategy together for that.

Yep sounds good, but I hope that any improvements you need to make here would
(may need to) also apply to the other types of extension actions.

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:303: //
part of the Feature system.
This can be convered to a EXTENSION_FUNCTION_VALIDATE when you change all
extension actions to be like the system indicator one in extension.cc.

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:309:
return false;
EXTENSION_FUNCTION_VALIDATE(ValidateArguments())

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:315:
extensions::ExtensionActionManager::Get(profile_);
nit: do this at top, then re-use is everywhere else above given
extensions::ExtensionActionManager::Get.

also, the abbr is a bit unnecessary, just "manager" is fine.

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:320:
EXTENSION_FUNCTION_VALIDATE(mgr->GetSystemIndicator(*GetExtension()));
actually... a more sensible check here is probably just

EXTENSION_FUNCTION_VALIDATE(type == BROWER ||
                            type == SYSTEM_INDICATOR)

since we've already validated there is an action associated with that value.

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:340:
EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &first_arg));
let's push up the call to EXTENSION_FUNCTION_VALIDATE to the caller, and just
return bools from here.

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:411:
void ExtensionActionFunction::NotifyStatusTrayChange() {
TODO(dewittj): implement (see http://crbug.com/142450).

or whatever

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.cc
(right):

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:17:
return true;
oh I didn't realise the reason for the change to ValidateArguments was in order
to override it here.

What ExtensionAction does is nearly good enough. They know how to deal with the
lack of a tab ID property, so that's fine.

I guess you just want to prevent APIs from adding a tab ID? In which case I'd
just do that as a little check inside extension_actions_api.cc (there are
already plenty of those for other things). That said, it's unlikely that the UI
implementation will care about the tab ID, so eh. This validation is already
being done on the renderer. The validation we do on the browser is only for
sanity and security-important stuff, which the tab ID isn't.

So yeah - I'd delete this stuff.

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
File chrome/browser/extensions/extension_action_manager.cc (right):

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/extension_action_manager.cc:142: 
delete blank line

https://codereview.chromium.org/11361189/diff/20064/chrome/common/extensions/...
File chrome/common/extensions/api/system_indicator.idl (right):

https://codereview.chromium.org/11361189/diff/20064/chrome/common/extensions/...
chrome/common/extensions/api/system_indicator.idl:6: // menubar, system tray, or
other visible area provided by the OS.
could you add note like "this is modelled as an extension action API, so
analogous to browserAction.json and pageAction.json".

https://codereview.chromium.org/11361189/diff/20064/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11361189/diff/20064/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2471:
api_permissions->insert(APIPermission::kSystemIndicator);
TODO(dewittj): do this for all extension action APIs.

https://codereview.chromium.org/11361189/diff/20064/chrome/renderer/resources...
File chrome/renderer/resources/extensions/system_indicator_custom_bindings.js
(right):

https://codereview.chromium.org/11361189/diff/20064/chrome/renderer/resources...
chrome/renderer/resources/extensions/system_indicator_custom_bindings.js:5: //
Custom bindings for the browserAction API.
update comment

https://codereview.chromium.org/11361189/diff/20064/chrome/renderer/resources...
chrome/renderer/resources/extensions/system_indicator_custom_bindings.js:10:
chromeHidden.registerCustomHook('systemIndicator', function(bindingsAPI) {
if you're up for a little bit of cleanup along the way, pull this stuff into a
helper module and call it from page_action_custom_bindings and
browser_action_custom_bindings (and here).

https://codereview.chromium.org/11361189/diff/20064/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/system_indicator/test.js (right):

https://codereview.chromium.org/11361189/diff/20064/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/system_indicator/test.js:16: 
I'm not really sure what the added value of these tests are since we presumably
already have ones for browser actions, which use the same machinery.

But if we don't already have such tests, cool.

[not at google - send to devlin, 2012-11-21 23:43:35]

https://codereview.chromium.org/11361189/diff/20064/chrome/renderer/resources...
File chrome/renderer/resources/extensions/system_indicator_custom_bindings.js
(right):

https://codereview.chromium.org/11361189/diff/20064/chrome/renderer/resources...
chrome/renderer/resources/extensions/system_indicator_custom_bindings.js:10:
chromeHidden.registerCustomHook('systemIndicator', function(bindingsAPI) {
On 2012/11/21 23:39:23, kalman wrote:
> if you're up for a little bit of cleanup along the way, pull this stuff into a
> helper module and call it from page_action_custom_bindings and
> browser_action_custom_bindings (and here).

A good time for this might be in your follow-up.

[dewittj, 2012-11-26 18:32:58]

Thanks for the feedback.  PTAL

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:303: //
part of the Feature system.
On 2012/11/21 23:39:23, kalman wrote:
> This can be convered to a EXTENSION_FUNCTION_VALIDATE when you change all
> extension actions to be like the system indicator one in extension.cc.

OK

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:309:
return false;
On 2012/11/21 23:39:23, kalman wrote:
> EXTENSION_FUNCTION_VALIDATE(ValidateArguments())
Done.  Renamed to more accurately reflect what's going on.

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:315:
extensions::ExtensionActionManager::Get(profile_);
On 2012/11/21 23:39:23, kalman wrote:
> nit: do this at top, then re-use is everywhere else above given
> extensions::ExtensionActionManager::Get.
> 
> also, the abbr is a bit unnecessary, just "manager" is fine.

Done.

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:320:
EXTENSION_FUNCTION_VALIDATE(mgr->GetSystemIndicator(*GetExtension()));
On 2012/11/21 23:39:23, kalman wrote:
> actually... a more sensible check here is probably just
> 
> EXTENSION_FUNCTION_VALIDATE(type == BROWER ||
>                             type == SYSTEM_INDICATOR)
> 
> since we've already validated there is an action associated with that value.

Good idea. Done.

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:340:
EXTENSION_FUNCTION_VALIDATE(args_->Get(0, &first_arg));
On 2012/11/21 23:39:23, kalman wrote:
> let's push up the call to EXTENSION_FUNCTION_VALIDATE to the caller, and just
> return bools from here.

Done.

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:411:
void ExtensionActionFunction::NotifyStatusTrayChange() {
On 2012/11/21 23:39:23, kalman wrote:
> TODO(dewittj): implement (see http://crbug.com/142450).
> 
> or whatever

Done.

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
File chrome/browser/extensions/api/system_indicator/system_indicator_api.cc
(right):

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc:17:
return true;
On 2012/11/21 23:39:23, kalman wrote:
> oh I didn't realise the reason for the change to ValidateArguments was in
order
> to override it here.
> 
> What ExtensionAction does is nearly good enough. They know how to deal with
the
> lack of a tab ID property, so that's fine.
> 
> I guess you just want to prevent APIs from adding a tab ID? In which case I'd
> just do that as a little check inside extension_actions_api.cc (there are
> already plenty of those for other things). That said, it's unlikely that the
UI
> implementation will care about the tab ID, so eh. This validation is already
> being done on the renderer. The validation we do on the browser is only for
> sanity and security-important stuff, which the tab ID isn't.
> 
> So yeah - I'd delete this stuff.

Done.

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
File chrome/browser/extensions/extension_action_manager.cc (right):

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions...
chrome/browser/extensions/extension_action_manager.cc:142: 
On 2012/11/21 23:39:23, kalman wrote:
> delete blank line

Done.

https://codereview.chromium.org/11361189/diff/20064/chrome/common/extensions/...
File chrome/common/extensions/api/system_indicator.idl (right):

https://codereview.chromium.org/11361189/diff/20064/chrome/common/extensions/...
chrome/common/extensions/api/system_indicator.idl:6: // menubar, system tray, or
other visible area provided by the OS.
On 2012/11/21 23:39:23, kalman wrote:
> could you add note like "this is modelled as an extension action API, so
> analogous to browserAction.json and pageAction.json".

Done.

https://codereview.chromium.org/11361189/diff/20064/chrome/common/extensions/...
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11361189/diff/20064/chrome/common/extensions/...
chrome/common/extensions/extension.cc:2471:
api_permissions->insert(APIPermission::kSystemIndicator);
On 2012/11/21 23:39:23, kalman wrote:
> TODO(dewittj): do this for all extension action APIs.

Done.

https://codereview.chromium.org/11361189/diff/20064/chrome/renderer/resources...
File chrome/renderer/resources/extensions/system_indicator_custom_bindings.js
(right):

https://codereview.chromium.org/11361189/diff/20064/chrome/renderer/resources...
chrome/renderer/resources/extensions/system_indicator_custom_bindings.js:5: //
Custom bindings for the browserAction API.
On 2012/11/21 23:39:23, kalman wrote:
> update comment

Done.

https://codereview.chromium.org/11361189/diff/20064/chrome/renderer/resources...
chrome/renderer/resources/extensions/system_indicator_custom_bindings.js:10:
chromeHidden.registerCustomHook('systemIndicator', function(bindingsAPI) {
On 2012/11/21 23:43:36, kalman wrote:
> On 2012/11/21 23:39:23, kalman wrote:
> > if you're up for a little bit of cleanup along the way, pull this stuff into
a
> > helper module and call it from page_action_custom_bindings and
> > browser_action_custom_bindings (and here).
> 
> A good time for this might be in your follow-up.

OK.

https://codereview.chromium.org/11361189/diff/20064/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/system_indicator/test.js (right):

https://codereview.chromium.org/11361189/diff/20064/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/system_indicator/test.js:16: 
On 2012/11/21 23:39:23, kalman wrote:
> I'm not really sure what the added value of these tests are since we
presumably
> already have ones for browser actions, which use the same machinery.
> 
> But if we don't already have such tests, cool.

Okay, I'll simplify the tests so that it just tries each of the API functions
once and add more specific tests once system indicator-specific behavior is
added.

[not at google - send to devlin, 2012-11-26 19:44:54]

lgtm, and you might also want to update the patch description

https://codereview.chromium.org/11361189/diff/19012/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

https://codereview.chromium.org/11361189/diff/19012/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:289:
base::StringPiece functionName(name());
chromium is unix_style not camelCase.

however, name() is just a simple accessor:
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions...

so using StartsWith(name(), "...") from string_util.h is probably more
appropriate than constructing a new StringPiece.

https://codereview.chromium.org/11361189/diff/19012/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:326:
extensions::Extension::ActionInfo::TYPE_SYSTEM_INDICATOR;
this is just making it more verbose?

https://codereview.chromium.org/11361189/diff/19012/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:344: }
generally leave out {} if it's just a 1 line body.

https://codereview.chromium.org/11361189/diff/19012/chrome/browser/extensions...
File chrome/browser/extensions/browser_event_router.cc (right):

https://codereview.chromium.org/11361189/diff/19012/chrome/browser/extensions...
chrome/browser/extensions/browser_event_router.cc:587: // The System Indicator
handles it's own clicks.
it's -> its

https://codereview.chromium.org/11361189/diff/19012/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/system_indicator/basics/test.js
(right):

https://codereview.chromium.org/11361189/diff/19012/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/system_indicator/basics/test.js:6: //
browser_tests.exe --gtest_filter=ExtensionApiTest.SystemIndicator
Do other tests have this? It seems unnecessary to me, generally people know how
to use --gtest_filter. I guess it might be useful saying that this test's
companion is in system_indicator_apitest.cc though.

https://codereview.chromium.org/11361189/diff/19012/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/system_indicator/basics/test.js:8: var
callback = function() {
unused?

https://codereview.chromium.org/11361189/diff/19012/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/system_indicator/basics/test.js:29:
chrome.systemIndicator.setIcon({ imageData: data }, function () {
use callbackPass here (see chrome/renderer/resources/extensions/apitest.js) like

chrome.sI.setIcon(..., chrome.test.callbackPass(function() {
  chrome.sI.enable();
  chrome.sI.disable();
}));

it will do the lastError check for you, and automatically call
chrome.test.succeed afterwards.

[dewittj, 2012-11-27 01:53:55]

Fixed testing issues and the style problems.

https://codereview.chromium.org/11361189/diff/19012/chrome/browser/extensions...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

https://codereview.chromium.org/11361189/diff/19012/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:289:
base::StringPiece functionName(name());
On 2012/11/26 19:44:54, kalman wrote:
> chromium is unix_style not camelCase.
> 
> however, name() is just a simple accessor:
>
https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/extensions...
> 
> so using StartsWith(name(), "...") from string_util.h is probably more
> appropriate than constructing a new StringPiece.

Done. StartsWithASCII added.

https://codereview.chromium.org/11361189/diff/19012/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:326:
extensions::Extension::ActionInfo::TYPE_SYSTEM_INDICATOR;
On 2012/11/26 19:44:54, kalman wrote:
> this is just making it more verbose?

This is making it fit in the 80 column limit.  Replaced with a block-local
typedef.

https://codereview.chromium.org/11361189/diff/19012/chrome/browser/extensions...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:344: }
On 2012/11/26 19:44:54, kalman wrote:
> generally leave out {} if it's just a 1 line body.

Done.

https://codereview.chromium.org/11361189/diff/19012/chrome/browser/extensions...
File chrome/browser/extensions/browser_event_router.cc (right):

https://codereview.chromium.org/11361189/diff/19012/chrome/browser/extensions...
chrome/browser/extensions/browser_event_router.cc:587: // The System Indicator
handles it's own clicks.
On 2012/11/26 19:44:54, kalman wrote:
> it's -> its

Done.

https://codereview.chromium.org/11361189/diff/19012/chrome/test/data/extensio...
File chrome/test/data/extensions/api_test/system_indicator/basics/test.js
(right):

https://codereview.chromium.org/11361189/diff/19012/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/system_indicator/basics/test.js:6: //
browser_tests.exe --gtest_filter=ExtensionApiTest.SystemIndicator
On 2012/11/26 19:44:54, kalman wrote:
> Do other tests have this? It seems unnecessary to me, generally people know
how
> to use --gtest_filter. I guess it might be useful saying that this test's
> companion is in system_indicator_apitest.cc though.

The test I first modeled this test after did have it, but I see that many others
don't.  I'm fine removing it.

https://codereview.chromium.org/11361189/diff/19012/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/system_indicator/basics/test.js:8: var
callback = function() {
On 2012/11/26 19:44:54, kalman wrote:
> unused?

Removed, replaced by callbackPass.

https://codereview.chromium.org/11361189/diff/19012/chrome/test/data/extensio...
chrome/test/data/extensions/api_test/system_indicator/basics/test.js:29:
chrome.systemIndicator.setIcon({ imageData: data }, function () {
On 2012/11/26 19:44:54, kalman wrote:
> use callbackPass here (see chrome/renderer/resources/extensions/apitest.js)
like
> 
> chrome.sI.setIcon(..., chrome.test.callbackPass(function() {
>   chrome.sI.enable();
>   chrome.sI.disable();
> }));
> 
> it will do the lastError check for you, and automatically call
> chrome.test.succeed afterwards.

Done.

[commit-bot: I haz the power, 2012-11-27 21:55:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/11361189/30010

[commit-bot: I haz the power, 2012-11-27 22:45:22]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-28 01:08:04]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/11361189/30010

[commit-bot: I haz the power, 2012-11-28 15:07:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/11361189/30010

[commit-bot: I haz the power, 2012-11-28 15:16:16]

Sorry for I got bad news for ya.
Compile failed with a clobber build on win.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-11-28 23:19:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dewittj@chromium.org/11361189/30010

[commit-bot: I haz the power, 2012-11-28 23:21:18]

Change committed as 170090

[Nico, 2015-07-06 17:39:20]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2015-07-06 17:39:21]

https://chromiumcodereview.appspot.com/11361189/diff/30010/chrome/browser/ext...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

https://chromiumcodereview.appspot.com/11361189/diff/30010/chrome/browser/ext...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:291: }
else if (StartsWithASCII(name(), "systemIndicator.", false)) {
Isn't javascript case-sensitive? Why is this passing false instead of true?

[dewittj, 2015-07-06 17:44:52]

https://chromiumcodereview.appspot.com/11361189/diff/30010/chrome/browser/ext...
File chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

https://chromiumcodereview.appspot.com/11361189/diff/30010/chrome/browser/ext...
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:291: }
else if (StartsWithASCII(name(), "systemIndicator.", false)) {
On 2015/07/06 17:39:21, Nico wrote:
> Isn't javascript case-sensitive? Why is this passing false instead of true?

Looks like it should be case sensitive.

[dewittj, 2015-07-06 18:13:26]

sorry for spam, please don't review this