Extracting more than one wallpaper prominent color

Extracting more than one wallpaper prominent color

Changes:
This is basically a refactor of code to support more than one wallpaper
prominent color extracting.
(1) group LumaRange and SaturationRange to ColorProfile, and a vector
of color_profiles are passed as parameter for calculation.
(2) updated UMA for Ash.Wallpaper.ColorExtractionResult to enumeration.

R=xdai@chromium.org, bruthig@chromium.org, estade@chromium.org,
danakj@chromium.org, isherman@chromium.org, jamescook@chromium.org
BUG=734761
TEST=covered by tests

Review-Url: https://codereview.chromium.org/2943333003
Cr-Commit-Position: refs/heads/master@{#481667}
Committed: https://chromium.googlesource.com/chromium/src/+/31e3bc75709ac8c54da431bee36fcec5b4d432c5

[Qiang(Joe) Xu, 2017-06-19 22:51:30]

Hi all, would you mind take a look at this CL first? I would also like to know
your opinions about user metrics. Thanks!

https://codereview.chromium.org/2943333003/diff/1/components/wallpaper/wallpa...
File components/wallpaper/wallpaper_color_calculator.cc (right):

https://codereview.chromium.org/2943333003/diff/1/components/wallpaper/wallpa...
components/wallpaper/wallpaper_color_calculator.cc:39: // TODO(warx): reconsider
these UMAs for multiple colors calculation.
This is a workaround to please the wallpaper_color_calculator_unittest

[bruthig, 2017-06-20 15:32:14]

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

[bruthig, 2017-06-20 15:32:15]

+estade@ for guidance on color_analysis.*

I've posted my suggestions for UMA on the crbug Issue.

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

https://codereview.chromium.org/2943333003/diff/1/ui/gfx/color_analysis.cc#ne...
ui/gfx/color_analysis.cc:506: return CalculateProminentColor(bitmap,
lower_bound, upper_bound, goal);
estade@ will be able to better advise but IIUC CalculateProminentColor() is the
expensive part of the algorithm and it would be much much cheaper to only
process the image once and pull out the colors for the color profiles.  Where-as
this appears to be processing the image once per color profile which will be
unacceptably slow.

[Evan Stade, 2017-06-20 17:11:08]

https://codereview.chromium.org/2943333003/diff/1/ash/shelf/shelf_background_...
File ash/shelf/shelf_background_animator.cc (right):

https://codereview.chromium.org/2943333003/diff/1/ash/shelf/shelf_background_...
ash/shelf/shelf_background_animator.cc:219:
wallpaper_controller_->GetShelfColorProfileIndex())
seems like the shelf color profile index should live in this file rather than
WallpaperController

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

https://codereview.chromium.org/2943333003/diff/1/ui/gfx/color_analysis.cc#ne...
ui/gfx/color_analysis.cc:506: return CalculateProminentColor(bitmap,
lower_bound, upper_bound, goal);
On 2017/06/20 15:32:14, bruthig wrote:
> estade@ will be able to better advise but IIUC CalculateProminentColor() is
the
> expensive part of the algorithm and it would be much much cheaper to only
> process the image once and pull out the colors for the color profiles. 
Where-as
> this appears to be processing the image once per color profile which will be
> unacceptably slow.

yes, as explained in the email, we only want to repeat the last loop in
CalculateProminentColor (the one over box_colors). The rest of that function is
expensive and need not be repeated.

[Qiang(Joe) Xu, 2017-06-20 18:38:21]

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

[commit-bot: I haz the power, 2017-06-20 18:38:58]

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-06-20 20:27:00]

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

[commit-bot: I haz the power, 2017-06-20 20:27:01]

Dry run: This issue passed the CQ dry run.

[Qiang(Joe) Xu, 2017-06-20 22:37:53]

Description was changed from

==========
Extracting more than one wallpaper prominent color

Changes:
This is basically a refactor of code to support more than one wallpaper
prominent color extracting.

TODO: dealing with metrics:
Ash.Wallpaper.ColorExtractionResult
Ash.Wallpaper.ColorExtraction.Durations
Ash.Wallpaper.ColorExtraction.UserDelay

