Bug 25057 – remove rangeCompliantEquivalent and replace it with Position methods

• Component: HTML Editing
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

remove rangeCompliantEquivalent and replace it with Position methods

As part of fixing bug 24763 Position now up-front knows if it's a neighbor anchored or parent-anchored and we don't need (or want) rangeCompliantEquivalent to do a "just in case" conversion any more.  We can either use a Position::toParentAnchoredEquivalent function or just computeOffsetInContainerNode() and containerNode() directly.

[Eric Seidel (no email)]

*** Bug 24045 has been marked as a duplicate of this bug. ***

[Levi Weintraub]

Created attachment 75855 [details]
Patch

[Levi Weintraub]

(In reply to comment #2)
> Created an attachment (id=75855) [details]
> Patch

Proposed patch that simplifies the logic by which parent anchored editing positions (DOM/range compliant) are created and changes to function to be a method of Position.

[WebKit Review Bot]

Attachment 75855 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/update-webkit']" exit_code: 2
Updating OpenSource
Incomplete data: Delta source ended unexpectedly at /usr/lib/git-core/git-svn line 5061

Died at WebKitTools/Scripts/update-webkit line 132.


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ryosuke Niwa]

Comment on attachment 75855 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75855&action=review

Thank you for taking this bug.  The patch looks good to me assuming that we still pass all the layout tests with your patch and the ordering in exp.in file is fixed.

> WebCore/WebCore.exp.in:519
> -__ZN7WebCore24rangeCompliantEquivalentERKNS_8PositionE
> +__ZNK7WebCore8Position24parentAnchoredEquivalentEv

I don't think this is the right place to put this.  Please lexicologically sort it.

> WebCore/dom/Position.cpp:167
> +    if (!m_anchorNode)
> +        return Position();
> +    
> +    // Handle quirks with tables and legacy positions offsets
> +    if (m_offset <= 0) {
> +        if (m_anchorNode->parentNode() && (editingIgnoresContent(m_anchorNode.get()) || isTableElement(m_anchorNode.get())))
> +            return positionInParentBeforeNode(m_anchorNode.get());
> +        return Position(m_anchorNode, 0);
> +    }
> +    if (!m_anchorNode->offsetInCharacters() && static_cast<unsigned>(m_offset) == m_anchorNode->childNodeCount()
> +        && (editingIgnoresContent(m_anchorNode.get()) || isTableElement(m_anchorNode.get()))) {
> +        return positionInParentAfterNode(m_anchorNode.get());
> +    }
> +
> +    return Position(containerNode(), computeOffsetInContainerNode());

This looks much simpler than rangeCompliantEquivalent.  Do we still pass all the layout tests with this new implementation?

> WebCore/dom/Position.h:83
> +    Position parentAnchoredEquivalent() const; // Convenience method for

Nit: Convenience method for what?

[Levi Weintraub]

Created attachment 75917 [details]
Patch

[Levi Weintraub]

