Allow platform apps to add themselves to the Action Box.

Allow platform apps to add themselves to the Action Box.

This change allows apps to declare a page_launcher key in the
manifest to add themselves to the action box. When the
corresponding action is clicked, the app will receive a
pageLauncher.onClicked event.

ActionBox design document: https://docs.google.com/a/google.com/document/d/1Qxiey4vnbX0YwCRi5BGQbeCuH3DxHGMOtYnQOjBpcCI/edit

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

[skare_, 2013-01-30 00:49:50]

lgtm

https://codereview.chromium.org/12095023/diff/24/chrome/common/extensions/api...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/12095023/diff/24/chrome/common/extensions/api...
chrome/common/extensions/api/_manifest_features.json:238: "channel": "stable",
should we be on dev at first?

https://codereview.chromium.org/12095023/diff/8038/chrome/browser/extensions/...
File chrome/browser/extensions/api/page_launcher/page_launcher_api.h (right):

https://codereview.chromium.org/12095023/diff/8038/chrome/browser/extensions/...
chrome/browser/extensions/api/page_launcher/page_launcher_api.h:47: #endif  //
CHROME_BROWSER_EXTENSIONS_API_PAGE_LAUNCHER_PAGE_LAUNCHER_API_H_
nit: add newline at EOF, here and in the .cc

https://codereview.chromium.org/12095023/diff/8038/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

https://codereview.chromium.org/12095023/diff/8038/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:111:
extension_command_ids_.begin();
tiny nit: line this multiline for loop and the one above consistently (4 or 5
spaces).

https://codereview.chromium.org/12095023/diff/8038/chrome/common/extensions/a...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/12095023/diff/8038/chrome/common/extensions/a...
chrome/common/extensions/api/_manifest_features.json:238: "channel": "stable",
(product-level): dev first?

[Rune Fevang, 2013-01-30 01:32:12]

https://codereview.chromium.org/12095023/diff/8038/chrome/browser/extensions/...
File chrome/browser/extensions/api/page_launcher/page_launcher_api.h (right):

https://codereview.chromium.org/12095023/diff/8038/chrome/browser/extensions/...
chrome/browser/extensions/api/page_launcher/page_launcher_api.h:47: #endif  //
CHROME_BROWSER_EXTENSIONS_API_PAGE_LAUNCHER_PAGE_LAUNCHER_API_H_
On 2013/01/30 00:49:51, Travis Skare wrote:
> nit: add newline at EOF, here and in the .cc

Not sure why the linter thinks its missing, it's complaining about all the added
files, but it's present locally at least.

https://codereview.chromium.org/12095023/diff/8038/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

https://codereview.chromium.org/12095023/diff/8038/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:111:
extension_command_ids_.begin();
On 2013/01/30 00:49:51, Travis Skare wrote:
> tiny nit: line this multiline for loop and the one above consistently (4 or 5
> spaces).

Good eye :) Changed this one to match the other two.

https://codereview.chromium.org/12095023/diff/8038/chrome/common/extensions/a...
File chrome/common/extensions/api/_manifest_features.json (right):

https://codereview.chromium.org/12095023/diff/8038/chrome/common/extensions/a...
chrome/common/extensions/api/_manifest_features.json:238: "channel": "stable",
On 2013/01/30 00:49:51, Travis Skare wrote:
> (product-level): dev first?

I'm not really sure how this is done usually, I set it to stable since it
shouldn't really do anything without the action box flag being on anyway.
Changed it to dev.

[not at google - send to devlin, 2013-02-01 16:39:47]

