Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
291653004:
Add extra overloads for color analysis. (Closed)
|
Created:
6 years, 7 months ago by calamity Modified:
6 years, 6 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionAdd extra overloads for color analysis.
This CL adds some extra overloads for CalculateKMeanColorOfBitmap and
CalculateKMeanColorOfPNG to provide a more convenient API. More
specifically, it provides both parameterized and unparameterized
versions of both functions.
BUG=376613
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=273102
Patch Set 1 #
Total comments: 4
Patch Set 2 : fix nits #
Total comments: 6
Patch Set 3 : fix nits #
Messages
Total messages: 29 (0 generated)
LGTM with fixing the order of the functions. https://codereview.chromium.org/291653004/diff/1/ui/gfx/color_analysis.h File ui/gfx/color_analysis.h (right): https://codereview.chromium.org/291653004/diff/1/ui/gfx/color_analysis.h#newc... ui/gfx/color_analysis.h:56: // image. nit: Join the next line onto this one. https://codereview.chromium.org/291653004/diff/1/ui/gfx/color_analysis.h#newc... ui/gfx/color_analysis.h:106: CalculateKMeanColorOfPNG(scoped_refptr<base::RefCountedMemory> png); All overloads of a function should appear together. Can you move this function *above* CalculateKMeanColorOfBitmap and duplicate the comments so they can be read individually? (Don't duplicate the giant comment; just copy the opening sentence and refer to the other function.)
+asvitkine for ui/gfx/ OWNERS. This is a precursor for https://codereview.chromium.org/289283004/ . https://codereview.chromium.org/291653004/diff/1/ui/gfx/color_analysis.h File ui/gfx/color_analysis.h (right): https://codereview.chromium.org/291653004/diff/1/ui/gfx/color_analysis.h#newc... ui/gfx/color_analysis.h:56: // image. On 2014/05/22 06:10:42, Matt Giuca wrote: > nit: Join the next line onto this one. Done. https://codereview.chromium.org/291653004/diff/1/ui/gfx/color_analysis.h#newc... ui/gfx/color_analysis.h:106: CalculateKMeanColorOfPNG(scoped_refptr<base::RefCountedMemory> png); On 2014/05/22 06:10:42, Matt Giuca wrote: > All overloads of a function should appear together. Can you move this function > *above* CalculateKMeanColorOfBitmap and duplicate the comments so they can be > read individually? > > (Don't duplicate the giant comment; just copy the opening sentence and refer to > the other function.) Done.
https://codereview.chromium.org/291653004/diff/40001/ui/gfx/color_analysis.h File ui/gfx/color_analysis.h (right): https://codereview.chromium.org/291653004/diff/40001/ui/gfx/color_analysis.h#... ui/gfx/color_analysis.h:58: // |png/bitmap| represents the data of a png/bitmap encoded image. |png|/|bitmap| (The || delimits specific identifiers.)
LGTM % nits https://codereview.chromium.org/291653004/diff/40001/ui/gfx/color_analysis.cc File ui/gfx/color_analysis.cc (right): https://codereview.chromium.org/291653004/diff/40001/ui/gfx/color_analysis.cc... ui/gfx/color_analysis.cc:402: CalculateKMeanColorOfBuffer(reinterpret_cast<uint8_t*>(image.get()), Nit: Just return it directly. https://codereview.chromium.org/291653004/diff/40001/ui/gfx/color_analysis.h File ui/gfx/color_analysis.h (right): https://codereview.chromium.org/291653004/diff/40001/ui/gfx/color_analysis.h#... ui/gfx/color_analysis.h:100: CalculateKMeanColorOfPNG(scoped_refptr<base::RefCountedMemory> png); Nit: Wrap after the ( instead.
https://codereview.chromium.org/291653004/diff/40001/ui/gfx/color_analysis.cc File ui/gfx/color_analysis.cc (right): https://codereview.chromium.org/291653004/diff/40001/ui/gfx/color_analysis.cc... ui/gfx/color_analysis.cc:402: CalculateKMeanColorOfBuffer(reinterpret_cast<uint8_t*>(image.get()), On 2014/05/26 18:20:49, Alexei Svitkine wrote: > Nit: Just return it directly. Done. https://codereview.chromium.org/291653004/diff/40001/ui/gfx/color_analysis.h File ui/gfx/color_analysis.h (right): https://codereview.chromium.org/291653004/diff/40001/ui/gfx/color_analysis.h#... ui/gfx/color_analysis.h:58: // |png/bitmap| represents the data of a png/bitmap encoded image. On 2014/05/23 03:20:14, Matt Giuca wrote: > |png|/|bitmap| > > (The || delimits specific identifiers.) Done. https://codereview.chromium.org/291653004/diff/40001/ui/gfx/color_analysis.h#... ui/gfx/color_analysis.h:100: CalculateKMeanColorOfPNG(scoped_refptr<base::RefCountedMemory> png); On 2014/05/26 18:20:49, Alexei Svitkine wrote: > Nit: Wrap after the ( instead. Done.
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/291653004/80001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...) android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/291653004/80001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/291653004/80001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/291653004/80001
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_aosp on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_aosp/builds/7...)
The CQ bit was checked by calamity@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/calamity@chromium.org/291653004/80001
Message was sent while issue was closed.
Change committed as 273102 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
