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

Issue 2745443003: DOM Range: surroundContents() should not check if newParent is in ancestors beforehand. (Closed)

Created:
3 years, 9 months ago by tkent
Modified:
3 years, 9 months ago
Reviewers:
yosin_UTC9
CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-w3ctests_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

DOM Range: surroundContents() should not check if newParent is in ancestors beforehand. https://dom.spec.whatwg.org/#dom-range-surroundcontents According to the DOM standard, we should not check it before starting DOM mutation. Step 5 throws HierarchyRequestError instead. This CL fixes 44 tests in wpt/dom/ranges/Range-surroundContents.html. * images/element-gcd-while-generating-alt-content.html: Removed. This CL changes behavior of the test. The test was for a crash bug in pre-Olilpan era, and we don't have code for the crash fix any longer. * fast/dom/Range/surroundContents-1.html: Removed. This is a pixel test to verify HierarchyRequestError which this CL removes. This behavior is tested well in WPT. * shadow-dom/range-surround-contents.html: Updated DOM trees are modified because this CL removes a precondition check. BUG=699861 Review-Url: https://codereview.chromium.org/2745443003 Cr-Commit-Position: refs/heads/master@{#455941} Committed: https://chromium.googlesource.com/chromium/src/+/a3c94e4cc81caa2693ee56579e74c71e2fcadb0f

Patch Set 1 #

Patch Set 2 : . #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+60 lines, -364 lines) Patch
M third_party/WebKit/LayoutTests/external/wpt/dom/ranges/Range-surroundContents-expected.txt View 14 chunks +45 lines, -83 lines 0 comments Download
D third_party/WebKit/LayoutTests/fast/dom/Range/surroundContents-1.html View 1 1 chunk +0 lines, -35 lines 0 comments Download
D third_party/WebKit/LayoutTests/images/element-gcd-while-generating-alt-content.html View 1 1 chunk +0 lines, -188 lines 0 comments Download
D third_party/WebKit/LayoutTests/images/element-gcd-while-generating-alt-content-expected.txt View 1 1 chunk +0 lines, -2 lines 0 comments Download
D third_party/WebKit/LayoutTests/platform/linux/fast/dom/Range/surroundContents-1-expected.png View 1 Binary file 0 comments Download
D third_party/WebKit/LayoutTests/platform/linux/fast/dom/Range/surroundContents-1-expected.txt View 1 1 chunk +0 lines, -13 lines 0 comments Download
D third_party/WebKit/LayoutTests/platform/mac/fast/dom/Range/surroundContents-1-expected.png View 1 Binary file 0 comments Download
D third_party/WebKit/LayoutTests/platform/mac/fast/dom/Range/surroundContents-1-expected.txt View 1 1 chunk +0 lines, -13 lines 0 comments Download
D third_party/WebKit/LayoutTests/platform/win/fast/dom/Range/surroundContents-1-expected.png View 1 Binary file 0 comments Download
D third_party/WebKit/LayoutTests/platform/win/fast/dom/Range/surroundContents-1-expected.txt View 1 1 chunk +0 lines, -13 lines 0 comments Download
M third_party/WebKit/LayoutTests/shadow-dom/range-surround-contents.html View 1 1 chunk +1 line, -2 lines 0 comments Download
M third_party/WebKit/Source/core/dom/Range.cpp View 1 4 chunks +14 lines, -15 lines 1 comment Download

Messages

Total messages: 22 (18 generated)
tkent
yosin@, would you review this please? https://codereview.chromium.org/2745443003/diff/20001/third_party/WebKit/Source/core/dom/Range.cpp File third_party/WebKit/Source/core/dom/Range.cpp (left): https://codereview.chromium.org/2745443003/diff/20001/third_party/WebKit/Source/core/dom/Range.cpp#oldcode1398 third_party/WebKit/Source/core/dom/Range.cpp:1398: // FIXME: Do ...
3 years, 9 months ago (2017-03-09 22:36:33 UTC) #15
yosin_UTC9
lgtm
3 years, 9 months ago (2017-03-10 01:07:01 UTC) #18
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2745443003/20001
3 years, 9 months ago (2017-03-10 01:07:33 UTC) #19
commit-bot: I haz the power
3 years, 9 months ago (2017-03-10 01:16:09 UTC) #22
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/a3c94e4cc81caa2693ee56579e74...

Powered by Google App Engine
This is Rietveld 408576698