R=xdai@chromium.org, bruthig@chromium.org
CC=estade@chromium.org
BUG=734761
TEST=covered by tests
==========

to

==========
Extracting more than one wallpaper prominent color

Changes:
This is basically a refactor of code to support more than one wallpaper
prominent color extracting.

TODO: dealing with metrics:
Ash.Wallpaper.ColorExtractionResult
Ash.Wallpaper.ColorExtraction.Durations
Ash.Wallpaper.ColorExtraction.UserDelay

R=xdai@chromium.org, bruthig@chromium.org, estade@chromium.org,
BUG=734761
TEST=covered by tests
==========

[Qiang(Joe) Xu, 2017-06-20 22:40:07]

Description was changed from

==========
Extracting more than one wallpaper prominent color

Changes:
This is basically a refactor of code to support more than one wallpaper
prominent color extracting.

TODO: dealing with metrics:
Ash.Wallpaper.ColorExtractionResult
Ash.Wallpaper.ColorExtraction.Durations
Ash.Wallpaper.ColorExtraction.UserDelay

R=xdai@chromium.org, bruthig@chromium.org, estade@chromium.org,
BUG=734761
TEST=covered by tests
==========

to

==========
Extracting more than one wallpaper prominent color

Changes:
This is basically a refactor of code to support more than one wallpaper
prominent color extracting.
(1) group LumaRange and SaturationRange to ColorProfile, and a vector
of color_profiles are passed as parameter for calculation.
(2) updated UMA for Ash.Wallpaper.ColorExtractionResult to enumeration.

R=xdai@chromium.org, bruthig@chromium.org, estade@chromium.org,
BUG=734761
TEST=covered by tests
==========

[Qiang(Joe) Xu, 2017-06-20 22:44:21]

Description was changed from

==========
Extracting more than one wallpaper prominent color

Changes:
This is basically a refactor of code to support more than one wallpaper
prominent color extracting.
(1) group LumaRange and SaturationRange to ColorProfile, and a vector
of color_profiles are passed as parameter for calculation.
(2) updated UMA for Ash.Wallpaper.ColorExtractionResult to enumeration.

R=xdai@chromium.org, bruthig@chromium.org, estade@chromium.org,
BUG=734761
TEST=covered by tests
==========

to

==========
Extracting more than one wallpaper prominent color

Changes:
This is basically a refactor of code to support more than one wallpaper
prominent color extracting.
(1) group LumaRange and SaturationRange to ColorProfile, and a vector
of color_profiles are passed as parameter for calculation.
(2) updated UMA for Ash.Wallpaper.ColorExtractionResult to enumeration.

R=xdai@chromium.org, bruthig@chromium.org, estade@chromium.org,
danakj@chromium.org, isherman@chromium.org 
BUG=734761
TEST=covered by tests
==========

[Qiang(Joe) Xu, 2017-06-20 22:44:21]

warx@chromium.org changed reviewers:
+ danakj@chromium.org, isherman@chromium.org

[Qiang(Joe) Xu, 2017-06-20 22:45:34]

Description was changed from

==========
Extracting more than one wallpaper prominent color

Changes:
This is basically a refactor of code to support more than one wallpaper
prominent color extracting.
(1) group LumaRange and SaturationRange to ColorProfile, and a vector
of color_profiles are passed as parameter for calculation.
(2) updated UMA for Ash.Wallpaper.ColorExtractionResult to enumeration.

R=xdai@chromium.org, bruthig@chromium.org, estade@chromium.org,
danakj@chromium.org, isherman@chromium.org 
BUG=734761
TEST=covered by tests
==========

to

==========
Extracting more than one wallpaper prominent color

Changes:
This is basically a refactor of code to support more than one wallpaper
prominent color extracting.
(1) group LumaRange and SaturationRange to ColorProfile, and a vector
of color_profiles are passed as parameter for calculation.
(2) updated UMA for Ash.Wallpaper.ColorExtractionResult to enumeration.

