Bug 25057 - remove rangeCompliantEquivalent and replace it with Position methods
Summary: remove rangeCompliantEquivalent and replace it with Position methods
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Enhancement
Assignee: Nobody
URL:
Keywords:
: 24045 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-04-06 02:22 PDT by Eric Seidel (no email)
Modified: 2011-01-19 01:32 PST (History)
11 users (show)

See Also:


Attachments
Patch (27.12 KB, patch)
2010-12-07 18:05 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (27.58 KB, patch)
2010-12-08 10:01 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (27.63 KB, patch)
2010-12-08 11:09 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (27.68 KB, patch)
2011-01-05 15:28 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (28.32 KB, patch)
2011-01-18 11:17 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (28.24 KB, patch)
2011-01-18 14:09 PST, Levi Weintraub
no flags Details | Formatted Diff | Diff
Here is the crash log (35.46 KB, text/plain)
2011-01-18 17:27 PST, Eric Seidel (no email)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2009-04-06 02:22:54 PDT
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.
Comment 1 Eric Seidel (no email) 2009-04-06 02:26:23 PDT
*** Bug 24045 has been marked as a duplicate of this bug. ***
Comment 2 Levi Weintraub 2010-12-07 18:05:56 PST
Created attachment 75855 [details]
Patch
Comment 3 Levi Weintraub 2010-12-07 18:08:39 PST
(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.
Comment 4 WebKit Review Bot 2010-12-07 22:00:06 PST
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.
Comment 5 Ryosuke Niwa 2010-12-07 22:26:40 PST
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?
Comment 6 Levi Weintraub 2010-12-08 10:01:31 PST
Created attachment 75917 [details]
Patch
Comment 7 Levi Weintraub 2010-12-08 10:04:06 PST
(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.
Comment 8 Ryosuke Niwa 2010-12-08 10:12:30 PST
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.
Comment 9 Levi Weintraub 2010-12-08 10:28:04 PST
(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 :)
Comment 10 Levi Weintraub 2010-12-08 11:09:28 PST
Created attachment 75931 [details]
Patch
Comment 11 Ryosuke Niwa 2010-12-08 11:31:25 PST
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.
Comment 12 Eric Seidel (no email) 2010-12-24 10:21:07 PST
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.
Comment 13 Ryosuke Niwa 2010-12-24 11:46:19 PST
(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.
Comment 14 Levi Weintraub 2011-01-05 08:28:43 PST
(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()
Comment 15 Levi Weintraub 2011-01-05 15:28:39 PST
Created attachment 78056 [details]
Patch
Comment 16 Levi Weintraub 2011-01-05 15:29:14 PST
(In reply to comment #15)
> Created an attachment (id=78056) [details]
> Patch

Adding a FIXME as per a conversation with Ryosuke.
Comment 17 Ryosuke Niwa 2011-01-05 15:35:41 PST
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.
Comment 18 Ryosuke Niwa 2011-01-05 15:36:22 PST
Oops, s/justining/justing/.  Sorry Justin.
Comment 19 Darin Adler 2011-01-05 21:15:55 PST
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?
Comment 20 Darin Adler 2011-01-05 21:17:12 PST
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.
Comment 21 Ryosuke Niwa 2011-01-05 21:30:30 PST
(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.
Comment 22 Levi Weintraub 2011-01-06 10:27:17 PST
(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.
Comment 23 Ryosuke Niwa 2011-01-18 09:15:53 PST
Darin, are you opposed to this change?  If not, I'm inclined to give r+.
Comment 24 Darin Adler 2011-01-18 09:25:27 PST
(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.
Comment 25 Ryosuke Niwa 2011-01-18 09:29:01 PST
(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.
Comment 26 WebKit Commit Bot 2011-01-18 09:36:43 PST
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
Comment 27 Levi Weintraub 2011-01-18 10:23:32 PST
(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.
Comment 28 Levi Weintraub 2011-01-18 11:17:44 PST
Created attachment 79298 [details]
Patch
Comment 29 WebKit Commit Bot 2011-01-18 13:48:26 PST
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
Comment 30 Levi Weintraub 2011-01-18 14:09:30 PST
Created attachment 79327 [details]
Patch
Comment 31 Levi Weintraub 2011-01-18 14:11:16 PST
Comment on attachment 79327 [details]
Patch

Updating again to fix merge issues with ToT
Comment 32 WebKit Commit Bot 2011-01-18 17:24:13 PST
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
Comment 33 Eric Seidel (no email) 2011-01-18 17:27:09 PST
Created attachment 79365 [details]
Here is the crash log
Comment 34 WebKit Commit Bot 2011-01-19 01:32:29 PST
Comment on attachment 79327 [details]
Patch

Clearing flags on attachment: 79327

Committed r76107: <http://trac.webkit.org/changeset/76107>
Comment 35 WebKit Commit Bot 2011-01-19 01:32:36 PST
All reviewed patches have been landed.  Closing bug.