Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by COLOR_NTP_BACKGROUND

Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by COLOR_NTP_BACKGROUND

This fixes issues where the ntp background image was
interrupted underneath the detached bookmark bar.

Also the ntp_background color is now forced to be opaque
even if a none opaque alpha value is provided from the
theme. This prevent issues with subpixel AA which does not
work on none opaque background. (and to avoid falling back
to grayscale AA)

Also see crbug.com/618278

BUG=618335
R=estade@chromium.org, pkasting@chromium.org

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

[Julien Isorce Samsung, 2016-06-15 11:01:10]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 11:01:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2062353002/1

[commit-bot: I haz the power, 2016-06-15 11:12:39]

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

[commit-bot: I haz the power, 2016-06-15 11:12:40]

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

[Julien Isorce Samsung, 2016-06-15 11:31:20]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 11:31:38]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2062353002/20001

[commit-bot: I haz the power, 2016-06-15 11:45:11]

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

[commit-bot: I haz the power, 2016-06-15 11:45:12]

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

[Julien Isorce Samsung, 2016-06-15 12:14:52]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-06-15 12:15:05]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2062353002/40001

[commit-bot: I haz the power, 2016-06-15 13:11:22]

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

[commit-bot: I haz the power, 2016-06-15 13:11:23]

Dry run: This issue passed the CQ dry run.

[Julien Isorce Samsung, 2016-06-15 13:30:48]

Description was changed from

==========
Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by
COLOR_NTP_BACKGROUND

BUG=
==========

to

==========
Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by
COLOR_NTP_BACKGROUND

BUG=618335

R=pkasting@chromium.org
==========

[Julien Isorce Samsung, 2016-06-15 13:30:48]

j.isorce@samsung.com changed reviewers:
+ estade@chromium.org, pkasting@chromium.org

[Julien Isorce Samsung, 2016-06-15 13:35:10]

On 2016/06/15 13:11:23, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

Hi, please have a look. I had to make it opaque in PaintDetachedBookmarkBar
otherwise the text does not render properly when ntp background is not opaque.
Also I am not sure about ConstrainedWindowCustomWindow, seems to be Mac
specific. Thx

[Peter Kasting, 2016-06-20 21:10:22]

I've been delaying reviewing this because before proceeding this direction I
need to better understand what the impact is for themes which set an NTP
background image.  We wouldn't want the background image to be interrupted
underneath the detached bookmark bar, I don't think.  I haven't yet patched this
in locally and tested with and without it.

[Peter Kasting, 2016-06-22 23:46:36]

Since I'm going to be out for several days, you're welcome to help gather some
of this info yourself: find a theme that sets an NTP background image and take
screenshots of the NTP with the detached and attached bookmark bars before and
after this change, and put them on the bug.

[Julien Isorce Samsung, 2016-06-27 09:34:15]

On 2016/06/22 23:46:36, Peter Kasting (OOO) wrote:
> Since I'm going to be out for several days, you're welcome to help gather some
> of this info yourself: find a theme that sets an NTP background image and take
> screenshots of the NTP with the detached and attached bookmark bars before and
> after this change, and put them on the bug.

Done here crbug.com/618335. Note that I made a very simple manifest.json that
just set the ntp_background color to [0, 150, 0].
Let me know if you really need it for "image", not color. But form the
screenshots
you will see that it already highlights what you described your n-1 message.

[Peter Kasting, 2016-07-01 01:10:02]

I think the effect here is not worse than today, so I'm OK with the general
direction.

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

https://codereview.chromium.org/2062353002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:229:
tp->GetColor(ThemeProperties::COLOR_NTP_BACKGROUND) | 0xFF000000);
Use SkColorSetA() instead of direct bit manipulation.

But maybe we should force this color to opaque down in the theme code?  Do we
ever want to allow a transparent color for COLOR_NTP_BACKGROUND?  What sorts of
effects do we get today with a partly-transparent NTP background color?

[Julien Isorce Samsung, 2016-07-01 13:27:45]

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

