Bug 42689 – Move setEditingBehavior() from layoutTestController to window.internals

• Component: WebKit2
• Status: RESOLVED FIXED

[Sam Weinig]

WebKitTestRunner needs layoutTestController.setEditingBehavior

[Sam Weinig]

<rdar://problem/8213852>

[Simon Fraser (smfr)]

Seems like a good candidate to move to window.internal.

*** Bug 81738 has been marked as a duplicate of this bug. ***

Added editing/selection/move-by-word-visually-crash-test-5.html to the WebKit2 skip list in <http://trac.webkit.org/r111498>.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #2)
> Seems like a good candidate to move to window.internal.

I'll give it a shot.

[Caio Marcelo de Oliveira Filho]

Created attachment 142581 [details]
Patch

[Build Bot]

Comment on attachment 142581 [details]
Patch

Attachment 142581 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12718698

[WebKit Review Bot]

Comment on attachment 142581 [details]
Patch

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

New failing tests:
fast/dom/Document/CaretRangeFromPoint/caretRangeFromPoint-in-zoom-and-scroll.html
fast/repaint/japanese-rl-selection-repaint.html
fast/text/international/khmer-selection.html
editing/selection/after-line-break.html

[WebKit Review Bot]

Created attachment 142619 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Gyuyoung Kim]

CC'ing Alexey. similar my patch made a build problem yesterday. So, in my humble opinion, this patch needs to be reviewed by many reviewers as well.

[Gyuyoung Kim]

Comment on attachment 142581 [details]
Patch

To avoid build break on gtk, win ports, you have to add add symbol to symbol filters.
 - Source/autotools/symbols.filter
 - Source/WebKit2/win/WebKit2.def

[Caio Marcelo de Oliveira Filho]

Comment on attachment 142581 [details]
Patch

Removing r? until I fix the problems already pointed out.

[Hajime Morrita]

Comment on attachment 142581 [details]
Patch

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

Thanks for taking this! This is one of typical methods which should be on Internals object.
I added a few comment.

> Source/WebCore/testing/InternalSettings.cpp:343
> +        fprintf(stderr, "setEditingBehavior(): Passed invalid editing behavior. Should be 'mac', 'win', or 'unix'.\n");

Instead of printing the error message, You can throw an exception by setting "ec" parameter.

> Tools/ChangeLog:3
> +        Move setEditingBehavior() from layoutTestController to window.internals

The diff looks broken.

[Alexey Proskuryakov]

> similar my patch made a build problem yesterday.

That patch removed a private Mac API, which is not happening here.

As far as I can tell, it should be OK to remove setEditingBehavior from WebPreferencesPrivate.h.

[Caio Marcelo de Oliveira Filho]

Created attachment 142776 [details]
Patch

[Caio Marcelo de Oliveira Filho]

(In reply to comment #14)
> As far as I can tell, it should be OK to remove setEditingBehavior from WebPreferencesPrivate.h.

Alexey, the patch currently doesn't remove it, but it could. Who should I ask about it?

[Caio Marcelo de Oliveira Filho]

Changed the approach a little bit. InternalSettings now store the previous value and restore it when resetting, instead of looking for default value.

I changed the platform/wk2/Skipped based on Qt WK2 and Gtk WK2. I couldn't test in Mac WK2 yet.

[Alexey Proskuryakov]

> Alexey, the patch currently doesn't remove it, but it could. Who should I ask about it?

I'm not sure if I understand the question. Yes, it seems fine to remove setEditingBehavior from WebPreferencesPrivate.h, but this is only what I said before.

[Kenneth Rohde Christiansen]

Comment on attachment 142776 [details]
Patch

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

Very nice patch!

> Source/WebKit/qt/WebCoreSupport/DumpRenderTreeSupportQt.cpp:657
> +void DumpRenderTreeSupportQt::setEditingWindowsBehavior(QWebPage* page)

setWindowsEditingBehavior ? useWindowsBehaviorAsEditingBehavior ?

I think it could be a bit more clear

> Tools/DumpRenderTree/blackberry/DumpRenderTree.cpp:311
> +        // FIXME: Remove this once blackberry calls resetInternalsObject().

bug number?

[Caio Marcelo de Oliveira Filho]

Created attachment 142809 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 142809 [details]
Patch

Checking if Mac and Win will be happy with the changes.

[Caio Marcelo de Oliveira Filho]

Created attachment 142813 [details]
Patch

trying to make win bot apply the patch

[Hajime Morrita]

Comment on attachment 142813 [details]
Patch

Looks good. I hope we could see the green win build.

[Caio Marcelo de Oliveira Filho]

Lucas, since you are the win bot admin you might know this: only win ews can't apply the patch, do you happen to know what's going on?

The file in question is LayoutTests/fast/forms/selection-direction.html and uses "DOS Line Ending". I've tried both converting to Unix line ending or keeping the DOS line ending (the last two patches). Neither approach works. I am using git-svn.

[Caio Marcelo de Oliveira Filho]

...and webkit-patch upload, to upload these patches.

[Caio Marcelo de Oliveira Filho]

Created attachment 142993 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Committed r117771: <http://trac.webkit.org/changeset/117771>

[Jesus Sanchez-Palencia]

Comment on attachment 142993 [details]
Patch

Clearing flags.

[Darin Adler]

Comment on attachment 142993 [details]
Patch

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

> LayoutTests/editing/deleting/delete-ligature-003.html:18
> -            if(window.layoutTestController)
> -                layoutTestController.setEditingBehavior(platform);
> +            if (window.internals)
> +                window.internals.settings.setEditingBehavior(platform);

No need for the "window." on the second line; just like the old code it can be just internals since we know it’s there. I noticed this patch adding “window.” prefixes in many places we don’t need to, which is a shame since “internals” would work fine alone.

[Caio Marcelo de Oliveira Filho]

> No need for the "window." on the second line; just like the old code it can be just internals since we know it’s there. I noticed this patch adding “window.” prefixes in many places we don’t need to, which is a shame since “internals” would work fine alone.

I'll fix those extra "window." that were added by this patch. Sorry for the noise.

[Caio Marcelo de Oliveira Filho]

(In reply to comment #30)
> I'll fix those extra "window." that were added by this patch. Sorry for the noise.

Bug 87059.