Bug 54986 – deprecatedEditingOffset should actually return the expected deprecated value for "after" positions

• Component: HTML Editing
• Status: RESOLVED FIXED

[Levi Weintraub]

In working on bug 52642, I've found numerous places in editing and non-editing code that calls deprecatedEditingOffset and does the wrong thing for new "after" positions, as this returns an offset of zero. As we transition away from deprecated positions, we need to do the right thing in these legacy call sites.

I propose changing deprecatedEditingOffset to return the lastEditingOffset for "after" positions. This will match the legacy behavior until we can update the places that directly rely on deprecatedEditingOffset. My proposed change will cause this function to not be inlined (lastOffsetForEditing if defined in htmlediting.h, which can't be included in Position.h), which is my only concern.

[Levi Weintraub]

Created attachment 83364 [details]
Patch

[Levi Weintraub]

Created attachment 83409 [details]
Patch

[Levi Weintraub]

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

Only using this path for new positions so it doesn't rely on "guessed" anchor types.

[Ryosuke Niwa]

Comment on attachment 83409 [details]
Patch

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

> Source/WebCore/dom/Position.cpp:153
> +int Position::deprecatedEditingOffset() const
> +{
> +    // This should probably ASSERT(m_isLegacyEditingPosition);
> +    return (m_isLegacyEditingPosition || m_anchorType != PositionIsAfterAnchor) ? m_offset : lastOffsetForEditing(m_anchorNode.get());
> +}

Why we can't keep this function inline?  I'm afraid that making this function non-inline will have some performance impact.

[Levi Weintraub]

(In reply to comment #4)
> (From update of attachment 83409 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83409&action=review
> 
> > Source/WebCore/dom/Position.cpp:153
> > +int Position::deprecatedEditingOffset() const
> > +{
> > +    // This should probably ASSERT(m_isLegacyEditingPosition);
> > +    return (m_isLegacyEditingPosition || m_anchorType != PositionIsAfterAnchor) ? m_offset : lastOffsetForEditing(m_anchorNode.get());
> > +}
> 
> Why we can't keep this function inline?  I'm afraid that making this function non-inline will have some performance impact.

See the first comment on this bug.

[Ryosuke Niwa]

Comment on attachment 83409 [details]
Patch

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

>>> Source/WebCore/dom/Position.cpp:153
>>> +}
>> 
>> Why we can't keep this function inline?  I'm afraid that making this function non-inline will have some performance impact.
> 
> See the first comment on this bug.

I see. We can't call lastOffsetInNode(anchorNode) instead?

[Levi Weintraub]

(In reply to comment #6)
> (From update of attachment 83409 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=83409&action=review
> 
> >>> Source/WebCore/dom/Position.cpp:153
> >>> +}
> >> 
> >> Why we can't keep this function inline?  I'm afraid that making this function non-inline will have some performance impact.
> > 
> > See the first comment on this bug.
> 
> I see. We can't call lastOffsetInNode(anchorNode) instead?

That won't properly handle nodes that return true for editingIgnoresContent :(

The other option is to have the fast-path for legacy positions that's inlined, and a fallback that calls a private member function that does the right thing for new positions. It's still not ideal (though closer), but then again nothing is quite ideal in this mixed new/legacy position world.

[Ryosuke Niwa]

(In reply to comment #7)
> The other option is to have the fast-path for legacy positions that's inlined, and a fallback that calls a private member function that does the right thing for new positions. It's still not ideal (though closer), but then again nothing is quite ideal in this mixed new/legacy position world.

That sounds reasonable.  I was going to suggest as well but I was concerned with the fact that'll result in two function calls in the slow patch.  But then lastEditingOffset already results in a bunch of function calls so that might be okay.

Eric & Darin, any opinions here?

[Levi Weintraub]

Created attachment 83422 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 83422 [details]
Patch

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

The patch looks good. r=me provided you all layout tests ran successfully.

> Source/WebCore/ChangeLog:11
> +        No tests. This is intended to simplify the transition to new Positions, not change behavior.

not "to" change behavior?

> Source/WebCore/dom/Position.h:97
> +        if (m_isLegacyEditingPosition || m_anchorType != PositionIsAfterAnchor)
> +            return m_offset;
> +        return offsetForPositionAfterAnchor();

Might be nice to special case position after node instead so that condition here matches the assertion?

[WebKit Review Bot]

Attachment 83422 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7952018

[Ryosuke Niwa]

Comment on attachment 83422 [details]
Patch

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

Taking back my r+ given the build failure.

> Source/WebCore/dom/Position.cpp:149
> +int Position::deprecatedOffsetForAfterPosition() const

This should be offsetForPositionAfterAnchor instead.

[Build Bot]

Attachment 83422 [details] did not build on win:
Build output: http://queues.webkit.org/results/7960034

[Eric Seidel (no email)]

Comment on attachment 83422 [details]
Patch

deprecatedEditingOffset was never meant to be able to be derived from new-style positions.  My thought had been instead that we would add the ASSERT(m_isLegacyEditingPosition) and then fix all the places that hit the ASSERT before adding new uses of new-style positions.

[Early Warning System Bot]

Attachment 83422 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7962039

[WebKit Review Bot]

Attachment 83422 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7964043

[Collabora GTK+ EWS bot]

Attachment 83422 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7985046

[WebKit Review Bot]

Attachment 83422 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7987031

[Levi Weintraub]

Created attachment 83509 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 83509 [details]
Patch

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

This was never intended to work.  But if this makes your transition job easier I guess it's OK.

> Source/WebCore/dom/Position.cpp:151
> +    ASSERT(m_anchorType == PositionIsAfterAnchor && !m_isLegacyEditingPosition);

Aren't these two redundant?  Also generally we try to have one ASSERT per line as that makes it easier to see from the debug traces which one failed.

[Eric Seidel (no email)]

Did you investigate making the ASSERT(m_isLegacy) a reality and seeing what callsites were assuming otherwise?

[Levi Weintraub]

(In reply to comment #20)
> > Source/WebCore/dom/Position.cpp:151
> > +    ASSERT(m_anchorType == PositionIsAfterAnchor && !m_isLegacyEditingPosition);
> 
> Aren't these two redundant?  Also generally we try to have one ASSERT per line as that makes it easier to see from the debug traces which one failed.

These aren't redundant because legacy positions call anchorTypeForLegacyEditingPosition(). We literally want to assert that we're only looking at After type positions for New positions, as only these have an offset that doesn't mesh with the legacy behavior.

(In reply to comment #21)
> Did you investigate making the ASSERT(m_isLegacy) a reality and seeing what callsites were assuming otherwise?

The move that finally necessitated this was my fix for 52642, which ends up creating many more "New" positions. What I found in testing the change is that there are numerous call sites that are currently only working because they are still being fed legacy positions from first/lastDeepEditingPositionForNode. Removing calls to this function is definitely one of the next steps.

[Levi Weintraub]

Hey Ryosuke, can I get a review now that the build failure is fixed?

[Ryosuke Niwa]

Comment on attachment 83509 [details]
Patch

Please split the assertion into 2 lines per Eric's comment before you land.

Thanks for tackling a hard bug.  Hopefully, we can remove deprecatedEditingOffset() soon.

[Levi Weintraub]

Thanks for the review! Making the change and checking in.

[Levi Weintraub]

Committed r80195: <http://trac.webkit.org/changeset/80195>