It does not consider nested Shadow DOM, so the result value might be wrong when comparing elements between different nested Shadow DOMs.
Created attachment 144329 [details] Patch
Comment on attachment 144329 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144329&action=review Don't we also need to fix Range::compareBoundaryPoints? If that's the case, we should share more code between comparePositions and compareBoundaryPoints first before making this change. > Source/WebCore/dom/TreeScope.cpp:258 > + Vector<TreeScope*> treeScopesA; > + listTreeScopes(nodeA, treeScopesA); > + > + Vector<TreeScope*> treeScopesB; > + listTreeScopes(nodeB, treeScopesB); We should probably give some inline buffers (around 4-5) so that we don't have to heap-allocate in common cases. > Source/WebCore/dom/TreeScope.cpp:268 > + Vector<TreeScope*>::reverse_iterator itA = treeScopesA.rbegin(); > + Vector<TreeScope*>::reverse_iterator itB = treeScopesB.rbegin(); > + > + for (; itA != treeScopesA.rend() && itB != treeScopesB.rend() && *itA == *itB; ++itA, ++itB) { } > + > + if (itA == treeScopesA.rbegin()) > + return 0; > + > + return *--itA; We prefer using indices over iterators for vectors. See the style guide. > Source/WebCore/editing/htmlediting.cpp:109 > + int offsetB; > + Node* descendentB; > + if (nodeB->treeScope() == commonScope) { > + descendentB = 0; > + offsetB = b.deprecatedEditingOffset(); > + } else { > + do { > + descendentB = nodeB; > + nodeB = nodeB->shadowAncestorNode(); > + } while (nodeB->treeScope() != commonScope); > + offsetB = 0; > + } I think it's better to combine two cases and do: Node* descendentB = 0; int offsetB = b.deprecatedEditingOffset(); while (nodeB->treeScope() != commonScope) { descendentB = nodeB; nodeB = nodeB->shadowAncestorNode(); offsetB = 0; }
Comment on attachment 144329 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144329&action=review > LayoutTests/editing/shadow/compare-positions-in-nested-shadow.html:20 > +// Returns div in nested Shadow DOM. This comment repeats what the code does. Please remove. > LayoutTests/editing/shadow/compare-positions-in-nested-shadow.html:27 > + var darkRoot = new WebKitShadowRoot(div); > + var darkDiv = document.createElement('div'); Why don't we just call this nestedShadowRoot? > LayoutTests/editing/shadow/compare-positions-in-nested-shadow.html:38 > +var div1 = results1[2]; > +var div2 = results2[2]; It would read better if createNestedShadowDOM just returned the nested shadow root instead of returning 3-tuple since you can get other nodes by calling methods.
(In reply to comment #2) > (From update of attachment 144329 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144329&action=review > > Don't we also need to fix Range::compareBoundaryPoints? If that's the case, we should share more code between comparePositions and compareBoundaryPoints first before making this change. > Actually it does not consider Shadow DOM at all. It would be OK for now. I'll update the patch soon.
Comment on attachment 144329 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144329&action=review >> Source/WebCore/dom/TreeScope.cpp:258 >> + listTreeScopes(nodeB, treeScopesB); > > We should probably give some inline buffers (around 4-5) so that we don't have to heap-allocate in common cases. Done. >> Source/WebCore/dom/TreeScope.cpp:268 >> + return *--itA; > > We prefer using indices over iterators for vectors. See the style guide. Done. >> Source/WebCore/editing/htmlediting.cpp:109 >> + } > > I think it's better to combine two cases and do: > Node* descendentB = 0; > int offsetB = b.deprecatedEditingOffset(); > while (nodeB->treeScope() != commonScope) { > descendentB = nodeB; > nodeB = nodeB->shadowAncestorNode(); > offsetB = 0; > } Done >> LayoutTests/editing/shadow/compare-positions-in-nested-shadow.html:20 >> +// Returns div in nested Shadow DOM. > > This comment repeats what the code does. Please remove. Done >> LayoutTests/editing/shadow/compare-positions-in-nested-shadow.html:27 >> + var darkDiv = document.createElement('div'); > > Why don't we just call this nestedShadowRoot? Done >> LayoutTests/editing/shadow/compare-positions-in-nested-shadow.html:38 >> +var div2 = results2[2]; > > It would read better if createNestedShadowDOM just returned the nested shadow root instead of returning 3-tuple > since you can get other nodes by calling methods. Done.
Created attachment 144485 [details] Patch
Comment on attachment 144485 [details] Patch Attachment 144485 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12848495 New failing tests: editing/pasteboard/drop-text-events.html editing/pasteboard/drop-inputtext-acquires-style.html fast/events/5056619.html editing/selection/select-across-readonly-input-2.html
Created attachment 144544 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment on attachment 144485 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144485&action=review Please make sure your patch passes tests. > Source/WebCore/editing/htmlediting.cpp:100 > + TreeScope* commonScope = commonTreeScope(a.deprecatedNode(), b.deprecatedNode()); > + > + ASSERT(commonScope); > + if (!commonScope) > + return 0; > + > Node* nodeA = a.deprecatedNode(); > ASSERT(nodeA); > + int offsetA = a.deprecatedEditingOffset(); > + bool hasDescendentA = adjustPositionToTreeScope(commonScope, nodeA, offsetA); > + > Node* nodeB = b.deprecatedNode(); > ASSERT(nodeB); > - int offsetA = a.deprecatedEditingOffset(); > int offsetB = b.deprecatedEditingOffset(); I don't know how hard it is to start using containerNode() and computeOffsetInContainerNode() here but we should move toward that eventually.
Created attachment 144689 [details] Patch
(In reply to comment #9) > (From update of attachment 144485 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=144485&action=review > > Please make sure your patch passes tests. Ah... sorry. the newly implemented algorithm was wrong. > I don't know how hard it is to start using containerNode() and computeOffsetInContainerNode() here > but we should move toward that eventually. Done.
Comment on attachment 144689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=144689&action=review This is a great improvement if it passes all the tests. > Source/WebCore/dom/TreeScope.cpp:268 > + if (treeScopesA[indexA] == treeScopesB[indexB]) > + return treeScopesA[indexA]; > + > + return 0; Maybe use ternary operator here?
Levi, Enrica, & Darin: fyi, this patch also removes legacy position code in comparePositions :)
Comment on attachment 144689 [details] Patch Attachment 144689 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12838870 New failing tests: editing/selection/document-mutation.html editing/deleting/25322-2.html
Created attachment 144715 [details] Archive of layout-test-results from ec2-cr-linux-01 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-01 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #14) > (From update of attachment 144689 [details]) > Attachment 144689 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/12838870 > > New failing tests: > editing/selection/document-mutation.html > editing/deleting/25322-2.html hmmmmmmmmmmmmmmmm
(In reply to comment #16) > (In reply to comment #14) > > (From update of attachment 144689 [details] [details]) > > Attachment 144689 [details] [details] did not pass chromium-ews (chromium-xvfb): > > Output: http://queues.webkit.org/results/12838870 > > > > New failing tests: > > editing/selection/document-mutation.html > > editing/deleting/25322-2.html > > hmmmmmmmmmmmmmmmm When using deprecated position, those tests pass...
(In reply to comment #17) > (In reply to comment #16) > > (In reply to comment #14) > > > (From update of attachment 144689 [details] [details] [details]) > > > Attachment 144689 [details] [details] [details] did not pass chromium-ews (chromium-xvfb): > > > Output: http://queues.webkit.org/results/12838870 > > > > > > New failing tests: > > > editing/selection/document-mutation.html > > > editing/deleting/25322-2.html > > > > hmmmmmmmmmmmmmmmm > > When using deprecated position, those tests pass... Right. That's why I said I don't know how hard it is.
Comment on attachment 144689 [details] Patch Attachment 144689 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/12844779 New failing tests: editing/selection/document-mutation.html editing/deleting/25322-2.html
Created attachment 144733 [details] Archive of layout-test-results from ec2-cr-linux-02 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
(In reply to comment #18) > (In reply to comment #17) > > (In reply to comment #16) > > > (In reply to comment #14) > > > > (From update of attachment 144689 [details] [details] [details] [details]) > > > > Attachment 144689 [details] [details] [details] [details] did not pass chromium-ews (chromium-xvfb): > > > > Output: http://queues.webkit.org/results/12838870 > > > > > > > > New failing tests: > > > > editing/selection/document-mutation.html > > > > editing/deleting/25322-2.html > > > > > > hmmmmmmmmmmmmmmmm > > > > When using deprecated position, those tests pass... > > > Right. That's why I said I don't know how hard it is. OK. I understand the reason. When calling comparePositions in case replacing text, the containerNode() might not have the replaced text yet. Maybe we should have an alternative version of computeOffsetInContainerNode() which does not check max length. Let me try.
(In reply to comment #21) > (In reply to comment #18) > > (In reply to comment #17) > > > (In reply to comment #16) > > > > (In reply to comment #14) > > > > > (From update of attachment 144689 [details] [details] [details] [details] [details]) > > > > > Attachment 144689 [details] [details] [details] [details] [details] did not pass chromium-ews (chromium-xvfb): > > > > > Output: http://queues.webkit.org/results/12838870 > > > > > > > > > > New failing tests: > > > > > editing/selection/document-mutation.html > > > > > editing/deleting/25322-2.html > > > > > > > > hmmmmmmmmmmmmmmmm > > > > > > When using deprecated position, those tests pass... > > > > > > Right. That's why I said I don't know how hard it is. > > OK. I understand the reason. When calling comparePositions in case replacing text, the containerNode() might not have the replaced text yet. Maybe we should have an alternative version of computeOffsetInContainerNode() which does not check max length. Let me try. Hmm... it only holds for document-mutation.html case... 25322-2.html is caused by another reason.
(In reply to comment #21) > OK. I understand the reason. When calling comparePositions in case replacing text, the containerNode() might not have the replaced text yet. Maybe we should have an alternative version of computeOffsetInContainerNode() which does not check max length. Let me try. I see. It appears to me that we need to change the order in which text is replaced and selection is updated. See https://bugs.webkit.org/show_bug.cgi?id=66120.
> Hmm... it only holds for document-mutation.html case... 25322-2.html is caused by another reason. 25322-2.html has the following case: <img><br> and m_start = (br, 0), and m_end = (img, 1). m_start and m_end should be the same, however, it does not hold in the deprecated way... And it is required to make DeleteSelectionCommand work for now. So I gave up removing deprecatedNode() and deprecatedEditingOffset() for now. We have to fix DeleteSelectionCommand to remove them.
Created attachment 144770 [details] Patch for landing
(In reply to comment #24) > > Hmm... it only holds for document-mutation.html case... 25322-2.html is caused by another reason. > > 25322-2.html has the following case: > > <img><br> > > and m_start = (br, 0), and m_end = (img, 1). > > m_start and m_end should be the same, however, it does not hold in the deprecated way... > And it is required to make DeleteSelectionCommand work for now. That's the exact problem I encountered the last time I tired it. > So I gave up removing deprecatedNode() and deprecatedEditingOffset() for now. We have to fix DeleteSelectionCommand to remove them. Okay. That's okay since this is a very difficult change to make.
Comment on attachment 144770 [details] Patch for landing Clearing flags on attachment: 144770 Committed r118945: <http://trac.webkit.org/changeset/118945>
All reviewed patches have been landed. Closing bug.
The word descendant was misspelled as descendent in this patch. The one with the “e” is a different word, an adjective rather than a noun.