Support Component Actions in the toolbar.

Support Component Actions in the toolbar.

This change supports component actions in the toolbar, which previously only supported extension actions. ExtensionToolbarModel now keep track of extension ids (strings) rather than Extensions.

This change also fixes the blank placement for the Media Router Action icon in new windows.

BUG=511292, 511427, 511302
Committed: https://crrev.com/f9cfc4d02e387612c90385a5998af884975942a5
Cr-Commit-Position: refs/heads/master@{#343808}

[apacible, 2015-07-22 00:12:14]

Patchset #1 (id:1) has been deleted

[apacible, 2015-07-22 00:34:43]

Patchset #1 (id:20001) has been deleted

[apacible, 2015-07-22 00:48:07]

Patchset #1 (id:40001) has been deleted

[apacible, 2015-07-22 00:48:15]

Patchset #1 (id:60001) has been deleted

[apacible, 2015-07-22 02:02:55]

Patchset #1 (id:80001) has been deleted

[apacible, 2015-07-22 15:42:50]

Patchset #1 (id:100001) has been deleted

[apacible, 2015-07-22 22:34:28]

Patchset #1 (id:120001) has been deleted

[apacible, 2015-07-23 22:08:21]

Patchset #2 (id:160001) has been deleted

[apacible, 2015-07-23 23:55:18]

Patchset #1 (id:140001) has been deleted

[apacible, 2015-07-24 00:54:16]

Patchset #1 (id:180001) has been deleted

[apacible, 2015-07-24 02:08:04]

Patchset #1 (id:200001) has been deleted

[apacible, 2015-07-24 02:08:08]

Patchset #1 (id:220001) has been deleted

[apacible, 2015-07-24 02:35:10]

Patchset #1 (id:240001) has been deleted

[apacible, 2015-07-24 02:52:28]

Patchset #1 (id:260001) has been deleted

[apacible, 2015-07-24 02:52:32]

Patchset #1 (id:280001) has been deleted

[apacible, 2015-07-24 06:23:10]

Patchset #1 (id:290001) has been deleted

[apacible, 2015-07-24 17:12:46]

Patchset #1 (id:310001) has been deleted

[apacible, 2015-07-24 22:07:04]

Patchset #1 (id:330001) has been deleted

[apacible, 2015-07-24 22:22:33]

Patchset #1 (id:350001) has been deleted

[apacible, 2015-07-24 22:25:14]

Patchset #1 (id:360001) has been deleted

[apacible, 2015-07-24 23:12:54]

Patchset #1 (id:380001) has been deleted

[apacible, 2015-07-27 03:36:52]

Patchset #2 (id:420001) has been deleted

[apacible, 2015-07-27 03:36:56]

Patchset #1 (id:400001) has been deleted

[apacible, 2015-07-27 06:09:16]

Patchset #1 (id:440001) has been deleted

[apacible, 2015-07-27 07:14:11]

Patchset #1 (id:460001) has been deleted

[apacible, 2015-07-27 07:51:36]

Patchset #1 (id:480001) has been deleted

[apacible, 2015-07-27 07:51:47]

Patchset #1 (id:480002) has been deleted

[apacible, 2015-07-27 08:07:47]

Patchset #1 (id:510001) has been deleted

[apacible, 2015-07-27 16:03:18]

Patchset #1 (id:530001) has been deleted

[apacible, 2015-07-27 16:21:16]

Patchset #1 (id:550001) has been deleted

[apacible, 2015-07-27 16:21:21]

Patchset #1 (id:570001) has been deleted

[apacible, 2015-07-27 16:21:36]

apacible@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[apacible, 2015-07-27 16:35:00]

PTAL, thanks!

Shows component action (MR Action) between some extensions and a component
extension:
LTR: https://screenshot.googleplex.com/H97GzfCgo5T.png
RTL: https://screenshot.googleplex.com/fT44XKpkqRz.png

For more context on 511302, opening a new window on Linux also leads to the
"ghost" icon, which looks like this:
https://screenshot.googleplex.com/6CtDc6T0Fs2.png

[Devlin, 2015-07-27 17:11:58]

The big thing this patch should be changing is that ExtensionToolbarModel
shouldn't really be an ExtensionToolbarModel anymore - it should be a
ToolbarActionsModel.  A lot of that will consist of simply renaming "Extension"
to "Action", but there are quite a few other places where what you have won't
really work.  For instance, it looks like you aren't really populating a single
array of actions in the model (i.e., toolbar_items_ only has extensions, and
component_ids_ has the component actions), but are allowing component actions
and extensions to both be moved, which will be in the toolbar_items_ array.  I
know it'll be a big change, but we need to loosen the dependency of the toolbar
model on extensions only.  Also, with a change like this, we'll definitely want
some tests added.

Feel free to ping me with any questions. :)

[apacible, 2015-07-27 18:50:24]

Patchset #2 (id:610001) has been deleted

[apacible, 2015-07-27 22:32:50]

Patchset #2 (id:630001) has been deleted

[apacible, 2015-07-28 00:55:14]

Patchset #2 (id:650001) has been deleted

[apacible, 2015-07-28 03:13:02]

Patchset #2 (id:660001) has been deleted

[apacible, 2015-07-28 04:35:55]

Patchset #2 (id:680001) has been deleted

[apacible, 2015-07-28 16:32:37]

Patchset #2 (id:700001) has been deleted

[apacible, 2015-07-28 17:59:16]

Patchset #2 (id:720001) has been deleted

[apacible, 2015-07-28 21:54:51]

Patchset #2 (id:740001) has been deleted

[apacible, 2015-07-28 22:05:13]

Thanks for the feedback! Second patch set is a bigger than the first one... :)

Changes include:
- Renamed ExtensionToolbarModel to ToolbarActionsModel and moved it to
c/b/ui/toolbar.
- Switched all ToolbarActionsModel public API to use action ids instead of
Extensions. I left some private functions to use Extensions since they'd be
called after initialization, which is the only time we get any component
actions.
- Keep track of both extension action ids and component action ids in the same
place; use the same behavior for placement for both.
- Moved ToolbarAction creation logic out of ToolbarActionsBar to
ToolbarActionsModel.
- Pulled out MockComponentToolbarActionsFactory to be used in multiple test
files.
- Updated tests to use component actions as well.
- Lots of renaming.

PTAL, thank you!

[apacible, 2015-07-28 23:24:35]

apacible@chromium.org changed reviewers:
+ mfoltz@chromium.org

[apacible, 2015-07-28 23:24:35]

mfoltz: PTAL, thanks!

[mark a. foltz, 2015-07-29 17:56:24]

This looks good from a style point of view.

However I had a more basic question about the design of the UI data model.  It
seems like we want the toolbar to support both actions provided by components
and by extensions.  The OO way to model this is to have a base class
ToolbarAction with subclasses ExtensionAction and ComponentAction.

Then the controller/container could keep pointers to ToolbarAction* instead of
lists of ids, which is both fragile and less performant (requiring linear search
of the list and Extension* lookup from the id).

Does this sound like a better alternative?

[Devlin, 2015-07-29 18:04:04]

Quick first pass.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:51: include_all_actions_(
nit: this should probably stay "include_all_extensions_"

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:173: ToolbarActionsBar* bar)
{
Sad that this takes a ToolbarActionsBar... but it stems from the fact that it's
sad the ExtensionActionViewController does the same.

No action required; just whining. ;)

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:178:
ScopedVector<ToolbarActionViewController> component_actions =
Technically, isn't this an ActionList?

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:188: for (std::string
action_id : toolbar_items()) {
this results in a copy.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:189: // Check if current
action_id is for an extension.
nit: indentation.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:196: action_id);
DCHECK extension, for kicks and grins?

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:205:
DCHECK(component_actions.empty() ||
not sure I understand the first clause.  When would we have an id that *isn't*
an extension, but not have component actions?

That is, I think the DCHECK should just be that the switch is on.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:323: const
extensions::Extension* extension) {
Shouldn't this take an id?  Can't this be used for any action?

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:28: }  // namespace extensions
nitty nit: probably don't need the // namespace extensions (we don't really use
it for forward declarations, or when the content is only a couple lines.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:32: typedef std::string
ActionId;
two thoughts:
- I'm not really keen on typedefing like this.  Sometimes, I prefer to know what
it is.  Otherwise, it raises questions about what methods it has, how
comparators/copying/etc works, and so on.  I know a lot about std::string, but I
don't know anything about an "ActionId".  In other words - I'd just stick with
std::string.
- FYI, prefer using ActionId = std::string in new code.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:34: typedef
ScopedVector<ToolbarActionViewController> ActionList;
(This typedef's fine, because it makes it more readable).
using ActionList = ScopedVector<...>;

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:59: // Signals that an action
with id |id| has been added to the toolbar at
... with |id| has been...

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:62: virtual void
OnToolbarActionAdded(const ActionId id, int index) = 0;
const &, not just const. (Everywhere)

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:172: HighlightType type);
nit: indentation

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:202: void
OnExtensionActionVisibilityChanged(const ActionId& extension_id,
nit: this relates to extensions, so this shouldn't have an "ActionId".

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:217: // Updates action with id
|id|'s browser action visibility pref if the
nit: with |id|'s...

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right):

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1: // Copyright 2014
The Chromium Authors. All rights reserved.
Shoot.  When you upload a new patchset, can you use a smaller similarity to see
if Rietveld catches this as a moved file?

[Devlin, 2015-07-29 18:10:22]

On 2015/07/29 17:56:24, mark a. foltz wrote:
> This looks good from a style point of view.
> 
> However I had a more basic question about the design of the UI data model.  It
> seems like we want the toolbar to support both actions provided by components
> and by extensions.  The OO way to model this is to have a base class
> ToolbarAction with subclasses ExtensionAction and ComponentAction.
> 
> Then the controller/container could keep pointers to ToolbarAction* instead of
> lists of ids, which is both fragile and less performant (requiring linear
search
> of the list and Extension* lookup from the id).
> 
> Does this sound like a better alternative?