Cool, mostly nits.

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/extensions...
File chrome/browser/extensions/api/page_launcher/page_launcher_api.cc (right):

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/extensions...
chrome/browser/extensions/api/page_launcher/page_launcher_api.cc:32: const
std::string& url,
GURL?

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.cc:58: for
(ExtensionSet::const_iterator iter = extensions->begin();
nit: this class uses "it" everywhere for iterator name, stick with that.

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.cc:98:
web_contents->GetURL().spec(),
send the url as a GURL?

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.cc:111: for
(ExtensionIdCommandMap::const_iterator iter =
iter -> it rename is arbitrary, please revert (also see above comment)

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.cc:116: }
single-line ifs omit {}

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.cc:149: for
(ExtensionIdCommandMap::iterator iter = extension_command_ids_.begin();
ditto

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.cc:155: }
single-line ifs omit {}

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/action_box_button_controller.h (right):

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.h:38: virtual void
ShowMenu(scoped_ptr<ActionBoxMenuModel> menu_model);
is there a c++-y reason for this change? AFAIK it's a pretty common idiom to
implement blank virtual methods inline like this.

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/views/a...
File chrome/browser/ui/views/action_box_menu.cc (right):

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/views/a...
chrome/browser/ui/views/action_box_menu.cc:23: public IconImage::Observer {
1 line

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/views/a...
chrome/browser/ui/views/action_box_menu.cc:42: }
  // namespace

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
File chrome/common/extensions/api/page_launcher.idl (right):

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher.idl:4: 
API-level description

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher.idl:6: dictionary PageData {
Description of PageData

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher.idl:15: };
I know we're not using tabs.Tab here explicitly, but I think it would actually
be a good thing to do at some point, or at least something like it.

Two things to note: there is a precedent for this (chrome.runtime) but it won't
actually work (bug that I hope to fix sometime) - and I think that fixing this
bug will involve figuring out a standard way to expose tabs to apps (maybe via
an abstraction like PageData) since I doubt this will be the only apps API that
will end up needing it.

Point being: we should make these fields match up with the tabs.Tab names. See
http://developer.chrome.com/extensions/tabs.html#type-Tab but in summary, just:

page_title -> title
selected_text -> selectionText (to match the context menus API - and yeah, use
camel case for things in these APIs)

Also it'd be good to add a comment like "Note: if adding properties to this then
make sure they're compatible with tabs.Tab" but I think that might get pulled in
as a documentation comment, so, never mind...

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher.idl:19: // |pageData| : The details
of the page the user was on when the button
other IDLs I've seen don't have a space between the | and the :. I only mention
this because I want to check that the IDL parser can figure out that this is the
documentation.

TBH I doubt you'd notice until you enable documentation for the API, but best to
avoid surprises then...

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
File chrome/common/extensions/api/page_launcher/page_launcher_handler.cc
(right):

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher/page_launcher_handler.cc:27:
string16* error) {
1 line

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
File chrome/common/extensions/api/page_launcher/page_launcher_handler.h (right):

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher/page_launcher_handler.h:15: 
docs

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher/page_launcher_handler.h:22: string16*
error) OVERRIDE;
1 line

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher/page_launcher_handler.h:26: }
  // namespace extensions

[Rune Fevang, 2013-02-01 22:12:54]

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/extensions...
File chrome/browser/extensions/api/page_launcher/page_launcher_api.cc (right):

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/extensions...
chrome/browser/extensions/api/page_launcher/page_launcher_api.cc:32: const
std::string& url,
On 2013/02/01 16:39:47, kalman wrote:
> GURL?