R=xdai@chromium.org, bruthig@chromium.org, estade@chromium.org,
danakj@chromium.org, isherman@chromium.org, jamescook@chromium.org
BUG=734761
TEST=covered by tests
==========

[Qiang(Joe) Xu, 2017-06-20 22:46:03]

warx@chromium.org changed reviewers:
+ jamescook@chromium.org

[Qiang(Joe) Xu, 2017-06-20 22:48:55]

Thanks, I think it is ready for a full review.

xdai, bruthig for wallpaper/ review
estade, danakj for ui/gfx review
jamescook for ash/ review
isherman for tools/metrics/ review

Thanks

https://codereview.chromium.org/2943333003/diff/1/ash/shelf/shelf_background_...
File ash/shelf/shelf_background_animator.cc (right):

https://codereview.chromium.org/2943333003/diff/1/ash/shelf/shelf_background_...
ash/shelf/shelf_background_animator.cc:219:
wallpaper_controller_->GetShelfColorProfileIndex())
On 2017/06/20 17:11:08, Evan Stade wrote:
> seems like the shelf color profile index should live in this file rather than
> WallpaperController

Done.

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

https://codereview.chromium.org/2943333003/diff/1/ui/gfx/color_analysis.cc#ne...
ui/gfx/color_analysis.cc:506: return CalculateProminentColor(bitmap,
lower_bound, upper_bound, goal);
On 2017/06/20 17:11:08, Evan Stade wrote:
> On 2017/06/20 15:32:14, bruthig wrote:
> > estade@ will be able to better advise but IIUC CalculateProminentColor() is
> the
> > expensive part of the algorithm and it would be much much cheaper to only
> > process the image once and pull out the colors for the color profiles. 
> Where-as
> > this appears to be processing the image once per color profile which will be
> > unacceptably slow.
> 
> yes, as explained in the email, we only want to repeat the last loop in
> CalculateProminentColor (the one over box_colors). The rest of that function
is
> expensive and need not be repeated.

done, thanks for explaining!

[James Cook, 2017-06-20 22:53:37]

What is the performance of the new color extraction code relative to the old
code?

[Qiang(Joe) Xu, 2017-06-20 22:58:01]

On 2017/06/20 22:53:37, James Cook wrote:
> What is the performance of the new color extraction code relative to the old
> code?

I updated the crbug. We have email discussion. Evan Stade: the performance
impact would be immeasurably small for calculating six colors in one time.

[Ilya Sherman, 2017-06-20 23:00:00]

https://codereview.chromium.org/2943333003/diff/60001/components/wallpaper/wa...
File components/wallpaper/wallpaper_color_calculator.cc (right):

https://codereview.chromium.org/2943333003/diff/60001/components/wallpaper/wa...
components/wallpaper/wallpaper_color_calculator.cc:92:
NUM_COLOR_EXTRACTION_RESULTS);
Please move the histogram macro invocation out of the switch-stmt; just set a
result value in the switch.  That both makes for shorter, easier-to-read code;
and it also helps the compiler avoid duplicating the work that the macro does.

https://codereview.chromium.org/2943333003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2943333003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:37660: +  <int value="0"
label="RESULT_DARK_VIBRANT_TRANSPARENT"/>
nit: I'd write this in the more human-friendly form of "Dark, vibrant,
transparent"; but if you really prefer to have the labels match the enum values,
that's ok too.

https://codereview.chromium.org/2943333003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:37672: +  <int value="12"
label="NUM_COLOR_EXTRACTION_RESULTS"/>
nit: No need to list this, since it won't be recorded.

https://codereview.chromium.org/2943333003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2943333003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:2548: +   
enum="WallpaperColorExtractionResult">
Hmm, none of the listed enum options look like a failure.  Since you're changing
the semantics of this histogram, please rename it as well, and mark the old one
as <obsolete>.

[Evan Stade, 2017-06-20 23:07:53]

