Linux UI: Fix profile chooser button background color

Linux UI: Fix profile chooser button background color

Previously, the profile chooser hovered background color was hardcoded to
0xEAEAEA.  This is about the same color as dark themes will use for their label
text, making the hovered text unreadable.  This CL changes the color to use the
menu item hovered color instead.  Images of the change at:
https://bugs.chromium.org/p/chromium/issues/detail?id=677286#c7

BUG=677286
R=sky@chromium.org

Review-Url: https://codereview.chromium.org/2624683002
Cr-Commit-Position: refs/heads/master@{#448542}
Committed: https://chromium.googlesource.com/chromium/src/+/f0de79a0e44c878b66df8012e77ff38896fe8faf

[Tom (Use chromium acct), 2017-01-10 03:16:31]

Description was changed from

==========
Linux UI: Fix profile choose button background color

BUG=677286
R=erg@chromium.org
==========

to

==========
Linux UI: Fix profile choose button background color

Previously, the profile chooser hovered background color was hardcoded to
0xeaeaea.  This is about the same color as dark themes will use for their label
text, making the hovered text unreadable.  This CL changes the color to use the
menu item hovered color instead.

BUG=677286
R=sky@chromium.org
==========

[Tom (Use chromium acct), 2017-01-10 03:16:32]

thomasanderson@google.com changed reviewers:
+ sky@chromium.org
- erg@chromium.org

[Tom (Use chromium acct), 2017-01-10 03:18:04]

Description was changed from

==========
Linux UI: Fix profile choose button background color

Previously, the profile chooser hovered background color was hardcoded to
0xeaeaea.  This is about the same color as dark themes will use for their label
text, making the hovered text unreadable.  This CL changes the color to use the
menu item hovered color instead.

BUG=677286
R=sky@chromium.org
==========

to

==========
Linux UI: Fix profile choose button background color

Previously, the profile chooser hovered background color was hardcoded to
0xEAEAEA.  This is about the same color as dark themes will use for their label
text, making the hovered text unreadable.  This CL changes the color to use the
menu item hovered color instead.  Images of the change at:
https://bugs.chromium.org/p/chromium/issues/detail?id=677286#c7

BUG=677286
R=sky@chromium.org
==========

[Tom (Use chromium acct), 2017-01-10 03:18:21]

sky@ ptal

[sky, 2017-01-10 15:58:23]

How are windows and chromeos effected? Is the nativetheme color the same as the
old?

[Tom (Use chromium acct), 2017-01-10 21:23:19]

On 2017/01/10 15:58:23, sky wrote:
> How are windows and chromeos effected? Is the nativetheme color the same as
the
> old?

The chromeos color is almost the same, it's now 0xEBEBEB instead of 0xEAEAEA.

I tested on Windows in a chromoting session, so the colors may not be perfect.
On windows the color used to be 0xECEBEC and is now 0xEDECED

[sky, 2017-01-10 23:37:52]

Please run this by the designers to make sure they are ok.

On Tue, Jan 10, 2017 at 1:23 PM, thomasanderson via Chromium-reviews
<chromium-reviews@chromium.org> wrote:
> On 2017/01/10 15:58:23, sky wrote:
>> How are windows and chromeos effected? Is the nativetheme color the same
>> as
> the
>> old?
>
> The chromeos color is almost the same, it's now 0xEBEBEB instead of
> 0xEAEAEA.
>
> I tested on Windows in a chromoting session, so the colors may not be
> perfect.
> On windows the color used to be 0xECEBEC and is now 0xEDECED
>
> https://codereview.chromium.org/2624683002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-01-10 23:38:06]

Also, I think chromoting likely gives different colors, but you should
verify that.

On Tue, Jan 10, 2017 at 3:37 PM, Scott Violet <sky@chromium.org> wrote:
> Please run this by the designers to make sure they are ok.
>
> On Tue, Jan 10, 2017 at 1:23 PM, thomasanderson via Chromium-reviews
> <chromium-reviews@chromium.org> wrote:
>> On 2017/01/10 15:58:23, sky wrote:
>>> How are windows and chromeos effected? Is the nativetheme color the same
>>> as
>> the
>>> old?
>>
>> The chromeos color is almost the same, it's now 0xEBEBEB instead of
>> 0xEAEAEA.
>>
>> I tested on Windows in a chromoting session, so the colors may not be
>> perfect.
>> On windows the color used to be 0xECEBEC and is now 0xEDECED
>>
>> https://codereview.chromium.org/2624683002/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Tom (Use chromium acct), 2017-01-12 00:11:44]

Description was changed from

==========
Linux UI: Fix profile choose button background color

Previously, the profile chooser hovered background color was hardcoded to
0xEAEAEA.  This is about the same color as dark themes will use for their label
text, making the hovered text unreadable.  This CL changes the color to use the
menu item hovered color instead.  Images of the change at:
https://bugs.chromium.org/p/chromium/issues/detail?id=677286#c7

BUG=677286
R=sky@chromium.org
==========

to

==========
Linux UI: Fix profile chooser button background color

Previously, the profile chooser hovered background color was hardcoded to
0xEAEAEA.  This is about the same color as dark themes will use for their label
text, making the hovered text unreadable.  This CL changes the color to use the
menu item hovered color instead.  Images of the change at:
https://bugs.chromium.org/p/chromium/issues/detail?id=677286#c7

BUG=677286
R=sky@chromium.org
==========

[Tom (Use chromium acct), 2017-02-02 21:47:08]

Patchset #2 (id:20001) has been deleted

[Tom (Use chromium acct), 2017-02-02 21:49:39]

sky@ I've added some ifdefs so this only affects linux

[sky, 2017-02-03 00:35:47]

LGTM

[Tom (Use chromium acct), 2017-02-03 00:42:11]

The CQ bit was checked by thomasanderson@google.com

[commit-bot: I haz the power, 2017-02-03 00:42:48]

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, 2017-02-03 01:05:33]

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

