Description was changed from ========== none none BUG= ========== to ========== none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ...
3 years, 8 months ago
(2017-04-04 19:47:54 UTC)
#1
Description was changed from
==========
none
none
BUG=
==========
to
==========
none
none
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
chrishtr
Description was changed from ========== none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Compute rounded-ness of ...
3 years, 8 months ago
(2017-04-04 19:49:32 UTC)
#2
Description was changed from
==========
none
none
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
Compute rounded-ness of visual rects.
This was not needed until https://codereview.chromium.org/2781863005.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
chrishtr
Description was changed from ========== Compute rounded-ness of visual rects. This was not needed until ...
3 years, 8 months ago
(2017-04-04 19:50:33 UTC)
#3
Description was changed from
==========
Compute rounded-ness of visual rects.
This was not needed until https://codereview.chromium.org/2781863005.
BUG=
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
Compute rounded-ness of visual rects.
This was not needed until https://codereview.chromium.org/2781863005.
BUG=707650
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
chrishtr
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
3 years, 8 months ago
(2017-04-04 19:51:34 UTC)
#4
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/421652)
3 years, 8 months ago
(2017-04-04 21:18:44 UTC)
#9
Updated to fix a small bug in GeometryMapper and adjust a layout test. https://codereview.chromium.org/2802443002/diff/60001/third_party/WebKit/LayoutTests/fast/clip/nested-rounded-rect-expected.html File ...
3 years, 8 months ago
(2017-04-04 22:02:57 UTC)
#11
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_tsan_rel_ng/builds/45994)
3 years, 8 months ago
(2017-04-04 22:43:12 UTC)
#14
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/355057)
3 years, 8 months ago
(2017-04-05 02:19:54 UTC)
#18
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/422779)
3 years, 8 months ago
(2017-04-05 19:03:55 UTC)
#22
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/422784)
3 years, 8 months ago
(2017-04-05 19:11:45 UTC)
#28
lgtm If I understand this change correctly, we were already computing the rounded-ness, but we ...
3 years, 8 months ago
(2017-04-05 20:06:47 UTC)
#29
lgtm
If I understand this change correctly, we were already computing the
rounded-ness, but we weren't propagating it to callers as the output was just a
FloatRect.
If so then could be worth slightly revising change description to note this.
https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/clip/nested-rounded-rect-expected.html
(right):
https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/clip/nested-rounded-rect-expected.html:3:
<div style="border: 1px solid black; border-radius: 4px 0px 0px 4px; background:
lightgray; width: 20px; height: 20px">
FYI the expected image (next file in patch) isn't rendering for me for some
reason
https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
(right):
https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp:127:
rectToMap = clipRect;
It looks like (but please confirm) this is the critical bit where we end up
propagating has-radius to the original callsites. Perhaps worth adding a comment
to call this out, since we work with just FloatRect in some places vs
FloatClipRect here, so reader might wonder why.
trchen
lgtm except for one TODO. https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h (right): https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h#newcode50 third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h:50: // |mappingRect| is both ...
3 years, 8 months ago
(2017-04-05 20:10:36 UTC)
#30
lgtm except for one TODO.
https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h (right):
https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h:50: //
|mappingRect| is both input and output.
As discussed, could you add a TODO here, stating that the output FloatClipRect
can contains false positive if there are non-trivial transforms in between, and
such false positive is not signaled by hasRadius, and why it is okay for current
implementation? e.g. something like this:
// TODO(crbug.com/xxxxxx): Need a way to signal false positive from
// non-trivial transforms. The visual rect functions can return rects
// bigger than the actual mapped regions, due to border radius and
// non-trivial transforms. This is fine for invalidation purposes,
// but would be an error for clipping purposes that requires an exact
// mapping. Rounded corners are signaled by hasRadius() bit on the
// output. Currently all callers that requires a exact clip rect
// never maps over non-trivial transforms. We need to add a DCHECK
// to ensure that stays the way.
chrishtr
Description was changed from ========== Compute rounded-ness of visual rects. This was not needed until ...
3 years, 8 months ago
(2017-04-05 20:12:00 UTC)
#31
Description was changed from
==========
Compute rounded-ness of visual rects.
This was not needed until https://codereview.chromium.org/2781863005.
BUG=707650
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
Compute rounded-ness of visual rects.
This was not needed until https://codereview.chromium.org/2781863005.
Previously we were computing rounded-ness internally to GeometryMapper,
but did not expose it through the *VisualRect* methods, only the
*ClipRect* ones. Thus this CL is just plumbing and API type updates.
BUG=707650
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
chrishtr
Also updated the CL description per suggestion. https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/LayoutTests/fast/clip/nested-rounded-rect-expected.html File third_party/WebKit/LayoutTests/fast/clip/nested-rounded-rect-expected.html (right): https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/LayoutTests/fast/clip/nested-rounded-rect-expected.html#newcode3 third_party/WebKit/LayoutTests/fast/clip/nested-rounded-rect-expected.html:3: <div style="border: ...
3 years, 8 months ago
(2017-04-05 20:22:16 UTC)
#32
Also updated the CL description per suggestion.
https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/fast/clip/nested-rounded-rect-expected.html
(right):
https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/fast/clip/nested-rounded-rect-expected.html:3:
<div style="border: 1px solid black; border-radius: 4px 0px 0px 4px; background:
lightgray; width: 20px; height: 20px">
On 2017/04/05 at 20:06:47, wkorman wrote:
> FYI the expected image (next file in patch) isn't rendering for me for some
reason
The next file in the patch was deleted in this patch, as I reverted this
test back to a reference test.
https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp
(right):
https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.cpp:127:
rectToMap = clipRect;
On 2017/04/05 at 20:06:47, wkorman wrote:
> It looks like (but please confirm) this is the critical bit where we end up
propagating has-radius to the original callsites. Perhaps worth adding a comment
to call this out, since we work with just FloatRect in some places vs
FloatClipRect here, so reader might wonder why.
Done.
https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h (right):
https://codereview.chromium.org/2802443002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/graphics/paint/GeometryMapper.h:50: //
|mappingRect| is both input and output.
On 2017/04/05 at 20:10:36, trchen wrote:
> As discussed, could you add a TODO here, stating that the output FloatClipRect
can contains false positive if there are non-trivial transforms in between, and
such false positive is not signaled by hasRadius, and why it is okay for current
implementation? e.g. something like this:
>
> // TODO(crbug.com/xxxxxx): Need a way to signal false positive from
> // non-trivial transforms. The visual rect functions can return rects
> // bigger than the actual mapped regions, due to border radius and
> // non-trivial transforms. This is fine for invalidation purposes,
> // but would be an error for clipping purposes that requires an exact
> // mapping. Rounded corners are signaled by hasRadius() bit on the
> // output. Currently all callers that requires a exact clip rect
> // never maps over non-trivial transforms. We need to add a DCHECK
> // to ensure that stays the way.
Done. I put some of the comments here and some in PaintLayerClipper.
chrishtr
The CQ bit was checked by chrishtr@chromium.org
3 years, 8 months ago
(2017-04-05 20:22:47 UTC)
#33
CQ is committing da patch. Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491423767230870, "parent_rev": "ac31b98fab70714a777f7a208ddeeb2e50ad0749", "commit_rev": "b6d12780972f3d9711de6d11c6dd6e0536020339"}
3 years, 8 months ago
(2017-04-05 22:20:11 UTC)
#36
CQ is committing da patch.
Bot data: {"patchset_id": 100001, "attempt_start_ts": 1491423767230870,
"parent_rev": "ac31b98fab70714a777f7a208ddeeb2e50ad0749", "commit_rev":
"b6d12780972f3d9711de6d11c6dd6e0536020339"}
commit-bot: I haz the power
Description was changed from ========== Compute rounded-ness of visual rects. This was not needed until ...
3 years, 8 months ago
(2017-04-05 22:21:02 UTC)
#37
Message was sent while issue was closed.
Description was changed from
==========
Compute rounded-ness of visual rects.
This was not needed until https://codereview.chromium.org/2781863005.
Previously we were computing rounded-ness internally to GeometryMapper,
but did not expose it through the *VisualRect* methods, only the
*ClipRect* ones. Thus this CL is just plumbing and API type updates.
BUG=707650
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========
to
==========
Compute rounded-ness of visual rects.
This was not needed until https://codereview.chromium.org/2781863005.
Previously we were computing rounded-ness internally to GeometryMapper,
but did not expose it through the *VisualRect* methods, only the
*ClipRect* ones. Thus this CL is just plumbing and API type updates.
BUG=707650
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2802443002
Cr-Commit-Position: refs/heads/master@{#462242}
Committed:
https://chromium.googlesource.com/chromium/src/+/b6d12780972f3d9711de6d11c6dd...
==========
commit-bot: I haz the power
Committed patchset #6 (id:100001) as https://chromium.googlesource.com/chromium/src/+/b6d12780972f3d9711de6d11c6dd6e0536020339
3 years, 8 months ago
(2017-04-05 22:21:03 UTC)
#38
Issue 2802443002: Compute rounded-ness of visual rects
(Closed)
Created 3 years, 8 months ago by chrishtr
Modified 3 years, 8 months ago
Reviewers: trchen, wkorman
Base URL:
Comments: 7
Chromium Code Reviews
Issue
2802443002:
Compute rounded-ness of visual rects (Closed)
