Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(78)

Issue 661493004: Add infrastructure for Chrome Actions (Closed)

Created:
6 years, 2 months ago by Devlin
Modified:
6 years, 2 months ago
Reviewers:
Finnur, sky
CC:
chromium-reviews, tfarina, extensions-reviews_chromium.org, chromium-apps-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Project:
chromium
Visibility:
Public.

Description

Add infrastructure for Chrome Actions Chrome Actions are designed to be actions that live on the toolbar, like extension actions, but are actually components of Chrome. Examples of this can be ChromeCast, Translate, Password Manager, etc. This change partially implements the basic infrastructure for these actions. (The main goal of this patch is to provide a shell that the chromecast team can expand upon and use to prototype.) * Create a generic ToolbarActionViewController, which can be implemented by a specific chrome action or to handle the general extensions case. * (Mostly) abstract the concept of extensions out of the BrowserActionView, so it can be used for either extension actions or chrome actions. * Create a simple ChromeActionsRegistry. * Display chrome actions in the overflow menu of the browser actions container. Known limitations: * Chrome actions cannot be reordered, dragged, or shown in the main action toolbar (we need to consolidate chrome actions with the extension toolbar in the model somehow). BUG=422976 Committed: https://crrev.com/3ab2d3060f13924948f3072936b4aea5aacd12b4 Cr-Commit-Position: refs/heads/master@{#300500}

Patch Set 1 : #

Total comments: 26

Patch Set 2 : Rename Chrome Actions #

Patch Set 3 : Sky's I #

Total comments: 15

Patch Set 4 : Abstract away more extensions #

Total comments: 5

Patch Set 5 : #

