Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2813303008:
Remove pixel-snapping from LayoutSVGRoot::OverflowClipRect (Closed)
|
Created:
3 years, 8 months ago by chrishtr Modified:
3 years, 8 months ago CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, krit, eae+blinkwatch, fmalita+watch_chromium.org, fs, gyuyoung2, jchaffraix+rendering, kouhei+svg_chromium.org, leviw+renderwatch, pdr+svgwatchlist_chromium.org, pdr+renderingwatchlist_chromium.org, rwlbuis, Stephen Chennney, szager+layoutwatch_chromium.org, zoltan1 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove pixel-snapping from LayoutSVGRoot::OverflowClipRect
It was added in https://codereview.chromium.org/2343673003, but it it
incorrect.
BUG=699198
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2813303008
Cr-Commit-Position: refs/heads/master@{#465394}
Committed: https://chromium.googlesource.com/chromium/src/+/595c489d8a9f798a6b55a645543dc05d210d5146
Patch Set 1 #
Total comments: 1
Patch Set 2 : none #Patch Set 3 : none #
Messages
Total messages: 27 (17 generated)
Description was changed from ========== none none BUG= ========== to ========== Remove pixel-snapping from LayoutSVGRoot::OverflowClipRect It was added in https://codereview.chromium.org/2343673003, but it it incorrect. BUG=699198 ==========
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
chrishtr@chromium.org changed reviewers: + trchen@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping on this one also.
pdr@chromium.org changed reviewers: + pdr@chromium.org
https://codereview.chromium.org/2813303008/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): https://codereview.chromium.org/2813303008/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:239: return LayoutReplaced::OverflowClipRect(location, How does this not change test results? Can we remove the LayoutSVGRoot::VisualOverflowRect override entirely?
On 2017/04/17 at 20:12:20, pdr wrote: > https://codereview.chromium.org/2813303008/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): > > https://codereview.chromium.org/2813303008/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:239: return LayoutReplaced::OverflowClipRect(location, > How does this not change test results? > > Can we remove the LayoutSVGRoot::VisualOverflowRect override entirely? Indeed - done.
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2017/04/17 at 20:45:46, chrishtr wrote: > On 2017/04/17 at 20:12:20, pdr wrote: > > https://codereview.chromium.org/2813303008/diff/1/third_party/WebKit/Source/c... > > File third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp (right): > > > > https://codereview.chromium.org/2813303008/diff/1/third_party/WebKit/Source/c... > > third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp:239: return LayoutReplaced::OverflowClipRect(location, > > How does this not change test results? > > > > Can we remove the LayoutSVGRoot::VisualOverflowRect override entirely? > > Indeed - done. LGTM
pdr@chromium.org changed reviewers: + wangxianzhu@chromium.org
+cc wangxianzhu, does this make sense to you too?
On 2017/04/17 21:01:48, pdr. wrote: > +cc wangxianzhu, does this make sense to you too? lgtm. Actually I tried to keep the existing behavior in https://codereview.chromium.org/2343673003 by moving pixelSnappedIntRect() from where the clip was applied into LayoutSVGRoot::overflowClipRect(). I didn't know whether the snapping was correct.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
I'm not entirely sure why scrollbar behavior was overridden. It may have something to do with SVGs crossing document boundary (so the scrollbars are considered a part of the contents). Anyway I think removing the override makes sense, so lgtm.
Description was changed from ========== Remove pixel-snapping from LayoutSVGRoot::OverflowClipRect It was added in https://codereview.chromium.org/2343673003, but it it incorrect. BUG=699198 ========== to ========== Remove pixel-snapping from LayoutSVGRoot::OverflowClipRect It was added in https://codereview.chromium.org/2343673003, but it it incorrect. BUG=699198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by chrishtr@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pdr@chromium.org, wangxianzhu@chromium.org, trchen@chromium.org Link to the patchset: https://codereview.chromium.org/2813303008/#ps40001 (title: "none")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1492546810205610,
"parent_rev": "f14333c9889b233634b98673489f1268ea722034", "commit_rev":
"595c489d8a9f798a6b55a645543dc05d210d5146"}
Message was sent while issue was closed.
Description was changed from ========== Remove pixel-snapping from LayoutSVGRoot::OverflowClipRect It was added in https://codereview.chromium.org/2343673003, but it it incorrect. BUG=699198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== Remove pixel-snapping from LayoutSVGRoot::OverflowClipRect It was added in https://codereview.chromium.org/2343673003, but it it incorrect. BUG=699198 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2813303008 Cr-Commit-Position: refs/heads/master@{#465394} Committed: https://chromium.googlesource.com/chromium/src/+/595c489d8a9f798a6b55a645543d... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/595c489d8a9f798a6b55a645543d... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
