[Extensions UI] Highlight toolbar extensions when the redesign bubble is active

[Extensions UI] Highlight toolbar extensions when the redesign bubble is active

When the redesign bubble is being displayed, highlight extension actions.
Clear the highlight when the bubble is dismissed. Also, instead of using the
scary developer-mode warning (yellow) highlight, use a softer (blue) highlight.

BUG=512905
TBR=oshima@chromium.org (added assets)

Committed: https://crrev.com/1f57f746ea2e3f384de0b8ce4ded894e15885b4f
Cr-Commit-Position: refs/heads/master@{#340196}

[Devlin, 2015-07-22 19:10:04]

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

[Devlin, 2015-07-22 19:10:05]

Peter, mind taking a look?

[Peter Kasting, 2015-07-23 00:00:12]

I ask a bunch of dumb questions here because I don't feel like I really
understand how any of this code worked before or works now, so I need basic
aspects of what's going on explained to me.  :(

https://codereview.chromium.org/1246643004/diff/1/chrome/app/theme/theme_reso...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/app/theme/theme_reso...
chrome/app/theme/theme_resources.grd:969: <structure type="chrome_scaled_image"
name="IDR_TOOLBAR_ACTION_HIGHLIGHT_BOTTOM_LEFT"
file="common/toolbar_action_highlight_bottom_left.png" />
Instead of adding 9 images at each scale size, can we add one image and then
split it in code?  I believe NineImagePainter, for example, can take a single
image and split it.

This should not only be briefer, but result in lower total image sizes and seek
requests.

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_toolbar_model.cc:256: for (const
scoped_refptr<const Extension>& extension : toolbar_items_)
Nit: If it's possible to use const auto& here, do so, this type is ugly

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_toolbar_model.cc:694: if
(visible_icon_count() < extension_ids.size())
How come this moved below the observer block?

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_toolbar_model.cc:719: int saved_icon_count =
prefs_->GetInteger(pref_names::kToolbarSize);
Same question as above, plus I'm not exactly sure what the stored pref bit is
doing.

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/toolbar/t...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:310: // highlight here, but
it's hard to do nicely since it's a nine-grid.
Maybe this all becomes better if you return one image instead of nine? 
Certainly this mapping code is ugly (and I'm not sure why depending on the
extension toolbar model is a bad thing anyway).

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:396: if
(!checked_extension_bubble_ && !is_highlighting()) {
I'm not totally clear on why this was added...

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:706:
!toolbar_action_views_.empty() && !in_overflow_mode()) {
Was this just an oversight before?  Seems like we would have wanted this
condition before this change.

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:711:
DCHECK_NE(ToolbarActionsBar::HIGHLIGHT_NONE, highlight_type);
Nit: Personally I'd leave out this comment and DCHECK since they're pretty
obvious.

[Devlin, 2015-07-23 02:43:30]

Just responding to questions so far; I'll make the code changes when back in the
office tomorrow.

https://codereview.chromium.org/1246643004/diff/1/chrome/app/theme/theme_reso...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/app/theme/theme_reso...
chrome/app/theme/theme_resources.grd:969: <structure type="chrome_scaled_image"
name="IDR_TOOLBAR_ACTION_HIGHLIGHT_BOTTOM_LEFT"
file="common/toolbar_action_highlight_bottom_left.png" />
On 2015/07/23 00:00:11, Peter Kasting wrote:
> Instead of adding 9 images at each scale size, can we add one image and then
> split it in code?  I believe NineImagePainter, for example, can take a single
> image and split it.
> 
> This should not only be briefer, but result in lower total image sizes and
seek
> requests.

Good question, and one that I'm not really an authority on.  The nine-grid-style
here is what's been used for the very-similar developer mode highlight, and for
other highlights.  I'd be a little concerned that taking a single image would
result in having some funny scaling issues, but I'm not sure one way or the
other.

For concrete reasons to do it this way:
- It's consistent, so if we change one, we can change both.
- Doing a single image requires new UI assets, and with UI designers being a
scarce resource, I don't think it's (yet) worth going back to them.

Fewer seek requests is good, but this highlight is pretty rare, so it's not
performance-sensitive.

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_toolbar_model.cc:694: if
(visible_icon_count() < extension_ids.size())
On 2015/07/23 00:00:11, Peter Kasting wrote:
> How come this moved below the observer block?

Implementation detail: When the highlight mode changes, actions are destroyed
and recreated.  Moving the SetVisibleIconCOunt() below it makes it so that we
still get smooth animations (since we aren't destroying all the actions).  It
results in a much smoother appearance.

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_toolbar_model.cc:719: int saved_icon_count =
prefs_->GetInteger(pref_names::kToolbarSize);
On 2015/07/23 00:00:11, Peter Kasting wrote:
> Same question as above, plus I'm not exactly sure what the stored pref bit is
> doing.

Same answer. :)  The stored pref is the number of visible actions before
Chromium started highlighting, so that after whatever we're showing the user is
done, their old preference is restored.

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/toolbar/t...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:310: // highlight here, but
it's hard to do nicely since it's a nine-grid.
On 2015/07/23 00:00:12, Peter Kasting wrote:
> Maybe this all becomes better if you return one image instead of nine? 
> Certainly this mapping code is ugly (and I'm not sure why depending on the
> extension toolbar model is a bad thing anyway).

It could be better with a single image, but see other comment.  It could also be
better if we made the NinePartImageIds struct in ui/base/cocoa/appkit_utils.h
for all platforms, and used that in place of our current int[] image grids for
views (which also has other advantages, like more type safety), because then we
could just return a NinePartImageIds here directly, which would be the best
solution.

As for why the dependency is bad, it's really a point of pride more than
anything else - it took a lot of effort for the view implementations to not
depend at all on the ExtensionToolbarModel, and I didn't want to re-introduce
that dependency just for an enum.  Of course, the fact that it's a point of
pride probably means it's not a good reason. :)

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:396: if
(!checked_extension_bubble_ && !is_highlighting()) {
On 2015/07/23 00:00:12, Peter Kasting wrote:
> I'm not totally clear on why this was added...

Extension bubbles can cause a subsection of extensions to be highlighted.  If
the toolbar is already highlighting a different set, we don't want to override
it.

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:706:
!toolbar_action_views_.empty() && !in_overflow_mode()) {
On 2015/07/23 00:00:12, Peter Kasting wrote:
> Was this just an oversight before?  Seems like we would have wanted this
> condition before this change.

It was implicitly implied by the highlight_painter_ check, since that's only set
for non-overflow bars (in LoadImages()).

[Peter Kasting, 2015-07-23 05:40:53]

https://codereview.chromium.org/1246643004/diff/1/chrome/app/theme/theme_reso...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/app/theme/theme_reso...
chrome/app/theme/theme_resources.grd:969: <structure type="chrome_scaled_image"
name="IDR_TOOLBAR_ACTION_HIGHLIGHT_BOTTOM_LEFT"
file="common/toolbar_action_highlight_bottom_left.png" />
On 2015/07/23 at 02:43:30, Devlin wrote:
> On 2015/07/23 00:00:11, Peter Kasting wrote:
> > Instead of adding 9 images at each scale size, can we add one image and then
> > split it in code?  I believe NineImagePainter, for example, can take a
single
> > image and split it.
> > 
> > This should not only be briefer, but result in lower total image sizes and
seek
> > requests.
> 
> Good question, and one that I'm not really an authority on.  The
nine-grid-style here is what's been used for the very-similar developer mode
highlight, and for other highlights.  I'd be a little concerned that taking a
single image would result in having some funny scaling issues, but I'm not sure
one way or the other.

We used to use nine images everywhere.  The code to use one is fairly recent, so
most of our assets aren't converted, but it's what we want to use in the future.

The two have equivalent behavior -- the one-image path effectively divides the
image into nine pieces and from there they're painted/tiled/scaled in the same
way the nine would be.  It's just a matter of the input image being smaller and
simpler, easier to maintain, and easier for the designers to work with (because
they start from a single image and manually divide it anyway).

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_toolbar_model.cc:694: if
(visible_icon_count() < extension_ids.size())
On 2015/07/23 at 02:43:30, Devlin wrote:
> On 2015/07/23 00:00:11, Peter Kasting wrote:
> > How come this moved below the observer block?
> 
> Implementation detail: When the highlight mode changes, actions are destroyed
and recreated.  Moving the SetVisibleIconCOunt() below it makes it so that we
still get smooth animations (since we aren't destroying all the actions).  It
results in a much smoother appearance.

That might be worth a comment, so someone doesn't arbitrarily move it back up
later.

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/toolbar/t...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:310: // highlight here, but
it's hard to do nicely since it's a nine-grid.
On 2015/07/23 at 02:43:30, Devlin wrote:
> On 2015/07/23 00:00:12, Peter Kasting wrote:
> > Maybe this all becomes better if you return one image instead of nine? 
> > Certainly this mapping code is ugly (and I'm not sure why depending on the
> > extension toolbar model is a bad thing anyway).
> 
> It could be better with a single image, but see other comment.  It could also
be better if we made the NinePartImageIds struct in ui/base/cocoa/appkit_utils.h
for all platforms, and used that in place of our current int[] image grids for
views (which also has other advantages, like more type safety), because then we
could just return a NinePartImageIds here directly, which would be the best
solution.
> 
> As for why the dependency is bad, it's really a point of pride more than
anything else - it took a lot of effort for the view implementations to not
depend at all on the ExtensionToolbarModel, and I didn't want to re-introduce
that dependency just for an enum.  Of course, the fact that it's a point of
pride probably means it's not a good reason. :)

I guess I'd probably ask the same thing with the rest of that work as I'm asking
here: what's the actual benefit?  Presumably there is some or you wouldn't have
done the other work to begin with.  But it seems like platform-specific UI code
depending on cross-platform UI code is OK.  We do that plenty of other places. 
And duplication seems potentially worse than dependency, to me.

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:396: if
(!checked_extension_bubble_ && !is_highlighting()) {
On 2015/07/23 at 02:43:30, Devlin wrote:
> On 2015/07/23 00:00:12, Peter Kasting wrote:
> > I'm not totally clear on why this was added...
> 
> Extension bubbles can cause a subsection of extensions to be highlighted.  If
the toolbar is already highlighting a different set, we don't want to override
it.

Ahhhh.  Once again, might be worth a descriptive comment.

[Devlin, 2015-07-23 15:30:14]

https://codereview.chromium.org/1246643004/diff/1/chrome/app/theme/theme_reso...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/app/theme/theme_reso...
chrome/app/theme/theme_resources.grd:969: <structure type="chrome_scaled_image"
name="IDR_TOOLBAR_ACTION_HIGHLIGHT_BOTTOM_LEFT"
file="common/toolbar_action_highlight_bottom_left.png" />
On 2015/07/23 05:40:53, Peter Kasting wrote:
> On 2015/07/23 at 02:43:30, Devlin wrote:
> > On 2015/07/23 00:00:11, Peter Kasting wrote:
> > > Instead of adding 9 images at each scale size, can we add one image and
then
> > > split it in code?  I believe NineImagePainter, for example, can take a
> single
> > > image and split it.
> > > 
> > > This should not only be briefer, but result in lower total image sizes and
> seek
> > > requests.
> > 
> > Good question, and one that I'm not really an authority on.  The
> nine-grid-style here is what's been used for the very-similar developer mode
> highlight, and for other highlights.  I'd be a little concerned that taking a
> single image would result in having some funny scaling issues, but I'm not
sure
> one way or the other.
> 
> We used to use nine images everywhere.  The code to use one is fairly recent,
so
> most of our assets aren't converted, but it's what we want to use in the
future.
> 
> The two have equivalent behavior -- the one-image path effectively divides the
> image into nine pieces and from there they're painted/tiled/scaled in the same
> way the nine would be.  It's just a matter of the input image being smaller
and
> simpler, easier to maintain, and easier for the designers to work with
(because
> they start from a single image and manually divide it anyway).

Ah, thanks for the explanation - a single image then is definitely preferable. 
Would you be okay with this patch landing, and then me updating the images in a
follow-up, just so this isn't gated on getting different UI assets (which can
take quite awhile)?

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/extensions/e...
File chrome/browser/extensions/extension_toolbar_model.cc (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_toolbar_model.cc:256: for (const
scoped_refptr<const Extension>& extension : toolbar_items_)
On 2015/07/23 00:00:11, Peter Kasting wrote:
> Nit: If it's possible to use const auto& here, do so, this type is ugly

Done.

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/extensions/e...
chrome/browser/extensions/extension_toolbar_model.cc:694: if
(visible_icon_count() < extension_ids.size())
On 2015/07/23 05:40:53, Peter Kasting wrote:
> On 2015/07/23 at 02:43:30, Devlin wrote:
> > On 2015/07/23 00:00:11, Peter Kasting wrote:
> > > How come this moved below the observer block?
> > 
> > Implementation detail: When the highlight mode changes, actions are
destroyed
> and recreated.  Moving the SetVisibleIconCOunt() below it makes it so that we
> still get smooth animations (since we aren't destroying all the actions).  It
> results in a much smoother appearance.
> 
> That might be worth a comment, so someone doesn't arbitrarily move it back up
> later.

Done.

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/toolbar/t...
File chrome/browser/ui/toolbar/toolbar_actions_bar.cc (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:310: // highlight here, but
it's hard to do nicely since it's a nine-grid.
On 2015/07/23 05:40:53, Peter Kasting wrote:
> On 2015/07/23 at 02:43:30, Devlin wrote:
> > On 2015/07/23 00:00:12, Peter Kasting wrote:
> > > Maybe this all becomes better if you return one image instead of nine? 
> > > Certainly this mapping code is ugly (and I'm not sure why depending on the
> > > extension toolbar model is a bad thing anyway).
> > 
> > It could be better with a single image, but see other comment.  It could
also
> be better if we made the NinePartImageIds struct in
ui/base/cocoa/appkit_utils.h
> for all platforms, and used that in place of our current int[] image grids for
> views (which also has other advantages, like more type safety), because then
we
> could just return a NinePartImageIds here directly, which would be the best
> solution.
> > 
> > As for why the dependency is bad, it's really a point of pride more than
> anything else - it took a lot of effort for the view implementations to not
> depend at all on the ExtensionToolbarModel, and I didn't want to re-introduce
> that dependency just for an enum.  Of course, the fact that it's a point of
> pride probably means it's not a good reason. :)
> 
> I guess I'd probably ask the same thing with the rest of that work as I'm
asking
> here: what's the actual benefit?  Presumably there is some or you wouldn't
have
> done the other work to begin with.  But it seems like platform-specific UI
code
> depending on cross-platform UI code is OK.  We do that plenty of other places.

> And duplication seems potentially worse than dependency, to me.

Benefit's not substantial enough.  Removed the mapping.

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/toolbar/t...
chrome/browser/ui/toolbar/toolbar_actions_bar.cc:396: if
(!checked_extension_bubble_ && !is_highlighting()) {
On 2015/07/23 05:40:53, Peter Kasting wrote:
> On 2015/07/23 at 02:43:30, Devlin wrote:
> > On 2015/07/23 00:00:12, Peter Kasting wrote:
> > > I'm not totally clear on why this was added...
> > 
> > Extension bubbles can cause a subsection of extensions to be highlighted. 
If
> the toolbar is already highlighting a different set, we don't want to override
> it.
> 
> Ahhhh.  Once again, might be worth a descriptive comment.

Done.

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/views/too...
File chrome/browser/ui/views/toolbar/browser_actions_container.cc (right):

https://codereview.chromium.org/1246643004/diff/1/chrome/browser/ui/views/too...
chrome/browser/ui/views/toolbar/browser_actions_container.cc:711:
DCHECK_NE(ToolbarActionsBar::HIGHLIGHT_NONE, highlight_type);
On 2015/07/23 00:00:12, Peter Kasting wrote:
> Nit: Personally I'd leave out this comment and DCHECK since they're pretty
> obvious.

Done.

[Peter Kasting, 2015-07-23 18:29:07]

lgtm

https://codereview.chromium.org/1246643004/diff/20001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right):

https://codereview.chromium.org/1246643004/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.h:113: HighlightType
GetHighlightType() const;
I think this declaration is a leftover?

[Devlin, 2015-07-23 20:39:35]

https://codereview.chromium.org/1246643004/diff/20001/chrome/browser/ui/toolb...
File chrome/browser/ui/toolbar/toolbar_actions_bar.h (right):

https://codereview.chromium.org/1246643004/diff/20001/chrome/browser/ui/toolb...
chrome/browser/ui/toolbar/toolbar_actions_bar.h:113: HighlightType
GetHighlightType() const;
On 2015/07/23 18:29:07, Peter Kasting wrote:
> I think this declaration is a leftover?

Whoops, removed.

[Devlin, 2015-07-23 20:39:40]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-07-23 20:39:40]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org
Link to the patchset: https://codereview.chromium.org/1246643004/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-07-23 20:40:10]

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

[commit-bot: I haz the power, 2015-07-23 20:51:56]

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

[commit-bot: I haz the power, 2015-07-23 20:51:56]

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

[Devlin, 2015-07-23 22:46:15]

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

[Devlin, 2015-07-23 22:46:19]

The CQ bit was checked by rdevlin.cronin@chromium.org

[commit-bot: I haz the power, 2015-07-23 22:47:11]

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

[commit-bot: I haz the power, 2015-07-23 22:53:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-07-23 22:54:31]

Patchset 3 (id:??) landed as
https://crrev.com/1f57f746ea2e3f384de0b8ce4ded894e15885b4f
Cr-Commit-Position: refs/heads/master@{#340196}