[commit-bot: I haz the power, 2017-02-03 01:05:34]

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

[Tom (Use chromium acct), 2017-02-04 01:22:09]

Patchset #3 (id:60001) has been deleted

[Tom (Use chromium acct), 2017-02-05 00:50:39]

Patchset #3 (id:80001) has been deleted

[Tom (Use chromium acct), 2017-02-06 22:09:49]

Patchset #3 (id:100001) has been deleted

[Tom (Use chromium acct), 2017-02-06 22:09:56]

Patchset #3 (id:120001) has been deleted

[Tom (Use chromium acct), 2017-02-06 22:10:01]

Patchset #3 (id:140001) has been deleted

[Tom (Use chromium acct), 2017-02-06 22:10:12]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-06 22:11:13]

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

[Tom (Use chromium acct), 2017-02-06 22:16:26]

sky@: I've rewritten this CL, can you take another look?

There were some things I didn't like about the old CL:
* The color on the linux aura theme changed when I didn't
  want it to
* If a gtk theme used white menuitem highlights, the text
  would be invisible, because the text color is hardcoded to
  white.
* Some build errors on other platforms.

The latest PS fixes all of the above issues.  It colors the
profile chooser button background AND text colors this
time.  The only platform affected should still be linux.

[sky, 2017-02-06 22:56:41]

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:222: void
set_title(views::Label* label) { title_ = label; }
Document what title and subtitle are for. At first I thought they would be child
views, but that isn't the case.

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:229:
ProfileChooserView* profile_chooser_view_;
functions then members.

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:247: if (was_prelight
&& !is_prelight) {
This code is fairly complex and warrants a description.

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:254:
normal_title_color_ = title->enabled_color();
Do you have to keep caching the colors? Can you ask for them once in the
constructor or the first style changed?

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:275: state_ = state();
You shouldn't have to cache the old state. StateChanged() should be supplied the
old state. I realize adding a ButtonState argument is a bigger change, you
should do it separately then return to this patch.

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.h:66: Browser* browser()
const { return browser_; }
this should either return a const Browser* or not be const. Otherwise you're
losing const'ness.

[commit-bot: I haz the power, 2017-02-06 23:38:24]

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

[commit-bot: I haz the power, 2017-02-06 23:38:25]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Tom (Use chromium acct), 2017-02-07 00:45:24]

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right):

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:222: void
set_title(views::Label* label) { title_ = label; }
On 2017/02/06 22:56:41, sky wrote:
> Document what title and subtitle are for. At first I thought they would be
child
> views, but that isn't the case.

