Remove a bunch of pre-MD toolbar code and assets.

Remove a bunch of pre-MD toolbar code and assets.

BUG=648281
Committed: https://crrev.com/51b431e3ba740400f6923a902a694221d4269615
Cr-Commit-Position: refs/heads/master@{#422180}

[Evan Stade, 2016-09-27 20:12:42]

estade@chromium.org changed reviewers:
+ ellyjones@chromium.org, pkasting@chromium.org

[Evan Stade, 2016-09-27 20:12:43]

This breaks mac but hopefully Elly will have a chance to remove
AppMenuButtonCell soon at which point that problem will be resolved.

https://codereview.chromium.org/2374803002/diff/1/chrome/browser/ui/toolbar/a...
File chrome/browser/ui/toolbar/app_menu_icon_painter.h (left):

https://codereview.chromium.org/2374803002/diff/1/chrome/browser/ui/toolbar/a...
chrome/browser/ui/toolbar/app_menu_icon_painter.h:26: class AppMenuIconPainter :
gfx::AnimationDelegate {
mac still nominally relies on this class but it's never used. AppMenuButtonCell
is the last user and should be removed.

[Evan Stade, 2016-09-27 20:12:51]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-27 20:13:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-27 20:30:34]

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

[commit-bot: I haz the power, 2016-09-27 20:30:35]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Peter Kasting, 2016-09-27 20:51:09]

LGTM

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_button.cc (left):

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:125: if (provider) {
Why did we used to check the theme provider?

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_button.cc (right):

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:54: type_ = type;
Tiny nit: This assignment order matches member declaration order but not
function arg declaration order... would be kinda nice if these matched (and if
the function name said it was setting the type, not just the severity)

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:142: DCHECK_EQ(severity_,
AppMenuIconController::Severity::NONE);
Nit: (expected, actual)

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/back_button.cc (left):

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/back_button.cc:26: const int inset =
LabelButton::kFocusRectInset;
Nit: You've removed the last two external uses of this constant, please modify
LabelButton to no longer publicize it (looks like it could become a
function-scope constant in label_button.cc now)

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (left):

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:76: if (provider) {
Another place I don't understand why we were checking the theme provider

[Evan Stade, 2016-09-28 01:33:50]

in the latest patch I think I fixed Mac by just gutting AppMenuButtonCell.

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_button.cc (left):

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:125: if (provider) {
On 2016/09/27 20:51:09, Peter Kasting wrote:
> Why did we used to check the theme provider?

it used to be provider && provider->IsUsingSystemTheme(). We removed the latter
check but the former for some reason persisted (an oversight  I assume).

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/app_menu_button.cc (right):

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:54: type_ = type;
On 2016/09/27 20:51:08, Peter Kasting wrote:
> Tiny nit: This assignment order matches member declaration order but not
> function arg declaration order... would be kinda nice if these matched 

done

> (and if
> the function name said it was setting the type, not just the severity)

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/app_menu_button.cc:142: DCHECK_EQ(severity_,
AppMenuIconController::Severity::NONE);
On 2016/09/27 20:51:09, Peter Kasting wrote:
> Nit: (expected, actual)

Done.

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/back_button.cc (left):

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/back_button.cc:26: const int inset =
LabelButton::kFocusRectInset;
On 2016/09/27 20:51:09, Peter Kasting wrote:
> Nit: You've removed the last two external uses of this constant, please modify
> LabelButton to no longer publicize it (looks like it could become a
> function-scope constant in label_button.cc now)

Done.

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_button.cc (left):

https://codereview.chromium.org/2374803002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_button.cc:76: if (provider) {
On 2016/09/27 20:51:09, Peter Kasting wrote:
> Another place I don't understand why we were checking the theme provider

same explanation

[Evan Stade, 2016-09-28 01:47:40]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 01:47:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 02:22:45]

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

[commit-bot: I haz the power, 2016-09-28 02:22:45]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Evan Stade, 2016-09-28 16:31:30]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 16:32:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 17:07:50]

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

[commit-bot: I haz the power, 2016-09-28 17:07:52]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Evan Stade, 2016-09-28 21:48:40]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 21:49:14]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 22:36:52]

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

[commit-bot: I haz the power, 2016-09-28 22:36:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Evan Stade, 2016-09-28 22:41:41]

The CQ bit was checked by estade@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-28 22:42:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Evan Stade, 2016-09-28 23:43:39]

Elly, ptal Mac changes.

[commit-bot: I haz the power, 2016-09-28 23:46:05]

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

[commit-bot: I haz the power, 2016-09-28 23:46:06]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-09-29 21:10:32]

estade@chromium.org changed reviewers:
+ oshima@chromium.org, sky@chromium.org

[Evan Stade, 2016-09-29 21:10:33]

+sky for ui/views
+oshima for c/a/t

[sky, 2016-09-29 22:07:56]

LGTM

[oshima, 2016-09-30 04:38:30]

lgtm

[Elly Fong-Jones, 2016-09-30 11:09:59]

chrome/browser/ui/cocoa lgtm (but be aware that I'm not an owner for it)

https://codereview.chromium.org/2374803002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/toolbar/app_toolbar_button_cell.mm (right):

https://codereview.chromium.org/2374803002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/toolbar/app_toolbar_button_cell.mm:34:
canvas.set_composite_alpha(true);
The overall effect of this method is now to apply the result of
imageAlphaForWindowState: to the entire canvas, right?

[Evan Stade, 2016-09-30 17:20:06]

https://codereview.chromium.org/2374803002/diff/120001/chrome/browser/ui/coco...
File chrome/browser/ui/cocoa/toolbar/app_toolbar_button_cell.mm (right):

https://codereview.chromium.org/2374803002/diff/120001/chrome/browser/ui/coco...
chrome/browser/ui/cocoa/toolbar/app_toolbar_button_cell.mm:34:
canvas.set_composite_alpha(true);
On 2016/09/30 11:09:59, Elly Fong-Jones wrote:
> The overall effect of this method is now to apply the result of
> imageAlphaForWindowState: to the entire canvas, right?

I don't think AppToolbarButtonCell is used or needs to continue to exist (I
mentioned this previously, perhaps it got lost in the shuffle). It's just a
placeholder right now in the nib which gets replaced by AppToolbarButton.

Thus I don't think it matters what this function does.

[Evan Stade, 2016-09-30 17:37:13]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-09-30 17:37:14]

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/2374803002/#ps120001
(title: "fix mac?")

[commit-bot: I haz the power, 2016-09-30 17:37:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-30 19:31:43]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-09-30 19:34:20]

Description was changed from

==========
Remove a bunch of pre-MD toolbar code and assets.

BUG=648281
==========

to

==========
Remove a bunch of pre-MD toolbar code and assets.

BUG=648281

Committed: https://crrev.com/51b431e3ba740400f6923a902a694221d4269615
Cr-Commit-Position: refs/heads/master@{#422180}
==========

[commit-bot: I haz the power, 2016-09-30 19:34:22]

Patchset 7 (id:??) landed as
https://crrev.com/51b431e3ba740400f6923a902a694221d4269615
Cr-Commit-Position: refs/heads/master@{#422180}