I've thought about that a few times, and I've always come to the conclusion that
it just doesn't make quite enough sense to justify the churn.  A few notes:
- The bottom line is that somewhere along this path, something has to know about
both component actions and extensions.  I think the model is a good place to
have that.  If we have the model store just ToolbarActions, we just then have to
map extensions to ToolbarActions, which is going to have to do the same thing
this is doing, but relying on ToolbarActions introduces more moving parts/extra
steps.
- That abstraction pretty much happens when we step to the ViewController level
- see ToolbarActionViewController, ExtensionActionViewController, and the MR
implementation, MediaRouterAction.  Introducing the abstraction earlier really
doesn't help much (at what point do you stop?).
- There isn't really much that a generic ToolbarAction would have, other than
just "GetId", and I'm really not a fan of having a super class that doesn't
really have an interface or do anything (though sometimes it's necessary).

I'm happy to talk about it more (feel free to chat or schedule a VC), but my gut
is that introducing another abstraction will just make an already complicated
structure more complicated, rather than less.

[mark a. foltz, 2015-07-29 20:22:54]

On 2015/07/29 at 18:10:22, rdevlin.cronin wrote:
> On 2015/07/29 17:56:24, mark a. foltz wrote:
> > This looks good from a style point of view.
> > 
> > However I had a more basic question about the design of the UI data model. 
It
> > seems like we want the toolbar to support both actions provided by
components
> > and by extensions.  The OO way to model this is to have a base class
> > ToolbarAction with subclasses ExtensionAction and ComponentAction.
> > 
> > Then the controller/container could keep pointers to ToolbarAction* instead
of
> > lists of ids, which is both fragile and less performant (requiring linear
search
> > of the list and Extension* lookup from the id).
> > 
> > Does this sound like a better alternative?
> 
> I've thought about that a few times, and I've always come to the conclusion
that it just doesn't make quite enough sense to justify the churn.  A few notes:
> - The bottom line is that somewhere along this path, something has to know
about both component actions and extensions.  I think the model is a good place
to have that.  If we have the model store just ToolbarActions, we just then have
to map extensions to ToolbarActions, which is going to have to do the same thing
this is doing, but relying on ToolbarActions introduces more moving parts/extra
steps.
> - That abstraction pretty much happens when we step to the ViewController
level - see ToolbarActionViewController, ExtensionActionViewController, and the
MR implementation, MediaRouterAction.  Introducing the abstraction earlier
really doesn't help much (at what point do you stop?).
> - There isn't really much that a generic ToolbarAction would have, other than
just "GetId", and I'm really not a fan of having a super class that doesn't
really have an interface or do anything (though sometimes it's necessary).
> 
> I'm happy to talk about it more (feel free to chat or schedule a VC), but my
gut is that introducing another abstraction will just make an already
complicated structure more complicated, rather than less.

I was reacting to some of the code in toolbar_actions.bar.cc like:

  // Check that |action_id| is not a component action since
  // OnToolbarActionAdded is only called after the toolbar model has been
  // initialized.
  if (std::find(component_ids_.begin(), component_ids_.end(), action_id) !=
      component_ids_.end())
    return;
  const extensions::Extension* extension =
      extensions::ExtensionRegistry::Get(browser_->profile())->
          enabled_extensions().GetByID(action_id);

It seems like if we were passing an object instead of an ID, it would be trivial
to find out whether it was associated with an extension or a component, instead
of having to search a list of ids and make sure the list of ids was set up
correctly to begin with.

Perhaps this logic could be refactored into the model to support this down the
road.

[apacible, 2015-07-30 22:52:47]

Patchset #6 (id:840001) has been deleted

[apacible, 2015-07-30 22:53:22]

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:51: include_all_actions_(
On 2015/07/29 18:04:04, Devlin wrote:
> nit: this should probably stay "include_all_extensions_"

Done.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:173: ToolbarActionsBar* bar)
{
On 2015/07/29 18:04:04, Devlin wrote:
> Sad that this takes a ToolbarActionsBar... but it stems from the fact that
it's
> sad the ExtensionActionViewController does the same.
> 
> No action required; just whining. ;)

Acknowledged.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:178:
ScopedVector<ToolbarActionViewController> component_actions =
On 2015/07/29 18:04:04, Devlin wrote:
> Technically, isn't this an ActionList?

Right. Updated.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:188: for (std::string
action_id : toolbar_items()) {
On 2015/07/29 18:04:04, Devlin wrote:
> this results in a copy.

Done.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:189: // Check if current
action_id is for an extension.
On 2015/07/29 18:04:04, Devlin wrote:
> nit: indentation.

Done.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:196: action_id);
On 2015/07/29 18:04:04, Devlin wrote:
> DCHECK extension, for kicks and grins?

Done.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:205:
DCHECK(component_actions.empty() ||
On 2015/07/29 18:04:04, Devlin wrote:
> not sure I understand the first clause.  When would we have an id that *isn't*
> an extension, but not have component actions?
> 
> That is, I think the DCHECK should just be that the switch is on.

We *shouldn't*. This was overly checking. Updated.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:323: const
extensions::Extension* extension) {
On 2015/07/29 18:04:04, Devlin wrote:
> Shouldn't this take an id?  Can't this be used for any action?

It's only used for AddExtension(Extension...), so I didn't change this. Should I
update this to take an id?

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:28: }  // namespace extensions
On 2015/07/29 18:04:04, Devlin wrote:
> nitty nit: probably don't need the // namespace extensions (we don't really
use
> it for forward declarations, or when the content is only a couple lines.

Done.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:32: typedef std::string
ActionId;
On 2015/07/29 18:04:04, Devlin wrote:
> two thoughts:
> - I'm not really keen on typedefing like this.  Sometimes, I prefer to know
what
> it is.  Otherwise, it raises questions about what methods it has, how
> comparators/copying/etc works, and so on.  I know a lot about std::string, but
I
> don't know anything about an "ActionId".  In other words - I'd just stick with
> std::string.
> - FYI, prefer using ActionId = std::string in new code.

I see, thanks for bringing this up! Switched to std::string here and elsewhere.
I updated the next two typedefs with using.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:34: typedef
ScopedVector<ToolbarActionViewController> ActionList;
On 2015/07/29 18:04:04, Devlin wrote:
> (This typedef's fine, because it makes it more readable).
> using ActionList = ScopedVector<...>;

Done.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:59: // Signals that an action
with id |id| has been added to the toolbar at
On 2015/07/29 18:04:04, Devlin wrote:
> ... with |id| has been...

Done.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:62: virtual void
OnToolbarActionAdded(const ActionId id, int index) = 0;
On 2015/07/29 18:04:04, Devlin wrote:
> const &, not just const. (Everywhere)

Done.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:172: HighlightType type);
On 2015/07/29 18:04:04, Devlin wrote:
> nit: indentation

Done.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:202: void
OnExtensionActionVisibilityChanged(const ActionId& extension_id,
On 2015/07/29 18:04:04, Devlin wrote:
> nit: this relates to extensions, so this shouldn't have an "ActionId".

Done.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:217: // Updates action with id
|id|'s browser action visibility pref if the
On 2015/07/29 18:04:04, Devlin wrote:
> nit: with |id|'s...

Done.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right):

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1: // Copyright 2014
The Chromium Authors. All rights reserved.
On 2015/07/29 18:04:04, Devlin wrote:
> Shoot.  When you upload a new patchset, can you use a smaller similarity to
see
> if Rietveld catches this as a moved file?

Using similarity=35 catches the proper moved files; updated in Patch Set 5. I'll
use that in the future for this CL!

[Devlin, 2015-08-01 01:36:59]

Sorry, didn't quite make it to this one today. I'll be sure to get it on Monday.

[apacible, 2015-08-01 02:06:15]

On 2015/08/01 01:36:59, Devlin wrote:
> Sorry, didn't quite make it to this one today. I'll be sure to get it on
Monday.

Thank you!

[Devlin, 2015-08-03 20:58:31]

Okay, next round :)

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:323: const
extensions::Extension* extension) {
On 2015/07/30 22:53:21, apacible wrote:
> On 2015/07/29 18:04:04, Devlin wrote:
> > Shouldn't this take an id?  Can't this be used for any action?
> 
> It's only used for AddExtension(Extension...), so I didn't change this. Should
I
> update this to take an id?

Yes, please.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:126:
component_ids_(ComponentToolbarActionsFactory::GetComponentIds()),
I don't think the ToolbarActionsBar should need knowledge of component ids.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:166: extension->id()) !=
toolbar_items_.end()) {
It looks like we don't actually need the extension here, right?  Just the
extension id.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:186: // Extension and
component actions are suppressed if the extension
This comment needs to be updated.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:189: // for (std::string
action_id : toolbar_items()) {
?

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:192: if
(std::find(component_ids_.begin(), component_ids_.end(),
Seeing all of these checks makes me think we should have a struct of
action_type, action_id, and store those.  This will help get rid of most of the
code that Mark was (rightly) worried about, while not requiring the upheaval to
make an abstract class.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:315: for (const auto&
extension_id : toolbar_items_)
now that this isn't an ugly scoped_refptr<const extensions::Extension>, I think
I'd prefer std::string over auto.  Also, we can call it "id" or "action_id"
instead of extension_id.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:362: // TODO(devlin):
Extension actions which are not visible should be moved to
This TODO can be deleted.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:545: it !=
component_ids_.end(); ++it) {
nit: indentation.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:596: toolbar_items_.size());
These UMA should be updated, so that BrowserActionsCount and
BrowserActionsVisible only record extension counts.  It would probably be useful
to also have an overall count, and/or component action count.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:845:
extensions::ExtensionRegistry::Get(profile_)->
We should cache the extension registry as a member variable.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:58: virtual void
OnToolbarActionAdded(const std::string& id, int index) = 0;
I think this should pass a scoped_ptr<ToolbarActionViewController> now.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right):

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:298: : "";
nitty nit: in this code, we typically prefer std::string() over "".

[apacible, 2015-08-04 15:59:37]

Patchset #7 (id:880001) has been deleted

[apacible, 2015-08-04 16:13:30]

Patchset #7 (id:900001) has been deleted

[mark a. foltz, 2015-08-04 21:57:22]

My comments are minor in relation to the new/modified code to implement the
feature.  I continue to have some concerns about the structure of existing code;
I prefaced them with "side comment:" but I don't consider them blocking commit
of this patch.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/extensi...
File chrome/browser/extensions/extension_action_test_util.cc (right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/extensi...
chrome/browser/extensions/extension_action_test_util.cc:64:
ExtensionRegistry::Get(profile)->enabled_extensions().GetByID(
What guarantees that the ids in toolbar_extensions will all be enabled
extensions?

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/extensi...
chrome/browser/extensions/extension_action_test_util.cc:66: ExtensionAction*
extension_action =
What if the extension doesn't have a page action?

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/extensi...
File chrome/browser/extensions/extension_message_bubble_controller.cc (right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/extensi...
chrome/browser/extensions/extension_message_bubble_controller.cc:155:
did_highlight_ = true;
Side comment:  There seems to be an encapsulation problem here, we are recording
the highlighted state and then side effecting the TAM.  It seems like we should
be asking the TAM the true highlight state.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/extensi...
chrome/browser/extensions/extension_message_bubble_controller.cc:240:
ToolbarActionsModel::Get(profile_)->StopHighlighting();
Side comment:  StopHighlighting() could be a no-op if not necessary, and then we
don't need to track highlighted state in this object.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:22: static const
char kMockActionId[]; // Only used for testing.
Nit: Two spaces between semicolon and comment.

Also I usually see naming like "kActionIdForTest[]", but stick with the style in
surrounding code if there is one.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:29: // Returns a
list of ids of the Chrome Actions.
Can you use the term "Component" instead of Chrome to be consistent with class
naming?

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:30: static
std::vector<std::string> GetComponentIds();
Since we don't support move rvalues yet can we pass in a pointer to a
std::vector as a return value?  Otherwise you could be allocating the vector 2
or 3 times.

Regardless, follow the style for returned STL containers in this code base.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.h:37: // (aka wrench) menu. The
main bar can have only a single row of icons with
Side comment: It's confusing that it's still called a wrench menu in the code. 
I prefer the name "hamburger" (versus "hotdog"), since most hot dogs the bun is
not the exact same length as the 'dog.  But I digress.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:117: (*iter)),
Parenthesis should not be needed here.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:174: DCHECK(browser);
DCHECK(bar)?

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:178: ActionList
component_actions =
Is this likely to change between calls?  Would it make more sense to make this a
member of TAM?

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:185: ActionIdList items =
toolbar_items();
Can you use |toolbar_items_|?

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:188: // of in |model_|.
This comment doesn't seem to apply to the code below - I don't see any check of
highlighting.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:189: // for (std::string
action_id : toolbar_items()) {
Remove commented out line

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:192: if
(std::find(component_ids_.begin(), component_ids_.end(),
Can you factor out a methods to determine if an action_id is a component or
extension?

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:194: // Get the extension.
Factor out GetExtensionById()

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:214: continue;
Can a component have multiple actions?

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:544: for
(ActionIdList::const_iterator it = component_ids_.begin();
Using a range loop, i.e. for (type value : container), would be more readable.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:612:
ToolbarActionsModel::Get(profile_->GetOriginalProfile());
Side comment: Is this fetching the parent profile that spawned the incognito
window?  What if there is no parent profile?

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:629: if
(ShouldAddExtension(extension)) {
Side comment: It's confusing to have one method that handles both the incognito
case and the regular case, called from two different methods that handle each
case separately.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:635: // Always show component
actions.
Don't we need to check that the component is incognito enabled?

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:239: PrefService* prefs_;
Rename to prefs_service_?  Otherwise it's hard to tell what this is for vs.
|extension_prefs_|.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:258: std::vector<std::string>
component_ids_;
ActionIdList?

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:265: ActionIdList
last_known_positions_;
This is a list of ids - so it's storing the positions by their order in the
list?  Maybe rephrase the comment to make this clearer.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
(right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:75: 
Can we add the mock component action to existing test cases in this browser
test?

[Devlin, 2015-08-04 22:12:59]

Answering a few of the comments. :)

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/extensi...
File chrome/browser/extensions/extension_action_test_util.cc (right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/extensi...
chrome/browser/extensions/extension_action_test_util.cc:64:
ExtensionRegistry::Get(profile)->enabled_extensions().GetByID(
On 2015/08/04 21:57:21, mark a. foltz wrote:
> What guarantees that the ids in toolbar_extensions will all be enabled
> extensions?

We don't add disabled extensions to the toolbar.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/extensi...
chrome/browser/extensions/extension_action_test_util.cc:66: ExtensionAction*
extension_action =
On 2015/08/04 21:57:21, mark a. foltz wrote:
> What if the extension doesn't have a page action?

Then it will fall into the iff below, and we won't count it - which is what we
want here.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/extensi...
File chrome/browser/extensions/extension_message_bubble_controller.cc (right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/extensi...
chrome/browser/extensions/extension_message_bubble_controller.cc:155:
did_highlight_ = true;
On 2015/08/04 21:57:21, mark a. foltz wrote:
> Side comment:  There seems to be an encapsulation problem here, we are
recording
> the highlighted state and then side effecting the TAM.  It seems like we
should
> be asking the TAM the true highlight state.

I'm not really sure I follow how this is side effecting the model..., or an
encapsulation error, since all calls are one-way into the model from here.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.h:37: // (aka wrench) menu. The
main bar can have only a single row of icons with
On 2015/08/04 21:57:21, mark a. foltz wrote:
> Side comment: It's confusing that it's still called a wrench menu in the code.

> I prefer the name "hamburger" (versus "hotdog"), since most hot dogs the bun
is
> not the exact same length as the 'dog.  But I digress.

The official name for the menu is now the "Chrome" menu, which is what the
comment says.  The fka wasn't a typo, it was "Formerly-known-as", because, well,
frankly, a bunch of us still call it the wrench menu, and don't know what the
"Chrome" menu is. ;)

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:178: ActionList
component_actions =
On 2015/08/04 21:57:21, mark a. foltz wrote:
> Is this likely to change between calls?  Would it make more sense to make this
a
> member of TAM?

A ToolbarActionViewController is a per-window object, whereas the model is
per-profile, and the ActionList is then passed out as a return value, so it
can't really be a member.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:185: ActionIdList items =
toolbar_items();
On 2015/08/04 21:57:21, mark a. foltz wrote:
> Can you use |toolbar_items_|?

toolbar_items() returns a different value based on whether or not the toolbar is
highlighting.  This is done as an abstraction layer for the views code, which
shouldn't really need to keep the separate sets.  The abstraction layer is a
little less valuable now, but the churn to change it might be excessive.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:188: // of in |model_|.
On 2015/08/04 21:57:21, mark a. foltz wrote:
> This comment doesn't seem to apply to the code below - I don't see any check
of
> highlighting.

See above.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:192: if
(std::find(component_ids_.begin(), component_ids_.end(),
On 2015/08/04 21:57:22, mark a. foltz wrote:
> Can you factor out a methods to determine if an action_id is a component or
> extension?

See my suggestion on this.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:612:
ToolbarActionsModel::Get(profile_->GetOriginalProfile());
On 2015/08/04 21:57:21, mark a. foltz wrote:
> Side comment: Is this fetching the parent profile that spawned the incognito
> window?  What if there is no parent profile?

When can that happen?

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:629: if
(ShouldAddExtension(extension)) {
On 2015/08/04 21:57:22, mark a. foltz wrote:
> Side comment: It's confusing to have one method that handles both the
incognito
> case and the regular case, called from two different methods that handle each
> case separately.

ShouldAddExtension() is a common ground, and is also called from
OnExtensionLoaded().  It would lead to a lot of duplicate code to try and inline
it everywhere its used.  The only difference between incognito populate and
populate is to do with the visible icon count; otherwise, it's basically add
everything that meets ShouldAddExtension().

[apacible, 2015-08-04 22:15:59]

Changes for rdevlin.cronin's comments; looking through mfoltz's comments now.

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/780001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:323: const
extensions::Extension* extension) {
On 2015/08/03 20:58:30, Devlin wrote:
> On 2015/07/30 22:53:21, apacible wrote:
> > On 2015/07/29 18:04:04, Devlin wrote:
> > > Shouldn't this take an id?  Can't this be used for any action?
> > 
> > It's only used for AddExtension(Extension...), so I didn't change this.
Should
> I
> > update this to take an id?
> 
> Yes, please.

Done.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:126:
component_ids_(ComponentToolbarActionsFactory::GetComponentIds()),
On 2015/08/03 20:58:30, Devlin wrote:
> I don't think the ToolbarActionsBar should need knowledge of component ids.

Done.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:166: extension->id()) !=
toolbar_items_.end()) {
On 2015/08/03 20:58:30, Devlin wrote:
> It looks like we don't actually need the extension here, right?  Just the
> extension id.

Done.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:186: // Extension and
component actions are suppressed if the extension
On 2015/08/03 20:58:30, Devlin wrote:
> This comment needs to be updated.

Done.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:189: // for (std::string
action_id : toolbar_items()) {
On 2015/08/03 20:58:30, Devlin wrote:
> ?

Oops, meant to delete this.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:192: if
(std::find(component_ids_.begin(), component_ids_.end(),
On 2015/08/03 20:58:30, Devlin wrote:
> Seeing all of these checks makes me think we should have a struct of
> action_type, action_id, and store those.  This will help get rid of most of
the
> code that Mark was (rightly) worried about, while not requiring the upheaval
to
> make an abstract class.

Added |action_id_to_type_| and created an enum for action types. We only call
ComponentToolbarActionsFactory::GetComponentIds() in Populate() now, rather than
every time we have to check whether an ID corresponds with an extension or an
action.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:315: for (const auto&
extension_id : toolbar_items_)
On 2015/08/03 20:58:30, Devlin wrote:
> now that this isn't an ugly scoped_refptr<const extensions::Extension>, I
think
> I'd prefer std::string over auto.  Also, we can call it "id" or "action_id"
> instead of extension_id.

Done.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:362: // TODO(devlin):
Extension actions which are not visible should be moved to
On 2015/08/03 20:58:30, Devlin wrote:
> This TODO can be deleted.

Done.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:545: it !=
component_ids_.end(); ++it) {
On 2015/08/03 20:58:30, Devlin wrote:
> nit: indentation.

Done.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:596: toolbar_items_.size());
On 2015/08/03 20:58:30, Devlin wrote:
> These UMA should be updated, so that BrowserActionsCount and
> BrowserActionsVisible only record extension counts.  It would probably be
useful
> to also have an overall count, and/or component action count.

Done. Added an overall and component action count.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:845:
extensions::ExtensionRegistry::Get(profile_)->
On 2015/08/03 20:58:30, Devlin wrote:
> We should cache the extension registry as a member variable.

Done.

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:58: virtual void
OnToolbarActionAdded(const std::string& id, int index) = 0;
On 2015/08/03 20:58:30, Devlin wrote:
> I think this should pass a scoped_ptr<ToolbarActionViewController> now.

This requires a ToolbarActionsBar to create a ExtensionActionViewController for
extensions. Those places currently are OnExtensionActionVisibilityChanged and
OnExtensionLoaded, which are overridden functions.

IIUC there isn't a way to just get an instance of the ToolbarActionsBar. Am I
missing something?

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right):

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:298: : "";
On 2015/08/03 20:58:30, Devlin wrote:
> nitty nit: in this code, we typically prefer std::string() over "".

Done.

[apacible, 2015-08-05 01:03:42]

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:22: static const
char kMockActionId[]; // Only used for testing.
On 2015/08/04 21:57:21, mark a. foltz wrote:
> Nit: Two spaces between semicolon and comment.
> 
> Also I usually see naming like "kActionIdForTest[]", but stick with the style
in
> surrounding code if there is one.

Done. There is some *ForTest / *ForTesting usage in c/b/ui/toolbar, so I updated
this to be "kActionIdForTest[]".

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:29: // Returns a
list of ids of the Chrome Actions.
On 2015/08/04 21:57:21, mark a. foltz wrote:
> Can you use the term "Component" instead of Chrome to be consistent with class
> naming?

Done.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:30: static
std::vector<std::string> GetComponentIds();
On 2015/08/04 21:57:21, mark a. foltz wrote:
> Since we don't support move rvalues yet can we pass in a pointer to a
> std::vector as a return value?  Otherwise you could be allocating the vector 2
> or 3 times.
> 
> Regardless, follow the style for returned STL containers in this code base.

Done.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.h:37: // (aka wrench) menu. The
main bar can have only a single row of icons with
On 2015/08/04 22:12:59, Devlin wrote:
> On 2015/08/04 21:57:21, mark a. foltz wrote:
> > Side comment: It's confusing that it's still called a wrench menu in the
code.
> 
> > I prefer the name "hamburger" (versus "hotdog"), since most hot dogs the bun
> is
> > not the exact same length as the 'dog.  But I digress.
> 
> The official name for the menu is now the "Chrome" menu, which is what the
> comment says.  The fka wasn't a typo, it was "Formerly-known-as", because,
well,
> frankly, a bunch of us still call it the wrench menu, and don't know what the
> "Chrome" menu is. ;)

Switched back to "fka".

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:117: (*iter)),
On 2015/08/04 21:57:21, mark a. foltz wrote:
> Parenthesis should not be needed here.

Removed.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:174: DCHECK(browser);
On 2015/08/04 21:57:21, mark a. foltz wrote:
> DCHECK(bar)?

Done.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:189: // for (std::string
action_id : toolbar_items()) {
On 2015/08/04 21:57:22, mark a. foltz wrote:
> Remove commented out line

Done.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:192: if
(std::find(component_ids_.begin(), component_ids_.end(),
On 2015/08/04 22:12:59, Devlin wrote:
> On 2015/08/04 21:57:22, mark a. foltz wrote:
> > Can you factor out a methods to determine if an action_id is a component or
> > extension?
> 
> See my suggestion on this.

Done in next patch.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:194: // Get the extension.
On 2015/08/04 21:57:22, mark a. foltz wrote:
> Factor out GetExtensionById()

Done.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:214: continue;
On 2015/08/04 21:57:21, mark a. foltz wrote:
> Can a component have multiple actions?

Each component id is supposed to correspond to only one component action. IIUC
there will be some form of RegisterChromeAction work happening in the future to
enforce the 1:1 mapping.

Devlin?

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:544: for
(ActionIdList::const_iterator it = component_ids_.begin();
On 2015/08/04 21:57:21, mark a. foltz wrote:
> Using a range loop, i.e. for (type value : container), would be more readable.

Done.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:635: // Always show component
actions.
On 2015/08/04 21:57:22, mark a. foltz wrote:
> Don't we need to check that the component is incognito enabled?

Right now, we're showing the component in both cases - normal and incognito.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:239: PrefService* prefs_;
On 2015/08/04 21:57:22, mark a. foltz wrote:
> Rename to prefs_service_?  Otherwise it's hard to tell what this is for vs.
> |extension_prefs_|.

Done.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:258: std::vector<std::string>
component_ids_;
On 2015/08/04 21:57:22, mark a. foltz wrote:
> ActionIdList?

Acknowledged; I removed component_ids_ in the next patch.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:265: ActionIdList
last_known_positions_;
On 2015/08/04 21:57:22, mark a. foltz wrote:
> This is a list of ids - so it's storing the positions by their order in the
> list?  Maybe rephrase the comment to make this clearer.

Done.

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
(right):

https://codereview.chromium.org/1241063003/diff/920001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:75: 
On 2015/08/04 21:57:22, mark a. foltz wrote:
> Can we add the mock component action to existing test cases in this browser
> test?

Devlin: These are currently written without extension action redesign enabled;
is it fine to tweak with this to add component actions?

[mark a. foltz, 2015-08-05 20:21:53]

lgtm with a few remaining comments!

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/extensi...
File chrome/browser/extensions/extension_action_test_util.cc (right):

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/extensi...
chrome/browser/extensions/extension_action_test_util.cc:61: for
(ExtensionIdList::const_iterator iter = toolbar_extensions.begin();
Replace with range loop.

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/extensi...
chrome/browser/extensions/extension_action_test_util.cc:65: (*iter));
Extra ().

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:38:
std::vector<std::string>* ComponentToolbarActionsFactory::GetComponentIds() {
IMO it would be better to return a scoped_ptr<> here and return with .Pass() so
that the caller knows that it has explicit ownership of the return value.

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:787: for
(ActionIdList::const_iterator action_id = ids.begin();
Use a range loop here.

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:789: for
(ActionIdList::const_iterator toolbar_item_id = toolbar_items_.begin();
And here :)

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:867: return extension;
Combine with previous statement.

[apacible, 2015-08-05 23:21:43]

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/extensi...
File chrome/browser/extensions/extension_action_test_util.cc (right):

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/extensi...
chrome/browser/extensions/extension_action_test_util.cc:61: for
(ExtensionIdList::const_iterator iter = toolbar_extensions.begin();
On 2015/08/05 20:21:53, mark a. foltz wrote:
> Replace with range loop.

Done.

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/extensi...
chrome/browser/extensions/extension_action_test_util.cc:65: (*iter));
On 2015/08/05 20:21:53, mark a. foltz wrote:
> Extra ().

Done.

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:38:
std::vector<std::string>* ComponentToolbarActionsFactory::GetComponentIds() {
On 2015/08/05 20:21:53, mark a. foltz wrote:
> IMO it would be better to return a scoped_ptr<> here and return with .Pass()
so
> that the caller knows that it has explicit ownership of the return value.

Switched to just returning std::vector<std::string>.

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:787: for
(ActionIdList::const_iterator action_id = ids.begin();
On 2015/08/05 20:21:53, mark a. foltz wrote:
> Use a range loop here.

Done.

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:789: for
(ActionIdList::const_iterator toolbar_item_id = toolbar_items_.begin();
On 2015/08/05 20:21:53, mark a. foltz wrote:
> And here :)

Done.

https://codereview.chromium.org/1241063003/diff/960001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:867: return extension;
On 2015/08/05 20:21:53, mark a. foltz wrote:
> Combine with previous statement.

Done.

[apacible, 2015-08-05 23:26:01]

apacible@chromium.org changed reviewers:
+ isherman@chromium.org, pkasting@chromium.org

[apacible, 2015-08-05 23:26:02]

Adding OWNERS:
isherman@ for:
  tools/metrics/histograms/histograms.xml
pkasting@ for:
  c/b/ui/cocoa/extensions
  c/b/ui/extensions
  c/b/ui/toolbar
  c/b/ui/views/toolbar

PTAL, thanks!

[Ilya Sherman, 2015-08-06 15:55:23]

https://codereview.chromium.org/1241063003/diff/980001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1241063003/diff/980001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:45927: +<histogram
name="ToolbarActionsModel.ComponentActionsCount">
It looks like you're introducing a new top-level group name,
ToolbarActionsModel.  This might be appropriate, but just to make sure: Have you
considered whether there is an existing top-level group name that's appropriate
for these histograms?  And, if not, is there a more general (but still
reasonably specific) name that you could choose for the top-level group?  For
example, you could do choose "Toolbar.ActionsModel.*".

[Devlin, 2015-08-06 16:36:55]

On 2015/08/05 23:26:02, apacible wrote:
> Adding OWNERS:
> isherman@ for:
>   tools/metrics/histograms/histograms.xml
> pkasting@ for:
>   c/b/ui/cocoa/extensions
>   c/b/ui/extensions
>   c/b/ui/toolbar
>   c/b/ui/views/toolbar
> 
> PTAL, thanks!

FYI, I think I'm owner for anything with toolbar_action in the name (the model,
the bar, etc), so you don't have to review all 1500 lines, Peter. :) 
(naturally, you're welcome to, but I know your review load is pretty heavy.)

[apacible, 2015-08-06 17:50:46]

https://codereview.chromium.org/1241063003/diff/980001/tools/metrics/histogra...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/1241063003/diff/980001/tools/metrics/histogra...
tools/metrics/histograms/histograms.xml:45927: +<histogram
name="ToolbarActionsModel.ComponentActionsCount">
On 2015/08/06 15:55:23, Ilya Sherman wrote:
> It looks like you're introducing a new top-level group name,
> ToolbarActionsModel.  This might be appropriate, but just to make sure: Have
you
> considered whether there is an existing top-level group name that's
appropriate
> for these histograms?  And, if not, is there a more general (but still
> reasonably specific) name that you could choose for the top-level group?  For
> example, you could do choose "Toolbar.ActionsModel.*".

Talked to rdevlin.cronin@ about a potential top-level group name; switched to
Toolbar.ActionsModel.*. Thanks for the suggestions!

[Ilya Sherman, 2015-08-06 17:54:56]

histograms lgtm, thanks

[Peter Kasting, 2015-08-06 20:15:18]

I didn't review chrome/browser/ui/toolbar/toolbar_actions*, left those for
rdevlin.cronin.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc:53:
browser_actions_bar.GetExtensionId(0));
Nit: Indent even

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:39:
std::vector<std::string>* component_ids = new std::vector<std::string>;
This leaks memory.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:44: #if
defined(ENABLE_MEDIA_ROUTER)
Nit: If possible, make it so kEnableMediaRouter is defined in all cases, but the
switch value will always be false when ENABLE_MEDIA_ROUTER is not set.  Then
this code, and anywhere else trying to read this flag, can do so without an
ifdef and still get correct behavior.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:20: // Component
Action IDs.
Nit: Why do we capitalize "Component Action" in all the comments in this file? 
Seems like using normal sentence case would be more correct.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:22: static const
char kActionIdForTest[];  // Only used for testing.
Nit: ForTesting

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:29: // Returns a
list of ids of the Component Actions.
Nit: This comment is technically incorrect, since a vector is not a "list".

More interestingly, this sounds similar to GetNumComponentActions() below (I'd
expect that to return the size of this vector), but that takes a Browser* and
this doesn't, and the comments don't make the distinction clear.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc
(right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc:17: for
(auto it = actions.begin(); it != actions.end(); ++it) {
Nit: Use range-based for

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc:30:
TestToolbarActionViewController* action =
Nit: I try to avoid ever using raw pointers to own objects; I'd prefer to either
inline this call into the push_back() below, or else use a scoped_ptr here with
release() in the next call.

(Since Chromium doesn't support exceptions this is not really a huge correctness
issue but it's nice to just be in the habit of safe code.)

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:18: virtual
~MockComponentToolbarActionsFactory();
Nit: Use override instead of virtual for destructors in non-base classes

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:27: // A set
of all action ids of created actions.
Nit: This type isn't actually a set, so don't use the word "set".

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:31: using ActionList =
ScopedVector<ToolbarActionViewController>;
Do not use "List" in the name of types that are not lists, as it can be very
confusing to readers of consuming code.

Convention is to name these "ActionIds" and "Actions", respectively.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/wrench_menu_model.cc:1034: // aren't shown in the main
container or if there are component actions.
Nit: I think this comment needs updating?

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/vie...
File chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
(right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/vie...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:68:
toolbar_model()->toolbar_items()[0]));
Nit: Why not use a temp to hold
extensions::ExtensionRegistry::Get(profile())->enabled_extensions() (and maybe
toolbar_model()->toolbar_items(), not sure), so these EXPECTs can be written
more briefly, with less redundancy?

[Devlin, 2015-08-06 22:04:57]

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/860001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:58: virtual void
OnToolbarActionAdded(const std::string& id, int index) = 0;
On 2015/08/04 22:15:59, apacible wrote:
> On 2015/08/03 20:58:30, Devlin wrote:
> > I think this should pass a scoped_ptr<ToolbarActionViewController> now.
> 
> This requires a ToolbarActionsBar to create a ExtensionActionViewController
for
> extensions. Those places currently are OnExtensionActionVisibilityChanged and
> OnExtensionLoaded, which are overridden functions.
> 
> IIUC there isn't a way to just get an instance of the ToolbarActionsBar. Am I
> missing something?

Shoot.  There are hacky ways, but... not worth it for now.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:258: // Good time for some
sanity checks: We should never try to display more
These checks should be updated - they were written for a time when the model
didn't have component actions.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:521: // Check that |action_id|
is not a component action since
Sounds like a DCHECK to me :)

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:528: const
extensions::Extension* extension =
Can you put a TODO (feel free to put it as me) saying that this is a minor
layering violation, and that the model should pass in an action directly?

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:566: // Component actions
cannot be removed.
Again, DCHECK?  Though actually I'm not *too* worried about this and the check
above, since there's a pretty clear flow from the model to here only for
extensions.  If removing them means you can also make IsKnownActionWIthType
private, I think I'd probably prefer that.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:696: const std::string ext) {
const &, not just const.  Also, this should probably be more than just "ext" now
(e.g., id)

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:49: include_all_extensions_(
Let's just rename this to, like, use_redesign_.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:109: // insert the action
with id |id| before it in both |toolbar_items_| and
nitty nit: just |id|, not id |id|.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:183: // is kept track of in
|model_|.
This is the model. ;)

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:184: for (size_t i = 0; i <
items.size(); i++) {
range-based for loop.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:198:
DCHECK(extensions::FeatureSwitch::extension_action_redesign()->
This is cached as a variable.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:201: for (size_t j = 0; j <
component_actions.size(); j++) {
range-based

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:204: continue;
this looks like it should be a break.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:210:
component_actions.weak_clear();
have a comment about why this is safe (i.e., doesn't leak).

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:246:
extension_registry_->GetExtensionById(
We have a method for this now, right?  (It shouldn't be possible for this to be
anything but an enabled extension, I think.)

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:261: for (size_t i = 0; i <
toolbar_items_.size(); i++) {
this can be std::find now.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:318: for
(ActionIdList::const_iterator iter_id =
range-based.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:540: if (pos !=
positions->end()) {
Instead of duplicating this logic, can we just generate a set of all ids
(extension and component), and then iterate over it to sort it?

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:557: if
(std::find(component_ids.begin(), component_ids.end(), action_id) ==
These lookups still make me wonder if we should have a struct of string, type. 
Was there a reason not to do that?

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:566: // We don't notify
observers of the added extension yet. Rather,
This comment applies to both component and extension actions.  Probably just put
it above the for loop.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:575: browser_actions_count++;
nitty nit: prefer pre-increment.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:606:
visible_browser_icon_size);
UMA for visible toolbar actions would be good, too.  (When everything launches
and stabilizes, we might get rid of the plain-old browser action one.)

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:639: // Always show component
actions.
Why not respect the user's choice to hide them?  I might not want the cast icon
always appearing in an incognito window.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:660: // Component Actions are
always shown; visibility should not change.
This isn't necessarily true.  That said, this pref has a lot wrong with it (and
is really just there for historical purposes).  I rephrase this to just say
"Component actions don't have prefs to update."

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:734: for (size_t i = 0; i <
toolbar_items_.size(); ++i) {
Again, toolbar_items() can potentially return different a different set than
toolbar_items_. Should probably stick with toolbar_items().

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:842: for (std::string id :
toolbar_items_) {
const &

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:47: ACTION_COMPONENT,
nitty nit: either do the standard enum naming of ACTION_TYPE_COMPONENT or have
it make english sense and do COMPONENT_ACTION.  (I have a slight preference for
the latter, but up to you).

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:52:
extensions::ExtensionPrefs* prefs_service_);
nit: no trailing underscore.

Also, it's probably better to call this "extenison_prefs", since we have a
PrefService class (which is different).

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:93: // No component actions
will be displayed.
Maybe they will - any reason to say they *definitely* won't?

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:140: ActionList
GetActions(Browser* browser, ToolbarActionsBar* bar);
nit: maybe CreateActions?

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:205:
scoped_ptr<MockComponentToolbarActionsFactory> mock_actions_factory_;
Can you add a DISALLOW_COPY_AND_ASSIGN here?

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:326: // One
component action was added when the model was initialized.
As discussed a bit offline, since 99.99% of our users (actually, probably more
like 99.999, but whatever ;)) don't have the redesign on, I'd like to keep the
majority of this test coverage *without* any component actions.  We can have a
separate test for component actions, and, since they are treated pretty equally
in the toolbar model, a basic test should cover most cases (this also allows us
to reduce the noise delta to this file a little bit).

As a side note, we have DCHECKs in the model for component actions requiring the
switch, but I don't see anything adding it here... why's it not blowing up?

[apacible, 2015-08-10 22:34:59]

Patchset #13 (id:1040001) has been deleted

[apacible, 2015-08-10 22:36:18]

Patchset #12 (id:1020001) has been deleted

[apacible, 2015-08-10 22:38:11]

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:258: // Good time for some
sanity checks: We should never try to display more
On 2015/08/06 22:04:55, Devlin (OOO til 8-13) wrote:
> These checks should be updated - they were written for a time when the model
> didn't have component actions.

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:521: // Check that |action_id|
is not a component action since
On 2015/08/06 22:04:55, Devlin (OOO til 8-13) wrote:
> Sounds like a DCHECK to me :)

Removed per below comment.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:528: const
extensions::Extension* extension =
On 2015/08/06 22:04:55, Devlin (OOO til 8-13) wrote:
> Can you put a TODO (feel free to put it as me) saying that this is a minor
> layering violation, and that the model should pass in an action directly?

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:566: // Component actions
cannot be removed.
On 2015/08/06 22:04:55, Devlin (OOO til 8-13) wrote:
> Again, DCHECK?  Though actually I'm not *too* worried about this and the check
> above, since there's a pretty clear flow from the model to here only for
> extensions.  If removing them means you can also make IsKnownActionWIthType
> private, I think I'd probably prefer that.

Removed both DCHECKs, kept IsKnownActionWithType as public for usage in
GetIconCount DCHECKs.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:696: const std::string ext) {
On 2015/08/06 22:04:55, Devlin (OOO til 8-13) wrote:
> const &, not just const.  Also, this should probably be more than just "ext"
now
> (e.g., id)

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:49: include_all_extensions_(
On 2015/08/06 22:04:56, Devlin wrote:
> Let's just rename this to, like, use_redesign_.

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:109: // insert the action
with id |id| before it in both |toolbar_items_| and
On 2015/08/06 22:04:56, Devlin wrote:
> nitty nit: just |id|, not id |id|.

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:183: // is kept track of in
|model_|.
On 2015/08/06 22:04:56, Devlin wrote:
> This is the model. ;)

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:184: for (size_t i = 0; i <
items.size(); i++) {
On 2015/08/06 22:04:56, Devlin wrote:
> range-based for loop.

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:198:
DCHECK(extensions::FeatureSwitch::extension_action_redesign()->
On 2015/08/06 22:04:56, Devlin wrote:
> This is cached as a variable.

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:201: for (size_t j = 0; j <
component_actions.size(); j++) {
On 2015/08/06 22:04:56, Devlin wrote:
> range-based

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:204: continue;
On 2015/08/06 22:04:55, Devlin wrote:
> this looks like it should be a break.

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:210:
component_actions.weak_clear();
On 2015/08/06 22:04:56, Devlin wrote:
> have a comment about why this is safe (i.e., doesn't leak).

Added above in the loop that adds the component actions into action_list.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:246:
extension_registry_->GetExtensionById(
On 2015/08/06 22:04:56, Devlin wrote:
> We have a method for this now, right?  (It shouldn't be possible for this to
be
> anything but an enabled extension, I think.)

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:261: for (size_t i = 0; i <
toolbar_items_.size(); i++) {
On 2015/08/06 22:04:56, Devlin wrote:
> this can be std::find now.

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:318: for
(ActionIdList::const_iterator iter_id =
On 2015/08/06 22:04:56, Devlin wrote:
> range-based.

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:540: if (pos !=
positions->end()) {
On 2015/08/06 22:04:56, Devlin (OOO til 8-13) wrote:
> Instead of duplicating this logic, can we just generate a set of all ids
> (extension and component), and then iterate over it to sort it?

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:557: if
(std::find(component_ids.begin(), component_ids.end(), action_id) ==
On 2015/08/06 22:04:55, Devlin wrote:
> These lookups still make me wonder if we should have a struct of string, type.

> Was there a reason not to do that?

I add to the struct of string, type after this check, right before pushing the
id into toolbar_items_. See L572, L579. This is the only time we do this
particular lookup.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:566: // We don't notify
observers of the added extension yet. Rather,
On 2015/08/06 22:04:55, Devlin wrote:
> This comment applies to both component and extension actions.  Probably just
put
> it above the for loop.

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:575: browser_actions_count++;
On 2015/08/06 22:04:56, Devlin wrote:
> nitty nit: prefer pre-increment.

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:606:
visible_browser_icon_size);
On 2015/08/06 22:04:56, Devlin wrote:
> UMA for visible toolbar actions would be good, too.  (When everything launches
> and stabilizes, we might get rid of the plain-old browser action one.)

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:639: // Always show component
actions.
On 2015/08/06 22:04:56, Devlin (OOO til 8-13) wrote:
> Why not respect the user's choice to hide them?  I might not want the cast
icon
> always appearing in an incognito window.

Good point. How would I go about finding how to hide/show an action? It
currently doesn't seem like there's a way to set/get that decision for component
actions.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:660: // Component Actions are
always shown; visibility should not change.
On 2015/08/06 22:04:56, Devlin wrote:
> This isn't necessarily true.  That said, this pref has a lot wrong with it
(and
> is really just there for historical purposes).  I rephrase this to just say
> "Component actions don't have prefs to update."

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:734: for (size_t i = 0; i <
toolbar_items_.size(); ++i) {
On 2015/08/06 22:04:56, Devlin wrote:
> Again, toolbar_items() can potentially return different a different set than
> toolbar_items_. Should probably stick with toolbar_items().

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:842: for (std::string id :
toolbar_items_) {
On 2015/08/06 22:04:55, Devlin wrote:
> const &

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:47: ACTION_COMPONENT,
On 2015/08/06 22:04:56, Devlin wrote:
> nitty nit: either do the standard enum naming of ACTION_TYPE_COMPONENT or have
> it make english sense and do COMPONENT_ACTION.  (I have a slight preference
for
> the latter, but up to you).

Switched to COMPONENT_ACTION and EXTENSION_ACTION.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:52:
extensions::ExtensionPrefs* prefs_service_);
On 2015/08/06 22:04:56, Devlin wrote:
> nit: no trailing underscore.
> 
> Also, it's probably better to call this "extenison_prefs", since we have a
> PrefService class (which is different).

Done.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:93: // No component actions
will be displayed.
On 2015/08/06 22:04:56, Devlin (OOO til 8-13) wrote:
> Maybe they will - any reason to say they *definitely* won't?

Updated comment.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.h:140: ActionList
GetActions(Browser* browser, ToolbarActionsBar* bar);
On 2015/08/06 22:04:56, Devlin (OOO til 8-13) wrote:
> nit: maybe CreateActions?

Done.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_browsertest.cc:53:
browser_actions_bar.GetExtensionId(0));
On 2015/08/06 20:15:17, Peter Kasting wrote:
> Nit: Indent even

Done.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:39:
std::vector<std::string>* component_ids = new std::vector<std::string>;
On 2015/08/06 20:15:17, Peter Kasting wrote:
> This leaks memory.

Switched to scoped_ptr per mfoltz@'s suggestion.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:44: #if
defined(ENABLE_MEDIA_ROUTER)
On 2015/08/06 20:15:17, Peter Kasting wrote:
> Nit: If possible, make it so kEnableMediaRouter is defined in all cases, but
the
> switch value will always be false when ENABLE_MEDIA_ROUTER is not set.  Then
> this code, and anywhere else trying to read this flag, can do so without an
> ifdef and still get correct behavior.

From talking to some teammates more familiar with our choice of ifdef and switch
usage:

There are some builds of Chrome where we don't build Media Router at all, so the
ifdef prevents any references to Media Router symbols from hitting the compiler,
which would cause linker errors.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:20: // Component
Action IDs.
On 2015/08/06 20:15:17, Peter Kasting wrote:
> Nit: Why do we capitalize "Component Action" in all the comments in this file?

> Seems like using normal sentence case would be more correct.

Made these lower case except at a beginning of a comment/sentence.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:22: static const
char kActionIdForTest[];  // Only used for testing.
Done.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:29: // Returns a
list of ids of the Component Actions.
On 2015/08/06 20:15:17, Peter Kasting wrote:
> Nit: This comment is technically incorrect, since a vector is not a "list".
> 
> More interestingly, this sounds similar to GetNumComponentActions() below (I'd
> expect that to return the size of this vector), but that takes a Browser* and
> this doesn't, and the comments don't make the distinction clear.

Updated.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc
(right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc:17: for
(auto it = actions.begin(); it != actions.end(); ++it) {
On 2015/08/06 20:15:17, Peter Kasting wrote:
> Nit: Use range-based for

Done.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc:30:
TestToolbarActionViewController* action =
On 2015/08/06 20:15:17, Peter Kasting wrote:
> Nit: I try to avoid ever using raw pointers to own objects; I'd prefer to
either
> inline this call into the push_back() below, or else use a scoped_ptr here
with
> release() in the next call.
> 
> (Since Chromium doesn't support exceptions this is not really a huge
correctness
> issue but it's nice to just be in the habit of safe code.)

Done.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:18: virtual
~MockComponentToolbarActionsFactory();
On 2015/08/06 20:15:18, Peter Kasting wrote:
> Nit: Use override instead of virtual for destructors in non-base classes

Done. Also updated ComponentToolbarActionsFactory destructor to be virtual.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:27: // A set
of all action ids of created actions.
On 2015/08/06 20:15:18, Peter Kasting wrote:
> Nit: This type isn't actually a set, so don't use the word "set".

Done.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:31: using ActionList =
ScopedVector<ToolbarActionViewController>;
On 2015/08/06 20:15:18, Peter Kasting wrote:
> Do not use "List" in the name of types that are not lists, as it can be very
> confusing to readers of consuming code.
> 
> Convention is to name these "ActionIds" and "Actions", respectively.

Done. Renamed:
ActionIdList -> ActionIds
ActionList -> Actions

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:205:
scoped_ptr<MockComponentToolbarActionsFactory> mock_actions_factory_;
On 2015/08/06 22:04:56, Devlin (OOO til 8-13) wrote:
> Can you add a DISALLOW_COPY_AND_ASSIGN here?

Done.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:326: // One
component action was added when the model was initialized.
On 2015/08/06 22:04:56, Devlin (OOO til 8-13) wrote:
> As discussed a bit offline, since 99.99% of our users (actually, probably more
> like 99.999, but whatever ;)) don't have the redesign on, I'd like to keep the
> majority of this test coverage *without* any component actions.  We can have a
> separate test for component actions, and, since they are treated pretty
equally
> in the toolbar model, a basic test should cover most cases (this also allows
us
> to reduce the noise delta to this file a little bit).
> 
> As a side note, we have DCHECKs in the model for component actions requiring
the
> switch, but I don't see anything adding it here... why's it not blowing up?

Acknowledged. Un-did the test changes to not use component actions, and added a
test for reordering with a component action at the end of the file. Other cases
e.g. incognito is covered in the *WithSwitch tests.

I didn't add a feature switch check for GetComponentIds, so the tests were
always pulling in kActionIdForTesting as a component action id. I've updated
that and now (without current changes in tests), the non-redesign-specific tests
fail.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/wrench_menu_model.cc (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/wrench_menu_model.cc:1034: // aren't shown in the main
container or if there are component actions.
On 2015/08/06 20:15:18, Peter Kasting wrote:
> Nit: I think this comment needs updating?

Done.

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/vie...
File chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
(right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/vie...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:68:
toolbar_model()->toolbar_items()[0]));
On 2015/08/06 20:15:18, Peter Kasting wrote:
> Nit: Why not use a temp to hold
> extensions::ExtensionRegistry::Get(profile())->enabled_extensions() (and maybe
> toolbar_model()->toolbar_items(), not sure), so these EXPECTs can be written
> more briefly, with less redundancy?

Done.

[Peter Kasting, 2015-08-11 19:10:49]

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:44: #if
defined(ENABLE_MEDIA_ROUTER)
On 2015/08/10 22:38:11, apacible wrote:
> On 2015/08/06 20:15:17, Peter Kasting wrote:
> > Nit: If possible, make it so kEnableMediaRouter is defined in all cases, but
> the
> > switch value will always be false when ENABLE_MEDIA_ROUTER is not set.  Then
> > this code, and anywhere else trying to read this flag, can do so without an
> > ifdef and still get correct behavior.
> 
> From talking to some teammates more familiar with our choice of ifdef and
switch
> usage:
> 
> There are some builds of Chrome where we don't build Media Router at all, so
the
> ifdef prevents any references to Media Router symbols from hitting the
compiler,
> which would cause linker errors.

To be clear, I'm explicitly suggesting making sure switches::kEnableMediaRouter
and ComponentToolbarActionsFactory::kMediaRouterActionId are well-defined even
when ENABLE_MEDIA_ROUTER is false.

This should allow you to compile and link this code properly.  And the fewer
#ifs we can have in the code the better.

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:40:
ComponentToolbarActionsFactory::GetComponentIds() {
It looks like this only ever returns 0 or 1 elements.  Can we just return a
string instead?  Or will this return more than one element in the future?

If not, do we really need to return a scoped_ptr?  It seems like it would be
simpler to just return the vector by copy (and stack-allocate it in this
function).

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:45: // designed
for the new toolbar
Nit: Comments should have trailing periods.

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:89: if
(num_component_actions_ == -1)
This member variable seems like over-optimization.  GetComponentIds() is
reasonably cheap, why not just always call that?

In fact, at that point, why not just eliminate this function entirely and have
callers always just get the vector?

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:20: // Component
action ids.
Nit: "ids" should probably still be "IDs" in the comments in this file.

Consider also naming the function/constant/etc. names with "ID" instead of "Id"
(up to you).

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc
(right):

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc:17: for
(auto action : actions)
Nit: const auto*

(const ToolbarActionViewController* would also be acceptable)

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:27: // All
the action ids.
Nit: This comment adds nothing to the code.  Only add comments if you're going
to give the reader additional information.

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:30: using ActionIds =
std::vector<std::string>;
Nit: Consider naming this ActionIDs instead (up to you)

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/vie...
File chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
(right):

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/vie...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:67:
ActionIds toolbar_items = toolbar_model()->toolbar_items();
Nit: Use a const ref here to avoid the copy

[apacible, 2015-08-11 23:47:51]

Patchset #14 (id:1100001) has been deleted

[apacible, 2015-08-12 05:26:15]

Patchset #16 (id:1160001) has been deleted

[apacible, 2015-08-12 05:26:45]

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/1241063003/diff/1000001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:44: #if
defined(ENABLE_MEDIA_ROUTER)
On 2015/08/11 19:10:49, Peter Kasting wrote:
> On 2015/08/10 22:38:11, apacible wrote:
> > On 2015/08/06 20:15:17, Peter Kasting wrote:
> > > Nit: If possible, make it so kEnableMediaRouter is defined in all cases,
but
> > the
> > > switch value will always be false when ENABLE_MEDIA_ROUTER is not set. 
Then
> > > this code, and anywhere else trying to read this flag, can do so without
an
> > > ifdef and still get correct behavior.
> > 
> > From talking to some teammates more familiar with our choice of ifdef and
> switch
> > usage:
> > 
> > There are some builds of Chrome where we don't build Media Router at all, so
> the
> > ifdef prevents any references to Media Router symbols from hitting the
> compiler,
> > which would cause linker errors.
> 
> To be clear, I'm explicitly suggesting making sure
switches::kEnableMediaRouter
> and ComponentToolbarActionsFactory::kMediaRouterActionId are well-defined even
> when ENABLE_MEDIA_ROUTER is false.
> 
> This should allow you to compile and link this code properly.  And the fewer
> #ifs we can have in the code the better.

Updated chrome_switches.cc so kEnableMediaRouter isn't defined only within an
ifdef. I've removed the ifdefs in this file; other uses should be cleaned up in
a separate patch.

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:40:
ComponentToolbarActionsFactory::GetComponentIds() {
On 2015/08/11 19:10:49, Peter Kasting wrote:
> It looks like this only ever returns 0 or 1 elements.  Can we just return a
> string instead?  Or will this return more than one element in the future?
> 
> If not, do we really need to return a scoped_ptr?  It seems like it would be
> simpler to just return the vector by copy (and stack-allocate it in this
> function).

In the future, we'll be returning more than one element if/as more features
switch to having a component toolbar action. Devlin has more background on what
other features may have upcoming plans for this.

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:45: // designed
for the new toolbar
On 2015/08/11 19:10:49, Peter Kasting wrote:
> Nit: Comments should have trailing periods.

Done.

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:89: if
(num_component_actions_ == -1)
On 2015/08/11 19:10:49, Peter Kasting wrote:
> This member variable seems like over-optimization.  GetComponentIds() is
> reasonably cheap, why not just always call that?
> 
> In fact, at that point, why not just eliminate this function entirely and have
> callers always just get the vector?

Eliminated GetNumComponentActions().

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.h:20: // Component
action ids.
On 2015/08/11 19:10:49, Peter Kasting wrote:
> Nit: "ids" should probably still be "IDs" in the comments in this file.
> 
> Consider also naming the function/constant/etc. names with "ID" instead of
"Id"
> (up to you).

Comments - Done.

For function/constant naming -- it looks like there's a mix of "ID" and "Id"
usage in c/b/ui/toolbar/, but the majority uses "Id", so I'll keep these as is.

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc
(right):

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.cc:17: for
(auto action : actions)
On 2015/08/11 19:10:49, Peter Kasting wrote:
> Nit: const auto*
> 
> (const ToolbarActionViewController* would also be acceptable)

Done.

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:27: // All
the action ids.
On 2015/08/11 19:10:49, Peter Kasting wrote:
> Nit: This comment adds nothing to the code.  Only add comments if you're going
> to give the reader additional information.

Removed.

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:30: using ActionIds =
std::vector<std::string>;
On 2015/08/11 19:10:49, Peter Kasting wrote:
> Nit: Consider naming this ActionIDs instead (up to you)

Same as other comment -- it looks like there's a mix of "ID" and "Id" usage in
c/b/ui/toolbar/, but the majority uses "Id", so I'll keep this as is.

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/vie...
File chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc
(right):

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/vie...
chrome/browser/ui/views/toolbar/browser_actions_container_browsertest.cc:67:
ActionIds toolbar_items = toolbar_model()->toolbar_items();
On 2015/08/11 19:10:49, Peter Kasting wrote:
> Nit: Use a const ref here to avoid the copy

Done.

[Peter Kasting, 2015-08-12 07:13:45]

LGTM

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:40:
ComponentToolbarActionsFactory::GetComponentIds() {
On 2015/08/12 05:26:44, apacible wrote:
> On 2015/08/11 19:10:49, Peter Kasting wrote:
> > It looks like this only ever returns 0 or 1 elements.  Can we just return a
> > string instead?  Or will this return more than one element in the future?
> > 
> > If not, do we really need to return a scoped_ptr?  It seems like it would be
> > simpler to just return the vector by copy (and stack-allocate it in this
> > function).
> 
> In the future, we'll be returning more than one element if/as more features
> switch to having a component toolbar action. Devlin has more background on
what
> other features may have upcoming plans for this.

OK.  I still think returning the vector by copy might be better, but I don't
care much, and you're in a better position to evaluate.

[Devlin, 2015-08-13 21:21:11]

Next round (there were a few files I couldn't quite make a pass on, but I have
meetings the rest of the day, so don't want you to be blocked on it).

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:557: if
(std::find(component_ids.begin(), component_ids.end(), action_id) ==
On 2015/08/10 22:38:10, apacible wrote:
> On 2015/08/06 22:04:55, Devlin wrote:
> > These lookups still make me wonder if we should have a struct of string,
type.
> 
> > Was there a reason not to do that?
> 
> I add to the struct of string, type after this check, right before pushing the
> id into toolbar_items_. See L572, L579. This is the only time we do this
> particular lookup.

That's not quite what I was suggesting.  I was wondering if we should have
toolbar_items be a std::vector<ToolbarAction>, where ToolbarItem is a struct {
std::string id, ActionType type}.  Then, this loop becomes
for (item : sorted) {
  if (item.type == EXTENSION)
    ...
  else
    ...
}

This would also simplify other methods like RedesignIsShowingNewIcons and
CreateActions, as well as being more performant (no lookup).  And it gets rid of
some cruft in the header file, which is really nice.

WDYT?

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:639: // Always show component
actions.
On 2015/08/10 22:38:10, apacible wrote:
> On 2015/08/06 22:04:56, Devlin (OOO til 8-13) wrote:
> > Why not respect the user's choice to hide them?  I might not want the cast
> icon
> > always appearing in an incognito window.
> 
> Good point. How would I go about finding how to hide/show an action? It
> currently doesn't seem like there's a way to set/get that decision for
component
> actions.

The decision is set by whether or not it's visible on the main bar.  Visibility
is indicated by its index being less than the visible icon count.  So you can
just check the main bar, just as we do for extensions.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/extens...
File chrome/browser/extensions/extension_action_test_util.cc (right):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/extens...
chrome/browser/extensions/extension_action_test_util.cc:61: for (std::string id
: toolbar_extensions) {
nit: const &

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/extens...
chrome/browser/extensions/extension_action_test_util.cc:63:
ExtensionRegistry::Get(profile)->enabled_extensions().GetByID(id);
I can see this causing problems when cast is a component, since it will fail at
this line.  We can worry about that later, but something to bear in mind.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (left):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:48: #if
defined(ENABLE_MEDIA_ROUTER)
Do we not need the #if defined blocks anymore?  (Fine if we don't; just
double-checking it's intentional)

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:39:
ComponentToolbarActionsFactory::GetComponentIds() {
Wait, why is this a scoped_ptr?

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:52: if
(switches::MediaRouterEnabled()) {
isn't this always the same as
else if (switches::MediaRouterEnabled()) {
}
?

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:24: const
ActionIds action_ids() const { return action_ids_; }
why not const&?

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:267: if
(crx_file::id_util::IdIsValid(action->GetId()) ||
In updating these, it looks like the checks here are pretty trivial (to the
point of basically being "make sure that this the action is some kind of
possible action"), so they're not really helpful.  I'd just do the basic DCHECKs
with toolbar_actions_.

That is,
DCHECK_LE(visible_icons, toolbar_actions_.size());
DCHECK_EQ(model_->toolbar_items().size(), num_actions);

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:547: // TODO(rdevlin.cronin):
This is a minor layering violation and the model
even though it's not the norm, my TODOs are (mostly) TODO(devlin).

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:551:
enabled_extensions().GetByID(action_id);
// Only extensions should be added after initialization.
DCHECK(extension)

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:179: // Some actions are
suppressed when others are highlighted.
This comment should really be on the line above, and should probably be
something more like
// toolbar_items() might not equate to toolbar_items_ in the case
// where a subset are highlighted.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:207:
component_actions.weak_clear();
I still think that this should be explicitly commented.  Methods like release()
and weak_clear() should set off alarm bells in your head, and it's good to have
the documentation right there to explain (and it makes it less likely to break
in a refactor).

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:480: //
ComponentToolbarActionsFactory ) with the ordering we get from the pref
no space before )

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:528: scoped_ptr<ActionIds>
component_ids =
this would be simpler as
all_action_ids.insert(all_action_ids.end(), component_ids.begin(),
component_ids.end());

Though this might be moot with other comments.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:636: // Always show component
actions.
See other comment for this.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:30: using ActionIds =
std::vector<std::string>;
As I mentioned before, I'm still not keen on these really-simple typedefs.  They
should be used to make things more readable, but these just make me wonder "What
is an ActionIds again?"  If you think they really do add a lot over just using
std::vector<std::string> (which is what I'd do), then I won't fight you on it -
but having a typdef called "Actions" in the global namespace seems like it's
asking for collisions.  It needs to be more specific, or nested (or eliminated
;)).

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:145: const
std::map<std::string, ActionType> action_id_to_type() const {
const&?

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:188: bool
IsKnownActionWithType(const std::string& id, ActionType type) const;
Once the DCHECKs in ToolbarActionsBar are updated, this can be removed.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:251:
extensions::ExtensionPrefs* prefs_service_;
This should be renamed, too (see
https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...)

[apacible, 2015-08-15 00:43:41]

Patchset #17 (id:1200001) has been deleted

[apacible, 2015-08-15 00:51:45]

Patchset #18 (id:1240001) has been deleted

[apacible, 2015-08-15 06:27:23]

Patchset #18 (id:1240001) has been deleted

[apacible, 2015-08-15 06:45:15]

Patchset #19 (id:1280001) has been deleted

[apacible, 2015-08-15 07:46:37]

Patchset #19 (id:1300001) has been deleted

[apacible, 2015-08-15 08:46:41]

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:557: if
(std::find(component_ids.begin(), component_ids.end(), action_id) ==
On 2015/08/13 21:21:10, Devlin wrote:
> On 2015/08/10 22:38:10, apacible wrote:
> > On 2015/08/06 22:04:55, Devlin wrote:
> > > These lookups still make me wonder if we should have a struct of string,
> type.
> > 
> > > Was there a reason not to do that?
> > 
> > I add to the struct of string, type after this check, right before pushing
the
> > id into toolbar_items_. See L572, L579. This is the only time we do this
> > particular lookup.
> 
> That's not quite what I was suggesting.  I was wondering if we should have
> toolbar_items be a std::vector<ToolbarAction>, where ToolbarItem is a struct {
> std::string id, ActionType type}.  Then, this loop becomes
> for (item : sorted) {
>   if (item.type == EXTENSION)
>     ...
>   else
>     ...
> }
> 
> This would also simplify other methods like RedesignIsShowingNewIcons and
> CreateActions, as well as being more performant (no lookup).  And it gets rid
of
> some cruft in the header file, which is really nice.
> 
> WDYT?

Talked offline:
- Created ToolbarItem struct.
- Pass around ToolbarItem rather than ID (string) in private functions; use ID
(string) in public functions.

This definitely helps simplify things in general.

https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:639: // Always show component
actions.
On 2015/08/13 21:21:10, Devlin wrote:
> On 2015/08/10 22:38:10, apacible wrote:
> > On 2015/08/06 22:04:56, Devlin (OOO til 8-13) wrote:
> > > Why not respect the user's choice to hide them?  I might not want the cast
> > icon
> > > always appearing in an incognito window.
> > 
> > Good point. How would I go about finding how to hide/show an action? It
> > currently doesn't seem like there's a way to set/get that decision for
> component
> > actions.
> 
> The decision is set by whether or not it's visible on the main bar. 
Visibility
> is indicated by its index being less than the visible icon count.  So you can
> just check the main bar, just as we do for extensions.

Talked offline; updated comment.

As a start, we'll always have component actions be present. Visibility is
determined by the main bar.

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/1241063003/diff/1080001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:40:
ComponentToolbarActionsFactory::GetComponentIds() {
On 2015/08/12 07:13:45, Peter Kasting wrote:
> On 2015/08/12 05:26:44, apacible wrote:
> > On 2015/08/11 19:10:49, Peter Kasting wrote:
> > > It looks like this only ever returns 0 or 1 elements.  Can we just return
a
> > > string instead?  Or will this return more than one element in the future?
> > > 
> > > If not, do we really need to return a scoped_ptr?  It seems like it would
be
> > > simpler to just return the vector by copy (and stack-allocate it in this
> > > function).
> > 
> > In the future, we'll be returning more than one element if/as more features
> > switch to having a component toolbar action. Devlin has more background on
> what
> > other features may have upcoming plans for this.
> 
> OK.  I still think returning the vector by copy might be better, but I don't
> care much, and you're in a better position to evaluate.

Acknowledged.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/extens...
File chrome/browser/extensions/extension_action_test_util.cc (right):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/extens...
chrome/browser/extensions/extension_action_test_util.cc:61: for (std::string id
: toolbar_extensions) {
On 2015/08/13 21:21:10, Devlin wrote:
> nit: const &

Done.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/extens...
chrome/browser/extensions/extension_action_test_util.cc:63:
ExtensionRegistry::Get(profile)->enabled_extensions().GetByID(id);
On 2015/08/13 21:21:10, Devlin wrote:
> I can see this causing problems when cast is a component, since it will fail
at
> this line.  We can worry about that later, but something to bear in mind.

Acknowledged. Added a check for extension action types.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (left):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:48: #if
defined(ENABLE_MEDIA_ROUTER)
On 2015/08/13 21:21:10, Devlin wrote:
> Do we not need the #if defined blocks anymore?  (Fine if we don't; just
> double-checking it's intentional)

No, pkasting@ gave some good feedback about minimizing #ifdefs so we made sure
kEnableMediaRouter is defined at all times. (whether or not it's enabled is
separate)

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:39:
ComponentToolbarActionsFactory::GetComponentIds() {
On 2015/08/13 21:21:10, Devlin wrote:
> Wait, why is this a scoped_ptr?

We switched it from returning a raw ptr to a scoped ptr so the caller knows it
has explicit ownership of the vector.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:52: if
(switches::MediaRouterEnabled()) {
On 2015/08/13 21:21:10, Devlin wrote:
> isn't this always the same as
> else if (switches::MediaRouterEnabled()) {
> }
> ?

Yes, updated.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h (right):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/mock_component_toolbar_actions_factory.h:24: const
ActionIds action_ids() const { return action_ids_; }
On 2015/08/13 21:21:10, Devlin wrote:
> why not const&?

Done.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:267: if
(crx_file::id_util::IdIsValid(action->GetId()) ||
On 2015/08/13 21:21:11, Devlin wrote:
> In updating these, it looks like the checks here are pretty trivial (to the
> point of basically being "make sure that this the action is some kind of
> possible action"), so they're not really helpful.  I'd just do the basic
DCHECKs
> with toolbar_actions_.
> 
> That is,
> DCHECK_LE(visible_icons, toolbar_actions_.size());
> DCHECK_EQ(model_->toolbar_items().size(), num_actions);

Done.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:547: // TODO(rdevlin.cronin):
This is a minor layering violation and the model
On 2015/08/13 21:21:11, Devlin wrote:
> even though it's not the norm, my TODOs are (mostly) TODO(devlin).

Done.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:551:
enabled_extensions().GetByID(action_id);
On 2015/08/13 21:21:11, Devlin wrote:
> // Only extensions should be added after initialization.
> DCHECK(extension)

Done.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:179: // Some actions are
suppressed when others are highlighted.
On 2015/08/13 21:21:11, Devlin wrote:
> This comment should really be on the line above, and should probably be
> something more like
> // toolbar_items() might not equate to toolbar_items_ in the case
> // where a subset are highlighted.

Done.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:207:
component_actions.weak_clear();
On 2015/08/13 21:21:11, Devlin wrote:
> I still think that this should be explicitly commented.  Methods like
release()
> and weak_clear() should set off alarm bells in your head, and it's good to
have
> the documentation right there to explain (and it makes it less likely to break
> in a refactor).

Done.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:480: //
ComponentToolbarActionsFactory ) with the ordering we get from the pref
On 2015/08/13 21:21:11, Devlin wrote:
> no space before )

Done.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:528: scoped_ptr<ActionIds>
component_ids =
On 2015/08/13 21:21:11, Devlin wrote:
> this would be simpler as
> all_action_ids.insert(all_action_ids.end(), component_ids.begin(),
> component_ids.end());
> 
> Though this might be moot with other comments.

Acknowledged.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:636: // Always show component
actions.
On 2015/08/13 21:21:11, Devlin wrote:
> See other comment for this.

Acknowledged.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:30: using ActionIds =
std::vector<std::string>;
On 2015/08/13 21:21:11, Devlin wrote:
> As I mentioned before, I'm still not keen on these really-simple typedefs. 
They
> should be used to make things more readable, but these just make me wonder
"What
> is an ActionIds again?"  If you think they really do add a lot over just using
> std::vector<std::string> (which is what I'd do), then I won't fight you on it
-
> but having a typdef called "Actions" in the global namespace seems like it's
> asking for collisions.  It needs to be more specific, or nested (or eliminated
> ;)).

Removed. :)

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:145: const
std::map<std::string, ActionType> action_id_to_type() const {
On 2015/08/13 21:21:11, Devlin wrote:
> const&?

Done.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:188: bool
IsKnownActionWithType(const std::string& id, ActionType type) const;
On 2015/08/13 21:21:11, Devlin wrote:
> Once the DCHECKs in ToolbarActionsBar are updated, this can be removed.

Done.

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:251:
extensions::ExtensionPrefs* prefs_service_;
On 2015/08/13 21:21:11, Devlin wrote:
> This should be renamed, too (see
>
https://codereview.chromium.org/1241063003/diff/980001/chrome/browser/ui/tool...)

Done. Kept trailing underscore for this renaming because it's a member variable.

[Devlin, 2015-08-17 16:32:22]

This is looking really good!  Super close!

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:39:
ComponentToolbarActionsFactory::GetComponentIds() {
On 2015/08/15 08:46:41, apacible wrote:
> On 2015/08/13 21:21:10, Devlin wrote:
> > Wait, why is this a scoped_ptr?
> 
> We switched it from returning a raw ptr to a scoped ptr so the caller knows it
> has explicit ownership of the vector.

But why return a ptr at all?  Why not just std::vector<std::string>?

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:601: component_actions_count;
visible_browser_icon_size sounds wrong for what this is.  Also, this will be a
pretty wonky value if visible_icon_count_ is -1.  What about instead:
visible_extension_actions = visible_icon_count_ == -1 ?
    visible_icon_count_ : visible_icon_count_ - component_actions_count;

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:606:
UMA_HISTOGRAM_COUNTS_100("Toolbar.ActionsModel.ToolbarActionsVisible",
This is only going to be interested (i.e., vary from BrowserActionsVisible) if
the redesign is enabled.  I think we should probably gate the recording on that,
add a comment saying such.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:629: if ((*iter).type ==
EXTENSION_ACTION) {
The body of this for-loop seems like it would be simpler as:
// The extension might not be shown in incognito mode. For now, all component
// actions are present.
if (iter->type == EXTENSION_ACTION && !
    ShouldAddExtension(GetExetnsionById(iter->id())
  continue;
toolbar_items_.push_back(*iter);
if (iter - original_model->toolbar_items_.begin() < original_visible)
  ++visible_icon_count_;

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:630: if
(ShouldAddExtension(GetExtensionById((*iter).id))) {
nit: prefer iter->id, rather than (*iter).id (this goes for anywhere).  The only
time we use (*iter) is when it's an iterator to a pointer, so that it comes out
like (*iter)->id.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:54: bool operator==(const
ToolbarItem& other) {
I'm pretty sure that since this is a POD struct, this will happen by default. 
The only reason to have a custom operator== is if you wanted to ever-so-slightly
tweak this to only compare ids, since no component action should ever have a
valid extension id.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:248: const
extensions::Extension* GetExtensionById(const std::string& id) const;
nitty nit: add a comment here.  Something like:
// Looks up and returns the extension with the given |id| in the set of enabled
extensions.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right):

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:142: // Returns the
action at the given index in the toolbar model, or NULL if one
Returns the action's id... or an empty string

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:190: std::string
mock_action_id_;
Is this used?

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:213:
mock_actions_factory_.reset(new MockComponentToolbarActionsFactory(NULL));
prefer nullptr over NULL in new code.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:263:
extensions::extension_action_test_util::PAGE_ACTION);
indentation here looks wrong - was this git cl format'd?

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:282:
extensions::extension_action_test_util::BROWSER_ACTION);
ditto

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:338: EXPECT_EQ("",
GetActionIdAtIndex(0u));
nittiest nit: It will totally depend who you ask, but I prefer std::string()
over "" (and it's more consistent with this particular existing code).

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:838: EXPECT_EQ(1u,
num_toolbar_items());
All the different possible configurations we have here makes testing a right
PITA (once these things are rolled out, it'll be a lot easier).  I think I'd
prefer to test:
1. Regular (no switches)
2. Only redesign switch
3. Redesign switch + component actions
(primarily because that's probably the order these will roll out in).

Right now, we have 1 and 3, but we're missing 2.  I think I'd change this to
have the mock factory not instantiated unless we're testing component actions,
as in the last test (see below comment).

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1273: // Test
various different reorderings, removals, and reinsertions.
... of the toolbar with component actions (probably also rename the test to
include "ComponentActions" somewhere).

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1276:
extensions::FeatureSwitch::ScopedOverride enable_redesign(
(Continuation from above comment) And then you can instantiate the factory here.

[apacible, 2015-08-17 18:23:33]

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc (right):

https://codereview.chromium.org/1241063003/diff/1180001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/component_toolbar_actions_factory.cc:39:
ComponentToolbarActionsFactory::GetComponentIds() {
On 2015/08/17 16:32:21, Devlin wrote:
> On 2015/08/15 08:46:41, apacible wrote:
> > On 2015/08/13 21:21:10, Devlin wrote:
> > > Wait, why is this a scoped_ptr?
> > 
> > We switched it from returning a raw ptr to a scoped ptr so the caller knows
it
> > has explicit ownership of the vector.
> 
> But why return a ptr at all?  Why not just std::vector<std::string>?

Talked offline, switched to vector.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:601: component_actions_count;
On 2015/08/17 16:32:21, Devlin wrote:
> visible_browser_icon_size sounds wrong for what this is.  Also, this will be a
> pretty wonky value if visible_icon_count_ is -1.  What about instead:
> visible_extension_actions = visible_icon_count_ == -1 ?
>     visible_icon_count_ : visible_icon_count_ - component_actions_count;

Done.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:606:
UMA_HISTOGRAM_COUNTS_100("Toolbar.ActionsModel.ToolbarActionsVisible",
On 2015/08/17 16:32:21, Devlin wrote:
> This is only going to be interested (i.e., vary from BrowserActionsVisible) if
> the redesign is enabled.  I think we should probably gate the recording on
that,
> add a comment saying such.

Done.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:629: if ((*iter).type ==
EXTENSION_ACTION) {
On 2015/08/17 16:32:21, Devlin wrote:
> The body of this for-loop seems like it would be simpler as:
> // The extension might not be shown in incognito mode. For now, all component
> // actions are present.
> if (iter->type == EXTENSION_ACTION && !
>     ShouldAddExtension(GetExetnsionById(iter->id())
>   continue;
> toolbar_items_.push_back(*iter);
> if (iter - original_model->toolbar_items_.begin() < original_visible)
>   ++visible_icon_count_;

Done.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:630: if
(ShouldAddExtension(GetExtensionById((*iter).id))) {
On 2015/08/17 16:32:21, Devlin wrote:
> nit: prefer iter->id, rather than (*iter).id (this goes for anywhere).  The
only
> time we use (*iter) is when it's an iterator to a pointer, so that it comes
out
> like (*iter)->id.

Done and changed elsewhere.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model.h (right):

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:54: bool operator==(const
ToolbarItem& other) {
On 2015/08/17 16:32:21, Devlin wrote:
> I'm pretty sure that since this is a POD struct, this will happen by default. 
> The only reason to have a custom operator== is if you wanted to
ever-so-slightly
> tweak this to only compare ids, since no component action should ever have a
> valid extension id.

Done.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.h:248: const
extensions::Extension* GetExtensionById(const std::string& id) const;
On 2015/08/17 16:32:21, Devlin wrote:
> nitty nit: add a comment here.  Something like:
> // Looks up and returns the extension with the given |id| in the set of
enabled
> extensions.

Done.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right):

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:142: // Returns the
action at the given index in the toolbar model, or NULL if one
On 2015/08/17 16:32:21, Devlin wrote:
> Returns the action's id... or an empty string

Done.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:190: std::string
mock_action_id_;
On 2015/08/17 16:32:21, Devlin wrote:
> Is this used?

Yes, with component_action_id().

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:213:
mock_actions_factory_.reset(new MockComponentToolbarActionsFactory(NULL));
On 2015/08/17 16:32:21, Devlin wrote:
> prefer nullptr over NULL in new code.

Done.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:263:
extensions::extension_action_test_util::PAGE_ACTION);
On 2015/08/17 16:32:21, Devlin wrote:
> indentation here looks wrong - was this git cl format'd?

git cl format'd in next patch!

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:282:
extensions::extension_action_test_util::BROWSER_ACTION);
On 2015/08/17 16:32:21, Devlin wrote:
> ditto

Done.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:338: EXPECT_EQ("",
GetActionIdAtIndex(0u));
On 2015/08/17 16:32:21, Devlin wrote:
> nittiest nit: It will totally depend who you ask, but I prefer std::string()
> over "" (and it's more consistent with this particular existing code).

Done and updated below.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:838: EXPECT_EQ(1u,
num_toolbar_items());
On 2015/08/17 16:32:21, Devlin wrote:
> All the different possible configurations we have here makes testing a right
> PITA (once these things are rolled out, it'll be a lot easier).  I think I'd
> prefer to test:
> 1. Regular (no switches)
> 2. Only redesign switch
> 3. Redesign switch + component actions
> (primarily because that's probably the order these will roll out in).
> 
> Right now, we have 1 and 3, but we're missing 2.  I think I'd change this to
> have the mock factory not instantiated unless we're testing component actions,
> as in the last test (see below comment).

Switched this to redesign switch only, and for when we test 3, set the mock
factory.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1273: // Test
various different reorderings, removals, and reinsertions.
On 2015/08/17 16:32:21, Devlin wrote:
> ... of the toolbar with component actions (probably also rename the test to
> include "ComponentActions" somewhere).

Done.

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1276:
extensions::FeatureSwitch::ScopedOverride enable_redesign(
On 2015/08/17 16:32:21, Devlin wrote:
> (Continuation from above comment) And then you can instantiate the factory
here.

Done.

[Devlin, 2015-08-17 18:41:55]

LGTM!  Woohoo!

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right):

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:190: std::string
mock_action_id_;
On 2015/08/17 18:23:32, apacible wrote:
> On 2015/08/17 16:32:21, Devlin wrote:
> > Is this used?
> 
> Yes, with component_action_id().

I think it's probably fine for component_action_id() to just return
kActionIdForTesting directly, since mock_action_id_ will never change.

https://codereview.chromium.org/1241063003/diff/1400001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/1400001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:595: if
(extensions::FeatureSwitch::extension_action_redesign()->IsEnabled()) {
this is just if (use_redesign_)

https://codereview.chromium.org/1241063003/diff/1400001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right):

https://codereview.chromium.org/1241063003/diff/1400001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1066:
SetMockActionsFactory(new MockComponentToolbarActionsFactory(nullptr));
Let's leave this one as redesign-only, too, and put all component action testing
in the last test.

https://codereview.chromium.org/1241063003/diff/1400001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1349:
EXPECT_EQ(component_action_id(), GetActionIdAtIndex(1u));
One more thing to add would be adjusting the visible icon count to make sure
that MCA is tested as on the main bar and in overflow.

[apacible, 2015-08-17 20:39:33]

Thanks, Devlin!

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right):

https://codereview.chromium.org/1241063003/diff/1320001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:190: std::string
mock_action_id_;
On 2015/08/17 18:41:55, Devlin wrote:
> On 2015/08/17 18:23:32, apacible wrote:
> > On 2015/08/17 16:32:21, Devlin wrote:
> > > Is this used?
> > 
> > Yes, with component_action_id().
> 
> I think it's probably fine for component_action_id() to just return
> kActionIdForTesting directly, since mock_action_id_ will never change.

Done.

https://codereview.chromium.org/1241063003/diff/1400001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model.cc (right):

https://codereview.chromium.org/1241063003/diff/1400001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model.cc:595: if
(extensions::FeatureSwitch::extension_action_redesign()->IsEnabled()) {
On 2015/08/17 18:41:55, Devlin wrote:
> this is just if (use_redesign_)

Done.

https://codereview.chromium.org/1241063003/diff/1400001/chrome/browser/ui/too...
File chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc (right):

https://codereview.chromium.org/1241063003/diff/1400001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1066:
SetMockActionsFactory(new MockComponentToolbarActionsFactory(nullptr));
On 2015/08/17 18:41:55, Devlin wrote:
> Let's leave this one as redesign-only, too, and put all component action
testing
> in the last test.

Done.

https://codereview.chromium.org/1241063003/diff/1400001/chrome/browser/ui/too...
chrome/browser/ui/toolbar/toolbar_actions_model_unittest.cc:1349:
EXPECT_EQ(component_action_id(), GetActionIdAtIndex(1u));
On 2015/08/17 18:41:55, Devlin wrote:
> One more thing to add would be adjusting the visible icon count to make sure
> that MCA is tested as on the main bar and in overflow.

Added visibility adjustment.

[apacible, 2015-08-17 20:40:10]

The CQ bit was checked by apacible@chromium.org

[apacible, 2015-08-17 20:40:11]

The patchset sent to the CQ was uploaded after l-g-t-m from mfoltz@chromium.org,
isherman@chromium.org, pkasting@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1241063003/#ps1460001
(title: "Switch GetComponentActionId to unix_hacker_style.")

[commit-bot: I haz the power, 2015-08-17 20:40:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241063003/1460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1241063003/1460001

[commit-bot: I haz the power, 2015-08-17 23:29:50]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-17 23:29:53]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[apacible, 2015-08-18 03:25:48]

The CQ bit was checked by apacible@chromium.org

[commit-bot: I haz the power, 2015-08-18 03:26:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1241063003/1460001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1241063003/1460001

[commit-bot: I haz the power, 2015-08-18 05:14:22]

Committed patchset #26 (id:1460001)

[commit-bot: I haz the power, 2015-08-18 05:15:03]

Patchset 26 (id:??) landed as
https://crrev.com/f9cfc4d02e387612c90385a5998af884975942a5
Cr-Commit-Position: refs/heads/master@{#343808}