Unified diffs Side-by-side diffs Delta from patch set Stats (+808 lines, -480 lines) Patch
M chrome/browser/extensions/browser_action_test_util.h View 1 2 3 1 chunk +0 lines, -3 lines 0 comments Download
M chrome/browser/extensions/extension_toolbar_model.h View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/extensions/extension_toolbar_model.cc View 1 2 3 4 chunks +13 lines, -11 lines 0 comments Download
M chrome/browser/extensions/extension_toolbar_model_unittest.cc View 1 2 3 8 chunks +9 lines, -9 lines 0 comments Download
M chrome/browser/ui/cocoa/extensions/browser_action_test_util_mac.mm View 1 2 3 1 chunk +0 lines, -5 lines 0 comments Download
M chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/toolbar/browser_actions_bar_browsertest.cc View 1 2 3 1 chunk +3 lines, -3 lines 0 comments Download
A chrome/browser/ui/toolbar/component_toolbar_actions_factory.h View 1 2 3 1 chunk +37 lines, -0 lines 0 comments Download
A chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc View 1 2 1 chunk +51 lines, -0 lines 0 comments Download
A chrome/browser/ui/toolbar/toolbar_action_view_controller.h View 1 2 3 1 chunk +89 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/extensions/extension_action_view_controller.h View 1 2 3 10 chunks +50 lines, -29 lines 0 comments Download
M chrome/browser/ui/views/extensions/extension_action_view_controller.cc View 1 2 3 10 chunks +139 lines, -39 lines 0 comments Download
D chrome/browser/ui/views/extensions/extension_action_view_delegate.h View 1 chunk +0 lines, -71 lines 0 comments Download
M chrome/browser/ui/views/location_bar/page_action_image_view.h View 1 2 3 4 chunks +4 lines, -4 lines 0 comments Download
M chrome/browser/ui/views/location_bar/page_action_image_view.cc View 1 2 3 7 chunks +10 lines, -11 lines 0 comments Download
M chrome/browser/ui/views/toolbar/browser_action_test_util_views.cc View 1 2 3 4 chunks +9 lines, -11 lines 0 comments Download
M chrome/browser/ui/views/toolbar/browser_action_view.h View 1 2 3 6 chunks +24 lines, -54 lines 0 comments Download
M chrome/browser/ui/views/toolbar/browser_action_view.cc View 1 2 3 4 10 chunks +44 lines, -105 lines 0 comments Download
M chrome/browser/ui/views/toolbar/browser_actions_container.h View 1 2 3 2 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/toolbar/browser_actions_container.cc View 1 2 3 4 14 chunks +89 lines, -35 lines 0 comments Download
M chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc View 1 2 3 9 chunks +38 lines, -39 lines 0 comments Download
M chrome/browser/ui/views/toolbar/chevron_menu_button.cc View 1 2 3 7 chunks +33 lines, -34 lines 0 comments Download
A chrome/browser/ui/views/toolbar/component_toolbar_actions_browsertest.cc View 1 2 3 1 chunk +142 lines, -0 lines 0 comments Download
A + chrome/browser/ui/views/toolbar/toolbar_action_view_delegate.h View 1 2 5 chunks +11 lines, -11 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 3 chunks +4 lines, -1 line 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 28 (7 generated)
Devlin
Scott, please take a look when you can. Sorry for the size - if you ...
6 years, 2 months ago (2014-10-15 20:44:29 UTC) #4
sky
I poked at this a bit and have some high level comments. https://codereview.chromium.org/661493004/diff/40001/chrome/browser/ui/views/toolbar/browser_action_view.h File chrome/browser/ui/views/toolbar/browser_action_view.h ...
6 years, 2 months ago (2014-10-15 23:52:32 UTC) #5
Devlin
https://codereview.chromium.org/661493004/diff/40001/chrome/browser/ui/views/toolbar/browser_action_view.h File chrome/browser/ui/views/toolbar/browser_action_view.h (right): https://codereview.chromium.org/661493004/diff/40001/chrome/browser/ui/views/toolbar/browser_action_view.h#newcode143 chrome/browser/ui/views/toolbar/browser_action_view.h:143: // ExtensionActionViewDelegate: On 2014/10/15 23:52:30, sky wrote: > ToolbarActionViewDelegate ...
6 years, 2 months ago (2014-10-16 16:46:39 UTC) #6
sky
https://codereview.chromium.org/661493004/diff/40001/chrome/browser/ui/views/toolbar/browser_actions_container.cc File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/661493004/diff/40001/chrome/browser/ui/views/toolbar/browser_actions_container.cc#newcode178 chrome/browser/ui/views/toolbar/browser_actions_container.cc:178: const extensions::ExtensionList& toolbar_items = model_->toolbar_items(); On 2014/10/16 16:46:38, Devlin ...
6 years, 2 months ago (2014-10-16 18:13:39 UTC) #7
Devlin
https://codereview.chromium.org/661493004/diff/40001/chrome/browser/ui/views/toolbar/browser_actions_container.cc File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right): https://codereview.chromium.org/661493004/diff/40001/chrome/browser/ui/views/toolbar/browser_actions_container.cc#newcode178 chrome/browser/ui/views/toolbar/browser_actions_container.cc:178: const extensions::ExtensionList& toolbar_items = model_->toolbar_items(); On 2014/10/16 18:13:39, sky ...
6 years, 2 months ago (2014-10-16 22:05:27 UTC) #9
Devlin
+Finnur
6 years, 2 months ago (2014-10-16 22:33:18 UTC) #11
sky
Here's some initial comments on the new interface. I need to ponder this more deeply. ...
6 years, 2 months ago (2014-10-16 23:03:34 UTC) #12
Devlin
https://codereview.chromium.org/661493004/diff/100001/chrome/browser/ui/toolbar/toolbar_action_view_controller.h File chrome/browser/ui/toolbar/toolbar_action_view_controller.h (right): https://codereview.chromium.org/661493004/diff/100001/chrome/browser/ui/toolbar/toolbar_action_view_controller.h#newcode36 chrome/browser/ui/toolbar/toolbar_action_view_controller.h:36: virtual Type GetType() const = 0; On 2014/10/16 23:03:34, ...
6 years, 2 months ago (2014-10-17 00:31:20 UTC) #13
Finnur
This looks like mostly just reorder/refactoring to facilitate new functionality to be added later, right? ...
6 years, 2 months ago (2014-10-17 09:57:02 UTC) #14
Finnur
Also: > This change partially the basic > infrastructure for these actions. Missing a verb ...
6 years, 2 months ago (2014-10-17 10:58:07 UTC) #15
Devlin
On 2014/10/17 09:57:02, Finnur wrote: > This looks like mostly just reorder/refactoring to facilitate new ...
6 years, 2 months ago (2014-10-17 16:33:33 UTC) #17
Finnur
LGTM
6 years, 2 months ago (2014-10-20 09:11:03 UTC) #18
sky
Ok, I like it. LGTM https://codereview.chromium.org/661493004/diff/140001/chrome/browser/ui/toolbar/toolbar_action_view_controller.h File chrome/browser/ui/toolbar/toolbar_action_view_controller.h (right): https://codereview.chromium.org/661493004/diff/140001/chrome/browser/ui/toolbar/toolbar_action_view_controller.h#newcode25 chrome/browser/ui/toolbar/toolbar_action_view_controller.h:25: class ToolbarActionViewController { Would ...
6 years, 2 months ago (2014-10-20 22:51:10 UTC) #19
Devlin
https://codereview.chromium.org/661493004/diff/140001/chrome/browser/ui/toolbar/toolbar_action_view_controller.h File chrome/browser/ui/toolbar/toolbar_action_view_controller.h (right): https://codereview.chromium.org/661493004/diff/140001/chrome/browser/ui/toolbar/toolbar_action_view_controller.h#newcode25 chrome/browser/ui/toolbar/toolbar_action_view_controller.h:25: class ToolbarActionViewController { On 2014/10/20 22:51:09, sky wrote: > ...
6 years, 2 months ago (2014-10-21 00:02:33 UTC) #20
Finnur
https://codereview.chromium.org/661493004/diff/140001/chrome/browser/ui/toolbar/toolbar_action_view_controller.h File chrome/browser/ui/toolbar/toolbar_action_view_controller.h (right): https://codereview.chromium.org/661493004/diff/140001/chrome/browser/ui/toolbar/toolbar_action_view_controller.h#newcode25 chrome/browser/ui/toolbar/toolbar_action_view_controller.h:25: class ToolbarActionViewController { Yeah, I'm with Devlin here. We ...
6 years, 2 months ago (2014-10-21 09:34:12 UTC) #21
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/661493004/160001
6 years, 2 months ago (2014-10-21 15:23:39 UTC) #23
commit-bot: I haz the power
Committed patchset #5 (id:160001)
6 years, 2 months ago (2014-10-21 16:47:21 UTC) #24
commit-bot: I haz the power
Patchset 5 (id:??) landed as https://crrev.com/3ab2d3060f13924948f3072936b4aea5aacd12b4 Cr-Commit-Position: refs/heads/master@{#300500}
6 years, 2 months ago (2014-10-21 16:47:53 UTC) #25
sky
I like ToolbarAction too. Can we rename existing classes to match then? For example, should ...
6 years, 2 months ago (2014-10-21 18:09:43 UTC) #26
Devlin
On 2014/10/21 18:09:43, sky wrote: > I like ToolbarAction too. Can we rename existing classes ...
6 years, 2 months ago (2014-10-23 17:10:24 UTC) #27
sky
6 years, 2 months ago (2014-10-23 17:27:31 UTC) #28
Message was sent while issue was closed.
TX!

On Thu, Oct 23, 2014 at 10:10 AM,  <rdevlin.cronin@chromium.org> wrote:
> On 2014/10/21 18:09:43, sky wrote:
>>
>> I like ToolbarAction too. Can we rename existing classes to match
>> then? For example, should BrowserActionView become ToolbarActionView?
>
>
> Sorry for the delay (sheriffing the last two days).  Yes, we can. :)
> crbug.com/426448
>
> https://codereview.chromium.org/661493004/

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Powered by Google App Engine
This is Rietveld 408576698