Bug 82698

Created attachment 141085 [details] WIP

Attachment 141085 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/edit..." exit_code: 1 Source/WebCore/dom/TreeScope.cpp:31: Alphabetical sorting problem. [build/include_order] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 141085 [details] WIP Attachment 141085 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12648758

Comment on attachment 141085 [details] WIP Attachment 141085 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/12644941

Comment on attachment 141085 [details] WIP Attachment 141085 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/12653803

Comment on attachment 141085 [details] WIP Attachment 141085 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/12649928

Comment on attachment 141085 [details] WIP Attachment 141085 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/12648764

Comment on attachment 141085 [details] WIP Attachment 141085 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/12656882

Comment on attachment 141085 [details] WIP Attachment 141085 [details] did not pass gtk-ews (gtk): Output: http://queues.webkit.org/results/12649944

Created attachment 141639 [details] Patch

Created attachment 141640 [details] Patch

Comment on attachment 141640 [details] Patch Attachment 141640 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12659014 New failing tests: fast/dom/shadow/selection-in-shadow.html

Created attachment 141645 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Ah, old selection tests are failing... It should be deprecated or something...

Created attachment 141646 [details] Patch

Comment on attachment 141646 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141646&action=review Could you test for following? - ShadowRoot of orphan(out-of-document) node > LayoutTests/ChangeLog:26 > + Duplicated entry.

(In reply to comment #16) > (From update of attachment 141646 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141646&action=review > > Could you test for following? > - ShadowRoot of orphan(out-of-document) node sure. wait a moment... > > > LayoutTests/ChangeLog:26 > > + > > Duplicated entry. Oops...

Created attachment 141662 [details] Patch

Comment on attachment 141662 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141662&action=review > Source/WebCore/dom/TreeScope.cpp:145 > + if (this != rootNode()->document()) > + return rootNode()->document()->getSelection(); Why are you making this change? You should at least explain why you're making this change in the change log. > Source/WebCore/dom/TreeScopeAdjuster.cpp:47 > - do { > + while (node) { > if (node->treeScope() == treeScope()) > return node; > if (!node->isInShadowTree()) > return 0; > - } while ((node = node->shadowAncestorNode())); > + node = node->shadowAncestorNode(); > + } Why are you re-writing this do-while loop to a while loop? Again, this needs to be explained somewhere. > Source/WebCore/page/DOMSelection.cpp:503 > +Node* DOMSelection::adjustedNode(const Position& position) const I would call it shadowAdjustedNode so that I know with respect to what it is adjusted. > Source/WebCore/page/DOMSelection.cpp:509 > + Node* adjustedNode = TreeScopeAdjuster(m_treeScope).ancestorInThisScope(containerNode); Why do we need a class for this? All these proliferations of tiny classes is going to increase our build time. It should have been much better if this was just a function in TreeScope, VisibleSelection, or even in htmlediting. > LayoutTests/editing/shadow/selection-of-shadowroot-expected.txt:4 > +PASS divs[0] is selection.anchorNode.parentNode > +PASS anchorTreeScopeRoot is internals.treeScopeRootNode(selection.focusNode) > +PASS anchorTreeScopeRoot is internals.treeScopeRootNode(selection.baseNode) > +PASS anchorTreeScopeRoot is internals.treeScopeRootNode(selection.extentNode) These outputs don't tell me what this test is testing. Ideal test outputs makes what and why we're asserting self-evident.

Comment on attachment 141662 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141662&action=review >> Source/WebCore/dom/TreeScope.cpp:145 >> + return rootNode()->document()->getSelection(); > > Why are you making this change? You should at least explain why you're making this change in the change log. This was because I would like to use the same instance of DOMSelection for document and shadow root. I'll comment it just before this in next patch. >> Source/WebCore/dom/TreeScopeAdjuster.cpp:47 >> + } > > Why are you re-writing this do-while loop to a while loop? > Again, this needs to be explained somewhere. This is because node can be null. >> Source/WebCore/page/DOMSelection.cpp:509 >> + Node* adjustedNode = TreeScopeAdjuster(m_treeScope).ancestorInThisScope(containerNode); > > Why do we need a class for this? All these proliferations of tiny classes is going to increase our build time. > It should have been much better if this was just a function in TreeScope, VisibleSelection, or even in htmlediting. Hmm... Actually I was proposed to create this class by morrita. But I'm OK to remove it. Let's do in another patch in that case... >> LayoutTests/editing/shadow/selection-of-shadowroot-expected.txt:4 >> +PASS anchorTreeScopeRoot is internals.treeScopeRootNode(selection.extentNode) > > These outputs don't tell me what this test is testing. > Ideal test outputs makes what and why we're asserting self-evident. OK. Thanks for mentioning this. I'll update the output to be comprehensive.

