endOfLine and logicalEndOfLine duplicate a lot of code. We should merge them as well as startOfLine and logicalStartOfLine.
Created attachment 112240 [details] cleanup
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?
(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 :)
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 :)
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?
> > 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.
(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?
(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?
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.
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
(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.
Created attachment 112250 [details] Fixed per comments
Ping reviewers.
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. :)
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.
Committed r98358: <http://trac.webkit.org/changeset/98358>