color_utils cleanup:

color_utils cleanup:

* Attempt to define and standardize usage of "luma" vs. "(relative) luminance" vs. "lightness", at least as far as color_utils functions, comments, and direct callers are concerned.  Especially the first two terms are often both called "luminance" (universally, not just in Chrome code), so the distinction is confusing.
* Misc. cleanups to make caller code shorter or clearer.
* Naming consistency: remove "Get" from function names where we're not doing some simple "getter" operation, but rather computing a value.
* Modify some caller behaviors to be hopefully-more-correct
* Note that GetReadableColor() can accept values with alpha, it will just ignore the alpha.
* Add a PickContrastingColor() function, which is basically a subset of GetReadableColor().  This will be useful for an upcoming change where we want to choose between two fixed colors instead of luma-inverting a single input.
* Use more precise multipliers in Luma().

BUG=none
TEST=none

Committed: https://crrev.com/6a3e439037809fd5dccf11c4cd0a91fb6a637702
Cr-Commit-Position: refs/heads/master@{#379643}

[Peter Kasting, 2016-03-04 01:35:16]

Description was changed from

==========
color_utils cleanup

BUG=none
TEST=none
==========

to

==========
color_utils cleanup:

* Attempt to define and standardize usage of "luma" vs. "(relative) luminance"
vs. "lightness", at least as far as color_utils functions, comments, and direct
callers are concerned.  Especially the first two terms are often both called
"luminance" (universally, not just in Chrome code), so the distinction is
confusing.
* Misc. cleanups to make caller code shorter or clearer.
* Naming consistency: remove "Get" from function names where we're not doing
some simple "getter" operation, but rather computing a value.
* Modify some caller behaviors to be hopefully-more-correct
* Note that GetReadableColor() can accept values with alpha, it will just ignore
the alpha.
* Add a PickContrastingColor() function, which is basically a subset of
GetReadableColor().  This will be useful for an upcoming change where we want to
choose between two fixed colors instead of luma-inverting a single input.
* Use more precise multipliers in Luma().

BUG=none
TEST=none
==========

[Peter Kasting, 2016-03-04 01:35:16]

pkasting@chromium.org changed reviewers:
+ benwells@chromium.org, danakj@chromium.org, erg@google.com,
pkotwicz@chromium.org

[Peter Kasting, 2016-03-04 01:39:26]

benwells:
apps/ui/views/app_window_frame_view.cc
chrome/browser/extensions/bookmark_app_helper.cc

danakj:
ui/gfx/

erg:
chrome/browser/ui/libgtk2ui/gtk2_ui.cc
mash/wm/frame/default_header_painter.cc

pkotwicz:
ash/frame/default_header_painter.cc
chrome/browser/themes/
components/favicon_base/fallback_icon_style.cc

https://codereview.chromium.org/1761183002/diff/1/ash/frame/default_header_pa...
File ash/frame/default_header_painter.cc (right):

https://codereview.chromium.org/1761183002/diff/1/ash/frame/default_header_pa...
ash/frame/default_header_painter.cc:280: return color_utils::IsDark(mode_ ==
MODE_INACTIVE ? inactive_frame_color_
This is not quite the same as the old code, but almost; it's basically "< 128"
instead of "< 125".  This difference is so small I have a hard time believing it
matters; using the standard function seems better.

https://codereview.chromium.org/1761183002/diff/1/chrome/browser/ui/views/dow...
File chrome/browser/ui/views/download/download_item_view.cc (right):

https://codereview.chromium.org/1761183002/diff/1/chrome/browser/ui/views/dow...
chrome/browser/ui/views/download/download_item_view.cc:703: if
(!color_utils::IsDark(file_name_color)) {
This is not the same as the old code.  IsDark() is based on luma, while
RelativeLuminance() has performed linearization.

These have fairly different results.  For example, when using "grey" colors (R =
G = B), !IsDark() will return true for everything (128, 128, 128) and higher,
while the old code would only return true for colors (188, 188, 188) and higher.

I suspect that the intent of the code was to do what I've changed it to.  We
would normally consider e.g. (160, 160, 160) to be "light" text. 
RelativeLuminance() is mostly of use when computing contrast ratios.

https://codereview.chromium.org/1761183002/diff/1/mash/wm/frame/default_heade...
File mash/wm/frame/default_header_painter.cc (right):

https://codereview.chromium.org/1761183002/diff/1/mash/wm/frame/default_heade...
mash/wm/frame/default_header_painter.cc:270: return color_utils::IsDark(
See comments in the ash/ version of this file

[Peter Kasting, 2016-03-04 01:40:41]

https://codereview.chromium.org/1761183002/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_properties.h (left):

https://codereview.chromium.org/1761183002/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_properties.h:128: COLOR_STATUS_BAR_TEXT,
Ignore this change; this slipped in erroneously and I will remove.

[danakj, 2016-03-04 01:54:49]

LGTM overall

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.cc#newco...
ui/gfx/color_utils.cc:59: return HSLToSkColor(hsl, SkColorGetA(color));
This is a behaviour change, can this be a separate CL? That'll make bugtracking
easier if it shows up.

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.cc#newco...
ui/gfx/color_utils.cc:87: return static_cast<uint8_t>(std::round((0.299 *
SkColorGetR(color)) +
These constants are diff too, similar request for separate CL

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.h
File ui/gfx/color_utils.h (right):

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.h#newcode29
ui/gfx/color_utils.h:29: GFX_EXPORT double ContrastRatio(SkColor color_a,
SkColor color_b);
GetContrastRatio is preferred (verbs not nouns for methods) or Compute or
ContrastRatioFromSkColors or something (bonus points for not overloading).. (see
recent blink-dev bikesheds for what goes wrong with noun methods ^_^)

Same for RelativeLuminance and Luma. These all seem like type constructors at
the callsites.

[benwells, 2016-03-04 02:00:33]

apps/ui/views/app_window_frame_view.cc
chrome/browser/extensions/bookmark_app_helper.cc

lgtm

[Peter Kasting, 2016-03-04 02:10:52]

I'm willing to make these separate changes, but I'd prefer not to unless you
feel strongly.

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.cc#newco...
ui/gfx/color_utils.cc:59: return HSLToSkColor(hsl, SkColorGetA(color));
On 2016/03/04 01:54:49, danakj wrote:
> This is a behaviour change, can this be a separate CL? That'll make
bugtracking
> easier if it shows up.

It's a behavior change, but only for GetReadableColor(), which to my knowledge
isn't currently called with alpha != 255 (and if somehow it is, this should be
more correct).

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.cc#newco...
ui/gfx/color_utils.cc:87: return static_cast<uint8_t>(std::round((0.299 *
SkColorGetR(color)) +
On 2016/03/04 01:54:49, danakj wrote:
> These constants are diff too, similar request for separate CL

It's true, this is a behavior change.  That said, it's extremely minor; it's
hard to imagine people noticing any significant effect of this.

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.h
File ui/gfx/color_utils.h (right):

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.h#newcode29
ui/gfx/color_utils.h:29: GFX_EXPORT double ContrastRatio(SkColor color_a,
SkColor color_b);
On 2016/03/04 01:54:49, danakj wrote:
> GetContrastRatio is preferred (verbs not nouns for methods) or Compute or
> ContrastRatioFromSkColors or something (bonus points for not overloading)..
(see
> recent blink-dev bikesheds for what goes wrong with noun methods ^_^)

That may be true in Blink, but in Chromium Get...() is normally discouraged
unless this is some kind of an accessor, from what I've seen.  (The Chromium and
Google style guides are both silent on this issue.)

> Same for RelativeLuminance and Luma. These all seem like type constructors at
> the callsites.

I don't know what you mean by "type constructors", since there are not distinct
luminance or luma types.

[Elliot Glaysher, 2016-03-04 18:22:50]

erg@chromium.org changed reviewers:
+ erg@chromium.org

[Elliot Glaysher, 2016-03-04 18:22:51]

lgtm

[danakj, 2016-03-04 19:05:55]

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.cc#newco...
ui/gfx/color_utils.cc:59: return HSLToSkColor(hsl, SkColorGetA(color));
On 2016/03/04 02:10:52, Peter Kasting wrote:
> On 2016/03/04 01:54:49, danakj wrote:
> > This is a behaviour change, can this be a separate CL? That'll make
> bugtracking
> > easier if it shows up.
> 
> It's a behavior change, but only for GetReadableColor(), which to my knowledge
> isn't currently called with alpha != 255 (and if somehow it is, this should be
> more correct).

Ah, ok then.

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.cc#newco...
ui/gfx/color_utils.cc:87: return static_cast<uint8_t>(std::round((0.299 *
SkColorGetR(color)) +
On 2016/03/04 02:10:52, Peter Kasting wrote:
> On 2016/03/04 01:54:49, danakj wrote:
> > These constants are diff too, similar request for separate CL
> 
> It's true, this is a behavior change.  That said, it's extremely minor; it's
> hard to imagine people noticing any significant effect of this.

Yeh, it's just good practice IMO. Like maybe they require rebaselines, or
something. You never know what weird effect something can have and burying it in
a big CL of refactoring is usually not a good plan.

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.h
File ui/gfx/color_utils.h (right):

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.h#newcode29
ui/gfx/color_utils.h:29: GFX_EXPORT double ContrastRatio(SkColor color_a,
SkColor color_b);
On 2016/03/04 02:10:52, Peter Kasting wrote:
> On 2016/03/04 01:54:49, danakj wrote:
> > GetContrastRatio is preferred (verbs not nouns for methods) or Compute or
> > ContrastRatioFromSkColors or something (bonus points for not overloading)..
> (see
> > recent blink-dev bikesheds for what goes wrong with noun methods ^_^)
> 
> That may be true in Blink, but in Chromium Get...() is normally discouraged
> unless this is some kind of an accessor, from what I've seen.  (The Chromium
and
> Google style guides are both silent on this issue.)

It's the other way. The WebKit style guide says to not use "get" in accessors.
Chromium/google style is to make methods verbs AFAIK. I also tried to find this
but failed. You're right Get is usually an accessor so I also brought some other
suggestions to make them verbs such as Compute or To or From.

> > Same for RelativeLuminance and Luma. These all seem like type constructors
at
> > the callsites.
> 
> I don't know what you mean by "type constructors", since there are not
distinct
> luminance or luma types.

Nouns are types, so Noun() looks like constructing a type. I realize there are
not such types but the callsites look like there are such types since they are
not a verb saying what the function does.

[Peter Kasting, 2016-03-04 20:28:58]

TLDR: I don't think there's a rule or pattern for verb names over noun ones, but
both are readable and we shouldn't spend more time on this, so I'll change the
names to add Get.

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.cc
File ui/gfx/color_utils.cc (right):

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.cc#newco...
ui/gfx/color_utils.cc:87: return static_cast<uint8_t>(std::round((0.299 *
SkColorGetR(color)) +
On 2016/03/04 19:05:55, danakj wrote:
> On 2016/03/04 02:10:52, Peter Kasting wrote:
> > On 2016/03/04 01:54:49, danakj wrote:
> > > These constants are diff too, similar request for separate CL
> > 
> > It's true, this is a behavior change.  That said, it's extremely minor; it's
> > hard to imagine people noticing any significant effect of this.
> 
> Yeh, it's just good practice IMO. Like maybe they require rebaselines, or
> something. You never know what weird effect something can have and burying it
in
> a big CL of refactoring is usually not a good plan.

I agree in general principle.

In this specific case, all of the places calling this are doing the same check:
"is this light, or dark?" (and then usually drawing black or white atop it). 
The fallout from this is that we might toggle our behavior for colors that
happen to fall almost exactly on the midpoint (because their computed luma value
may change by at most 1).  In such cases, picking either light or dark is about
equally good, so I feel comfortable enough letting this change be part of this
CL.

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.h
File ui/gfx/color_utils.h (right):

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.h#newcode29
ui/gfx/color_utils.h:29: GFX_EXPORT double ContrastRatio(SkColor color_a,
SkColor color_b);
On 2016/03/04 19:05:55, danakj wrote:
> On 2016/03/04 02:10:52, Peter Kasting wrote:
> > On 2016/03/04 01:54:49, danakj wrote:
> > > GetContrastRatio is preferred (verbs not nouns for methods) or Compute or
> > > ContrastRatioFromSkColors or something (bonus points for not
overloading)..
> > (see
> > > recent blink-dev bikesheds for what goes wrong with noun methods ^_^)
> > 
> > That may be true in Blink, but in Chromium Get...() is normally discouraged
> > unless this is some kind of an accessor, from what I've seen.  (The Chromium
> and
> > Google style guides are both silent on this issue.)
> 
> It's the other way. The WebKit style guide says to not use "get" in accessors.
> Chromium/google style is to make methods verbs AFAIK.

I can't find any such text in the Google style guide or Chromium style guide,
and the parts of Chromium I'm familiar with do both; noun function names are
fairly common, for example, from autocomplete_input.h:

FormattedStringWithEquivalentMeaning()
NumNonHostComponents()

Or from tab.h:

IconCapacity();

I mean, it's not a huge deal -- adding Get will not make the function of these
APIs less clear, it's just a bit more verbose -- so since I gather this is the
pattern you want ui/gfx/ to have, I will do that.  I just thought it read a bit
better without it.

> > > Same for RelativeLuminance and Luma. These all seem like type constructors
> at
> > > the callsites.
> > 
> > I don't know what you mean by "type constructors", since there are not
> distinct
> > luminance or luma types.
> 
> Nouns are types, so Noun() looks like constructing a type. I realize there are
> not such types but the callsites look like there are such types since they are
> not a verb saying what the function does.

Ah, I understand.  That actually seems sort of like a pro more than a con -- in
a sense, we are "constructing" the luma or luminance value, so reading like
we're doing so may be good?  Perhaps I am smoking crack.

[danakj, 2016-03-04 21:10:00]

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.h
File ui/gfx/color_utils.h (right):

https://codereview.chromium.org/1761183002/diff/1/ui/gfx/color_utils.h#newcode29
ui/gfx/color_utils.h:29: GFX_EXPORT double ContrastRatio(SkColor color_a,
SkColor color_b);
On 2016/03/04 20:28:57, Peter Kasting wrote:
> On 2016/03/04 19:05:55, danakj wrote:
> > On 2016/03/04 02:10:52, Peter Kasting wrote:
> > > On 2016/03/04 01:54:49, danakj wrote:
> > > > GetContrastRatio is preferred (verbs not nouns for methods) or Compute
or
> > > > ContrastRatioFromSkColors or something (bonus points for not
> overloading)..
> > > (see
> > > > recent blink-dev bikesheds for what goes wrong with noun methods ^_^)
> > > 
> > > That may be true in Blink, but in Chromium Get...() is normally
discouraged
> > > unless this is some kind of an accessor, from what I've seen.  (The
Chromium
> > and
> > > Google style guides are both silent on this issue.)
> > 
> > It's the other way. The WebKit style guide says to not use "get" in
accessors.
> > Chromium/google style is to make methods verbs AFAIK.
> 
> I can't find any such text in the Google style guide or Chromium style guide,
> and the parts of Chromium I'm familiar with do both; noun function names are
> fairly common, for example, from autocomplete_input.h:
> 
> FormattedStringWithEquivalentMeaning()
> NumNonHostComponents()
> 
> Or from tab.h:
> 
> IconCapacity();
> 
> I mean, it's not a huge deal -- adding Get will not make the function of these
> APIs less clear, it's just a bit more verbose -- so since I gather this is the
> pattern you want ui/gfx/ to have, I will do that.  I just thought it read a
bit
> better without it.
> 
> > > > Same for RelativeLuminance and Luma. These all seem like type
constructors
> > at
> > > > the callsites.
> > > 
> > > I don't know what you mean by "type constructors", since there are not
> > distinct
> > > luminance or luma types.
> > 
> > Nouns are types, so Noun() looks like constructing a type. I realize there
are
> > not such types but the callsites look like there are such types since they
are
> > not a verb saying what the function does.
> 
> Ah, I understand.  That actually seems sort of like a pro more than a con --
in
> a sense, we are "constructing" the luma or luminance value, so reading like
> we're doing so may be good?  Perhaps I am smoking crack.

I'm probably being over sensitive to this cuz all I do is rename things in Blink
right now :/

[pkotwicz, 2016-03-07 14:29:31]

chrome/browser/themes LGTM

[Peter Kasting, 2016-03-07 19:35:46]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2016-03-07 19:35:47]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
benwells@chromium.org, erg@chromium.org
Link to the patchset: https://codereview.chromium.org/1761183002/#ps40001
(title: "Rename functions")

[commit-bot: I haz the power, 2016-03-07 19:36:14]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1761183002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1761183002/40001

[commit-bot: I haz the power, 2016-03-07 21:13:31]

Description was changed from

==========
color_utils cleanup:

* Attempt to define and standardize usage of "luma" vs. "(relative) luminance"
vs. "lightness", at least as far as color_utils functions, comments, and direct
callers are concerned.  Especially the first two terms are often both called
"luminance" (universally, not just in Chrome code), so the distinction is
confusing.
* Misc. cleanups to make caller code shorter or clearer.
* Naming consistency: remove "Get" from function names where we're not doing
some simple "getter" operation, but rather computing a value.
* Modify some caller behaviors to be hopefully-more-correct
* Note that GetReadableColor() can accept values with alpha, it will just ignore
the alpha.
* Add a PickContrastingColor() function, which is basically a subset of
GetReadableColor().  This will be useful for an upcoming change where we want to
choose between two fixed colors instead of luma-inverting a single input.
* Use more precise multipliers in Luma().

BUG=none
TEST=none
==========

to

==========
color_utils cleanup:

* Attempt to define and standardize usage of "luma" vs. "(relative) luminance"
vs. "lightness", at least as far as color_utils functions, comments, and direct
callers are concerned.  Especially the first two terms are often both called
"luminance" (universally, not just in Chrome code), so the distinction is
confusing.
* Misc. cleanups to make caller code shorter or clearer.
* Naming consistency: remove "Get" from function names where we're not doing
some simple "getter" operation, but rather computing a value.
* Modify some caller behaviors to be hopefully-more-correct
* Note that GetReadableColor() can accept values with alpha, it will just ignore
the alpha.
* Add a PickContrastingColor() function, which is basically a subset of
GetReadableColor().  This will be useful for an upcoming change where we want to
choose between two fixed colors instead of luma-inverting a single input.
* Use more precise multipliers in Luma().

BUG=none
TEST=none
==========

[commit-bot: I haz the power, 2016-03-07 21:13:32]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-03-07 21:14:45]

Description was changed from

==========
color_utils cleanup:

* Attempt to define and standardize usage of "luma" vs. "(relative) luminance"
vs. "lightness", at least as far as color_utils functions, comments, and direct
callers are concerned.  Especially the first two terms are often both called
"luminance" (universally, not just in Chrome code), so the distinction is
confusing.
* Misc. cleanups to make caller code shorter or clearer.
* Naming consistency: remove "Get" from function names where we're not doing
some simple "getter" operation, but rather computing a value.
* Modify some caller behaviors to be hopefully-more-correct
* Note that GetReadableColor() can accept values with alpha, it will just ignore
the alpha.
* Add a PickContrastingColor() function, which is basically a subset of
GetReadableColor().  This will be useful for an upcoming change where we want to
choose between two fixed colors instead of luma-inverting a single input.
* Use more precise multipliers in Luma().

BUG=none
TEST=none
==========

to

==========
color_utils cleanup:

* Attempt to define and standardize usage of "luma" vs. "(relative) luminance"
vs. "lightness", at least as far as color_utils functions, comments, and direct
callers are concerned.  Especially the first two terms are often both called
"luminance" (universally, not just in Chrome code), so the distinction is
confusing.
* Misc. cleanups to make caller code shorter or clearer.
* Naming consistency: remove "Get" from function names where we're not doing
some simple "getter" operation, but rather computing a value.
* Modify some caller behaviors to be hopefully-more-correct
* Note that GetReadableColor() can accept values with alpha, it will just ignore
the alpha.
* Add a PickContrastingColor() function, which is basically a subset of
GetReadableColor().  This will be useful for an upcoming change where we want to
choose between two fixed colors instead of luma-inverting a single input.
* Use more precise multipliers in Luma().

BUG=none
TEST=none

Committed: https://crrev.com/6a3e439037809fd5dccf11c4cd0a91fb6a637702
Cr-Commit-Position: refs/heads/master@{#379643}
==========

[commit-bot: I haz the power, 2016-03-07 21:14:46]

Patchset 3 (id:??) landed as
https://crrev.com/6a3e439037809fd5dccf11c4cd0a91fb6a637702
Cr-Commit-Position: refs/heads/master@{#379643}