Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1123393007:
Fix SkBitmapOperations::CreateDropShadow drawing too large a shadow. (Closed)
|
Created:
5 years, 7 months ago by calamity Modified:
5 years, 7 months ago Reviewers:
Alexei Svitkine (slow) CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix SkBitmapOperations::CreateDropShadow drawing too large a shadow.
This CL fixes an issue where shadows were larger than
ShadowValue::GetMargin(). This caused certain shadows to clip to the
view boundaries which looks bad.
BUG=489533
Committed: https://crrev.com/31b6277f8e08aa83d8417d9cb34c44938f395643
Cr-Commit-Position: refs/heads/master@{#330912}
Patch Set 1 : #
Total comments: 6
Patch Set 2 : address comments #Messages
Total messages: 16 (6 generated)
The CQ bit was checked by calamity@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1123393007/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #1 (id:1) has been deleted
calamity@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine for ui/gfx/ OWNERS
https://codereview.chromium.org/1123393007/diff/20001/ui/gfx/skbitmap_operati... File ui/gfx/skbitmap_operations.cc (right): https://codereview.chromium.org/1123393007/diff/20001/ui/gfx/skbitmap_operati... ui/gfx/skbitmap_operations.cc:745: SkBlurImageFilter::Create(SkDoubleToScalar(shadow.blur() / 2), It's not clear to me how this ensures that the shadows stay within the margin? Wouldn't it depend on the size of the margins? Also, wouldn't this affect all the places using this - even ones which may not have the problem? (i.e. they would now look different due to this change - which may not be desired?)
https://codereview.chromium.org/1123393007/diff/20001/ui/gfx/skbitmap_operati... File ui/gfx/skbitmap_operations.cc (right): https://codereview.chromium.org/1123393007/diff/20001/ui/gfx/skbitmap_operati... ui/gfx/skbitmap_operations.cc:745: SkBlurImageFilter::Create(SkDoubleToScalar(shadow.blur() / 2), On 2015/05/19 15:12:04, Alexei Svitkine wrote: > It's not clear to me how this ensures that the shadows stay within the margin? > Wouldn't it depend on the size of the margins? > > Also, wouldn't this affect all the places using this - even ones which may not > have the problem? (i.e. they would now look different due to this change - which > may not be desired?) Sorry, I should have explained better. At the beginning of CreateDropShadow(), the result canvas is enlarged by ShadowValue::GetMargin() so that the painted shadow will fit. The current code produces a shadow that's twice as large as ShadowValue::GetMargin()'s insets. I vetted the other callsites and there's only the app list and the ash shelf buttons. The ash shelf buttons aren't affected by this because they use a blur value of 1 and the inset size for that is 1px. I can change the shadow value for the shelf to be 2 if you want pixel perfect congruence with the old behavior.
lgtm % comments https://codereview.chromium.org/1123393007/diff/20001/ui/gfx/skbitmap_operati... File ui/gfx/skbitmap_operations.cc (right): https://codereview.chromium.org/1123393007/diff/20001/ui/gfx/skbitmap_operati... ui/gfx/skbitmap_operations.cc:745: SkBlurImageFilter::Create(SkDoubleToScalar(shadow.blur() / 2), On 2015/05/20 04:26:51, calamity wrote: > On 2015/05/19 15:12:04, Alexei Svitkine wrote: > > It's not clear to me how this ensures that the shadows stay within the margin? > > Wouldn't it depend on the size of the margins? > > > > Also, wouldn't this affect all the places using this - even ones which may not > > have the problem? (i.e. they would now look different due to this change - > which > > may not be desired?) > > Sorry, I should have explained better. At the beginning of CreateDropShadow(), > the result canvas is enlarged by ShadowValue::GetMargin() so that the painted > shadow will fit. The current code produces a shadow that's twice as large as > ShadowValue::GetMargin()'s insets. > > I vetted the other callsites and there's only the app list and the ash shelf > buttons. The ash shelf buttons aren't affected by this because they use a blur > value of 1 and the inset size for that is 1px. > > I can change the shadow value for the shelf to be 2 if you want pixel perfect > congruence with the old behavior. Thanks for the explanation, that makes sense. Please add a comment above the division by 2 to also make it clear to the reader of the code. https://codereview.chromium.org/1123393007/diff/20001/ui/gfx/skbitmap_operati... ui/gfx/skbitmap_operations.cc:746: SkDoubleToScalar(shadow.blur() / 2))); Nit: Since you use SkDoubleToScalar(shadow.blur() / 2) twice, please make a local var above to make the code both more concise and more efficient.
https://codereview.chromium.org/1123393007/diff/20001/ui/gfx/skbitmap_operati... File ui/gfx/skbitmap_operations.cc (right): https://codereview.chromium.org/1123393007/diff/20001/ui/gfx/skbitmap_operati... ui/gfx/skbitmap_operations.cc:745: SkBlurImageFilter::Create(SkDoubleToScalar(shadow.blur() / 2), On 2015/05/20 20:47:42, Alexei Svitkine wrote: > On 2015/05/20 04:26:51, calamity wrote: > > On 2015/05/19 15:12:04, Alexei Svitkine wrote: > > > It's not clear to me how this ensures that the shadows stay within the > margin? > > > Wouldn't it depend on the size of the margins? > > > > > > Also, wouldn't this affect all the places using this - even ones which may > not > > > have the problem? (i.e. they would now look different due to this change - > > which > > > may not be desired?) > > > > Sorry, I should have explained better. At the beginning of CreateDropShadow(), > > the result canvas is enlarged by ShadowValue::GetMargin() so that the painted > > shadow will fit. The current code produces a shadow that's twice as large as > > ShadowValue::GetMargin()'s insets. > > > > I vetted the other callsites and there's only the app list and the ash shelf > > buttons. The ash shelf buttons aren't affected by this because they use a blur > > value of 1 and the inset size for that is 1px. > > > > I can change the shadow value for the shelf to be 2 if you want pixel perfect > > congruence with the old behavior. > > Thanks for the explanation, that makes sense. Please add a comment above the > division by 2 to also make it clear to the reader of the code. Done. https://codereview.chromium.org/1123393007/diff/20001/ui/gfx/skbitmap_operati... ui/gfx/skbitmap_operations.cc:746: SkDoubleToScalar(shadow.blur() / 2))); On 2015/05/20 20:47:42, Alexei Svitkine wrote: > Nit: Since you use SkDoubleToScalar(shadow.blur() / 2) twice, please make a > local var above to make the code both more concise and more efficient. Done.
The CQ bit was checked by calamity@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org Link to the patchset: https://codereview.chromium.org/1123393007/#ps40001 (title: "address comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1123393007/40001
Message was sent while issue was closed.
Committed patchset #2 (id:40001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/31b6277f8e08aa83d8417d9cb34c44938f395643 Cr-Commit-Position: refs/heads/master@{#330912} |