https://codereview.chromium.org/2062353002/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:229:
tp->GetColor(ThemeProperties::COLOR_NTP_BACKGROUND) | 0xFF000000);
On 2016/07/01 01:10:02, Peter Kasting wrote:
> Use SkColorSetA() instead of direct bit manipulation.
> 

Done thx!

> But maybe we should force this color to opaque down in the theme code?  Do we
> ever want to allow a transparent color for COLOR_NTP_BACKGROUND?  What sorts
of
> effects do we get today with a partly-transparent NTP background color?

I added 2 new screenshots in the bug. One that does not force opaque in that
place. (in browser_view.cc), and second screenshot that shows result with this
CL.

I think we should keep the force opaque just here, not down in the theme because
I think alpha channel should be customizable too.

[Julien Isorce Samsung, 2016-07-01 13:27:53]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-01 13:28:00]

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-07-01 14:11:57]

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

[commit-bot: I haz the power, 2016-07-01 14:11:58]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-07-01 20:18:21]

Based on the screenshots in the bug, we should force this to opaque in the theme
machinery.  We don't want the NTP to actually be semi-transparent, and it can
cause bugs with font rendering.

This isn't the first such change made recently; I'm beginning to wonder how many
theme colors should be allowed to be transparent at all...

[Julien Isorce Samsung, 2016-07-04 16:47:37]

On 2016/07/01 20:18:21, Peter Kasting wrote:
> Based on the screenshots in the bug, we should force this to opaque in the
theme
> machinery.  We don't want the NTP to actually be semi-transparent, and it can
> cause bugs with font rendering.
> 

Don't you think this can break some existing theme if we do that ?
Also rrom my point of view I think this is useful to customize the alpha
channel.
I was actually thinking to build a such theme. And if there are bugs we should
fix them.
Could be guarded by a flag as a start: --enable-alpha-channel-for-ntp-background
?

> This isn't the first such change made recently; I'm beginning to wonder how
many
> theme colors should be allowed to be transparent at all...

[Peter Kasting, 2016-07-06 20:42:48]

On 2016/07/04 16:47:37, Julien Isorce wrote:
> On 2016/07/01 20:18:21, Peter Kasting wrote:
> > Based on the screenshots in the bug, we should force this to opaque in the
> theme
> > machinery.  We don't want the NTP to actually be semi-transparent, and it
can
> > cause bugs with font rendering.
> > 
> 
> Don't you think this can break some existing theme if we do that ?
> Also rrom my point of view I think this is useful to customize the alpha
> channel.
> I was actually thinking to build a such theme. And if there are bugs we should
> fix them.

You can't render text with subpixel AA on a non-opaque background.  That makes
this unsupportable.

Themes doing this are doing something we never intended to allow anyway, so we
should just disable this and be done with it.

[Julien Isorce Samsung, 2016-07-07 00:17:43]

On 2016/07/06 20:42:48, Peter Kasting wrote:
> On 2016/07/04 16:47:37, Julien Isorce wrote:
> > On 2016/07/01 20:18:21, Peter Kasting wrote:
> > > Based on the screenshots in the bug, we should force this to opaque in the
> > theme
> > > machinery.  We don't want the NTP to actually be semi-transparent, and it
> can
> > > cause bugs with font rendering.
> > > 
> > 
> > Don't you think this can break some existing theme if we do that ?
> > Also rrom my point of view I think this is useful to customize the alpha
> > channel.
> > I was actually thinking to build a such theme. And if there are bugs we
should
> > fix them.
> 
> You can't render text with subpixel AA on a non-opaque background.  That makes
> this unsupportable.

Isn't supposed to fallback to grayscale AA in that case ? I am saying this
because I read
http://www.html5rocks.com/en/tutorials/internals/antialiasing-101/ and it seems
this
logic is implemented for all layers (root and non-root).

> 
> Themes doing this are doing something we never intended to allow anyway, so we
> should just disable this and be done with it.

No problem I will make it "force ntp background to opaque in the theme
machinery" as you said.

Thx.

[Julien Isorce Samsung, 2016-07-07 09:52:16]

Description was changed from

==========
Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by
COLOR_NTP_BACKGROUND

BUG=618335

R=pkasting@chromium.org
==========

to

