Include individual share intents in the action box and allow direct triggering without an intermedi…

Skips the picker step for the action box by including individual share intent menu items and a webstore link. Removes the command this deprecates from browser_commands.


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

[sky, 2012-10-11 23:36:25]

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:36:
kCWSFindShareIntentsCommandId = 0xE000,
Enums use ALL_CAPS style.

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:63:
weak_ptr_factory_.InvalidateWeakPtrs();
Isn't this done for you by the destructor of WeakPtrFactory?

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:112: // And show the
menu.
How do you know how long it takes to get the services? Might the user have
clicked else where?

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:205: void
ActionBoxButtonController::NavigateToWebStoreShareIntentsList() {
make order match header.

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:206: Profile* profile
= Profile::FromBrowserContext(
browser_->profile()

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:208: if (!profile)
Remove this.

[skare_, 2012-10-12 01:02:06]

Have a question on how to resolve one, and had a style question on the first
item.

Thanks!

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:36:
kCWSFindShareIntentsCommandId = 0xE000,
On 2012/10/11 23:36:25, sky wrote:
> Enums use ALL_CAPS style.

Done.
Switched to IDC_* names for command ids even though they're local. I am more
familiar with another google client project; please let me know if that prefix
is reserved for higher-level command identifiers.

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:63:
weak_ptr_factory_.InvalidateWeakPtrs();
On 2012/10/11 23:36:25, sky wrote:
> Isn't this done for you by the destructor of WeakPtrFactory?

Done.

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:112: // And show the
menu.
On 2012/10/11 23:36:25, sky wrote:
> How do you know how long it takes to get the services? Might the user have
> clicked else where?

is the pattern to watch for closed and then cancel the action?

Alternatively we could enumerate extensions on controller startup (since it's
longer-lived than the menu) and watch for new ones.

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:205: void
ActionBoxButtonController::NavigateToWebStoreShareIntentsList() {
On 2012/10/11 23:36:25, sky wrote:
> make order match header.

Done.

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:206: Profile* profile
= Profile::FromBrowserContext(
On 2012/10/11 23:36:25, sky wrote:
> browser_->profile()

done and inlined below, thanks.

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:208: if (!profile)
On 2012/10/11 23:36:25, sky wrote:
> Remove this.

Done.

[sky, 2012-10-12 14:33:25]

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:36:
kCWSFindShareIntentsCommandId = 0xE000,
On 2012/10/12 01:02:06, Travis Skare wrote:
> On 2012/10/11 23:36:25, sky wrote:
> > Enums use ALL_CAPS style.
> 
> Done.
> Switched to IDC_* names for command ids even though they're local. I am more
> familiar with another google client project; please let me know if that prefix
> is reserved for higher-level command identifiers.

Don't do that, it's easy to misread when using IDC_.

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:112: // And show the
menu.
On 2012/10/12 01:02:06, Travis Skare wrote:
> On 2012/10/11 23:36:25, sky wrote:
> > How do you know how long it takes to get the services? Might the user have
> > clicked else where?
> 
> is the pattern to watch for closed and then cancel the action?

I don't know that there is anything you could look for.

> Alternatively we could enumerate extensions on controller startup (since it's
> longer-lived than the menu) and watch for new ones.

That would make the menu instant, which I like. Are there any down sides? Would
it slow down startup at all?

[Greg Billock, 2012-10-12 18:41:50]

http://codereview.chromium.org/11073009/diff/17001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

http://codereview.chromium.org/11073009/diff/17001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:213:
static_cast<content::WebContentsDelegate*>(browser_)->
The new API for you is now checked in. You can call

TabContents::web_intents_picker_controller()->SetDispatcher(dispatcher.release());

pickerController->InvokeServiceWithSelection(menu_model.selected_service())

I think you have a tab contents available from your update from the location
bar, and the picker controller is available from there.

[skare_, 2012-10-12 20:21:07]

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:36:
kCWSFindShareIntentsCommandId = 0xE000,
On 2012/10/12 14:33:25, sky wrote:
> On 2012/10/12 01:02:06, Travis Skare wrote:
> > On 2012/10/11 23:36:25, sky wrote:
> > > Enums use ALL_CAPS style.
> > 
> > Done.
> > Switched to IDC_* names for command ids even though they're local. I am more
> > familiar with another google client project; please let me know if that
prefix
> > is reserved for higher-level command identifiers.
> 
> Don't do that, it's easy to misread when using IDC_.

Done.

http://codereview.chromium.org/11073009/diff/9007/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/action_box_button_controller.cc:112: // And show the
menu.
On 2012/10/12 14:33:25, sky wrote:
> On 2012/10/12 01:02:06, Travis Skare wrote:
> > On 2012/10/11 23:36:25, sky wrote:
> > > How do you know how long it takes to get the services? Might the user have
> > > clicked else where?
> > 
> > is the pattern to watch for closed and then cancel the action?
> 
> I don't know that there is anything you could look for.
> 
> > Alternatively we could enumerate extensions on controller startup (since
it's
> > longer-lived than the menu) and watch for new ones.
> 
> That would make the menu instant, which I like. Are there any down sides?
Would
> it slow down startup at all?

Resolved -- in the latest revision the population is synchronous again (intents
provides as synchronous service check if you lookup per-extension). The menu
shows up immediately again.

In case that doesn't work, the asynch population would be fast. My concern there
is that it places some load, however small, on all users during startup and
whenever extensions are loaded or unloaded. The synchronous approach will only
query the intent registry if users open the action box.

[skare_, 2012-10-12 23:42:51]

[this comment affects only the new intent API sidethread comment]

http://codereview.chromium.org/11073009/diff/17001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

http://codereview.chromium.org/11073009/diff/17001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:213:
static_cast<content::WebContentsDelegate*>(browser_)->
On 2012/10/12 18:41:50, Greg Billock wrote:
> The new API for you is now checked in. You can call
> 
>
TabContents::web_intents_picker_controller()->SetDispatcher(dispatcher.release());
> 
> pickerController->InvokeServiceWithSelection(menu_model.selected_service())
> 
> I think you have a tab contents available from your update from the location
> bar, and the picker controller is available from there.

Thanks! I made this change in a local git branch and uploaded to 
http://codereview.chromium.org/11150005/
to minimize churn in this cl.

should there be user-visible differences with the new API?
I see the same behavior as the existing approach as the intent picker launches
and loads an inline intent.

[Greg Billock, 2012-10-12 23:45:13]

On 2012/10/12 23:42:51, Travis Skare wrote:
> [this comment affects only the new intent API sidethread comment]
> 
>
http://codereview.chromium.org/11073009/diff/17001/chrome/browser/ui/toolbar/...
> File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):
> 
>
http://codereview.chromium.org/11073009/diff/17001/chrome/browser/ui/toolbar/...
> chrome/browser/ui/toolbar/action_box_button_controller.cc:213:
> static_cast<content::WebContentsDelegate*>(browser_)->
> On 2012/10/12 18:41:50, Greg Billock wrote:
> > The new API for you is now checked in. You can call
> > 
> >
>
TabContents::web_intents_picker_controller()->SetDispatcher(dispatcher.release());
> > 
> > pickerController->InvokeServiceWithSelection(menu_model.selected_service())
> > 
> > I think you have a tab contents available from your update from the location
> > bar, and the picker controller is available from there.
> 
> Thanks! I made this change in a local git branch and uploaded to 
> http://codereview.chromium.org/11150005/
> to minimize churn in this cl.
> 
> should there be user-visible differences with the new API?
> I see the same behavior as the existing approach as the intent picker launches
> and loads an inline intent.

Yeah, I was hoping to get the inline-only patch in a couple of hours ago. The
picker dialog is reluctant to cooperate, though, so I'm having to do more
surgery on it than I expected. I'll get it in soon though.

[tfarina, 2012-10-13 19:43:37]

http://codereview.chromium.org/11073009/diff/26001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

http://codereview.chromium.org/11073009/diff/26001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:226:
"http://webintents.org/share",
nit: looks like you already have constants for these two strings
(kShareIntentAction, kShareIntentMimeType).

[skare_, 2012-10-15 02:41:44]

http://codereview.chromium.org/11073009/diff/26001/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

http://codereview.chromium.org/11073009/diff/26001/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:226:
"http://webintents.org/share",
On 2012/10/13 19:43:37, tfarina wrote:
> nit: looks like you already have constants for these two strings
> (kShareIntentAction, kShareIntentMimeType).

done, thanks!

[skare_, 2012-10-15 17:58:45]

+pkasting -- Hi Peter, can you take a look for /browser/ui if you have time?
sky@ is OOO today (I'll address any comments there too)

[Peter Kasting, 2012-10-15 19:33:09]

LGTM

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:75:
next_share_intent_command_id_ = SHARE_COMMAND_FIRST;
Nit: Why does this need to be a member?  It seems like it can just be a temp in
this function.

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:79:
WebIntentsRegistryFactory::GetForProfile(browser_->profile());
Nit: Indent 4

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:89: if
(services.size() > 0) {
Nit: !empty()

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:91: if (command_id >
SHARE_COMMAND_LAST) {
Nit: {} unnecessary

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:138:
TriggerExplicitShareIntent(share_intent_service_ids_[command_id]);
Nit: I probably wouldn't call these out to their own functions since they're
simple and only called once, but whatever

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:218:
scoped_ptr<content::WebIntentsDispatcher> dispatcher(
Nit: Given that we don't use exceptions, is there a benefit to putting this in a
scoped_ptr?  Seems like just inlining into the next statement would be fine.

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:225: GURL query_url =
extension_urls::GetWebstoreIntentQueryURL(
Nit: Above you used a const GURL&, here a GURL.  Why the inconsistency?

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.h (right):

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.h:82: // The command ID
to assign to the next extension that needs one.
Nit: Comment is wrong

[skare_, 2012-10-15 21:07:26]

+Nico for /chrome/app OWNERS: this removes an obsoleted command id and changes a
string in our menu.

Thanks Peter -- addressed nits aside from inlining a couple functions into
ExecuteCommand, which I can do if that's more consistent with the rest of UI
code.

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:75:
next_share_intent_command_id_ = SHARE_COMMAND_FIRST;
On 2012/10/15 19:33:09, Peter Kasting wrote:
> Nit: Why does this need to be a member?  It seems like it can just be a temp
in
> this function.

Done.

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:79:
WebIntentsRegistryFactory::GetForProfile(browser_->profile());
On 2012/10/15 19:33:09, Peter Kasting wrote:
> Nit: Indent 4

Done.

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:89: if
(services.size() > 0) {
On 2012/10/15 19:33:09, Peter Kasting wrote:
> Nit: !empty()

Done.

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:91: if (command_id >
SHARE_COMMAND_LAST) {
On 2012/10/15 19:33:09, Peter Kasting wrote:
> Nit: {} unnecessary

Done.

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:138:
TriggerExplicitShareIntent(share_intent_service_ids_[command_id]);
Just to keep this function from growing too long; I can inline them if desired
though.

On 2012/10/15 19:33:09, Peter Kasting wrote:
> Nit: I probably wouldn't call these out to their own functions since they're
> simple and only called once, but whatever

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:218:
scoped_ptr<content::WebIntentsDispatcher> dispatcher(
On 2012/10/15 19:33:09, Peter Kasting wrote:
> Nit: Given that we don't use exceptions, is there a benefit to putting this in
a
> scoped_ptr?  Seems like just inlining into the next statement would be fine.

Done.

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:225: GURL query_url =
extension_urls::GetWebstoreIntentQueryURL(
On 2012/10/15 19:33:09, Peter Kasting wrote:
> Nit: Above you used a const GURL&, here a GURL.  Why the inconsistency?
made const&

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.h (right):

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.h:82: // The command ID
to assign to the next extension that needs one.
removed member
On 2012/10/15 19:33:09, Peter Kasting wrote:
> Nit: Comment is wrong

[Peter Kasting, 2012-10-15 21:54:39]

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
File chrome/browser/ui/toolbar/action_box_button_controller.cc (right):

http://codereview.chromium.org/11073009/diff/27010/chrome/browser/ui/toolbar/...
chrome/browser/ui/toolbar/action_box_button_controller.cc:138:
TriggerExplicitShareIntent(share_intent_service_ids_[command_id]);
On 2012/10/15 21:07:27, Travis Skare wrote:
> Just to keep this function from growing too long; I can inline them if desired
> though.

It doesn't really matter much.  There are pros and cons to both, do whichever
you think is overall most readable.

[Nico, 2012-10-15 21:59:17]

chrome/app lgtm

Please have tracking bugs for features and include BUG= lines that mention these
bugs.

[commit-bot: I haz the power, 2012-10-15 22:19:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skare@chromium.org/11073009/29002

[commit-bot: I haz the power, 2012-10-16 01:40:16]

Change committed as 162032