Revert Range firstNode/pastLastNode() refactoring. (issue 1121633004 by sigbjornf@opera.com)

0 views
Skip to first unread message

sigb...@opera.com

unread,
May 5, 2015, 4:26:22 AM5/5/15
to yo...@chromium.org, tk...@chromium.org, har...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, rob....@samsung.com
Reviewers: yosi_ooo_til_may_17, Slow until end of May, haraken,

Message:
Please take a look.

A regression it'd be good to address before M44 is cut.

Description:
Revert Range firstNode/pastLastNode() refactoring.

The calculation of these two end points cannot in general be done by way of
anchor offsetting of the start/end node -- such an anchor may not exist.

Hence, revert their implementations back to how they were prior to r191892.
The
functionality that r191892 added to Position has separate uses, and is kept.

R=
BUG=484103

Please review this at https://codereview.chromium.org/1121633004/

Base URL: svn://svn.chromium.org/blink/trunk

Affected files (+30, -4 lines):
A LayoutTests/fast/dom/Range/range-toString-non-anchor-no-crash.html
A +
LayoutTests/fast/dom/Range/range-toString-non-anchor-no-crash-expected.txt
M Source/core/dom/Range.cpp


Index:
LayoutTests/fast/dom/Range/range-toString-non-anchor-no-crash-expected.txt
diff --git a/LayoutTests/fast/dom/custom/frameElement-crash-expected.txt
b/LayoutTests/fast/dom/Range/range-toString-non-anchor-no-crash-expected.txt
similarity index 61%
copy from LayoutTests/fast/dom/custom/frameElement-crash-expected.txt
copy to
LayoutTests/fast/dom/Range/range-toString-non-anchor-no-crash-expected.txt
index
54bdbe5403889003325b0b7860f31b64c996cc21..f1bd3fdd74e6697cb1d32fa44ca7633ae1d85b81
100644
--- a/LayoutTests/fast/dom/custom/frameElement-crash-expected.txt
+++
b/LayoutTests/fast/dom/Range/range-toString-non-anchor-no-crash-expected.txt
@@ -1,9 +1,9 @@
-Test that accessing window.frameElement from a custom iframe does not
crash.
+Tests that performing toString() over a Range without a root does not
crash.

On success, you will see a series of "PASS" messages, followed by "TEST
COMPLETE".


-PASS Did not crash
+PASS r.toString() is "PASS"
PASS successfullyParsed is true

TEST COMPLETE
Index: LayoutTests/fast/dom/Range/range-toString-non-anchor-no-crash.html
diff --git
a/LayoutTests/fast/dom/Range/range-toString-non-anchor-no-crash.html
b/LayoutTests/fast/dom/Range/range-toString-non-anchor-no-crash.html
new file mode 100644
index
0000000000000000000000000000000000000000..d5ec507dd968df224cf71e611bde362592072107
--- /dev/null
+++ b/LayoutTests/fast/dom/Range/range-toString-non-anchor-no-crash.html
@@ -0,0 +1,16 @@
+<!DOCTYPE html>
+<script src="../../../resources/js-test.js"></script>
+<script>
+description('Tests that performing toString() over a Range without a root
does not crash.');
+
+var t1 = document.createElement("textarea");
+var t2 = document.createElement("textarea");
+t1.appendChild(document.createTextNode("PASS"));
+t1.appendChild(t2);
+
+var r = document.createRange();
+r.setStart(t1, 0);
+r.setEnd(t2, 0);
+
+shouldBeEqualToString('r.toString()', 'PASS');
+</script>
Index: Source/core/dom/Range.cpp
diff --git a/Source/core/dom/Range.cpp b/Source/core/dom/Range.cpp
index
e777350724f99d3ecc62789be5dc047c97a7e8cc..e4438717e0f3f9d886cd9f5ed90bdad76482f7d7
100644
--- a/Source/core/dom/Range.cpp
+++ b/Source/core/dom/Range.cpp
@@ -1329,7 +1329,13 @@ void Range::checkExtractPrecondition(ExceptionState&
exceptionState)

Node* Range::firstNode() const
{
- return startPosition().toOffsetInAnchor().nodeAsRangeFirstNode();
+ if (m_start.container()->offsetInCharacters())
+ return m_start.container();
+ if (Node* child = NodeTraversal::childAt(*m_start.container(),
m_start.offset()))
+ return child;
+ if (!m_start.offset())
+ return m_start.container();
+ return NodeTraversal::nextSkippingChildren(*m_start.container());
}

ShadowRoot* Range::shadowRoot() const
@@ -1339,7 +1345,11 @@ ShadowRoot* Range::shadowRoot() const

Node* Range::pastLastNode() const
{
- return endPosition().toOffsetInAnchor().nodeAsRangePastLastNode();
+ if (m_end.container()->offsetInCharacters())
+ return NodeTraversal::nextSkippingChildren(*m_end.container());
+ if (Node* child = NodeTraversal::childAt(*m_end.container(),
m_end.offset()))
+ return child;
+ return NodeTraversal::nextSkippingChildren(*m_end.container());
}

IntRect Range::boundingBox() const


har...@chromium.org

unread,
May 5, 2015, 6:36:54 AM5/5/15
to sigb...@opera.com, yo...@chromium.org, tk...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, sigb...@opera.com, eae+bli...@chromium.org, rob....@samsung.com
LGTM

This should be reviewed by yosin@, but given that Japan is a holiday season
and
this is just a partial revert CL, I think it's ok to land this.


https://codereview.chromium.org/1121633004/

sigb...@opera.com

unread,
May 5, 2015, 7:06:00 AM5/5/15
to yo...@chromium.org, tk...@chromium.org, har...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, rob....@samsung.com
On 2015/05/05 10:36:53, haraken wrote:
> LGTM

> This should be reviewed by yosin@, but given that Japan is a holiday
> season
and
> this is just a partial revert CL, I think it's ok to land this.

Thanks - once back, I'd really appreciate that. Best address the regressions
here in the meantime.

https://codereview.chromium.org/1121633004/

commi...@chromium.org

unread,
May 5, 2015, 7:06:32 AM5/5/15
to sigb...@opera.com, yo...@chromium.org, tk...@chromium.org, har...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, rob....@samsung.com
CQ is trying da patch. Follow status at
https://chromium-cq-status.appspot.com/patch-status/1121633004/20001


https://codereview.chromium.org/1121633004/

commi...@chromium.org

unread,
May 5, 2015, 7:10:02 AM5/5/15
to sigb...@opera.com, yo...@chromium.org, tk...@chromium.org, har...@chromium.org, blink-...@chromium.org, blink-re...@chromium.org, dglazko...@chromium.org, eae+bli...@chromium.org, rob....@samsung.com
Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=194918

https://codereview.chromium.org/1121633004/
Reply all
Reply to author
Forward
0 new messages