==========
Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by
COLOR_NTP_BACKGROUND

This fixes issues where the ntp background image was interrupted underneath the
detached bookmark bar.

Also the ntp_background color is now forced to be opaque even if a none opaque
alpha value is provided from the theme. This prevent issues with subpixel AA
which does not work on none opaque background. (and to avoid falling back to
grayscale AA)

Also see crbug.com/618278

BUG=618335

R=pkasting@chromium.org
==========

[Julien Isorce Samsung, 2016-07-07 09:52:28]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

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

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-07-07 10:28:14]

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

[commit-bot: I haz the power, 2016-07-07 10:28:15]

Dry run: This issue passed the CQ dry run.

[Julien Isorce Samsung, 2016-07-08 09:55:11]

On 2016/07/07 10:28:15, commit-bot: I haz the power wrote:
> Dry run: This issue passed the CQ dry run.

On 2016/07/01 20:18:21, Peter Kasting wrote:
> we should force this to opaque in the theme

Done in Patch Set 5, please have a look. Thx.

[Peter Kasting, 2016-07-11 01:41:58]

LGTM

https://codereview.chromium.org/2062353002/diff/80001/chrome/browser/themes/b...
File chrome/browser/themes/browser_theme_pack.cc (right):

https://codereview.chromium.org/2062353002/diff/80001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack.cc:797: case
ThemeProperties::COLOR_NTP_BACKGROUND:
Nit: This is fine, but I probably would have just done it with an if statement
as before, which is shorter and not really less readable.

https://codereview.chromium.org/2062353002/diff/80001/chrome/browser/themes/b...
File chrome/browser/themes/browser_theme_pack_unittest.cc (right):

https://codereview.chromium.org/2062353002/diff/80001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack_unittest.cc:453: // Verify that the
theme machinery forces ntp_background color to be opaque.
Nit: Let's combine the toolbar and NTP background color checks into one block
below the other color checks, with one comment, a la "The theme machinery forces
these colors to be opaque."

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

https://codereview.chromium.org/2062353002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:226: // artifacts, so we recreate
the contents container base color here.
The detached background color no longer exists, and the NTP background color
can't be transparent, so this comment is no longer correct.  Maybe:

The detached bar overlaps the |contents_container_| and so uses the same
background color (the NTP background color).

[Julien Isorce Samsung, 2016-07-13 15:01:33]

Thx for the review, all done in Patch Set 6.

https://codereview.chromium.org/2062353002/diff/80001/chrome/browser/themes/b...
File chrome/browser/themes/browser_theme_pack.cc (right):

https://codereview.chromium.org/2062353002/diff/80001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack.cc:797: case
ThemeProperties::COLOR_NTP_BACKGROUND:
On 2016/07/11 01:41:57, Peter Kasting (OOO til Jul 18) wrote:
> Nit: This is fine, but I probably would have just done it with an if statement
> as before, which is shorter and not really less readable.

Done.

https://codereview.chromium.org/2062353002/diff/80001/chrome/browser/themes/b...
File chrome/browser/themes/browser_theme_pack_unittest.cc (right):

https://codereview.chromium.org/2062353002/diff/80001/chrome/browser/themes/b...
chrome/browser/themes/browser_theme_pack_unittest.cc:453: // Verify that the
theme machinery forces ntp_background color to be opaque.
On 2016/07/11 01:41:58, Peter Kasting (OOO til Jul 18) wrote:
> Nit: Let's combine the toolbar and NTP background color checks into one block
> below the other color checks, with one comment, a la "The theme machinery
forces
> these colors to be opaque."

Done.

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

https://codereview.chromium.org/2062353002/diff/80001/chrome/browser/ui/views...
chrome/browser/ui/views/frame/browser_view.cc:226: // artifacts, so we recreate
the contents container base color here.
On 2016/07/11 01:41:58, Peter Kasting (OOO til Jul 18) wrote:
> The detached background color no longer exists, and the NTP background color
> can't be transparent, so this comment is no longer correct.  Maybe:
> 
> The detached bar overlaps the |contents_container_| and so uses the same
> background color (the NTP background color).

Done.

