Bug 81593 – Merge nextRootInlineBox with nextLinePosition

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ryosuke Niwa]

Merge nextRootInlineBox with nextLinePosition

[Ryosuke Niwa]

Created attachment 132721 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 132721 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        Refactroed nextLinePosition and previousLinePosition to use nextRootInlineBox and previousRootInlineBox.
> +

Can you give me a bit more high-level overview of what's going on here? What did these fucntions used to do?  Why are we merging them, etc...

[Ryosuke Niwa]

(In reply to comment #2)
> (From update of attachment 132721 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=132721&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        Refactroed nextLinePosition and previousLinePosition to use nextRootInlineBox and previousRootInlineBox.
> > +
> 
> Can you give me a bit more high-level overview of what's going on here? What did these fucntions used to do?  Why are we merging them, etc...

nextRootInlineBox and previousRootInlineBox look for previous or next root inline boxes across renderers, and nextLinePosition and previousLinePosition had duplicated code in them.

I had asked xji (who recently added nextRootInlineBox and previousRootInlineBox) to refactor them later but it turned out that I needed this refactoring in order to fix the bug 81490.

[WebKit Review Bot]

Comment on attachment 132721 [details]
Patch

Attachment 132721 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12000071

New failing tests:
editing/selection/3690703-2.html
editing/selection/3690703.html

[Ryosuke Niwa]

Odd. This failure doesn't reproduce on Windows port either :(

[Tony Chang]

Diffs from chromium-linux:

--- /src/chrome/src/webkit/Release/layout-test-results/editing/selection/3690703-2-expected.txt 
+++ /src/chrome/src/webkit/Release/layout-test-results/editing/selection/3690703-2-actual.txt 
@@ -147,5 +147,5 @@
           RenderInline {FONT} at (0,0) size 60x12
             RenderText {#text} at (362,0) size 60x12
               text run at (362,0) width 60: "\x{A9}2005 Google"
-selection start: position 0 of child 3 {INPUT} of child 1 {TD} of child 0 {TR} of child 0 {TBODY} of child 2 {TABLE} of child 4 {DIV} of child 0 {CENTER} of body
-selection end:   position 1 of child 2 {BR} of child 0 {FONT} of child 2 {TD} of child 0 {TR} of child 0 {TBODY} of child 2 {TABLE} of child 4 {DIV} of child 0 {CENTER} of body
+selection start: position 0 of child 0 {#text} of child 0 {FONT} of child 2 {TD} of child 0 {TR} of child 0 {TBODY} of child 2 {TABLE} of child 4 {DIV} of child 0 {CENTER} of body
+selection end:   position 1 of child 5 {BR} of child 0 {FONT} of child 2 {TD} of child 0 {TR} of child 0 {TBODY} of child 2 {TABLE} of child 4 {DIV} of child 0 {CENTER} of body

--- /src/chrome/src/webkit/Release/layout-test-results/editing/selection/3690703-expected.txt 
+++ /src/chrome/src/webkit/Release/layout-test-results/editing/selection/3690703-actual.txt 
@@ -1,6 +1,5 @@
 EDITING DELEGATE: shouldBeginEditingInDOMRange:range from 0 of DIV > CENTER > BODY > HTML > #document to 6 of DIV > CENTER > BODY > HTML > #document
 EDITING DELEGATE: webViewDidBeginEditing:WebViewDidBeginEditingNotification
-EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification
 EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification

[Ryosuke Niwa]

I don't quite understand what's causing the failure. As far as I know, there should be no difference between Mac and Chromium Linux when moving or extending selection via Selection.modify.

Unfortunately, it seems intractable to debug this regression without access to cr-linux, so I'll give up on this patch.

[Ryosuke Niwa]

Created attachment 141880 [details]
Less intrusive patch

[Ryosuke Niwa]

Xiaomei must have cleaned up the code there since this patch was much easier to write.

[Ryosuke Niwa]

Comment on attachment 141880 [details]
Less intrusive patch

Ugh... that's because I didn't complete the second half.

[Ryosuke Niwa]

Created attachment 141901 [details]
Deleted more code but considerably more complicated patch

[Enrica Casucci]

Comment on attachment 141901 [details]
Deleted more code but considerably more complicated patch

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

The change looks good.

> Source/WebCore/ChangeLog:9
> +        to share the code. Moved out the nullity check of startBox and extraction of the renderer's node from

should be: "and extracted the renderer's node..."

[Ryosuke Niwa]

Committed r117359: <http://trac.webkit.org/changeset/117359>