Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1836993004:
Fix bubble client area clipping regression. (Closed)
|
Created:
4 years, 8 months ago by Evan Stade Modified:
4 years, 8 months ago CC:
chromium-reviews, tfarina, msw+watch_chromium.org, groby+bubble_chromium.org, hcarmona+bubble_chromium.org, rouslan+bubble_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix bubble client area clipping regression.
BUG=597931
Committed: https://crrev.com/e2531f2f86f1063032e972bbad3a2a238e5d34ed
Cr-Commit-Position: refs/heads/master@{#384130}
Patch Set 1 #
Total comments: 10
Patch Set 2 : actually upload #
Messages
Total messages: 20 (6 generated)
estade@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/1836993004/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1836993004/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:143: // If the client bounds don't touch the edges, no need to mask. This additional early return is not necessary for correctness, it's just a perf optimization to avoid clipping with AA if we don't need to. https://codereview.chromium.org/1836993004/diff/1/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/1836993004/diff/1/ui/views/view.cc#newcode816 ui/views/view.cc:816: clip_recorder.ClipPathWithAntiAliasing(clip_path_in_parent); I tried to avoid this before and instead inset by 0.5 for perf reasons, but as the bug(s) point out, this doesn't work.
estade@chromium.org changed reviewers: + msw@chromium.org, sadrul@chromium.org
+msw, sadrul as sky is ooo
https://codereview.chromium.org/1836993004/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1836993004/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:145: content_insets.bottom() > radius && content_insets.right()) { Did you mean to check |content_insets.right() > radius|? Maybe check if the min dimension (calculated here) is > radius? https://codereview.chromium.org/1836993004/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:148: gfx::RectF rect((gfx::Rect(size))); nit: remove extra parens?
https://codereview.chromium.org/1836993004/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1836993004/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:145: content_insets.bottom() > radius && content_insets.right()) { On 2016/03/29 22:54:55, msw wrote: > Did you mean to check |content_insets.right() > radius|? > Maybe check if the min dimension (calculated here) is > radius? good idea, done. https://codereview.chromium.org/1836993004/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:148: gfx::RectF rect((gfx::Rect(size))); On 2016/03/29 22:54:55, msw wrote: > nit: remove extra parens? that produces an error message: ../../ui/views/bubble/bubble_frame_view.cc:148:18: error: parentheses were disambiguated as a function declaration [-Werror,-Wvexing-parse] gfx::RectF rect(gfx::Rect(size)); ^~~~~~~~~~~~~~~~~ ../../ui/views/bubble/bubble_frame_view.cc:148:19: note: add a pair of parentheses to declare a variable gfx::RectF rect(gfx::Rect(size)); ^
Didn't see your new patch set uploaded https://codereview.chromium.org/1836993004/diff/1/ui/views/bubble/bubble_fram... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/1836993004/diff/1/ui/views/bubble/bubble_fram... ui/views/bubble/bubble_frame_view.cc:148: gfx::RectF rect((gfx::Rect(size))); On 2016/03/29 23:04:41, Evan Stade wrote: > On 2016/03/29 22:54:55, msw wrote: > > nit: remove extra parens? > > that produces an error message: > > ../../ui/views/bubble/bubble_frame_view.cc:148:18: error: parentheses were > disambiguated as a function declaration [-Werror,-Wvexing-parse] > gfx::RectF rect(gfx::Rect(size)); > ^~~~~~~~~~~~~~~~~ > ../../ui/views/bubble/bubble_frame_view.cc:148:19: note: add a pair of > parentheses to declare a variable > gfx::RectF rect(gfx::Rect(size)); > ^ Weird, but this seems familiar, and likely came up in another review. nbd, but you could just pass the width/height to RectF(float width, float height). https://codereview.chromium.org/1836993004/diff/1/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/1836993004/diff/1/ui/views/view.cc#newcode816 ui/views/view.cc:816: clip_recorder.ClipPathWithAntiAliasing(clip_path_in_parent); On 2016/03/29 01:27:58, Evan Stade wrote: > I tried to avoid this before and instead inset by 0.5 for perf reasons, but as > the bug(s) point out, this doesn't work. Maybe add a comment about why the aa is needed here?
oops, uploaded https://codereview.chromium.org/1836993004/diff/1/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/1836993004/diff/1/ui/views/view.cc#newcode816 ui/views/view.cc:816: clip_recorder.ClipPathWithAntiAliasing(clip_path_in_parent); On 2016/03/29 23:08:27, msw wrote: > On 2016/03/29 01:27:58, Evan Stade wrote: > > I tried to avoid this before and instead inset by 0.5 for perf reasons, but as > > the bug(s) point out, this doesn't work. > > Maybe add a comment about why the aa is needed here? Do you have a suggestion for a good comment? It seems to boil down to "the clip path could have partially transparent pixels, so use AA" but that seems sort of self evident (as in, that's the only reason you'd use AA to begin with).
lgtm with ignorance about the *new* need for aa. https://codereview.chromium.org/1836993004/diff/1/ui/views/view.cc File ui/views/view.cc (right): https://codereview.chromium.org/1836993004/diff/1/ui/views/view.cc#newcode816 ui/views/view.cc:816: clip_recorder.ClipPathWithAntiAliasing(clip_path_in_parent); On 2016/03/29 23:11:52, Evan Stade wrote: > On 2016/03/29 23:08:27, msw wrote: > > On 2016/03/29 01:27:58, Evan Stade wrote: > > > I tried to avoid this before and instead inset by 0.5 for perf reasons, but > as > > > the bug(s) point out, this doesn't work. > > > > Maybe add a comment about why the aa is needed here? > > Do you have a suggestion for a good comment? It seems to boil down to "the clip > path could have partially transparent pixels, so use AA" but that seems sort of > self evident (as in, that's the only reason you'd use AA to begin with). Shrug, I don't understand why it would have transparent pixels now, but not before this change... shouldn't the round rect corners yield some mid-pixel paths regardless of your old 0.5f extra inset? I dunno, I suppose it might be self-evident to people more familiar with clip paths than myself.
On 2016/03/29 23:19:13, msw wrote: > shouldn't the round rect corners yield some mid-pixel > paths regardless of your old 0.5f extra inset? Yes, but it didn't seem to be a problem until the bugs started pouring in. I should have noticed before committing the last patch, but it wasn't an issue on the one bubble I was using most for testing.
lgtm
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1836993004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1836993004/20001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by estade@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1836993004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1836993004/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix bubble client area clipping regression. BUG=597931 ========== to ========== Fix bubble client area clipping regression. BUG=597931 Committed: https://crrev.com/e2531f2f86f1063032e972bbad3a2a238e5d34ed Cr-Commit-Position: refs/heads/master@{#384130} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/e2531f2f86f1063032e972bbad3a2a238e5d34ed Cr-Commit-Position: refs/heads/master@{#384130} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
