Bug 145285 – previousRootInlineBoxCandidatePosition & nextRootInlineBoxCandidatePosition FIXME to remove duplicate code

• Status: NEW
• Component: Text

[Doug Russell]

Consolidate the code common to previousRootInlineBoxCandidatePosition & nextRootInlineBoxCandidatePosition into a shared function.

[Doug Russell]

Created attachment 253547 [details]
patch

[WebKit Commit Bot]

Attachment 253547 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/VisibleUnits.cpp:76:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 1 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Doug Russell]

Created attachment 253548 [details]
patch

style fix

[Build Bot]

Comment on attachment 253548 [details]
patch

Attachment 253548 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/4850688005767168

New failing tests:
editing/deleting/delete-at-paragraph-boundaries-006.html
editing/deleting/delete-at-paragraph-boundaries-004.html
editing/deleting/delete-at-paragraph-boundaries-011.html
editing/selection/move-by-line-002.html
editing/deleting/delete-at-paragraph-boundaries-001.html
editing/selection/3690703.html
editing/selection/move-by-word-visually-multi-line.html
editing/deleting/delete-line-017.html
editing/deleting/delete-at-paragraph-boundaries-005.html
editing/selection/move-by-word-visually-mac.html
editing/deleting/delete-at-paragraph-boundaries-003.html
editing/selection/previous-line-position.html
editing/pasteboard/paste-text-017.html
editing/deleting/delete-at-paragraph-boundaries-002.html

[Build Bot]

Created attachment 253549 [details]
Archive of layout-test-results from ews102 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 253548 [details]
patch

Attachment 253548 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5669659885961216

New failing tests:
editing/deleting/delete-at-paragraph-boundaries-006.html
editing/deleting/delete-at-paragraph-boundaries-004.html
editing/deleting/delete-at-paragraph-boundaries-011.html
editing/selection/move-by-line-002.html
editing/deleting/delete-at-paragraph-boundaries-001.html
editing/selection/3690703.html
editing/selection/move-by-word-visually-multi-line.html
editing/deleting/delete-line-017.html
editing/deleting/delete-at-paragraph-boundaries-005.html
editing/selection/move-by-word-visually-mac.html
editing/deleting/delete-at-paragraph-boundaries-003.html
editing/selection/previous-line-position.html
editing/pasteboard/paste-text-017.html
editing/deleting/delete-at-paragraph-boundaries-002.html

[Build Bot]

Created attachment 253550 [details]
Archive of layout-test-results from ews105 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Doug Russell]

Created attachment 253551 [details]
patch

[Darin Adler]

Comment on attachment 253551 [details]
patch

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

Does seem like an improvement. But we should do a little more to make this even better.

> Source/WebCore/editing/VisibleUnits.cpp:78
> +};

Extraneous semicolon here. This should be an enum class, not a namespaced non-class enum.

> Source/WebCore/editing/VisibleUnits.cpp:83
> +    Node* leaf = direction == BoxCandidatePosition::Next ? nextLeafWithSameEditability(node, editableType) : previousLeafWithSameEditability(node, editableType);

I think this would read better with a helper function to avoid the ? : every time.

[Doug Russell]

Created attachment 253716 [details]
patch

[Build Bot]

Comment on attachment 253716 [details]
patch

Attachment 253716 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6720744683732992

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 253723 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Ryosuke Niwa]

Comment on attachment 253716 [details]
patch

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

> Source/WebCore/editing/VisibleUnits.cpp:86
> +    Node* leaf = leafWithSameEditability(direction, node, editableType, editableType);

We should be consistent with whether "direction" is the first argument or the last argument.

> Source/WebCore/editing/VisibleUnits.cpp:102
> +            if (leaf->hasTagName(brTag))
> +                pos = positionBeforeNode(leaf);
> +            else
> +                pos = createLegacyEditingPosition(leaf, caretMaxOffset(leaf));
> +        } else
> +            pos = createLegacyEditingPosition(leaf, caretMinOffset(leaf));

Do you know why only previous direction requires a check for BR?
Not saying that you need to figure out why but it deserves a why comment.
We can add a FIXME like "Why do we only do this for previous direction?"

[Doug Russell]

(In reply to comment #13)
> Comment on attachment 253716 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=253716&action=review
> 
> > Source/WebCore/editing/VisibleUnits.cpp:86
> > +    Node* leaf = leafWithSameEditability(direction, node, editableType, editableType);
> 
> We should be consistent with whether "direction" is the first argument or
> the last argument.

I'll move it to the last arg in the helper function

> 
> > Source/WebCore/editing/VisibleUnits.cpp:102
> > +            if (leaf->hasTagName(brTag))
> > +                pos = positionBeforeNode(leaf);
> > +            else
> > +                pos = createLegacyEditingPosition(leaf, caretMaxOffset(leaf));
> > +        } else
> > +            pos = createLegacyEditingPosition(leaf, caretMinOffset(leaf));
> 
> Do you know why only previous direction requires a check for BR?
> Not saying that you need to figure out why but it deserves a why comment.
> We can add a FIXME like "Why do we only do this for previous direction?"

Not sure. I'll add the FIXME.

Should I add a similar FIXME for editableType being fixed to ContentIsEditable only in the next direction for VisibleUnits.cpp:89?

[Build Bot]

Comment on attachment 253716 [details]
patch

Attachment 253716 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4524733273997312

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 253728 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Doug Russell]

Created attachment 253743 [details]
patch

[Build Bot]

Comment on attachment 253743 [details]
patch

Attachment 253743 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5158051971596288

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 253745 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 253743 [details]
patch

Attachment 253743 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6313273620168704

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 253749 [details]
Archive of layout-test-results from ews106 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Doug Russell]

Looks like something is wrong with tests unrelated to this patch?

[Ryosuke Niwa]

(In reply to comment #22)
> Looks like something is wrong with tests unrelated to this patch?

They're editing tests so you probbaly broke something.

[Darin Adler]

Comment on attachment 253743 [details]
patch

Lots of editing tests failing. Please upload a new patch but run editing tests locally first.