145285 – previousRootInlineBoxCandidatePosition & nextRootInlineBoxCandidatePosition FIXME to remove duplicate code

Bug 145285 - previousRootInlineBoxCandidatePosition & nextRootInlineBoxCandidatePosition FIXME to remove duplicate code

Summary: previousRootInlineBoxCandidatePosition & nextRootInlineBoxCandidatePosition F...

Status:	NEW

Alias:	None

Product:	WebKit
Classification:	Unclassified
Component:	Text (show other bugs)
Version:	528+ (Nightly build)
Hardware:	Unspecified Unspecified

Importance:	P2 Normal
Assignee:	Nobody

URL:
Keywords:

Depends on:
Blocks:

Reported:	2015-05-21 14:51 PDT by Doug Russell
Modified:	2015-05-27 09:48 PDT (History)
CC List:	3 users (show)

See Also:

Attachments
patch (4.58 KB, patch) 2015-05-21 15:13 PDT, Doug Russell	no flags	Details \| Formatted Diff \| Diff
patch (5.89 KB, patch) 2015-05-21 15:27 PDT, Doug Russell	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews102 for mac-mavericks (1.46 MB, application/zip) 2015-05-21 16:00 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews105 for mac-mavericks-wk2 (1.50 MB, application/zip) 2015-05-21 16:05 PDT, Build Bot	no flags	Details
patch (6.01 KB, patch) 2015-05-21 16:40 PDT, Doug Russell	darin: review-	Details \| Formatted Diff \| Diff
patch (6.12 KB, patch) 2015-05-26 12:31 PDT, Doug Russell	buildbot: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews103 for mac-mavericks (627.52 KB, application/zip) 2015-05-26 13:10 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews104 for mac-mavericks-wk2 (301.42 KB, application/zip) 2015-05-26 13:32 PDT, Build Bot	no flags	Details
patch (6.23 KB, patch) 2015-05-26 14:59 PDT, Doug Russell	darin: review- buildbot: commit-queue-	Details \| Formatted Diff \| Diff
Archive of layout-test-results from ews100 for mac-mavericks (359.94 KB, application/zip) 2015-05-26 15:18 PDT, Build Bot	no flags	Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (706.48 KB, application/zip) 2015-05-26 15:39 PDT, Build Bot	no flags	Details
Show Obsolete (4) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.

Description Doug Russell 2015-05-21 14:51:00 PDT

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

Comment 1 Doug Russell 2015-05-21 15:13:22 PDT

Created attachment 253547 [details]
patch

Comment 2 WebKit Commit Bot 2015-05-21 15:16:15 PDT

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.

Comment 3 Doug Russell 2015-05-21 15:27:15 PDT

Created attachment 253548 [details]
patch

style fix

Comment 4 Build Bot 2015-05-21 16:00:57 PDT

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

Comment 5 Build Bot 2015-05-21 16:00:59 PDT

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

Comment 6 Build Bot 2015-05-21 16:05:05 PDT

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

Comment 7 Build Bot 2015-05-21 16:05:08 PDT

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

Comment 8 Doug Russell 2015-05-21 16:40:38 PDT

Created attachment 253551 [details]
patch

Comment 9 Darin Adler 2015-05-26 11:42:58 PDT

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.

Comment 10 Doug Russell 2015-05-26 12:31:17 PDT

Created attachment 253716 [details]
patch

Comment 11 Build Bot 2015-05-26 13:10:40 PDT

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.

Comment 12 Build Bot 2015-05-26 13:10:43 PDT

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

Comment 13 Ryosuke Niwa 2015-05-26 13:28:31 PDT

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?"

Comment 14 Doug Russell 2015-05-26 13:31:23 PDT

(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?

Comment 15 Build Bot 2015-05-26 13:32:03 PDT

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.

Comment 16 Build Bot 2015-05-26 13:32:06 PDT

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

Comment 17 Doug Russell 2015-05-26 14:59:31 PDT

Created attachment 253743 [details]
patch

Comment 18 Build Bot 2015-05-26 15:18:29 PDT

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.

Comment 19 Build Bot 2015-05-26 15:18:33 PDT

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

Comment 20 Build Bot 2015-05-26 15:39:26 PDT

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.

Comment 21 Build Bot 2015-05-26 15:39:30 PDT

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

Comment 22 Doug Russell 2015-05-26 15:50:09 PDT

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

Comment 23 Ryosuke Niwa 2015-05-26 19:57:42 PDT

(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.

Comment 24 Darin Adler 2015-05-27 09:48:26 PDT

Comment on attachment 253743 [details]
patch

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