[Julien Isorce Samsung, 2016-07-13 15:05:55]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-13 15:06:09]

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-07-13 15:49:40]

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

[commit-bot: I haz the power, 2016-07-13 15:49:41]

Dry run: This issue passed the CQ dry run.

[Julien Isorce Samsung, 2016-07-14 05:30:09]

The CQ bit was checked by j.isorce@samsung.com

[Julien Isorce Samsung, 2016-07-14 05:30:10]

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/2062353002/#ps120001
(title: "Add brackets")

[commit-bot: I haz the power, 2016-07-14 05:30:31]

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-07-14 05:38:41]

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

[commit-bot: I haz the power, 2016-07-14 05:38:42]

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...)

[Julien Isorce Samsung, 2016-07-14 08:45:27]

j.isorce@samsung.com changed reviewers:
+ ben@chromium.org, sky@chromium.org

[Julien Isorce Samsung, 2016-07-14 08:47:11]

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    chrome/browser/themes/browser_theme_pack.cc
    chrome/browser/themes/browser_theme_pack_unittest.cc
    chrome/browser/themes/theme_properties.cc
    chrome/browser/themes/theme_properties.h
    chrome/browser/themes/theme_service.cc

Evan, Ben, @sky, who can review these changes ? Thx

[Evan Stade, 2016-07-14 20:41:51]

c/b/themes lgtm

https://codereview.chromium.org/2062353002/diff/120001/chrome/browser/themes/...
File chrome/browser/themes/browser_theme_pack_unittest.cc (right):

https://codereview.chromium.org/2062353002/diff/120001/chrome/browser/themes/...
chrome/browser/themes/browser_theme_pack_unittest.cc:454:
colors[ThemeProperties::COLOR_TOOLBAR] = SkColorSetARGB(255, 0, 20, 40);
Just SetRGB since a == 0xff

[Julien Isorce Samsung, 2016-07-15 06:40:53]

Description was changed from

==========
Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by
COLOR_NTP_BACKGROUND

This fixes issues where the ntp background image was interrupted underneath the
detached bookmark bar.

Also the ntp_background color is now forced to be opaque even if a none opaque
alpha value is provided from the theme. This prevent issues with subpixel AA
which does not work on none opaque background. (and to avoid falling back to
grayscale AA)

Also see crbug.com/618278

BUG=618335

R=pkasting@chromium.org
==========

to

==========
Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by
COLOR_NTP_BACKGROUND

This fixes issues where the ntp background image was
interrupted underneath the detached bookmark bar.

Also the ntp_background color is now forced to be opaque
even if a none opaque alpha value is provided from the
theme. This prevent issues with subpixel AA which does not
work on none opaque background. (and to avoid falling back
to grayscale AA)

Also see crbug.com/618278

BUG=618335

R=pkasting@chromium.org
==========

[Julien Isorce Samsung, 2016-07-15 06:43:51]

Description was changed from

==========
Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by
COLOR_NTP_BACKGROUND

This fixes issues where the ntp background image was
interrupted underneath the detached bookmark bar.

Also the ntp_background color is now forced to be opaque
even if a none opaque alpha value is provided from the
theme. This prevent issues with subpixel AA which does not
work on none opaque background. (and to avoid falling back
to grayscale AA)

Also see crbug.com/618278

BUG=618335

R=pkasting@chromium.org
==========

to

==========
Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by
COLOR_NTP_BACKGROUND

This fixes issues where the ntp background image was
interrupted underneath the detached bookmark bar.

Also the ntp_background color is now forced to be opaque
even if a none opaque alpha value is provided from the
theme. This prevent issues with subpixel AA which does not
work on none opaque background. (and to avoid falling back
to grayscale AA)

Also see crbug.com/618278

BUG=618335

R=estade@chromium.org, pkasting@chromium.org
==========

[Julien Isorce Samsung, 2016-07-15 06:43:51]

j.isorce@samsung.com changed reviewers:
- ben@chromium.org, sky@chromium.org

[Julien Isorce Samsung, 2016-07-15 06:44:11]

Thx for the review.

