Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1836483002:
Fix up and rename VisibleMargins() in preparation for making use of it. (Closed)
|
Created:
4 years, 9 months ago by Peter Kasting Modified:
4 years, 8 months ago CC:
chromium-reviews, rsesek+watch_chromium.org, varkha Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix up and rename VisibleMargins() in preparation for making use of it.
It turns out this function is currently unused (but tested), and its behavior is
rather odd and inconsistent -- rather than report the "margins" around an image
it reports the columns in which it finds visible pixels, and there are some
bugs.
I've revamped this to actually report the widths of any "margins" at the edges.
I've also cleaned up and simplified the code and added more tests.
I made one other seemingly-unconnected change, to paint_vector_icon.cc. This is
because VisibleMargins() used to early-exit when HasRepresentation() returned
false. I believe that caused problems if you called the function on an image
for which you had not yet obtained any representations, because if
GetRepresentation() would have succeeded (and then HasRepresentation would
succeed), the function would basically flakily fail. To avoid this, I made it
simply call GetRepresentation() instead, from which we can still detect when the
returned representation is an empty "no representation" placeholder.
However, this caused a different problem: if you called this on
VECTOR_ICON_NONE, the vector painting code DCHECKed (because you shouldn't be
painting that). After some consideration, I decided that it was OK for
VectorIconSource::Draw() to be called, and simply paint nothing, with this
value, while PaintVectorIcon() would still DCHECK if called directly. This
allows GetRepresentation() to return an empty representation for
VECTOR_ICON_NONE instead of crashing, which in turn allows calling
GetVisibleMargins() on that, which is important in the upcoming patch where I'll
make use of this because we'll be trying to call this on SkImages that may or
may not be that icon under-the-hood.
BUG=586423
TEST=none
Committed: https://crrev.com/723dfe66b1985ff2178590b15d1cc63911300737
Cr-Commit-Position: refs/heads/master@{#384157}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Review comments #
Total comments: 1
Patch Set 3 : Revert vector icon change #Patch Set 4 : Add back check for HasRepresentation() #Patch Set 5 : Resync #Patch Set 6 : Resync #
Depends on Patchset: Messages
Total messages: 26 (11 generated)
pkasting@chromium.org changed reviewers: + danakj@chromium.org, estade@chromium.org
estade: paint_vector_icon.cc -- see CL description for explanation danakj: rest
LGTM https://codereview.chromium.org/1836483002/diff/1/ui/gfx/image/image_util.cc File ui/gfx/image/image_util.cc (right): https://codereview.chromium.org/1836483002/diff/1/ui/gfx/image/image_util.cc#... ui/gfx/image/image_util.cc:20: // Returns whether column |x| of |bitmap| has any pixels with opacity > min, min looks like it's hardcoded, maybe it was a param before? update the comment? https://codereview.chromium.org/1836483002/diff/1/ui/gfx/image/image_util.cc#... ui/gfx/image/image_util.cc:92: // Since we already know column *leading is non-transparent, we can avoid leading->left ? https://codereview.chromium.org/1836483002/diff/1/ui/gfx/image/image_util_uni... File ui/gfx/image/image_util_unittest.cc (right): https://codereview.chromium.org/1836483002/diff/1/ui/gfx/image/image_util_uni... ui/gfx/image/image_util_unittest.cc:35: bitmap.allocN32Pixels(16, 16); less symmetry would be nice, what if the code confused width and height somewhere? https://codereview.chromium.org/1836483002/diff/1/ui/gfx/paint_vector_icon.cc File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/1836483002/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:389: DCHECK(id != VectorIconId::VECTOR_ICON_NONE); DCHECK_NE if you can
New snap not yet up https://codereview.chromium.org/1836483002/diff/1/ui/gfx/image/image_util.cc File ui/gfx/image/image_util.cc (right): https://codereview.chromium.org/1836483002/diff/1/ui/gfx/image/image_util.cc#... ui/gfx/image/image_util.cc:20: // Returns whether column |x| of |bitmap| has any pixels with opacity > min, On 2016/03/25 23:35:28, danakj wrote: > min looks like it's hardcoded, maybe it was a param before? update the comment? I actually wrote the comment to describe this hardcoded behavior, so it sounds like I didn't write a good enough comment. I'll try harder. (Also, spelling.) https://codereview.chromium.org/1836483002/diff/1/ui/gfx/image/image_util.cc#... ui/gfx/image/image_util.cc:92: // Since we already know column *leading is non-transparent, we can avoid On 2016/03/25 23:35:28, danakj wrote: > leading->left ? Yeah, oops. https://codereview.chromium.org/1836483002/diff/1/ui/gfx/image/image_util_uni... File ui/gfx/image/image_util_unittest.cc (right): https://codereview.chromium.org/1836483002/diff/1/ui/gfx/image/image_util_uni... ui/gfx/image/image_util_unittest.cc:35: bitmap.allocN32Pixels(16, 16); On 2016/03/25 23:35:28, danakj wrote: > less symmetry would be nice, what if the code confused width and height > somewhere? Sure, I can go through these and ensure that height/width calculations will always give different results. https://codereview.chromium.org/1836483002/diff/1/ui/gfx/paint_vector_icon.cc File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/1836483002/diff/1/ui/gfx/paint_vector_icon.cc... ui/gfx/paint_vector_icon.cc:389: DCHECK(id != VectorIconId::VECTOR_ICON_NONE); On 2016/03/25 23:35:28, danakj wrote: > DCHECK_NE if you can Can't (I tried that first); no operator<< for this enum class (and I didn't want to static cast both to int or something)
New snap up to address Dana's comments (re-review not necessary).
On 2016/03/26 04:45:02, Peter Kasting wrote: > New snap up to address Dana's comments (re-review not necessary). is it possible to put off the change to paint_vector_icon until it's necessary (i.e. do it as part of the upcoming patch)?
On 2016/03/28 19:32:46, Evan Stade wrote: > On 2016/03/26 04:45:02, Peter Kasting wrote: > > New snap up to address Dana's comments (re-review not necessary). > > is it possible to put off the change to paint_vector_icon until it's necessary > (i.e. do it as part of the upcoming patch)? Yes. That patch is already up for review as https://codereview.chromium.org/1829353002/ . Would you like me to move this file into that CL? The change will look the same either way so if you have review comments you'd share once that's done, feel free to make them, and I can try to respond at the same time I make the move.
https://codereview.chromium.org/1836483002/diff/20001/ui/gfx/paint_vector_ico... File ui/gfx/paint_vector_icon.cc (right): https://codereview.chromium.org/1836483002/diff/20001/ui/gfx/paint_vector_ico... ui/gfx/paint_vector_icon.cc:401: ImageSkia CreateVectorIconWithBadge(VectorIconId id, So we're calling this with id == VECTOR_ICON_NONE? It's not clear to me that isn't some kind of error, not knowing where this is happening from. (It still wasn't obvious to me after looking at the other CL.) But perhaps if it's decided this is valid, we should instead insert this early-out: if (id == VECTOR_ICON_NONE) return gfx::ImageSkia();
Thanks for the comment. I'll move this change to the other CL, add you as a reviewer, and post the relevant callstack.
The CQ bit was checked by pkasting@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1836483002/#ps40001 (title: "Revert vector icon change")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1836483002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1836483002/40001
The CQ bit was unchecked by pkasting@chromium.org
Actually, let me hold off committing this. I'm writing up a detailed analysis on the other CL, and depending on what we decide there, this could change.
I've now re-added the check for HasRepresentation(), with the necessary changes to that function in a CL this depends on. I don't think this needs re-review, this is just FYI. If the CLs this depends on land, I'll land this as-is.
Description was changed from ========== Fix up and rename VisibleMargins() in preparation for making use of it. It turns out this function is currently unused (but tested), and its behavior is rather odd and inconsistent -- rather than report the "margins" around an image it reports the columns in which it finds visible pixels, and there are some bugs. I've revamped this to actually report the widths of any "margins" at the edges. I've also cleaned up and simplified the code and added more tests. I made one other seemingly-unconnected change, to paint_vector_icon.cc. This is because VisibleMargins() used to early-exit when HasRepresentation() returned false. I believe that caused problems if you called the function on an image for which you had not yet obtained any representations, because if GetRepresentation() would have succeeded (and then HasRepresentation would succeed), the function would basically flakily fail. To avoid this, I made it simply call GetRepresentation() instead, from which we can still detect when the returned representation is an empty "no representation" placeholder. However, this caused a different problem: if you called this on VECTOR_ICON_NONE, the vector painting code DCHECKed (because you shouldn't be painting that). After some consideration, I decided that it was OK for VectorIconSource::Draw() to be called, and simply paint nothing, with this value, while PaintVectorIcon() would still DCHECK if called directly. This allows GetRepresentation() to return an empty representation for VECTOR_ICON_NONE instead of crashing, which in turn allows calling GetVisibleMargins() on that, which is important in the upcoming patch where I'll make use of this because we'll be trying to call this on SkImages that may or may not be that icon under-the-hood. BUG=586423 TEST=none ========== to ========== Fix up and rename VisibleMargins() in preparation for making use of it. It turns out this function is currently unused (but tested), and its behavior is rather odd and inconsistent -- rather than report the "margins" around an image it reports the columns in which it finds visible pixels, and there are some bugs. I've revamped this to actually report the widths of any "margins" at the edges. I've also cleaned up and simplified the code and added more tests. I made one other seemingly-unconnected change, to paint_vector_icon.cc. This is because VisibleMargins() used to early-exit when HasRepresentation() returned false. I believe that caused problems if you called the function on an image for which you had not yet obtained any representations, because if GetRepresentation() would have succeeded (and then HasRepresentation would succeed), the function would basically flakily fail. To avoid this, I made it simply call GetRepresentation() instead, from which we can still detect when the returned representation is an empty "no representation" placeholder. However, this caused a different problem: if you called this on VECTOR_ICON_NONE, the vector painting code DCHECKed (because you shouldn't be painting that). After some consideration, I decided that it was OK for VectorIconSource::Draw() to be called, and simply paint nothing, with this value, while PaintVectorIcon() would still DCHECK if called directly. This allows GetRepresentation() to return an empty representation for VECTOR_ICON_NONE instead of crashing, which in turn allows calling GetVisibleMargins() on that, which is important in the upcoming patch where I'll make use of this because we'll be trying to call this on SkImages that may or may not be that icon under-the-hood. BUG=586423 TEST=none ==========
The CQ bit was checked by pkasting@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org Link to the patchset: https://codereview.chromium.org/1836483002/#ps100001 (title: "Resync")
The CQ bit was unchecked by commit-bot@chromium.org
This CL has an open dependency (Issue 1846513002 Patch 60001). Please resolve the dependency and try again.
The CQ bit was checked by pkasting@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1836483002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1836483002/100001
Message was sent while issue was closed.
Description was changed from ========== Fix up and rename VisibleMargins() in preparation for making use of it. It turns out this function is currently unused (but tested), and its behavior is rather odd and inconsistent -- rather than report the "margins" around an image it reports the columns in which it finds visible pixels, and there are some bugs. I've revamped this to actually report the widths of any "margins" at the edges. I've also cleaned up and simplified the code and added more tests. I made one other seemingly-unconnected change, to paint_vector_icon.cc. This is because VisibleMargins() used to early-exit when HasRepresentation() returned false. I believe that caused problems if you called the function on an image for which you had not yet obtained any representations, because if GetRepresentation() would have succeeded (and then HasRepresentation would succeed), the function would basically flakily fail. To avoid this, I made it simply call GetRepresentation() instead, from which we can still detect when the returned representation is an empty "no representation" placeholder. However, this caused a different problem: if you called this on VECTOR_ICON_NONE, the vector painting code DCHECKed (because you shouldn't be painting that). After some consideration, I decided that it was OK for VectorIconSource::Draw() to be called, and simply paint nothing, with this value, while PaintVectorIcon() would still DCHECK if called directly. This allows GetRepresentation() to return an empty representation for VECTOR_ICON_NONE instead of crashing, which in turn allows calling GetVisibleMargins() on that, which is important in the upcoming patch where I'll make use of this because we'll be trying to call this on SkImages that may or may not be that icon under-the-hood. BUG=586423 TEST=none ========== to ========== Fix up and rename VisibleMargins() in preparation for making use of it. It turns out this function is currently unused (but tested), and its behavior is rather odd and inconsistent -- rather than report the "margins" around an image it reports the columns in which it finds visible pixels, and there are some bugs. I've revamped this to actually report the widths of any "margins" at the edges. I've also cleaned up and simplified the code and added more tests. I made one other seemingly-unconnected change, to paint_vector_icon.cc. This is because VisibleMargins() used to early-exit when HasRepresentation() returned false. I believe that caused problems if you called the function on an image for which you had not yet obtained any representations, because if GetRepresentation() would have succeeded (and then HasRepresentation would succeed), the function would basically flakily fail. To avoid this, I made it simply call GetRepresentation() instead, from which we can still detect when the returned representation is an empty "no representation" placeholder. However, this caused a different problem: if you called this on VECTOR_ICON_NONE, the vector painting code DCHECKed (because you shouldn't be painting that). After some consideration, I decided that it was OK for VectorIconSource::Draw() to be called, and simply paint nothing, with this value, while PaintVectorIcon() would still DCHECK if called directly. This allows GetRepresentation() to return an empty representation for VECTOR_ICON_NONE instead of crashing, which in turn allows calling GetVisibleMargins() on that, which is important in the upcoming patch where I'll make use of this because we'll be trying to call this on SkImages that may or may not be that icon under-the-hood. BUG=586423 TEST=none ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Fix up and rename VisibleMargins() in preparation for making use of it. It turns out this function is currently unused (but tested), and its behavior is rather odd and inconsistent -- rather than report the "margins" around an image it reports the columns in which it finds visible pixels, and there are some bugs. I've revamped this to actually report the widths of any "margins" at the edges. I've also cleaned up and simplified the code and added more tests. I made one other seemingly-unconnected change, to paint_vector_icon.cc. This is because VisibleMargins() used to early-exit when HasRepresentation() returned false. I believe that caused problems if you called the function on an image for which you had not yet obtained any representations, because if GetRepresentation() would have succeeded (and then HasRepresentation would succeed), the function would basically flakily fail. To avoid this, I made it simply call GetRepresentation() instead, from which we can still detect when the returned representation is an empty "no representation" placeholder. However, this caused a different problem: if you called this on VECTOR_ICON_NONE, the vector painting code DCHECKed (because you shouldn't be painting that). After some consideration, I decided that it was OK for VectorIconSource::Draw() to be called, and simply paint nothing, with this value, while PaintVectorIcon() would still DCHECK if called directly. This allows GetRepresentation() to return an empty representation for VECTOR_ICON_NONE instead of crashing, which in turn allows calling GetVisibleMargins() on that, which is important in the upcoming patch where I'll make use of this because we'll be trying to call this on SkImages that may or may not be that icon under-the-hood. BUG=586423 TEST=none ========== to ========== Fix up and rename VisibleMargins() in preparation for making use of it. It turns out this function is currently unused (but tested), and its behavior is rather odd and inconsistent -- rather than report the "margins" around an image it reports the columns in which it finds visible pixels, and there are some bugs. I've revamped this to actually report the widths of any "margins" at the edges. I've also cleaned up and simplified the code and added more tests. I made one other seemingly-unconnected change, to paint_vector_icon.cc. This is because VisibleMargins() used to early-exit when HasRepresentation() returned false. I believe that caused problems if you called the function on an image for which you had not yet obtained any representations, because if GetRepresentation() would have succeeded (and then HasRepresentation would succeed), the function would basically flakily fail. To avoid this, I made it simply call GetRepresentation() instead, from which we can still detect when the returned representation is an empty "no representation" placeholder. However, this caused a different problem: if you called this on VECTOR_ICON_NONE, the vector painting code DCHECKed (because you shouldn't be painting that). After some consideration, I decided that it was OK for VectorIconSource::Draw() to be called, and simply paint nothing, with this value, while PaintVectorIcon() would still DCHECK if called directly. This allows GetRepresentation() to return an empty representation for VECTOR_ICON_NONE instead of crashing, which in turn allows calling GetVisibleMargins() on that, which is important in the upcoming patch where I'll make use of this because we'll be trying to call this on SkImages that may or may not be that icon under-the-hood. BUG=586423 TEST=none Committed: https://crrev.com/723dfe66b1985ff2178590b15d1cc63911300737 Cr-Commit-Position: refs/heads/master@{#384157} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/723dfe66b1985ff2178590b15d1cc63911300737 Cr-Commit-Position: refs/heads/master@{#384157} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