Done.

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.cc:58: for
(ExtensionSet::const_iterator iter = extensions->begin();
On 2013/02/01 16:39:47, kalman wrote:
> nit: this class uses "it" everywhere for iterator name, stick with that.

Done.

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.cc:98:
web_contents->GetURL().spec(),
On 2013/02/01 16:39:47, kalman wrote:
> send the url as a GURL?

Done.

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.cc:111: for
(ExtensionIdCommandMap::const_iterator iter =
On 2013/02/01 16:39:47, kalman wrote:
> iter -> it rename is arbitrary, please revert (also see above comment)

Done.

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.cc:116: }
On 2013/02/01 16:39:47, kalman wrote:
> single-line ifs omit {}

Done.

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.cc:149: for
(ExtensionIdCommandMap::iterator iter = extension_command_ids_.begin();
On 2013/02/01 16:39:47, kalman wrote:
> ditto

Done.

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.cc:155: }
On 2013/02/01 16:39:47, kalman wrote:
> single-line ifs omit {}

Done.

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/action_box_button_controller.h (right):

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.h:38: virtual void
ShowMenu(scoped_ptr<ActionBoxMenuModel> menu_model);
On 2013/02/01 16:39:47, kalman wrote:
> is there a c++-y reason for this change? AFAIK it's a pretty common idiom to
> implement blank virtual methods inline like this.

It allows ActionBoxMenuModel to be forward declared.

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/views/a...
File chrome/browser/ui/views/action_box_menu.cc (right):

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/views/a...
chrome/browser/ui/views/action_box_menu.cc:23: public IconImage::Observer {
On 2013/02/01 16:39:47, kalman wrote:
> 1 line

Done.

https://codereview.chromium.org/12095023/diff/13005/chrome/browser/ui/views/a...
chrome/browser/ui/views/action_box_menu.cc:42: }
On 2013/02/01 16:39:47, kalman wrote:
>   // namespace

Done.

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
File chrome/common/extensions/api/page_launcher.idl (right):

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher.idl:4: 
On 2013/02/01 16:39:47, kalman wrote:
> API-level description

I added some documentation. I can't find anything in any of the other .idl files
to use as a reference, so please let me know if you had something else in mind.

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher.idl:6: dictionary PageData {
On 2013/02/01 16:39:47, kalman wrote:
> Description of PageData

Done.

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher.idl:15: };
On 2013/02/01 16:39:47, kalman wrote:
> I know we're not using tabs.Tab here explicitly, but I think it would actually
> be a good thing to do at some point, or at least something like it.
> 
> Two things to note: there is a precedent for this (chrome.runtime) but it
won't
> actually work (bug that I hope to fix sometime) - and I think that fixing this
> bug will involve figuring out a standard way to expose tabs to apps (maybe via
> an abstraction like PageData) since I doubt this will be the only apps API
that
> will end up needing it.
> 
> Point being: we should make these fields match up with the tabs.Tab names. See
> http://developer.chrome.com/extensions/tabs.html#type-Tab but in summary,
just:
> 
> page_title -> title
> selected_text -> selectionText (to match the context menus API - and yeah, use
> camel case for things in these APIs)
> 
> Also it'd be good to add a comment like "Note: if adding properties to this
then
> make sure they're compatible with tabs.Tab" but I think that might get pulled
in
> as a documentation comment, so, never mind...

Done.

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher.idl:19: // |pageData| : The details
of the page the user was on when the button
On 2013/02/01 16:39:47, kalman wrote:
> other IDLs I've seen don't have a space between the | and the :. I only
mention
> this because I want to check that the IDL parser can figure out that this is
the
> documentation.
> 
> TBH I doubt you'd notice until you enable documentation for the API, but best
to
> avoid surprises then...

I just opened 6 random .idl files and only one of them omitted the space. I
don't mind changing it, but it seems like this is at least not abnormal.

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
File chrome/common/extensions/api/page_launcher/page_launcher_handler.cc
(right):

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher/page_launcher_handler.cc:27:
string16* error) {
On 2013/02/01 16:39:47, kalman wrote:
> 1 line

Done.

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
File chrome/common/extensions/api/page_launcher/page_launcher_handler.h (right):

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher/page_launcher_handler.h:15: 
On 2013/02/01 16:39:47, kalman wrote:
> docs

Done.

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher/page_launcher_handler.h:22: string16*
error) OVERRIDE;
On 2013/02/01 16:39:47, kalman wrote:
> 1 line

Done.

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher/page_launcher_handler.h:26: }
On 2013/02/01 16:39:47, kalman wrote:
>   // namespace extensions

Done.

[not at google - send to devlin, 2013-02-01 22:17:11]

lgtm

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
File chrome/common/extensions/api/page_launcher.idl (right):

https://codereview.chromium.org/12095023/diff/13005/chrome/common/extensions/...
chrome/common/extensions/api/page_launcher.idl:19: // |pageData| : The details
of the page the user was on when the button
On 2013/02/01 22:12:54, Rune Fevang wrote:
> On 2013/02/01 16:39:47, kalman wrote:
> > other IDLs I've seen don't have a space between the | and the :. I only
> mention
> > this because I want to check that the IDL parser can figure out that this is
> the
> > documentation.
> > 
> > TBH I doubt you'd notice until you enable documentation for the API, but
best
> to
> > avoid surprises then...
> 
> I just opened 6 random .idl files and only one of them omitted the space. I
> don't mind changing it, but it seems like this is at least not abnormal.

I guess I opened the wrong random IDL file.

[not at google - send to devlin, 2013-02-01 22:18:34]

