Re: Initial skeleton for System Indicator API (issue 11361189)

dew...@chromium.org

unread,

Nov 20, 2012, 11:48:00 AM11/20/12

to jia...@chromium.org, pet...@chromium.org, b...@chromium.org, kal...@chromium.org, chromium...@chromium.org, a...@chromium.org, chromium-a...@chromium.org, dim...@chromium.org

On 2012/11/16 19:21:09, kalman wrote:

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

https://codereview.chromium.org/11361189/diff/22002/chrome/browser/extensions/api/system_indicator/system_indicator_api.h#newcode11
> 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/permissions/api_permission.h
> File chrome/common/extensions/permissions/api_permission.h (right):

https://codereview.chromium.org/11361189/diff/22002/chrome/common/extensions/permissions/api_permission.h#newcode97
> 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.

https://codereview.chromium.org/11361189/

kal...@chromium.org

unread,

Nov 20, 2012, 11:56:09 AM11/20/12

to dew...@chromium.org, jia...@chromium.org, pet...@chromium.org, b...@chromium.org, chromium...@chromium.org, a...@chromium.org, chromium-a...@chromium.org, dim...@chromium.org

> 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"?

https://codereview.chromium.org/11361189/

dew...@chromium.org

unread,

Nov 20, 2012, 1:37:04 PM11/20/12

to jia...@chromium.org, pet...@chromium.org, b...@chromium.org, kal...@chromium.org, chromium...@chromium.org, a...@chromium.org, chromium-a...@chromium.org, dim...@chromium.org

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?

https://codereview.chromium.org/11361189/

kal...@chromium.org

unread,

Nov 20, 2012, 1:49:55 PM11/20/12

to dew...@chromium.org, jia...@chromium.org, pet...@chromium.org, b...@chromium.org, chromium...@chromium.org, a...@chromium.org, chromium-a...@chromium.org, dim...@chromium.org

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.

https://codereview.chromium.org/11361189/

dew...@chromium.org

unread,

Nov 20, 2012, 6:16:02 PM11/20/12

to jia...@chromium.org, pet...@chromium.org, b...@chromium.org, kal...@chromium.org, chromium...@chromium.org, a...@chromium.org, chromium-a...@chromium.org, dim...@chromium.org

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.

https://codereview.chromium.org/11361189/

kal...@chromium.org

unread,

Nov 20, 2012, 6:58:08 PM11/20/12

to dew...@chromium.org, jia...@chromium.org, pet...@chromium.org, b...@chromium.org, chromium...@chromium.org, a...@chromium.org, chromium-a...@chromium.org, dim...@chromium.org

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/api/system_indicator/system_indicator_api.h
File
chrome/browser/extensions/api/system_indicator/system_indicator_api.h
(right):

https://codereview.chromium.org/11361189/diff/24007/chrome/browser/extensions/api/system_indicator/system_indicator_api.h#newcode12
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/extension.cc
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/extension.cc#newcode2461
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/extension.cc#newcode2464
chrome/common/extensions/extension.cc:2464: return true;
Why not just implement it now?

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

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/extension.h#newcode913
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.

https://codereview.chromium.org/11361189/

kal...@chromium.org

unread,

Nov 20, 2012, 7:00:07 PM11/20/12

to dew...@chromium.org, jia...@chromium.org, pet...@chromium.org, b...@chromium.org, chromium...@chromium.org, a...@chromium.org, chromium-a...@chromium.org, dim...@chromium.org

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.

https://codereview.chromium.org/11361189/

dew...@chromium.org

unread,

Nov 21, 2012, 5:09:41 PM11/21/12

to jia...@chromium.org, pet...@chromium.org, b...@chromium.org, kal...@chromium.org, chromium...@chromium.org, a...@chromium.org, chromium-a...@chromium.org, dim...@chromium.org

Main changes revolve around integrating with extension action.

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

https://codereview.chromium.org/11361189/diff/24007/chrome/browser/extensions/api/system_indicator/system_indicator_api.h#newcode12
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/extension.cc
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/extension.cc#newcode2461
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/extension.cc#newcode2464
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/extension.h
File chrome/common/extensions/extension.h (right):

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/extension.h#newcode913
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.

https://codereview.chromium.org/11361189/

kal...@chromium.org

unread,

Nov 21, 2012, 6:39:23 PM11/21/12

to dew...@chromium.org, jia...@chromium.org, pet...@chromium.org, b...@chromium.org, chromium...@chromium.org, a...@chromium.org, chromium-a...@chromium.org, dim...@chromium.org

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

https://codereview.chromium.org/11361189/diff/24007/chrome/common/extensions/extension.cc#newcode2461
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/extension.cc#newcode2464
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/api/extension_action/extension_actions_api.cc
File
chrome/browser/extensions/api/extension_action/extension_actions_api.cc
(right):

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions/api/extension_action/extension_actions_api.cc#newcode303
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/api/extension_action/extension_actions_api.cc#newcode309
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/api/extension_action/extension_actions_api.cc#newcode315
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/api/extension_action/extension_actions_api.cc#newcode320
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/api/extension_action/extension_actions_api.cc#newcode340
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/api/extension_action/extension_actions_api.cc#newcode411
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/api/system_indicator/system_indicator_api.cc
File
chrome/browser/extensions/api/system_indicator/system_indicator_api.cc
(right):

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions/api/system_indicator/system_indicator_api.cc#newcode17
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/extension_action_manager.cc
File chrome/browser/extensions/extension_action_manager.cc (right):

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

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

https://codereview.chromium.org/11361189/diff/20064/chrome/common/extensions/api/system_indicator.idl#newcode6
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/extension.cc
File chrome/common/extensions/extension.cc (right):

https://codereview.chromium.org/11361189/diff/20064/chrome/common/extensions/extension.cc#newcode2471
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/extensions/system_indicator_custom_bindings.js
File
chrome/renderer/resources/extensions/system_indicator_custom_bindings.js
(right):

https://codereview.chromium.org/11361189/diff/20064/chrome/renderer/resources/extensions/system_indicator_custom_bindings.js#newcode5
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/extensions/system_indicator_custom_bindings.js#newcode10
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/extensions/api_test/system_indicator/test.js
File chrome/test/data/extensions/api_test/system_indicator/test.js
(right):

https://codereview.chromium.org/11361189/diff/20064/chrome/test/data/extensions/api_test/system_indicator/test.js#newcode16
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.

https://codereview.chromium.org/11361189/

kal...@chromium.org

unread,

Nov 21, 2012, 6:43:36 PM11/21/12

to dew...@chromium.org, jia...@chromium.org, pet...@chromium.org, b...@chromium.org, chromium...@chromium.org, a...@chromium.org, chromium-a...@chromium.org, dim...@chromium.org

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

https://codereview.chromium.org/11361189/diff/20064/chrome/renderer/resources/extensions/system_indicator_custom_bindings.js#newcode10
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.

https://codereview.chromium.org/11361189/

dew...@chromium.org

unread,

Nov 26, 2012, 1:32:58 PM11/26/12

to jia...@chromium.org, pet...@chromium.org, b...@chromium.org, kal...@chromium.org, chromium...@chromium.org, a...@chromium.org, chromium-a...@chromium.org, dim...@chromium.org

Thanks for the feedback. PTAL

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

https://codereview.chromium.org/11361189/diff/20064/chrome/browser/extensions/api/extension_action/extension_actions_api.cc#newcode303
chrome/browser/extensions/api/extension_action/extension_actions_api.cc:303:
// part of the Feature system.