| Summary: | previousRootInlineBoxCandidatePosition & nextRootInlineBoxCandidatePosition FIXME to remove duplicate code | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Doug Russell <d_russell> | ||||||||||||||||||||||||
| Component: | Text | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||||||||||
| Status: | NEW --- | ||||||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, rniwa | ||||||||||||||||||||||||
| Priority: | P2 | ||||||||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||
|
Description
Doug Russell
2015-05-21 14:51:00 PDT
Created attachment 253547 [details]
patch
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.
Created attachment 253548 [details]
patch
style fix
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 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 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 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
Created attachment 253551 [details]
patch
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. Created attachment 253716 [details]
patch
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. 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 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?" (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 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. 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
Created attachment 253743 [details]
patch
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. 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 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. 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
Looks like something is wrong with tests unrelated to this patch? (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 on attachment 253743 [details]
patch
Lots of editing tests failing. Please upload a new patch but run editing tests locally first.
|