Bug 70755 – Merge endOfLine with logicalEndOfLine and startOfLine with logicalStartOfLine

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Ryosuke Niwa]

endOfLine and logicalEndOfLine duplicate a lot of code. We should merge them as well as startOfLine and logicalStartOfLine.

[Ryosuke Niwa]

Created attachment 112240 [details]
cleanup

[Levi Weintraub]

Yay code simplification! What failed when you simply replaced one with the other?

Also, did you file bugs for placing the position after a table for end and for placing the position at [table, 0] for start?

[Ryosuke Niwa]

(In reply to comment #2)
> Yay code simplification! What failed when you simply replaced one with the other?

There was some performance concern since VisiblePosition::VisiblePosition calls inSameLine

> Also, did you file bugs for placing the position after a table for end and for placing the position at [table, 0] for start?

I'm about to :)

[Levi Weintraub]

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

> Source/WebCore/editing/visible_units.cpp:332
> +enum ShouldUseLogicalStart { UseLogicalStart, DoNotUseLogicalStart };

It seems like it'd be more informative to use a second descriptor like Visual instead of negating Logical.

> Source/WebCore/editing/visible_units.cpp:408
> +enum ShouldUseLogicalEnd { DoNotUseLogicalEnd, UseLogicalEnd };
> +static VisiblePosition endPositionForLine(const VisiblePosition& c, bool shouldUseLogicalEnd)

It seems kind of like overkill to define new enums for both of these static functions instead of re-using one that just says to behave logically or not. Either way, you're using a bool in this function instead of the enum :)

[Kenneth Rohde Christiansen]

Comment on attachment 112240 [details]
cleanup

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

Some comments for now. It is late here but I might be able to take another look tomorrow.

> Source/WebCore/editing/visible_units.cpp:332
> -static VisiblePosition startPositionForLine(const VisiblePosition& c)
> +enum ShouldUseLogicalStart { UseLogicalStart, DoNotUseLogicalStart };

This sounds a bit weird, what is the actual difference? something like the below?

enum PositionType { LogicalPosition, ActualPosition }; ?

> Source/WebCore/editing/visible_units.cpp:357
> +        // Generated content (e.g. list markers and CSS :before and :after pseudoelements) have no corresponding DOM element,
> +        // and so cannot be represented by a VisiblePosition. Use whatever follows instead.

I normally try to keep comments within 80-100 chars. These are pretty long

> Source/WebCore/editing/visible_units.cpp:359
> +        while (1) {

I would have used while (true)

> Source/WebCore/editing/visible_units.cpp:388
> +    if (shouldUseLogicalStart == UseLogicalStart) {
> +        if (Node* editableRoot = highestEditableRoot(c.deepEquivalent())) {
> +            if (!editableRoot->contains(visPos.deepEquivalent().containerNode()))

What about &&'ing these?

> Source/WebCore/editing/visible_units.cpp:397
> +VisiblePosition startOfLine(const VisiblePosition& currentPosition)

inline?

> Source/WebCore/editing/visible_units.cpp:443
> +            endNode = endRenderer->node();
> +            if (endNode)
> +                break;

You don't seem to be using endNode after this, so why not if (endNode = endRenderer->node()) ?

> Source/WebCore/editing/visible_units.cpp:453
> +        pos = Position(static_cast<Text*>(endNode), static_cast<InlineTextBox *>(endBox)->caretMaxOffset());

Coding style, *.

Don't we have a toInlineTextBox?

[Kenneth Rohde Christiansen]

> > Source/WebCore/editing/visible_units.cpp:408
> > +enum ShouldUseLogicalEnd { DoNotUseLogicalEnd, UseLogicalEnd };
> > +static VisiblePosition endPositionForLine(const VisiblePosition& c, bool shouldUseLogicalEnd)
> 
> It seems kind of like overkill to define new enums for both of these static functions instead of re-using one that just says to behave logically or not. Either way, you're using a bool in this function instead of the enum :)

It makes the call sites a lot more clear, so I guess it is fine.

[Ryosuke Niwa]

(In reply to comment #4)
> View in context: https://bugs.webkit.org/attachment.cgi?id=112240&action=review
> 
> > Source/WebCore/editing/visible_units.cpp:332
> > +enum ShouldUseLogicalStart { UseLogicalStart, DoNotUseLogicalStart };
> 
> It seems like it'd be more informative to use a second descriptor like Visual instead of negating Logical.

The problem is that it's not visual either. Obtaining the first leaf or the last leaf result in grabbing whatever box that happens to be at the end. i.e. So the caret can be rendered anywhere.

As you know, we convert WebKit convention (node, offset) pair to (inlinebox, offset) pair in getInlineBoxAndOffset? endOfLine and startOfLine are bypassing this process and giving you whatever node that happens to render text at the beginning of at the end of a line. It basically gives you bogus result.

IMHO, we can call these functions only if we're deciding whether two positions belong to the same line or not.

> > Source/WebCore/editing/visible_units.cpp:408
> > +enum ShouldUseLogicalEnd { DoNotUseLogicalEnd, UseLogicalEnd };
> > +static VisiblePosition endPositionForLine(const VisiblePosition& c, bool shouldUseLogicalEnd)
> 
> It seems kind of like overkill to define new enums for both of these static functions instead of re-using one that just says to behave logically or not. Either way, you're using a bool in this function instead of the enum :)

Any suggestions?

[Levi Weintraub]

(In reply to comment #7)
> (In reply to comment #4)
> > View in context: https://bugs.webkit.org/attachment.cgi?id=112240&action=review
> > 
> > > Source/WebCore/editing/visible_units.cpp:332
> > > +enum ShouldUseLogicalStart { UseLogicalStart, DoNotUseLogicalStart };
> > 
> > It seems like it'd be more informative to use a second descriptor like Visual instead of negating Logical.
> 
> The problem is that it's not visual either. Obtaining the first leaf or the last leaf result in grabbing whatever box that happens to be at the end. i.e. So the caret can be rendered anywhere.
> 
> As you know, we convert WebKit convention (node, offset) pair to (inlinebox, offset) pair in getInlineBoxAndOffset? endOfLine and startOfLine are bypassing this process and giving you whatever node that happens to render text at the beginning of at the end of a line. It basically gives you bogus result.
> 
> IMHO, we can call these functions only if we're deciding whether two positions belong to the same line or not.

That's totally fair. Sounds like UseIllogicalStart to me ;)

> 
> > > Source/WebCore/editing/visible_units.cpp:408
> > > +enum ShouldUseLogicalEnd { DoNotUseLogicalEnd, UseLogicalEnd };
> > > +static VisiblePosition endPositionForLine(const VisiblePosition& c, bool shouldUseLogicalEnd)
> > 
> > It seems kind of like overkill to define new enums for both of these static functions instead of re-using one that just says to behave logically or not. Either way, you're using a bool in this function instead of the enum :)
> 
> Any suggestions?

UseLogicalOrdering, UseNativeOrdering?

[Chang Shu]

Comment on attachment 112240 [details]
cleanup

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

> Source/WebCore/editing/visible_units.cpp:344
> -            return positionAvoidingFirstPositionInTable(c);
> +            return c;

It doesn't look like a code simplification here to me. I noticed you have another bug dealing with this function.

[Enrica Casucci]

Comment on attachment 112240 [details]
cleanup

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

> Source/WebCore/editing/visible_units.cpp:344
> +            return c;

Why this change in this patch?

> Source/WebCore/editing/visible_units.cpp:407
> +enum ShouldUseLogicalEnd { DoNotUseLogicalEnd, UseLogicalEnd };

Is it really necessary to have a separate enum for start and end? The only distinction is UseLogical, don't use logical.

> Source/WebCore/editing/visible_units.cpp:433
> +        while (1) {

Use while(true) here too

[Ryosuke Niwa]

(In reply to comment #5)
> > Source/WebCore/editing/visible_units.cpp:332
> > -static VisiblePosition startPositionForLine(const VisiblePosition& c)
> > +enum ShouldUseLogicalStart { UseLogicalStart, DoNotUseLogicalStart };
> 
> This sounds a bit weird, what is the actual difference? something like the below?
> 
> enum PositionType { LogicalPosition, ActualPosition }; ?

> > Source/WebCore/editing/visible_units.cpp:407
> > +enum ShouldUseLogicalEnd { DoNotUseLogicalEnd, UseLogicalEnd };
> 
> Is it really necessary to have a separate enum for start and end? The only distinction is UseLogical, don't use logical.

Replaced by enum LineEndpointComputationMode { UseLogicalOrdering, UseInlineBoxOrdering }; per discussion with Levi.

> > Source/WebCore/editing/visible_units.cpp:357
> > +        // Generated content (e.g. list markers and CSS :before and :after pseudoelements) have no corresponding DOM element,
> > +        // and so cannot be represented by a VisiblePosition. Use whatever follows instead.
> 
> I normally try to keep comments within 80-100 chars. These are pretty long

The comment is shorter than the line 342.

> > Source/WebCore/editing/visible_units.cpp:359
> > +        while (1) {
> 
> I would have used while (true)

Done.

> > Source/WebCore/editing/visible_units.cpp:388
> > +    if (shouldUseLogicalStart == UseLogicalStart) {
> > +        if (Node* editableRoot = highestEditableRoot(c.deepEquivalent())) {
> > +            if (!editableRoot->contains(visPos.deepEquivalent().containerNode()))
> 
> What about &&'ing these?

Can't because of the second if and the changing the order of if's will add O(n) when shouldUseLogicalStart = DoNotUseLogicalStart.

> > Source/WebCore/editing/visible_units.cpp:397
> > +VisiblePosition startOfLine(const VisiblePosition& currentPosition)
> 
> inline?

Can't startOfLine is called by other cpp files. I don't expect it to be a performance hit, and I'd like to keep the version that takes enum to be local static for now.

> > Source/WebCore/editing/visible_units.cpp:443
> > +            endNode = endRenderer->node();
> > +            if (endNode)
> > +                break;
> 
> You don't seem to be using endNode after this, so why not if (endNode = endRenderer->node()) ?

We do use it. See if's at the end of the function.

> > Source/WebCore/editing/visible_units.cpp:453
> > +        pos = Position(static_cast<Text*>(endNode), static_cast<InlineTextBox *>(endBox)->caretMaxOffset());
> 
> Coding style, *.

Done.

> Don't we have a toInlineTextBox?

Yes, fixed.

(In reply to comment #9)
> > Source/WebCore/editing/visible_units.cpp:344
> > -            return positionAvoidingFirstPositionInTable(c);
> > +            return c;
> 
> It doesn't look like a code simplification here to me. I noticed you have another bug dealing with this function.

(In reply to comment #10)
> > Source/WebCore/editing/visible_units.cpp:344
> > +            return c;
> 
> Why this change in this patch?

Oops, this is a left over from the other patch. Reverted.

> > Source/WebCore/editing/visible_units.cpp:433
> > +        while (1) {
> 
> Use while(true) here too

Done.

[Ryosuke Niwa]

Created attachment 112250 [details]
Fixed per comments

[Ryosuke Niwa]

Ping reviewers.

[Chang Shu]

Comment on attachment 112250 [details]
Fixed per comments

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

looks good to me.

> Source/WebCore/editing/visible_units.cpp:453
> -    } else if (endBox->isInlineTextBox() && endNode->isTextNode()) {
> -        InlineTextBox* endTextBox = static_cast<InlineTextBox *>(endBox);
> -        int endOffset = endTextBox->start();
> -        if (!endTextBox->isLineBreak())
> -            endOffset += endTextBox->len();
> -        pos = Position(static_cast<Text*>(endNode), endOffset);
> -    } else
> +    else if (endBox->isInlineTextBox() && endNode->isTextNode())
> +        pos = Position(static_cast<Text*>(endNode), toInlineTextBox(endBox)->caretMaxOffset());
> +    else

I trust you they are equivalent. :)

[Ryosuke Niwa]

Comment on attachment 112250 [details]
Fixed per comments

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

>> Source/WebCore/editing/visible_units.cpp:453

> 
> I trust you they are equivalent. :)

Okay, let me revert this part of the change just to be safe.

[Ryosuke Niwa]

Committed r98358: <http://trac.webkit.org/changeset/98358>