Done.

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:229:
ProfileChooserView* profile_chooser_view_;
On 2017/02/06 22:56:40, sky wrote:
> functions then members.

Done.

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:247: if (was_prelight
&& !is_prelight) {
On 2017/02/06 22:56:40, sky wrote:
> This code is fairly complex and warrants a description.

Done.

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:254:
normal_title_color_ = title->enabled_color();
On 2017/02/06 22:56:40, sky wrote:
> Do you have to keep caching the colors? Can you ask for them once in the
> constructor or the first style changed?

Done.

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.cc:275: state_ = state();
On 2017/02/06 22:56:41, sky wrote:
> You shouldn't have to cache the old state. StateChanged() should be supplied
the
> old state. I realize adding a ButtonState argument is a bigger change, you
> should do it separately then return to this patch.

Done.

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/profile_chooser_view.h (right):

https://codereview.chromium.org/2624683002/diff/160001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/profile_chooser_view.h:66: Browser* browser()
const { return browser_; }
On 2017/02/06 22:56:41, sky wrote:
> this should either return a const Browser* or not be const. Otherwise you're
> losing const'ness.

Done.

[sky, 2017-02-07 00:58:24]

LGTM

[Tom (Use chromium acct), 2017-02-07 01:02:37]

The CQ bit was checked by thomasanderson@google.com

[commit-bot: I haz the power, 2017-02-07 01:03:42]

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

[commit-bot: I haz the power, 2017-02-07 01:03:44]

This CL has an open dependency (Issue 2680643002 Patch 20001). Please resolve
the dependency and try again.
If you are sure that there is no real dependency, please use one of the options
listed in https://goo.gl/9Es4OR to land the CL.

[Tom (Use chromium acct), 2017-02-07 01:15:01]

The CQ bit was checked by thomasanderson@google.com

[commit-bot: I haz the power, 2017-02-07 01:16:30]

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, 2017-02-07 01:58:45]

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

[commit-bot: I haz the power, 2017-02-07 01:58:46]

Try jobs failed on following builders:
  win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Tom (Use chromium acct), 2017-02-07 02:50:43]

The CQ bit was checked by thomasanderson@google.com to run a CQ dry run

[commit-bot: I haz the power, 2017-02-07 02:51:07]

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, 2017-02-07 04:22:42]

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

[commit-bot: I haz the power, 2017-02-07 04:22:43]

Dry run: This issue passed the CQ dry run.

[Tom (Use chromium acct), 2017-02-07 04:24:39]

The CQ bit was checked by thomasanderson@google.com

[Tom (Use chromium acct), 2017-02-07 04:24:40]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/2624683002/#ps200001
(title: "fix win build")

[commit-bot: I haz the power, 2017-02-07 04:24:58]

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, 2017-02-07 04:29:30]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1486441479854060,
"parent_rev": "ecd303a7ad5531522b00dcb6268b769717c14952", "commit_rev":
"f0de79a0e44c878b66df8012e77ff38896fe8faf"}

[commit-bot: I haz the power, 2017-02-07 04:30:27]

Description was changed from

==========
Linux UI: Fix profile chooser button background color

Previously, the profile chooser hovered background color was hardcoded to
0xEAEAEA.  This is about the same color as dark themes will use for their label
text, making the hovered text unreadable.  This CL changes the color to use the
menu item hovered color instead.  Images of the change at:
https://bugs.chromium.org/p/chromium/issues/detail?id=677286#c7

BUG=677286
R=sky@chromium.org
==========

to

==========
Linux UI: Fix profile chooser button background color

Previously, the profile chooser hovered background color was hardcoded to
0xEAEAEA.  This is about the same color as dark themes will use for their label
text, making the hovered text unreadable.  This CL changes the color to use the
menu item hovered color instead.  Images of the change at:
https://bugs.chromium.org/p/chromium/issues/detail?id=677286#c7

BUG=677286
R=sky@chromium.org

Review-Url: https://codereview.chromium.org/2624683002
Cr-Commit-Position: refs/heads/master@{#448542}
Committed:
https://chromium.googlesource.com/chromium/src/+/f0de79a0e44c878b66df8012e77f...
==========

[commit-bot: I haz the power, 2017-02-07 04:30:28]

Committed patchset #5 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/f0de79a0e44c878b66df8012e77f...