lgtm with nits

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis.cc
File ui/gfx/color_analysis.cc (right):

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis.c...
ui/gfx/color_analysis.cc:399: return std::vector<SkColor>(goals.size(),
SK_ColorTRANSPARENT);
nit: maybe put the best_colors initialization above this line and return that
here (without having added any colors to it)

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis.c...
ui/gfx/color_analysis.cc:439: for (size_t i = 0; i < goals.size(); ++i) {
nit: I think this loop would be cleaner (and you'd head off the possibility of
weird states like the vectors being different sizes) if you passed in a vector
of structs instead of 3 vectors of HSLs.

[danakj, 2017-06-20 23:22:03]

LGTM % estade

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis.cc
File ui/gfx/color_analysis.cc (right):

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis.c...
ui/gfx/color_analysis.cc:737: std::vector<HSL> lower_bounds(size, {-1});
3 mallocs per call to this function is not awesome. estade's suggestion of a
vector of structs would reduce this to 1 malloc.

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis.h
File ui/gfx/color_analysis.h (right):

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis.h...
ui/gfx/color_analysis.h:133: using ColorProfiles = std::vector<ColorProfile>;
It is not too onerous to write out the full type, and it would help readers know
what it is without indirection, so I'd prefer no type alias.

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis_u...
File ui/gfx/color_analysis_unittest.cc (right):

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis_u...
ui/gfx/color_analysis_unittest.cc:507: color_profiles.push_back(ColorProfile(l,
s));
nit: .emplace_back(l, s)

[Qiang(Joe) Xu, 2017-06-21 01:10:53]

feedback from ps4 is addressed, PTAL thanks

https://codereview.chromium.org/2943333003/diff/60001/components/wallpaper/wa...
File components/wallpaper/wallpaper_color_calculator.cc (right):

https://codereview.chromium.org/2943333003/diff/60001/components/wallpaper/wa...
components/wallpaper/wallpaper_color_calculator.cc:92:
NUM_COLOR_EXTRACTION_RESULTS);
On 2017/06/20 23:00:00, Ilya Sherman wrote:
> Please move the histogram macro invocation out of the switch-stmt; just set a
> result value in the switch.  That both makes for shorter, easier-to-read code;
> and it also helps the compiler avoid duplicating the work that the macro does.

Done.

https://codereview.chromium.org/2943333003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/enums.xml (right):

https://codereview.chromium.org/2943333003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:37660: +  <int value="0"
label="RESULT_DARK_VIBRANT_TRANSPARENT"/>
On 2017/06/20 23:00:00, Ilya Sherman wrote:
> nit: I'd write this in the more human-friendly form of "Dark, vibrant,
> transparent"; but if you really prefer to have the labels match the enum
values,
> that's ok too.

I added comments to wallpaper_color_extraction_result.h. So I keep the format
here.

https://codereview.chromium.org/2943333003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/enums.xml:37672: +  <int value="12"
label="NUM_COLOR_EXTRACTION_RESULTS"/>
On 2017/06/20 23:00:00, Ilya Sherman wrote:
> nit: No need to list this, since it won't be recorded.

Done.

https://codereview.chromium.org/2943333003/diff/60001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2943333003/diff/60001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:2548: +   
enum="WallpaperColorExtractionResult">
On 2017/06/20 23:00:00, Ilya Sherman wrote:
> Hmm, none of the listed enum options look like a failure.  Since you're
changing
> the semantics of this histogram, please rename it as well, and mark the old
one
> as <obsolete>.

Done.

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis.cc
File ui/gfx/color_analysis.cc (right):

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis.c...
ui/gfx/color_analysis.cc:399: return std::vector<SkColor>(goals.size(),
SK_ColorTRANSPARENT);
On 2017/06/20 23:07:53, Evan Stade wrote:
> nit: maybe put the best_colors initialization above this line and return that
> here (without having added any colors to it)