Comment on attachment 141662 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141662&action=review >>> Source/WebCore/dom/TreeScope.cpp:145 >>> + return rootNode()->document()->getSelection(); >> >> Why are you making this change? You should at least explain why you're making this change in the change log. > > This was because I would like to use the same instance of DOMSelection for document and shadow root. > I'll comment it just before this in next patch. Let me discard the previous comment... I'll comment it in ChangeLog.

Created attachment 141899 [details] Patch

Comment on attachment 141899 [details] Patch Attachment 141899 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12706250

Comment on attachment 141899 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=141899&action=review > Source/WebCore/page/DOMSelection.cpp:532 > + return position.offsetInContainerNode(); This is going to assert when position is not of the type PositionIsOffsetInAnchor. You should call computeOffsetInContainerNode() instead. > LayoutTests/editing/shadow/selection-of-orphan-shadowroot-expected.txt:1 > +Nodes of the selection for orphan shadow root should return null. Nit: for an orphan shadow root. > LayoutTests/editing/shadow/selection-of-shadowroot-expected.txt:14 > +Dragging from divs[0] to divs[2] I still don't understand what divs[0] and divs[2] mean, and what output I should be expecting. Also, all outputs on the right of PASS appear to be identical for each test case.

Created attachment 142169 [details] Patch

Created attachment 142171 [details] Patch

Thanks for reviewing! (In reply to comment #24) > (From update of attachment 141899 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=141899&action=review > > > Source/WebCore/page/DOMSelection.cpp:532 > > + return position.offsetInContainerNode(); > > This is going to assert when position is not of the type PositionIsOffsetInAnchor. You should call computeOffsetInContainerNode() instead. > > > LayoutTests/editing/shadow/selection-of-orphan-shadowroot-expected.txt:1 > > +Nodes of the selection for orphan shadow root should return null. > > Nit: for an orphan shadow root. > > > LayoutTests/editing/shadow/selection-of-shadowroot-expected.txt:14 > > +Dragging from divs[0] to divs[2] > > I still don't understand what divs[0] and divs[2] mean, and what output I should be expecting. > Also, all outputs on the right of PASS appear to be identical for each test case. Hmm... I've updated the patch to reduce the test and added a few description. I think this is much more understandable than before... But if you don't think so, could you suggest an advice to make it more comprehensive?

Comment on attachment 142171 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=142171&action=review > Source/WebCore/ChangeLog:25 > + (WebCore::TreeScope::getSelection): > + When shadow DOM feature is not enabled, we want to use the same instance of DOMSelection > + among Document and ShadowRoot. We normally start description immediately after: followed by a single space, and we don't indent descriptions by two-spaces. > Source/WebCore/ChangeLog:29 > + Since node could be null, I've added a node check code. Ditto. > Source/WebCore/ChangeLog:43 > + Gets the corresponding node in the m_treeScope from the Position. Ditto. > Source/WebCore/ChangeLog:46 > + Gets the corresponding node offset in the m_treeScope from the Position. Ditto.

(In reply to comment #28) > (From update of attachment 142171 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=142171&action=review > > > Source/WebCore/ChangeLog:25 > > + (WebCore::TreeScope::getSelection): > > + When shadow DOM feature is not enabled, we want to use the same instance of DOMSelection > > + among Document and ShadowRoot. > > We normally start description immediately after: followed by a single space, and we don't indent descriptions by two-spaces. > > > Source/WebCore/ChangeLog:29 > > + Since node could be null, I've added a node check code. > > Ditto. > > > Source/WebCore/ChangeLog:43 > > + Gets the corresponding node in the m_treeScope from the Position. > > Ditto. > > > Source/WebCore/ChangeLog:46 > > + Gets the corresponding node offset in the m_treeScope from the Position. > > Ditto. Thanks for mentioning this... I didn't know that rule!

Created attachment 142193 [details] Patch for landing

After the bots become green, I'll land this patch.

(In reply to comment #31) > After the bots become green, I'll land this patch. Since cr-linux was tested by me, it should be OK to land now...

Committed r117249: <http://trac.webkit.org/changeset/117249>