https://codereview.chromium.org/2062353002/diff/120001/chrome/browser/themes/...
File chrome/browser/themes/browser_theme_pack_unittest.cc (right):

https://codereview.chromium.org/2062353002/diff/120001/chrome/browser/themes/...
chrome/browser/themes/browser_theme_pack_unittest.cc:454:
colors[ThemeProperties::COLOR_TOOLBAR] = SkColorSetARGB(255, 0, 20, 40);
On 2016/07/14 20:41:50, Evan Stade wrote:
> Just SetRGB since a == 0xff

Done.

[Julien Isorce Samsung, 2016-07-15 06:46:10]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-07-15 06:46:24]

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-07-15 07:49:47]

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

[commit-bot: I haz the power, 2016-07-15 07:49:49]

Dry run: This issue passed the CQ dry run.

[Julien Isorce Samsung, 2016-07-15 08:30:16]

The CQ bit was checked by j.isorce@samsung.com

[Julien Isorce Samsung, 2016-07-15 08:30:16]

The patchset sent to the CQ was uploaded after l-g-t-m from
pkasting@chromium.org, estade@chromium.org
Link to the patchset: https://codereview.chromium.org/2062353002/#ps140001
(title: "Rebase and use SetRGB everywhere")

[commit-bot: I haz the power, 2016-07-15 08:30: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-07-15 08:35:00]

Description was changed from

==========
Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by
COLOR_NTP_BACKGROUND

This fixes issues where the ntp background image was
interrupted underneath the detached bookmark bar.

Also the ntp_background color is now forced to be opaque
even if a none opaque alpha value is provided from the
theme. This prevent issues with subpixel AA which does not
work on none opaque background. (and to avoid falling back
to grayscale AA)

Also see crbug.com/618278

BUG=618335

R=estade@chromium.org, pkasting@chromium.org
==========

to

==========
Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by
COLOR_NTP_BACKGROUND

This fixes issues where the ntp background image was
interrupted underneath the detached bookmark bar.

Also the ntp_background color is now forced to be opaque
even if a none opaque alpha value is provided from the
theme. This prevent issues with subpixel AA which does not
work on none opaque background. (and to avoid falling back
to grayscale AA)

Also see crbug.com/618278

BUG=618335

R=estade@chromium.org, pkasting@chromium.org
==========

[commit-bot: I haz the power, 2016-07-15 08:35:01]

Committed patchset #8 (id:140001)

[commit-bot: I haz the power, 2016-07-15 08:35:05]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 08:36:54]

Description was changed from

==========
Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by
COLOR_NTP_BACKGROUND

This fixes issues where the ntp background image was
interrupted underneath the detached bookmark bar.

Also the ntp_background color is now forced to be opaque
even if a none opaque alpha value is provided from the
theme. This prevent issues with subpixel AA which does not
work on none opaque background. (and to avoid falling back
to grayscale AA)

Also see crbug.com/618278

BUG=618335

R=estade@chromium.org, pkasting@chromium.org
==========

to

==========
Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by
COLOR_NTP_BACKGROUND

This fixes issues where the ntp background image was
interrupted underneath the detached bookmark bar.

Also the ntp_background color is now forced to be opaque
even if a none opaque alpha value is provided from the
theme. This prevent issues with subpixel AA which does not
work on none opaque background. (and to avoid falling back
to grayscale AA)

Also see crbug.com/618278

BUG=618335

R=estade@chromium.org, pkasting@chromium.org

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

[commit-bot: I haz the power, 2016-07-15 08:36:56]

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

[Evan Stade, 2016-07-18 16:55:16]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2159833002/ by estade@chromium.org.

The reason for reverting is: caused regression: crbug.com/628970.

[Julien Isorce Samsung, 2016-08-02 23:55:40]

Description was changed from

==========
Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by
COLOR_NTP_BACKGROUND

This fixes issues where the ntp background image was
interrupted underneath the detached bookmark bar.

Also the ntp_background color is now forced to be opaque
even if a none opaque alpha value is provided from the
theme. This prevent issues with subpixel AA which does not
work on none opaque background. (and to avoid falling back
to grayscale AA)

Also see crbug.com/618278

BUG=618335