btw I'd be nice to test, but I don't know how invokable the action box is. I'm
ok with adding a test in a follow up.

[Rune Fevang, 2013-02-07 00:20:25]

sky, could you do an OWNERS review of the /ui stuff? Thanks.

[sky, 2013-02-11 16:37:09]

LGTM

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/api/page_launcher/page_launcher_api.h (right):

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/api/page_launcher/page_launcher_api.h:28:
scoped_ptr<std::string> page_title,
Why does this take scoped_ptr<std::string> why not const std::string& ?

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/action_box_button_controller.h (right):

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.h:38: virtual void
ShowMenu(scoped_ptr<ActionBoxMenuModel> menu_model);
It's ok to inline virtual method that have no return value.

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/action_box_menu.cc (right):

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/ui/views/a...
chrome/browser/ui/views/action_box_menu.cc:34: private:
nit: newline between 33/34

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/ui/views/a...
chrome/browser/ui/views/action_box_menu.cc:40: };
DISALLOW...

[Rune Fevang, 2013-02-11 20:52:51]

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/api/page_launcher/page_launcher_api.h (right):

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/api/page_launcher/page_launcher_api.h:28:
scoped_ptr<std::string> page_title,
On 2013/02/11 16:37:09, sky wrote:
> Why does this take scoped_ptr<std::string> why not const std::string& ?

To be able to distinguish between no title and an empty title. I don't know if
that makes sense from a web perspective, but since the api as specced has the
title as an optional parameter I think it makes sense to maintain that in this
helper method.

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/ui/toolbar...
File chrome/browser/ui/toolbar/action_box_button_controller.h (right):

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/ui/toolbar...
chrome/browser/ui/toolbar/action_box_button_controller.h:38: virtual void
ShowMenu(scoped_ptr<ActionBoxMenuModel> menu_model);
On 2013/02/11 16:37:09, sky wrote:
> It's ok to inline virtual method that have no return value.

The reason I changed it is to allow forward declaration of ActionBoxMenuModel.

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/ui/views/a...
File chrome/browser/ui/views/action_box_menu.cc (right):

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/ui/views/a...
chrome/browser/ui/views/action_box_menu.cc:34: private:
On 2013/02/11 16:37:09, sky wrote:
> nit: newline between 33/34

Done.

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/ui/views/a...
chrome/browser/ui/views/action_box_menu.cc:40: };
On 2013/02/11 16:37:09, sky wrote:
> DISALLOW...

Done.

[sky, 2013-02-11 21:57:47]

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/api/page_launcher/page_launcher_api.h (right):

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/api/page_launcher/page_launcher_api.h:28:
scoped_ptr<std::string> page_title,
On 2013/02/11 20:52:52, Rune Fevang wrote:
> On 2013/02/11 16:37:09, sky wrote:
> > Why does this take scoped_ptr<std::string> why not const std::string& ?
> 
> To be able to distinguish between no title and an empty title. I don't know if
> that makes sense from a web perspective, but since the api as specced has the
> title as an optional parameter I think it makes sense to maintain that in this
> helper method.

I think const std::string* would be better than.

[Rune Fevang, 2013-02-11 23:36:46]

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/extensions...
File chrome/browser/extensions/api/page_launcher/page_launcher_api.h (right):

https://codereview.chromium.org/12095023/diff/25001/chrome/browser/extensions...
chrome/browser/extensions/api/page_launcher/page_launcher_api.h:28:
scoped_ptr<std::string> page_title,
On 2013/02/11 21:57:47, sky wrote:
> On 2013/02/11 20:52:52, Rune Fevang wrote:
> > On 2013/02/11 16:37:09, sky wrote:
> > > Why does this take scoped_ptr<std::string> why not const std::string& ?
> > 
> > To be able to distinguish between no title and an empty title. I don't know
if
> > that makes sense from a web perspective, but since the api as specced has
the
> > title as an optional parameter I think it makes sense to maintain that in
this
> > helper method.
> 
> I think const std::string* would be better than.

Done.

[commit-bot: I haz the power, 2013-02-12 00:04:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rfevang@chromium.org/12095023/37001

[commit-bot: I haz the power, 2013-02-12 03:46:14]

Change committed as 181837