Bug 54986 - deprecatedEditingOffset should actually return the expected deprecated value for "after" positions
Summary: deprecatedEditingOffset should actually return the expected deprecated value ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 52642
  Show dependency treegraph
 
Reported: 2011-02-22 12:29 PST by Levi Weintraub
Modified: 2011-03-02 18:30 PST (History)
10 users (show)

See Also:


Attachments
Patch (2.27 KB, patch)
2011-02-22 12:38 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (2.30 KB, patch)
2011-02-22 16:22 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (2.66 KB, patch)
2011-02-22 18:15 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (2.66 KB, patch)
2011-02-23 11:23 PST, Levi Weintraub
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Levi Weintraub 2011-02-22 12:29:59 PST
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.
Comment 1 Levi Weintraub 2011-02-22 12:38:52 PST
Created attachment 83364 [details]
Patch
Comment 2 Levi Weintraub 2011-02-22 16:22:29 PST
Created attachment 83409 [details]
Patch
Comment 3 Levi Weintraub 2011-02-22 16:23:22 PST
(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.
Comment 4 Ryosuke Niwa 2011-02-22 17:01:57 PST
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.
Comment 5 Levi Weintraub 2011-02-22 17:04:24 PST
(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.
Comment 6 Ryosuke Niwa 2011-02-22 17:09:40 PST
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?
Comment 7 Levi Weintraub 2011-02-22 17:20:48 PST
(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.
Comment 8 Ryosuke Niwa 2011-02-22 17:47:55 PST
(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?
Comment 9 Levi Weintraub 2011-02-22 18:15:08 PST
Created attachment 83422 [details]
Patch
Comment 10 Ryosuke Niwa 2011-02-22 18:25:48 PST
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?
Comment 11 WebKit Review Bot 2011-02-22 18:30:42 PST
Attachment 83422 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7952018
Comment 12 Ryosuke Niwa 2011-02-22 18:34:10 PST
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.
Comment 13 Build Bot 2011-02-22 18:47:03 PST
Attachment 83422 [details] did not build on win:
Build output: http://queues.webkit.org/results/7960034
Comment 14 Eric Seidel (no email) 2011-02-22 19:22:58 PST
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.
Comment 15 Early Warning System Bot 2011-02-22 19:35:45 PST
Attachment 83422 [details] did not build on qt:
Build output: http://queues.webkit.org/results/7962039
Comment 16 WebKit Review Bot 2011-02-22 20:42:10 PST
Attachment 83422 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/7964043
Comment 17 Collabora GTK+ EWS bot 2011-02-22 22:59:08 PST
Attachment 83422 [details] did not build on gtk:
Build output: http://queues.webkit.org/results/7985046
Comment 18 WebKit Review Bot 2011-02-22 23:08:01 PST
Attachment 83422 [details] did not build on mac:
Build output: http://queues.webkit.org/results/7987031
Comment 19 Levi Weintraub 2011-02-23 11:23:43 PST
Created attachment 83509 [details]
Patch
Comment 20 Eric Seidel (no email) 2011-02-23 12:40:51 PST
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.
Comment 21 Eric Seidel (no email) 2011-02-23 12:41:19 PST
Did you investigate making the ASSERT(m_isLegacy) a reality and seeing what callsites were assuming otherwise?
Comment 22 Levi Weintraub 2011-02-23 13:48:14 PST
(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.
Comment 23 Levi Weintraub 2011-03-02 15:03:25 PST
Hey Ryosuke, can I get a review now that the build failure is fixed?
Comment 24 Ryosuke Niwa 2011-03-02 18:00:56 PST
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.
Comment 25 Levi Weintraub 2011-03-02 18:16:15 PST
Thanks for the review! Making the change and checking in.
Comment 26 Levi Weintraub 2011-03-02 18:30:39 PST
Committed r80195: <http://trac.webkit.org/changeset/80195>