R=estade@chromium.org, pkasting@chromium.org

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

to

==========
Replace CONTROL_BACKGROUND and DETACHED_BOOKMARK_BAR_BACKGROUND by
COLOR_NTP_BACKGROUND

This fixes issues where the ntp background image was
interrupted underneath the detached bookmark bar.

Also the ntp_background color is now forced to be opaque
even if a none opaque alpha value is provided from the
theme. This prevent issues with subpixel AA which does not
work on none opaque background. (and to avoid falling back
to grayscale AA)

Also see crbug.com/618278

BUG=618335

R=estade@chromium.org, pkasting@chromium.org

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

[Julien Isorce Samsung, 2016-08-14 23:49:27]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-14 23:49:40]

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

[Julien Isorce Samsung, 2016-08-14 23:54:15]

On 2016/07/18 16:55:16, Evan Stade wrote:
> The reason for reverting is: caused regression: crbug.com/628970.
To fix this regression I did this:
https://codereview.chromium.org/2062353002/diff2/160001:180001/chrome/browser...

In Patch Set 10, I implemented 1 and 2 from
https://bugs.chromium.org/p/chromium/issues/detail?id=618335#c12 .
Please have a look. Thx.

[commit-bot: I haz the power, 2016-08-15 00:02:29]

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

[commit-bot: I haz the power, 2016-08-15 00:02:31]

Dry run: Exceeded global retry quota

[Julien Isorce Samsung, 2016-08-15 00:03:49]

https://codereview.chromium.org/2062353002/diff/180001/chrome/browser/ui/view...
File chrome/browser/ui/views/frame/browser_view.cc (right):

https://codereview.chromium.org/2062353002/diff/180001/chrome/browser/ui/view...
chrome/browser/ui/views/frame/browser_view.cc:230: :
ThemeProperties::COLOR_NTP_BACKGROUND));
To avoid regression of
https://bugs.chromium.org/p/chromium/issues/detail?id=628970. Or we can decide
to break these themes since it would allow to choose a different color than the
tool bar.

[Julien Isorce Samsung, 2016-08-15 00:08:55]

The CQ bit was checked by j.isorce@samsung.com to run a CQ dry run

[commit-bot: I haz the power, 2016-08-15 00:09:00]

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-08-15 01:51:09]

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

[commit-bot: I haz the power, 2016-08-15 01:51:11]

Dry run: This issue passed the CQ dry run.

[Peter Kasting, 2016-08-15 22:10:44]

On 2016/08/14 23:54:15, Julien Isorce wrote:
> On 2016/07/18 16:55:16, Evan Stade wrote:
> > The reason for reverting is: caused regression: crbug.com/628970.
> To fix this regression I did this:
>
https://codereview.chromium.org/2062353002/diff2/160001:180001/chrome/browser...
> 
> In Patch Set 10, I implemented 1 and 2 from
> https://bugs.chromium.org/p/chromium/issues/detail?id=618335#c12 .
> Please have a look. Thx.

Please don't post new patches on CLs that have already been landed, even if
reverted.  It makes it very confusing later to tell what actually landed here
when clicking the link from the original commit.

Instead:
(1) Delete the patch set here that was posted after landing
(2) Upload a new CL, with the first patch being the thing that landed here
(ideally, synced to current trunk), and then a second change set with what you
want to land now.  This makes it easy to do a diff of patch set 1 against 2 to
see just the new changes.

[Julien Isorce Samsung, 2016-08-15 23:41:21]

On 2016/08/15 22:10:44, Peter Kasting wrote:
> On 2016/08/14 23:54:15, Julien Isorce wrote:
> > On 2016/07/18 16:55:16, Evan Stade wrote:
> > > The reason for reverting is: caused regression: crbug.com/628970.
> > To fix this regression I did this:
> >
>
https://codereview.chromium.org/2062353002/diff2/160001:180001/chrome/browser...
> > 
> > In Patch Set 10, I implemented 1 and 2 from
> > https://bugs.chromium.org/p/chromium/issues/detail?id=618335#c12 .
> > Please have a look. Thx.
> 
> Please don't post new patches on CLs that have already been landed, even if
> reverted.  It makes it very confusing later to tell what actually landed here
> when clicking the link from the original commit.
> 
> Instead:
> (1) Delete the patch set here that was posted after landing
> (2) Upload a new CL, with the first patch being the thing that landed here
> (ideally, synced to current trunk), and then a second change set with what you
> want to land now.  This makes it easy to do a diff of patch set 1 against 2 to
> see just the new changes.