(In reply to comment #6)
> Created an attachment (id=75917) [details]
> Patch

Thanks Ryosuke!

I fixed the ordering problem in WebCore.exp.in and the comment in Position.h, now:

Position parentAnchoredEquivalent() const; // Convenience method for DOM positions that also fixes up some positions for editing

I've validated we still pass all the layout tests.

[Ryosuke Niwa]

Comment on attachment 75917 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75917&action=review

> WebCore/dom/Position.cpp:160
> +        return Position(m_anchorNode, 0);

Oops, this would create a legacy Position object.  You need to explicitly pass PositionIsOffsetInAnchor as in Position(m_anchorNode, 0, PositionIsOffsetInAnchor); to create a non-legacy Position object.

> WebCore/dom/Position.cpp:167
> +    return Position(containerNode(), computeOffsetInContainerNode());

Ditto.

[Levi Weintraub]

(In reply to comment #8)
> (From update of attachment 75917 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75917&action=review
> 
> > WebCore/dom/Position.cpp:160
> > +        return Position(m_anchorNode, 0);
> 
> Oops, this would create a legacy Position object.  You need to explicitly pass PositionIsOffsetInAnchor as in Position(m_anchorNode, 0, PositionIsOffsetInAnchor); to create a non-legacy Position object.
> 
> > WebCore/dom/Position.cpp:167
> > +    return Position(containerNode(), computeOffsetInContainerNode());
> 
> Ditto.

You're right, missed that somehow. Trying again :)

[Levi Weintraub]

Created attachment 75931 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 75931 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75931&action=review

Sorry, I keep noticing something new.

> WebCore/dom/Position.cpp:157
> +    // Handle quirks with tables and legacy positions offsets
> +    if (m_offset <= 0) {

On my second thought, shouldn't we also check that it's a legacy position?  m_offset <= 0 is also true when m_anchorType != PositionIsOffsetInAnchor, no?

> WebCore/dom/Position.cpp:162
> +    if (!m_anchorNode->offsetInCharacters() && static_cast<unsigned>(m_offset) == m_anchorNode->childNodeCount()

Ditto here.

[Eric Seidel (no email)]

Comment on attachment 75931 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=75931&action=review

> WebCore/dom/Position.cpp:151
> +Position Position::parentAnchoredEquivalent() const

When I started with Position re-writes of yore, Darin Adler raised the concern that every time we return a Posiion object we do a ref-thrash to the underlying node, since we don't have any PassPosition concept.  Obviously PassPosition is a separate patch, but you should be aware, there may be ref-thrash when returning Position objects.

[Ryosuke Niwa]

(In reply to comment #12)
> (From update of attachment 75931 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75931&action=review
> 
> > WebCore/dom/Position.cpp:151
> > +Position Position::parentAnchoredEquivalent() const
> 
> When I started with Position re-writes of yore, Darin Adler raised the concern that every time we return a Posiion object we do a ref-thrash to the underlying node, since we don't have any PassPosition concept.  Obviously PassPosition is a separate patch, but you should be aware, there may be ref-thrash when returning Position objects.

This patch wouldn't regress the performance though because the function we're replacing also returns Position.  Ideally, we can remove this function entirely once we cleaned up other editing code.

[Levi Weintraub]

(In reply to comment #11)
> (From update of attachment 75931 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=75931&action=review
> 
> Sorry, I keep noticing something new.
> 
> > WebCore/dom/Position.cpp:157
> > +    // Handle quirks with tables and legacy positions offsets
> > +    if (m_offset <= 0) {
> 
> On my second thought, shouldn't we also check that it's a legacy position?  m_offset <= 0 is also true when m_anchorType != PositionIsOffsetInAnchor, no?
> 
> > WebCore/dom/Position.cpp:162
> > +    if (!m_anchorNode->offsetInCharacters() && static_cast<unsigned>(m_offset) == m_anchorNode->childNodeCount()
> 
> Ditto here.

We also need to go through this code path for positions before and after Tables, which aren't legacy positions. I could use the following instead:

    if (m_isLegacyEditingPosition || isTableElement(m_anchorNode.get())) {
        if (m_offset <= 0) {
            if (m_anchorNode->parentNode() && (editingIgnoresContent(m_anchorNode.get()) || isTableElement(m_anchorNode.get())))
                return positionInParentBeforeNode(m_anchorNode.get());
            return Position(m_anchorNode, 0);
        }
        if (!m_anchorNode->offsetInCharacters() && static_cast<unsigned>(m_offset) == m_anchorNode->childNodeCount() &&
            (editingIgnoresContent(m_anchorNode.get()) || isTableElement(m_anchorNode.get()))) {
            return positionInParentAfterNode(m_anchorNode.get());
        }
    }


if (!m_anchorNode->offsetInCharacters() && static_cast<unsigned>(m_offset) == m_anchorNode->childNodeCount()

[Levi Weintraub]

Created attachment 78056 [details]
Patch

[Levi Weintraub]

(In reply to comment #15)
> Created an attachment (id=78056) [details]
> Patch

Adding a FIXME as per a conversation with Ryosuke.

[Ryosuke Niwa]

Comment on attachment 78056 [details]
Patch

LGTM but I'd like eseidel, darin, or justining take a look at the patch before r+ing it.

[Ryosuke Niwa]

Oops, s/justining/justing/.  Sorry Justin.

[Darin Adler]

Comment on attachment 78056 [details]
Patch

So two things here are 1) a rename and 2) change to a member function because you like that better than a non-member function. Neither of those seems all that great to me.

But maybe the real point here is that we can do a more efficient implementation inside Position because of how Position works now?

[Darin Adler]

Comment on attachment 78056 [details]
Patch

I think it’s lame that both the old function and new function are named as if they did something mechanical but instead are full of editing magic. We should choose a name and perhaps even a location in the source code that acknowledges the editing magic. Or factor out the editing magic into a separate function.

[Ryosuke Niwa]

(In reply to comment #20)
> (From update of attachment 78056 [details])
> I think it’s lame that both the old function and new function are named as if they did something mechanical but instead are full of editing magic. We should choose a name and perhaps even a location in the source code that acknowledges the editing magic. Or factor out the editing magic into a separate function.

This function should be solely mechanical once we eliminated:
1. Legacy editing positions
2. [table, 0]

I'm sure we have a bug for 1 but we should probably be filing a bug for 2.

[Levi Weintraub]

(In reply to comment #19)
> (From update of attachment 78056 [details])
> So two things here are 1) a rename and 2) change to a member function because you like that better than a non-member function. Neither of those seems all that great to me.
> 
> But maybe the real point here is that we can do a more efficient implementation inside Position because of how Position works now?

There's also reduction and code cleanup (35 lines down to 16). Part of the purpose of this is to call out the editing magic currently happening with the intention of making it go away once we've done away with the cases Ryosuke called out.

[Ryosuke Niwa]

Darin, are you opposed to this change?  If not, I'm inclined to give r+.

[Darin Adler]

(In reply to comment #23)
> Darin, are you opposed to this change?  If not, I'm inclined to give r+.

I don’t love it, but it seems OK. Maybe I’ll appreciate this more when we take the next steps.

[Ryosuke Niwa]

(In reply to comment #24)
> (In reply to comment #23)
> > Darin, are you opposed to this change?  If not, I'm inclined to give r+.
> 
> I don’t love it, but it seems OK. Maybe I’ll appreciate this more when we take the next steps.

Ok, I'm aggressively removing legacy editing positions so hopefully we'll have a good news for you within the next couple of months.

[WebKit Commit Bot]

Comment on attachment 78056 [details]
Patch

Rejecting attachment 78056 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-4', 'apply-..." exit_code: 2

Last 500 characters of output:
 Source/WebCore/editing/htmlediting.cpp
patching file Source/WebCore/editing/htmlediting.h
patching file Source/WebCore/editing/visible_units.cpp
patching file Source/WebCore/page/DOMSelection.cpp
patching file Source/WebKit/mac/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/mac/WebView/WebFrame.mm
Hunk #1 succeeded at 802 (offset 1 line).

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Ryosuke Niwa', u'--for..." exit_code: 1

Full output: http://queues.webkit.org/results/7626149

[Levi Weintraub]

(In reply to comment #26)
> (From update of attachment 78056 [details])
> Rejecting attachment 78056 [details] from commit-queue.

Looks like this needs to be updated for ToT. I'll get on that.

[Levi Weintraub]

Created attachment 79298 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 79298 [details]
Patch

Rejecting attachment 79298 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-sf-cq', 'ap..." exit_code: 2

Last 500 characters of output:
 Source/WebCore/editing/htmlediting.cpp.rej
patching file Source/WebCore/editing/htmlediting.h
patching file Source/WebCore/editing/visible_units.cpp
Hunk #2 succeeded at 138 (offset -4 lines).
patching file Source/WebCore/page/DOMSelection.cpp
patching file Source/WebKit/mac/ChangeLog
Hunk #1 succeeded at 1 with fuzz 3.
patching file Source/WebKit/mac/WebView/WebFrame.mm

Failed to run "[u'/Projects/CommitQueue/Tools/Scripts/svn-apply', u'--reviewer', u'Ryosuke Niwa', u'--force']" exit_code: 1

Full output: http://queues.webkit.org/results/7523203

[Levi Weintraub]

Created attachment 79327 [details]
Patch

[Levi Weintraub]

Comment on attachment 79327 [details]
Patch

Updating again to fix merge issues with ToT

[WebKit Commit Bot]

Comment on attachment 79327 [details]
Patch

Rejecting attachment 79327 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-sf-cq', 'bu..." exit_code: 2

Last 500 characters of output:
.........................................................
fast/box-shadow ......
fast/box-sizing ....
fast/canvas ..........................................................................................................................................
fast/canvas/webgl ............
fast/canvas/webgl/constants.html -> crashed

Exiting early after 1 failures. 6247 tests run.
250.27s total testing time

6246 test cases (99%) succeeded
1 test case (<1%) crashed
3 test cases (<1%) had stderr output

Full output: http://queues.webkit.org/results/7550177

[Eric Seidel (no email)]

Created attachment 79365 [details]
Here is the crash log

[WebKit Commit Bot]

Comment on attachment 79327 [details]
Patch

Clearing flags on attachment: 79327

Committed r76107: <http://trac.webkit.org/changeset/76107>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.