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.
Created attachment 83364 [details] Patch
Created attachment 83409 [details] Patch
(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 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.
(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 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?
(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.
(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?
Created attachment 83422 [details] Patch
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?
Attachment 83422 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7952018
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.
Attachment 83422 [details] did not build on win: Build output: http://queues.webkit.org/results/7960034
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.
Attachment 83422 [details] did not build on qt: Build output: http://queues.webkit.org/results/7962039
Attachment 83422 [details] did not build on chromium: Build output: http://queues.webkit.org/results/7964043
Attachment 83422 [details] did not build on gtk: Build output: http://queues.webkit.org/results/7985046
Attachment 83422 [details] did not build on mac: Build output: http://queues.webkit.org/results/7987031
Created attachment 83509 [details] Patch
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.
Did you investigate making the ASSERT(m_isLegacy) a reality and seeing what callsites were assuming otherwise?
(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.
Hey Ryosuke, can I get a review now that the build failure is fixed?
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.
Thanks for the review! Making the change and checking in.
Committed r80195: <http://trac.webkit.org/changeset/80195>