Ah ok sorry I did not know. (Just to double check, it is not a new CL here,
really just a new Patch Set)
Also note that I uploaded a rebase here before the additional changes.
These additional changes are the delta between patch 9 and 10.
But no problem, tomorrow I will do what you suggested above.

[Julien Isorce Samsung, 2016-08-16 15:50:44]

On 2016/08/15 23:41:21, Julien Isorce wrote:
> On 2016/08/15 22:10:44, Peter Kasting wrote:
> > On 2016/08/14 23:54:15, Julien Isorce wrote:
> > > On 2016/07/18 16:55:16, Evan Stade wrote:
> > > > The reason for reverting is: caused regression: crbug.com/628970.
> > > To fix this regression I did this:
> > >
> >
>
https://codereview.chromium.org/2062353002/diff2/160001:180001/chrome/browser...
> > > 
> > > In Patch Set 10, I implemented 1 and 2 from
> > > https://bugs.chromium.org/p/chromium/issues/detail?id=618335#c12 .
> > > Please have a look. Thx.
> > 
> > Please don't post new patches on CLs that have already been landed, even if
> > reverted.  It makes it very confusing later to tell what actually landed
here
> > when clicking the link from the original commit.
> > 
> > Instead:
> > (1) Delete the patch set here that was posted after landing
> > (2) Upload a new CL, with the first patch being the thing that landed here
> > (ideally, synced to current trunk), and then a second change set with what
you
> > want to land now.  This makes it easy to do a diff of patch set 1 against 2
to
> > see just the new changes.
> 
> Ah ok sorry I did not know. (Just to double check, it is not a new CL here,
> really just a new Patch Set)
> Also note that I uploaded a rebase here before the additional changes.
> These additional changes are the delta between patch 9 and 10.
> But no problem, tomorrow I will do what you suggested above.

Done here https://codereview.chromium.org/2252703002

[Julien Isorce Samsung, 2016-08-16 15:58:20]

On 2016/08/16 15:50:44, Julien Isorce wrote:
> On 2016/08/15 23:41:21, Julien Isorce wrote:
> > On 2016/08/15 22:10:44, Peter Kasting wrote:
> > > On 2016/08/14 23:54:15, Julien Isorce wrote:
> > > > On 2016/07/18 16:55:16, Evan Stade wrote:
> > > > > The reason for reverting is: caused regression: crbug.com/628970.
> > > > To fix this regression I did this:
> > > >
> > >
> >
>
https://codereview.chromium.org/2062353002/diff2/160001:180001/chrome/browser...
> > > > 
> > > > In Patch Set 10, I implemented 1 and 2 from
> > > > https://bugs.chromium.org/p/chromium/issues/detail?id=618335#c12 .
> > > > Please have a look. Thx.
> > > 
> > > Please don't post new patches on CLs that have already been landed, even
if
> > > reverted.  It makes it very confusing later to tell what actually landed
> here
> > > when clicking the link from the original commit.
> > > 
> > > Instead:
> > > (1) Delete the patch set here that was posted after landing
> > > (2) Upload a new CL, with the first patch being the thing that landed here
> > > (ideally, synced to current trunk), and then a second change set with what
> you
> > > want to land now.  This makes it easy to do a diff of patch set 1 against
2
> to
> > > see just the new changes.
> > 
> > Ah ok sorry I did not know. (Just to double check, it is not a new CL here,
> > really just a new Patch Set)
> > Also note that I uploaded a rebase here before the additional changes.
> > These additional changes are the delta between patch 9 and 10.
> > But no problem, tomorrow I will do what you suggested above.
> 
> Done here https://codereview.chromium.org/2252703002

Oups mistake, done here https://codereview.chromium.org/2248103002