Done.

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis.c...
ui/gfx/color_analysis.cc:439: for (size_t i = 0; i < goals.size(); ++i) {
On 2017/06/20 23:07:53, Evan Stade wrote:
> nit: I think this loop would be cleaner (and you'd head off the possibility of
> weird states like the vectors being different sizes) if you passed in a vector
> of structs instead of 3 vectors of HSLs.

Done.

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis.c...
ui/gfx/color_analysis.cc:737: std::vector<HSL> lower_bounds(size, {-1});
On 2017/06/20 23:22:03, danakj wrote:
> 3 mallocs per call to this function is not awesome. estade's suggestion of a
> vector of structs would reduce this to 1 malloc.

Done.

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis.h
File ui/gfx/color_analysis.h (right):

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis.h...
ui/gfx/color_analysis.h:133: using ColorProfiles = std::vector<ColorProfile>;
On 2017/06/20 23:22:03, danakj wrote:
> It is not too onerous to write out the full type, and it would help readers
know
> what it is without indirection, so I'd prefer no type alias.

Done.

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis_u...
File ui/gfx/color_analysis_unittest.cc (right):

https://codereview.chromium.org/2943333003/diff/60001/ui/gfx/color_analysis_u...
ui/gfx/color_analysis_unittest.cc:507: color_profiles.push_back(ColorProfile(l,
s));
On 2017/06/20 23:22:03, danakj wrote:
> nit: .emplace_back(l, s)

Done.

[bruthig, 2017-06-21 15:54:17]

https://codereview.chromium.org/2943333003/diff/80001/ash/wallpaper/wallpaper...
File ash/wallpaper/wallpaper_controller.cc (right):

https://codereview.chromium.org/2943333003/diff/80001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.cc:420: !image.isNull();
nitty-nit-nit: This could return false if color_profiles_.empty().

https://codereview.chromium.org/2943333003/diff/80001/ash/wallpaper/wallpaper...
File ash/wallpaper/wallpaper_controller.h (right):

https://codereview.chromium.org/2943333003/diff/80001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.h:88: WallpaperController::ColorProfileIndex
GetColorProfileIndex(
nit:Do WallpaperController clients really need to know about an index and that
the prominent colors are stored in a vector?  Forcing clients to index into a
vector doesn't seem very user friendly to me.
Consider replacing with:

SkColor GetProminentColor(color_utils::ColorProfile) const;

(Sorry I didn't comment on this in earlier patches)

https://codereview.chromium.org/2943333003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/2943333003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:951: SkColor
color = color_calculator_->prominent_colors()[0];
'0' seems like a magic number here.

https://codereview.chromium.org/2943333003/diff/80001/ui/gfx/color_analysis.cc
File ui/gfx/color_analysis.cc (right):

https://codereview.chromium.org/2943333003/diff/80001/ui/gfx/color_analysis.c...
ui/gfx/color_analysis.cc:741: if (bitmap.empty() || bitmap.isNull())
This should probably return early if color_profiles.empty() as well.

[xdai1, 2017-06-21 16:29:30]

lgtm with nits

https://codereview.chromium.org/2943333003/diff/80001/ash/wallpaper/wallpaper...
File ash/wallpaper/wallpaper_controller.h (right):

https://codereview.chromium.org/2943333003/diff/80001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.h:72: static const SkColor kInvalidColor;
I think we prefer constexpr now?

And for static const integral or enum type, it can be declared and defined in
the class.

https://codereview.chromium.org/2943333003/diff/80001/components/wallpaper/wa...
File components/wallpaper/wallpaper_color_extraction_result.h (right):

https://codereview.chromium.org/2943333003/diff/80001/components/wallpaper/wa...
components/wallpaper/wallpaper_color_extraction_result.h:12: enum
WallpaperColorExtractionResult {
I think we prefer enum class?

[Ilya Sherman, 2017-06-21 17:07:52]

Metrics LGTM % comments:

https://codereview.chromium.org/2943333003/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2943333003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:2559: +<histogram
name="Ash.Wallpaper.ColorExtractionResultEnum"
Optional nit: I'd name this ColorExtractionResult2 instead of "Enum", but up to
you and the other metric owners.

https://codereview.chromium.org/2943333003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:2566: +    color.
Is this description still correct?  It doesn't seem like the enum values measure
success; at least, if they do, it's not clear what values are success vs.
failure.

[James Cook, 2017-06-21 18:05:08]

ash LGTM but please resolve xdai and bruthig comments before landing.

[Qiang(Joe) Xu, 2017-06-21 22:35:27]

feedback from ps5 is addressed.

xdai/bruthig/isherman, you might want to take another look.

Thanks!

https://codereview.chromium.org/2943333003/diff/80001/ash/wallpaper/wallpaper...
File ash/wallpaper/wallpaper_controller.cc (right):

https://codereview.chromium.org/2943333003/diff/80001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.cc:420: !image.isNull();
On 2017/06/21 15:54:17, bruthig wrote:
> nitty-nit-nit: This could return false if color_profiles_.empty().

Done.

https://codereview.chromium.org/2943333003/diff/80001/ash/wallpaper/wallpaper...
File ash/wallpaper/wallpaper_controller.h (right):

https://codereview.chromium.org/2943333003/diff/80001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.h:72: static const SkColor kInvalidColor;
On 2017/06/21 16:29:30, xdai1 wrote:
> I think we prefer constexpr now?
> 
> And for static const integral or enum type, it can be declared and defined in
> the class.

as discussed offline with xiyuan, constexpr by default doesn't generate storage.
But vector constructor I am using is taking reference:

explicit vector (size_type n, const value_type& val = value_type(),
                 const allocator_type& alloc = allocator_type());

So no change here.

https://codereview.chromium.org/2943333003/diff/80001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.h:88: WallpaperController::ColorProfileIndex
GetColorProfileIndex(
On 2017/06/21 15:54:17, bruthig wrote:
> nit:Do WallpaperController clients really need to know about an index and that
> the prominent colors are stored in a vector?  Forcing clients to index into a
> vector doesn't seem very user friendly to me.
> Consider replacing with:
> 
> SkColor GetProminentColor(color_utils::ColorProfile) const;
> 
> (Sorry I didn't comment on this in earlier patches)

Done.

https://codereview.chromium.org/2943333003/diff/80001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc (right):

https://codereview.chromium.org/2943333003/diff/80001/chrome/browser/chromeos...
chrome/browser/chromeos/login/users/wallpaper/wallpaper_manager.cc:951: SkColor
color = color_calculator_->prominent_colors()[0];
On 2017/06/21 15:54:17, bruthig wrote:
> '0' seems like a magic number here.

Done.

https://codereview.chromium.org/2943333003/diff/80001/components/wallpaper/wa...
File components/wallpaper/wallpaper_color_extraction_result.h (right):

https://codereview.chromium.org/2943333003/diff/80001/components/wallpaper/wa...
components/wallpaper/wallpaper_color_extraction_result.h:12: enum
WallpaperColorExtractionResult {
On 2017/06/21 16:29:30, xdai1 wrote:
> I think we prefer enum class?

We might still need enum.
base::Bucket in the test needs integer parameter:
https://cs.chromium.org/chromium/src/base/test/histogram_tester.h?type=cs&q=b...

https://codereview.chromium.org/2943333003/diff/80001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2943333003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:2559: +<histogram
name="Ash.Wallpaper.ColorExtractionResultEnum"
On 2017/06/21 17:07:51, Ilya Sherman wrote:
> Optional nit: I'd name this ColorExtractionResult2 instead of "Enum", but up
to
> you and the other metric owners.

Done.

https://codereview.chromium.org/2943333003/diff/80001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:2566: +    color.
On 2017/06/21 17:07:51, Ilya Sherman wrote:
> Is this description still correct?  It doesn't seem like the enum values
measure
> success; at least, if they do, it's not clear what values are success vs.
> failure.

I think it is still correct. I changed the label description in enums.xml for
representing success vs failure.

https://codereview.chromium.org/2943333003/diff/80001/ui/gfx/color_analysis.cc
File ui/gfx/color_analysis.cc (right):

https://codereview.chromium.org/2943333003/diff/80001/ui/gfx/color_analysis.c...
ui/gfx/color_analysis.cc:741: if (bitmap.empty() || bitmap.isNull())
On 2017/06/21 15:54:17, bruthig wrote:
> This should probably return early if color_profiles.empty() as well.

Done.

[bruthig, 2017-06-22 15:31:09]

lgtm

https://codereview.chromium.org/2943333003/diff/80001/ash/wallpaper/wallpaper...
File ash/wallpaper/wallpaper_controller.h (right):

https://codereview.chromium.org/2943333003/diff/80001/ash/wallpaper/wallpaper...
ash/wallpaper/wallpaper_controller.h:88: WallpaperController::ColorProfileIndex
GetColorProfileIndex(
On 2017/06/21 22:35:27, Qiang(Joe) Xu wrote:
> On 2017/06/21 15:54:17, bruthig wrote:
> > nit:Do WallpaperController clients really need to know about an index and
that
> > the prominent colors are stored in a vector?  Forcing clients to index into
a
> > vector doesn't seem very user friendly to me.
> > Consider replacing with:
> > 
> > SkColor GetProminentColor(color_utils::ColorProfile) const;
> > 
> > (Sorry I didn't comment on this in earlier patches)
> 
> Done.

Much thanks =D

[xdai1, 2017-06-22 16:50:42]

still lgtm

[Qiang(Joe) Xu, 2017-06-22 17:19:47]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-06-22 17:19:48]

The patchset sent to the CQ was uploaded after l-g-t-m from estade@chromium.org,
danakj@chromium.org, jamescook@chromium.org, isherman@chromium.org,
bruthig@chromium.org, xdai@chromium.org
Link to the patchset: https://codereview.chromium.org/2943333003/#ps120001
(title: "rebase")

[commit-bot: I haz the power, 2017-06-22 17:20:05]

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-06-22 18:12:37]

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

[commit-bot: I haz the power, 2017-06-22 18:12:39]

Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Qiang(Joe) Xu, 2017-06-22 20:18:25]

The CQ bit was checked by warx@chromium.org

[Qiang(Joe) Xu, 2017-06-22 20:18:26]

The patchset sent to the CQ was uploaded after l-g-t-m from
jamescook@chromium.org, bruthig@chromium.org, estade@chromium.org,
danakj@chromium.org, xdai@chromium.org, isherman@chromium.org
Link to the patchset: https://codereview.chromium.org/2943333003/#ps140001
(title: "possible uninitialized local value")

[commit-bot: I haz the power, 2017-06-22 20:18:50]

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-06-22 21:25:02]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1498162705495540,
"parent_rev": "9a264f3ff6535a674c6ef1555952832a6cc2fd21", "commit_rev":
"31e3bc75709ac8c54da431bee36fcec5b4d432c5"}

[commit-bot: I haz the power, 2017-06-22 21:25:17]

Description was changed from

==========
Extracting more than one wallpaper prominent color

Changes:
This is basically a refactor of code to support more than one wallpaper
prominent color extracting.
(1) group LumaRange and SaturationRange to ColorProfile, and a vector
of color_profiles are passed as parameter for calculation.
(2) updated UMA for Ash.Wallpaper.ColorExtractionResult to enumeration.

R=xdai@chromium.org, bruthig@chromium.org, estade@chromium.org,
danakj@chromium.org, isherman@chromium.org, jamescook@chromium.org
BUG=734761
TEST=covered by tests
==========

to

==========
Extracting more than one wallpaper prominent color

Changes:
This is basically a refactor of code to support more than one wallpaper
prominent color extracting.
(1) group LumaRange and SaturationRange to ColorProfile, and a vector
of color_profiles are passed as parameter for calculation.
(2) updated UMA for Ash.Wallpaper.ColorExtractionResult to enumeration.

R=xdai@chromium.org, bruthig@chromium.org, estade@chromium.org,
danakj@chromium.org, isherman@chromium.org, jamescook@chromium.org
BUG=734761
TEST=covered by tests

Review-Url: https://codereview.chromium.org/2943333003
Cr-Commit-Position: refs/heads/master@{#481667}
Committed:
https://chromium.googlesource.com/chromium/src/+/31e3bc75709ac8c54da431bee36f...
==========

[commit-bot: I haz the power, 2017-06-22 21:25:19]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/31e3bc75709ac8c54da431bee36f...