Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
148093013:
[gfx] Add a utility to calculate visible margins of an image. (Closed)
|
Created:
6 years, 10 months ago by Greg Billock Modified:
6 years, 10 months ago Reviewers:
Robert Sesek CC:
chromium-reviews, rsesek+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[gfx] Add a utility to calculate visible margins of an image.
R=rsesek@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=248513
Patch Set 1 #
Total comments: 8
Patch Set 2 : spacing #Patch Set 3 : rebase #
Total comments: 2
Patch Set 4 : naming #Patch Set 5 : adjust const type #
Messages
Total messages: 41 (0 generated)
https://codereview.chromium.org/148093013/diff/1/ui/gfx/image/image_util.cc File ui/gfx/image/image_util.cc (right): https://codereview.chromium.org/148093013/diff/1/ui/gfx/image/image_util.cc#n... ui/gfx/image/image_util.cc:50: nit: spacing around - https://codereview.chromium.org/148093013/diff/1/ui/gfx/image/image_util.cc#n... ui/gfx/image/image_util.cc:69: Where does 12 come from? https://codereview.chromium.org/148093013/diff/1/ui/gfx/image/image_util.cc#n... ui/gfx/image/image_util.cc:79: nit: spacing around -
https://codereview.chromium.org/148093013/diff/1/ui/gfx/image/image_util.cc File ui/gfx/image/image_util.cc (right): https://codereview.chromium.org/148093013/diff/1/ui/gfx/image/image_util.cc#n... ui/gfx/image/image_util.cc:50: On 2014/01/29 17:05:29, rsesek wrote: > nit: spacing around - Done. https://codereview.chromium.org/148093013/diff/1/ui/gfx/image/image_util.cc#n... ui/gfx/image/image_util.cc:69: On 2014/01/29 17:05:29, rsesek wrote: > Where does 12 come from? 5% of 255, basically. It's the value for "transparent" the unit tests use (CheckIsTransparent), so I went with it. There doesn't seem to be a really firm consensus that I can find. Just an "it depends, and human vision is non-linear." For small graphics where this makes sense to call, the gradients should be pretty sharp. https://codereview.chromium.org/148093013/diff/1/ui/gfx/image/image_util.cc#n... ui/gfx/image/image_util.cc:79: On 2014/01/29 17:05:29, rsesek wrote: > nit: spacing around - Done.
Sorry, I obliviously missed the naming nits. https://codereview.chromium.org/148093013/diff/1/ui/gfx/image/image_util.cc File ui/gfx/image/image_util.cc (right): https://codereview.chromium.org/148093013/diff/1/ui/gfx/image/image_util.cc#n... ui/gfx/image/image_util.cc:69: On 2014/01/29 22:03:46, Greg Billock wrote: > On 2014/01/29 17:05:29, rsesek wrote: > > Where does 12 come from? > > 5% of 255, basically. It's the value for "transparent" the unit tests use > (CheckIsTransparent), so I went with it. There doesn't seem to be a really firm > consensus that I can find. Just an "it depends, and human vision is non-linear." > For small graphics where this makes sense to call, the gradients should be > pretty sharp. OK. Maybe make it a const int at the top of the function, or at least leave a comment. https://codereview.chromium.org/148093013/diff/40001/ui/gfx/image/image_util.cc File ui/gfx/image/image_util.cc (right): https://codereview.chromium.org/148093013/diff/40001/ui/gfx/image/image_util.... ui/gfx/image/image_util.cc:66: int innerMin = bitmap.width(); naming: Use under_scores, not camelCase. Here and on line 78.
https://codereview.chromium.org/148093013/diff/1/ui/gfx/image/image_util.cc File ui/gfx/image/image_util.cc (right): https://codereview.chromium.org/148093013/diff/1/ui/gfx/image/image_util.cc#n... ui/gfx/image/image_util.cc:69: On 2014/01/30 20:41:37, rsesek wrote: > On 2014/01/29 22:03:46, Greg Billock wrote: > > On 2014/01/29 17:05:29, rsesek wrote: > > > Where does 12 come from? > > > > 5% of 255, basically. It's the value for "transparent" the unit tests use > > (CheckIsTransparent), so I went with it. There doesn't seem to be a really > firm > > consensus that I can find. Just an "it depends, and human vision is > non-linear." > > For small graphics where this makes sense to call, the gradients should be > > pretty sharp. > > OK. Maybe make it a const int at the top of the function, or at least leave a > comment. Done. https://codereview.chromium.org/148093013/diff/40001/ui/gfx/image/image_util.cc File ui/gfx/image/image_util.cc (right): https://codereview.chromium.org/148093013/diff/40001/ui/gfx/image/image_util.... ui/gfx/image/image_util.cc:66: int innerMin = bitmap.width(); On 2014/01/30 20:41:38, rsesek wrote: > naming: Use under_scores, not camelCase. Here and on line 78. Done.
LGTM
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/148093013/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_aosp for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/148093013/60001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on android_aosp for step(s) compile http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_ao...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/148093013/460001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) base_unittests, browser_tests, interactive_ui_tests, net_unittests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/148093013/460001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_rel for step(s) net_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/148093013/460001
The CQ bit was unchecked by commit-bot@chromium.org
Retried try job too often on linux_aura for step(s) app_list_unittests, aura_unittests, browser_tests, compositor_unittests, content_browsertests, content_unittests, events_unittests, interactive_ui_tests, unit_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...
CQ bit was unchecked on CL. Ignoring.
CQ bit was unchecked on CL. Ignoring.
The CQ bit was checked by gbillock@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gbillock@chromium.org/148093013/460001
Message was sent while issue was closed.
Change committed as 248513
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring.
Message was sent while issue was closed.
CQ bit was unchecked on CL. Ignoring. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
