Remove some more !IsModeMaterial code.

Remove some more !IsModeMaterial code.

Remove last references to non-custom IDR_THEME_TOOLBAR.
Remove some unused SHADE images.
Remove DropdownBarView, which is nothing but a bunch of dead code.

BUG=648281
TBR=oshima@chromium.org

Committed: https://crrev.com/c65c5751b770c0f79f60e6eee8ccabe380804146
Cr-Commit-Position: refs/heads/master@{#427062}

[Evan Stade, 2016-10-20 23:01:33]

Description was changed from

==========
Remove some more !IsModeMaterial code.

Remove last references to non-custom IDR_THEME_TOOLBAR.
Remove DropdownBarView, which is nothing but a bunch of dead code.

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

to

==========
Remove some more !IsModeMaterial code.

Remove last references to non-custom IDR_THEME_TOOLBAR.
Remove some unused SHADE images.
Remove DropdownBarView, which is nothing but a bunch of dead code.

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

[Evan Stade, 2016-10-20 23:01:33]

estade@chromium.org changed reviewers:
+ oshima@chromium.org, pkasting@chromium.org, tdanderson@chromium.org

[Evan Stade, 2016-10-20 23:02:16]

+pkasting for review
+oshima for c/a/t
+tdanderson for FYI/optional review

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (left):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:574: //
TODO(pkasting): The "2 *" part of this makes no sense to me.
you can sleep better now

[Evan Stade, 2016-10-20 23:02:21]

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

[commit-bot: I haz the power, 2016-10-20 23:02:48]

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-10-21 01:05:26]

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

[commit-bot: I haz the power, 2016-10-21 01:05:27]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-10-21 21:07:51]

LGTM

