Remove pre-MD infobar code (on Views).

Remove pre-MD infobar code (on Views).

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

Committed: https://crrev.com/9e2ba781cab15260aaf2b14c3584b22a361a5448
Cr-Commit-Position: refs/heads/master@{#419778}

[Evan Stade, 2016-09-16 21:21:20]

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

[commit-bot: I haz the power, 2016-09-16 21:21:51]

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-16 22:14:26]

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

[commit-bot: I haz the power, 2016-09-16 22:14:27]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-09-16 22:49:34]

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

[Peter Kasting, 2016-09-19 19:04:06]

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/confirm_infobar.cc (right):

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/confirm_infobar.cc:98: // native theme.
Were you waiting on removing pre-MD to implement this TODO?  Just wondering if
there are still blockers or it should be done during the "rip out pre-MD" phase.

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/infobar_background.cc (left):

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/infobar_background.cc:20:
bottom_color_(infobars::InfoBar::GetBottomColor(infobar_type)) {
It looks like with this call removed only Mac code calls GetBottomColor().

Is Mac not using MD-style infobars?  I'm hoping we can clean up this API some.

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/infobar_container_view.cc (right):

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/infobar_container_view.cc:68: content_shadow_ =
new ContentShadow();
Nit: Might as well do this in the initializer list

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/infobar_container_view.cc:83: total_height -=
InfoBarContainerDelegate::kSeparatorLineHeight;
Should we be doing this in GetVerticalOverlap() instead?

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/infobar_view.cc (left):

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/infobar_view.cc:145: views::LabelButton* button
= new views::LabelButton(listener, text);
Can we now kill the label_button.h #include?

Any other #includes we can remove now that we've removed big chunks of code in
this file?

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/infobar_view.cc (right):

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/infobar_view.cc:187: int height =
InfoBarContainerDelegate::kDefaultBarTargetHeightMd;
It'd be nice to rename this to drop the "md" suffix if we can kill the old
constant; looks like most uses of the old one are guarded by MD checks except
for one place in Mac code (that might be wrong)?

[Evan Stade, 2016-09-19 20:05:59]

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

[Evan Stade, 2016-09-19 20:06:02]

also removed a bunch of images (forgot to make that pass earlier)

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/confirm_infobar.cc (right):

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/confirm_infobar.cc:98: // native theme.
On 2016/09/19 19:04:06, Peter Kasting wrote:
> Were you waiting on removing pre-MD to implement this TODO?  Just wondering if
> there are still blockers or it should be done during the "rip out pre-MD"
phase.

the blocker at this point is figuring out an elegant way to apply the normal
native theme to infobars. If we just remove this right now we get a red button
on incognito infobars (because there's no color defined for the normal button).
We also get a slightly wrong bg color for the call to action case, but it's just
the wrong shade of blue so doesn't look terrible.

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/infobar_background.cc (left):

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/infobar_background.cc:20:
bottom_color_(infobars::InfoBar::GetBottomColor(infobar_type)) {
On 2016/09/19 19:04:06, Peter Kasting wrote:
> It looks like with this call removed only Mac code calls GetBottomColor().
> 
> Is Mac not using MD-style infobars?  I'm hoping we can clean up this API some.

I dunno what mac is doing. Presumably when we switch to macviews that usage will
go away and then this API could be simplified.

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/infobar_container_view.cc (right):

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/infobar_container_view.cc:68: content_shadow_ =
new ContentShadow();
On 2016/09/19 19:04:06, Peter Kasting wrote:
> Nit: Might as well do this in the initializer list

Done.

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/infobar_container_view.cc:83: total_height -=
InfoBarContainerDelegate::kSeparatorLineHeight;
On 2016/09/19 19:04:06, Peter Kasting wrote:
> Should we be doing this in GetVerticalOverlap() instead?

I'd be worried about affecting mac.

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/infobar_view.cc (left):

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/infobar_view.cc:145: views::LabelButton* button
= new views::LabelButton(listener, text);
On 2016/09/19 19:04:06, Peter Kasting wrote:
> Can we now kill the label_button.h #include?
> 
> Any other #includes we can remove now that we've removed big chunks of code in
> this file?

removed a few

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/infobar_view.cc (right):

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/infobar_view.cc:187: int height =
InfoBarContainerDelegate::kDefaultBarTargetHeightMd;
On 2016/09/19 19:04:06, Peter Kasting wrote:
> It'd be nice to rename this to drop the "md" suffix if we can kill the old
> constant; looks like most uses of the old one are guarded by MD checks except
> for one place in Mac code (that might be wrong)?

Yea, we can/should rename this, but only when mac code is updated (old code is
ripped out).

[commit-bot: I haz the power, 2016-09-19 20:06:37]

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-09-19 20:12:48]

LGTM

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/confirm_infobar.cc (right):

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/confirm_infobar.cc:98: // native theme.
On 2016/09/19 20:06:02, Evan Stade wrote:
> On 2016/09/19 19:04:06, Peter Kasting wrote:
> > Were you waiting on removing pre-MD to implement this TODO?  Just wondering
if
> > there are still blockers or it should be done during the "rip out pre-MD"
> phase.
> 
> the blocker at this point is figuring out an elegant way to apply the normal
> native theme to infobars. If we just remove this right now we get a red button
> on incognito infobars (because there's no color defined for the normal
button).
> We also get a slightly wrong bg color for the call to action case, but it's
just
> the wrong shade of blue so doesn't look terrible.

Ah.  Is there a bug tracking figuring this out?  I'm fine with punting it, just
don't want it to get lost.

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/infobar_container_view.cc (right):

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/infobar_container_view.cc:83: total_height -=
InfoBarContainerDelegate::kSeparatorLineHeight;
On 2016/09/19 20:06:02, Evan Stade wrote:
> On 2016/09/19 19:04:06, Peter Kasting wrote:
> > Should we be doing this in GetVerticalOverlap() instead?
> 
> I'd be worried about affecting mac.

Hmm.  It would be nice to at least know how Mac was affected so we could TODO
properly, or maybe Mac's current behavior is actually wrong and should just be
fixed?  I don't want to make you fix Mac but at the same time I'm worried that
we'll end up leaving this.

[Evan Stade, 2016-09-19 20:25:06]

Description was changed from

==========
Remove pre-MD infobar code (on Views).

BUG=none
==========

to

==========
Remove pre-MD infobar code (on Views).

BUG=648381, 648281
==========

[Evan Stade, 2016-09-19 20:25:15]

The CQ bit was unchecked by estade@chromium.org

[Evan Stade, 2016-09-19 20:25:21]

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
File chrome/browser/ui/views/infobars/infobar_container_view.cc (right):

https://codereview.chromium.org/2347773003/diff/1/chrome/browser/ui/views/inf...
chrome/browser/ui/views/infobars/infobar_container_view.cc:83: total_height -=
InfoBarContainerDelegate::kSeparatorLineHeight;
filed a bug for these two things

[Evan Stade, 2016-09-19 20:25:26]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-09-19 20:25:36]

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-19 20:34:52]

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

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

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

