Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(131)

Issue 1477463003: Remove unnecessary assert in LayoutSVGRoot (Closed)

Created:
5 years ago by Stephen Chennney
Modified:
5 years ago
Reviewers:
pdr.
CC:
blink-reviews, blink-reviews-layout_chromium.org, chromium-reviews, krit, eae+blinkwatch, f(malita), fs, gyuyoung2, jchaffraix+rendering, kouhei+svg_chromium.org, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney, szager+layoutwatch_chromium.org, zoltan1
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Remove unnecessary assert in LayoutSVGRoot In LayoutSVGRoot::mapLocalToContainer we were asserting that the transform mode was UseTransforms, an assertion that was violated when the SVG layer was squashed into certain other layers. The assertion has been commented out for more than a year. Testing various configurations of transforms and SVG with squashing, such that the assert would hit, reveals no changes in rendered behavior. Code inspection suggests that the assertion is unnecessary because there is no place where SVG code actually checks the mode (although there are comments that indicate we assume it is set). Furthermore, SVGLayoutSupport::mapLocalToContainer explicitly sets the mode to UseTransforms. Given the lack of impact on behavior and the absence of failures or bugs since the assertion was disabled, this patch removes the assertion entirely. R=pdr BUG=231541, 364901 Committed: https://crrev.com/1761ef2855918b3e54d974a7fa1a860d604d660f Cr-Commit-Position: refs/heads/master@{#361703}

Patch Set 1 #

Patch Set 2 : Added test #

Unified diffs Side-by-side diffs Delta from patch set Stats (+2 lines, -9 lines) Patch
A + third_party/WebKit/LayoutTests/svg/custom/svg-fixed-position.svg View 1 1 chunk +1 line, -3 lines 0 comments Download
A + third_party/WebKit/LayoutTests/svg/custom/svg-fixed-position-expected.svg View 1 1 chunk +1 line, -3 lines 0 comments Download
M third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp View 1 chunk +0 lines, -3 lines 0 comments Download

Messages

Total messages: 15 (6 generated)
Stephen Chennney
I think it's time we lay this one to rest.
5 years ago (2015-11-24 20:08:18 UTC) #2
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477463003/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477463003/1
5 years ago (2015-11-24 20:08:44 UTC) #3
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years ago (2015-11-24 21:29:37 UTC) #5
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477463003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477463003/20001
5 years ago (2015-11-24 22:09:47 UTC) #7
pdr.
lgtm
5 years ago (2015-11-24 22:09:53 UTC) #8
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
5 years ago (2015-11-25 02:21:06 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1477463003/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1477463003/20001
5 years ago (2015-11-25 18:18:41 UTC) #12
commit-bot: I haz the power
Committed patchset #2 (id:20001)
5 years ago (2015-11-25 18:27:20 UTC) #13
commit-bot: I haz the power
5 years ago (2015-11-25 18:28:44 UTC) #15
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/1761ef2855918b3e54d974a7fa1a860d604d660f
Cr-Commit-Position: refs/heads/master@{#361703}

Powered by Google App Engine
This is Rietveld 408576698