https://codereview.chromium.org/2441863002/diff/20001/chrome/app/theme/theme_...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/app/theme/theme_...
chrome/app/theme/theme_resources.grd:646: <structure type="chrome_scaled_image"
name="IDR_THEME_TOOLBAR" file="notused.png" />
Nit: Might be good to have a comment noting that we still need this identifier
so themes can provide a custom image for it?  (Although I suppose if Mac/iOS
dropped the image, we could drop it from here, and just define it as some
theme-specific identifier in some other enum...)

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/find_bar_view.h (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/find_bar_view.h:71: // Claims focus for the text field
and selects its contents.
Nit: Change to "DropdownBarHostDelegate:"

Reorder functions here and in .cc to match base class declaration order (or
reorder base class order)

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:489: //
Background.
Nit: Maybe move this above the above line?  (Or move that line up one?  I'm
mostly just trying to not have a comment that applies to a whole bunch of code
appear in the middle of a block.)

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:503:
gfx::Rect separator_rect = toolbar_bounds;
Nit: Can move down lower and then replace this and the set_...() calls with:

  const gfx::Rect separator_rect(toolbar_bounds.x(), tabstrip_bounds.bottom(),
                                 toolbar_bounds.width(), 0);

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:517:
toolbar_bounds,
Nit: Unusual line-break here

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc:435: int
avatar_height = avatar_visible ? avatar_bottom - avatar_y : 0;
Nit: Can now be

  const int avatar_height = avatar_visible ? incognito_icon.height() : 0;

Though I wonder why this has to be 0 when the avatar is not visible, if we
already set it to non-visible... maybe we rely on these bounds to lay out some
other things.

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc:478: //
Background.
Nit: See _ash.cc comments for this function

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (left):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:264: // TODO(estade): remove
IDR_THEME_TOOLBAR when MD is default.
Not sure what this meant

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:249: // non-default image.
Nit: Update comment

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1221: // to avoid caching a background image
that isn't the same for all tabs.
Nit: This comment is slightly confusing now that you broke the hover handling
into a separate block below.

Honestly, I'd probably just leave this structured as it was before (i.e. handle
hover in this block as well).  No changes to the current body of the block
should be needed (there's no need to detect hover and force the fill_id to zero
or anything).  And the resulting code winds up being shorter too.

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1300: if (fill_id != 0) {
Nit: To avoid the else reading like a double-negative, I would either write this
as "if (fill_id)", or else flip the conditional and reverse the arms.

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.h (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.h:236: // Paint the background of an inactive
tab.
Nit: Paints

(Bonus if you also fix the other function comments above similarly)

[Evan Stade, 2016-10-21 22:01:23]

Description was changed from

==========
Remove some more !IsModeMaterial code.

Remove last references to non-custom IDR_THEME_TOOLBAR.
Remove some unused SHADE images.
Remove DropdownBarView, which is nothing but a bunch of dead code.

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

to

==========
Remove some more !IsModeMaterial code.

Remove last references to non-custom IDR_THEME_TOOLBAR.
Remove some unused SHADE images.
Remove DropdownBarView, which is nothing but a bunch of dead code.

BUG=648281
TBR=oshima@chromium.org
==========

[Evan Stade, 2016-10-21 22:11:12]

TBR oshima since it's just deletions in c/a/t

https://codereview.chromium.org/2441863002/diff/20001/chrome/app/theme/theme_...
File chrome/app/theme/theme_resources.grd (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/app/theme/theme_...
chrome/app/theme/theme_resources.grd:646: <structure type="chrome_scaled_image"
name="IDR_THEME_TOOLBAR" file="notused.png" />
On 2016/10/21 21:07:50, Peter Kasting wrote:
> Nit: Might be good to have a comment noting that we still need this identifier
> so themes can provide a custom image for it?  (Although I suppose if Mac/iOS
> dropped the image, we could drop it from here, and just define it as some
> theme-specific identifier in some other enum...)

Done.

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/find_bar_view.h (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/find_bar_view.h:71: // Claims focus for the text field
and selects its contents.
On 2016/10/21 21:07:50, Peter Kasting wrote:
> Nit: Change to "DropdownBarHostDelegate:"
> 
> Reorder functions here and in .cc to match base class declaration order (or
> reorder base class order)

Done.

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:489: //
Background.
On 2016/10/21 21:07:50, Peter Kasting wrote:
> Nit: Maybe move this above the above line?  (Or move that line up one?  I'm
> mostly just trying to not have a comment that applies to a whole bunch of code
> appear in the middle of a block.)

Done.

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:503:
gfx::Rect separator_rect = toolbar_bounds;
On 2016/10/21 21:07:50, Peter Kasting wrote:
> Nit: Can move down lower and then replace this and the set_...() calls with:
> 
>   const gfx::Rect separator_rect(toolbar_bounds.x(), tabstrip_bounds.bottom(),
>                                  toolbar_bounds.width(), 0);

Done.

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc:517:
toolbar_bounds,
On 2016/10/21 21:07:50, Peter Kasting wrote:
> Nit: Unusual line-break here

Done.

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc:435: int
avatar_height = avatar_visible ? avatar_bottom - avatar_y : 0;
On 2016/10/21 21:07:50, Peter Kasting wrote:
> Nit: Can now be
> 
>   const int avatar_height = avatar_visible ? incognito_icon.height() : 0;
> 
> Though I wonder why this has to be 0 when the avatar is not visible, if we
> already set it to non-visible... maybe we rely on these bounds to lay out some
> other things.

Done.

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_non_client_frame_view_mus.cc:478: //
Background.
On 2016/10/21 21:07:50, Peter Kasting wrote:
> Nit: See _ash.cc comments for this function

Done.

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (left):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:264: // TODO(estade): remove
IDR_THEME_TOOLBAR when MD is default.
On 2016/10/21 21:07:50, Peter Kasting wrote:
> Not sure what this meant

it meant the asset could be removed (i.e. what this CL does)

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:249: // non-default image.
On 2016/10/21 21:07:50, Peter Kasting wrote:
> Nit: Update comment

Done.

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1221: // to avoid caching a background image
that isn't the same for all tabs.
On 2016/10/21 21:07:50, Peter Kasting wrote:
> (there's no need to detect hover and force the fill_id to zero or anything)

actually, there would be a need for 

  ..., has_custom_image ? fill_id : 0, ...

and since both fill_id and y_offset both aren't needed if !has_custom_image, I
thought it was clearer to separate.

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1300: if (fill_id != 0) {
On 2016/10/21 21:07:51, Peter Kasting wrote:
> Nit: To avoid the else reading like a double-negative, I would either write
this
> as "if (fill_id)", or else flip the conditional and reverse the arms.

Done.

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.h (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.h:236: // Paint the background of an inactive
tab.
On 2016/10/21 21:07:51, Peter Kasting wrote:
> Nit: Paints
> 
> (Bonus if you also fix the other function comments above similarly)

Done.

[Evan Stade, 2016-10-21 22:11:21]

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

[commit-bot: I haz the power, 2016-10-21 22:11:47]

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

[Peter Kasting, 2016-10-22 00:14:44]

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1221: // to avoid caching a background image
that isn't the same for all tabs.
On 2016/10/21 22:11:12, Evan Stade wrote:
> On 2016/10/21 21:07:50, Peter Kasting wrote:
> > (there's no need to detect hover and force the fill_id to zero or anything)
> 
> actually, there would be a need for 
> 
>   ..., has_custom_image ? fill_id : 0, ...
> 
> and since both fill_id and y_offset both aren't needed if !has_custom_image, I
> thought it was clearer to separate.

Ah, you're right.  Man, I thought through this like three times and still didn't
see that that would, in fact, be needed.

Maybe just delete the blank line between these two chunks, then, since the
comment above describes both?

[commit-bot: I haz the power, 2016-10-22 02:11:35]

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

[commit-bot: I haz the power, 2016-10-22 02:11:36]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-10-24 13:01:20]

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/tabs/tab.cc (right):

https://codereview.chromium.org/2441863002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/tabs/tab.cc:1221: // to avoid caching a background image
that isn't the same for all tabs.
On 2016/10/22 00:14:44, Peter Kasting wrote:
> On 2016/10/21 22:11:12, Evan Stade wrote:
> > On 2016/10/21 21:07:50, Peter Kasting wrote:
> > > (there's no need to detect hover and force the fill_id to zero or
anything)
> > 
> > actually, there would be a need for 
> > 
> >   ..., has_custom_image ? fill_id : 0, ...
> > 
> > and since both fill_id and y_offset both aren't needed if !has_custom_image,
I
> > thought it was clearer to separate.
> 
> Ah, you're right.  Man, I thought through this like three times and still
didn't
> see that that would, in fact, be needed.
> 
> Maybe just delete the blank line between these two chunks, then, since the
> comment above describes both?

incidentally, already done in latest patchset

[Evan Stade, 2016-10-24 13:01:27]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2016-10-24 13:01:28]

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/2441863002/#ps60001
(title: "restore")

[commit-bot: I haz the power, 2016-10-24 13:01:38]

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-10-24 14:03:27]

Description was changed from

==========
Remove some more !IsModeMaterial code.

Remove last references to non-custom IDR_THEME_TOOLBAR.
Remove some unused SHADE images.
Remove DropdownBarView, which is nothing but a bunch of dead code.

BUG=648281
TBR=oshima@chromium.org
==========

to

==========
Remove some more !IsModeMaterial code.

Remove last references to non-custom IDR_THEME_TOOLBAR.
Remove some unused SHADE images.
Remove DropdownBarView, which is nothing but a bunch of dead code.

BUG=648281
TBR=oshima@chromium.org
==========

[commit-bot: I haz the power, 2016-10-24 14:03:28]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-10-24 14:05:44]

Description was changed from

==========
Remove some more !IsModeMaterial code.

Remove last references to non-custom IDR_THEME_TOOLBAR.
Remove some unused SHADE images.
Remove DropdownBarView, which is nothing but a bunch of dead code.

BUG=648281
TBR=oshima@chromium.org
==========

to

==========
Remove some more !IsModeMaterial code.

Remove last references to non-custom IDR_THEME_TOOLBAR.
Remove some unused SHADE images.
Remove DropdownBarView, which is nothing but a bunch of dead code.

BUG=648281
TBR=oshima@chromium.org

Committed: https://crrev.com/c65c5751b770c0f79f60e6eee8ccabe380804146
Cr-Commit-Position: refs/heads/master@{#427062}
==========

[commit-bot: I haz the power, 2016-10-24 14:05:45]

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