[Evan Stade, 2016-09-20 15:49:27]

Description was changed from

==========
Remove pre-MD infobar code (on Views).

BUG=648381, 648281
==========

to

==========
Remove pre-MD infobar code (on Views).

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

[Evan Stade, 2016-09-20 15:49:27]

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

[Evan Stade, 2016-09-20 15:49:39]

TBR oshima for asset deletion

[Evan Stade, 2016-09-20 15:49:43]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2016-09-20 15:50:02]

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-20 15:55:56]

Description was changed from

==========
Remove pre-MD infobar code (on Views).

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

to

==========
Remove pre-MD infobar code (on Views).

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

[commit-bot: I haz the power, 2016-09-20 15:55:58]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-20 15:59:03]

Description was changed from

==========
Remove pre-MD infobar code (on Views).

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

to

==========
Remove pre-MD infobar code (on Views).

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

Committed: https://crrev.com/9e2ba781cab15260aaf2b14c3584b22a361a5448
Cr-Commit-Position: refs/heads/master@{#419778}
==========

[commit-bot: I haz the power, 2016-09-20 15:59:04]

Patchset 2 (id:??) landed as
https://crrev.com/9e2ba781cab15260aaf2b14c3584b22a361a5448
Cr-Commit-Position: refs/heads/master@{#419778}