Bug 42689 - Move setEditingBehavior() from layoutTestController to window.internals
Summary: Move setEditingBehavior() from layoutTestController to window.internals
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Caio Marcelo de Oliveira Filho
URL:
Keywords: InRadar, MakingBotsRed
: 81738 (view as bug list)
Depends on:
Blocks: 87284 84595
  Show dependency treegraph
 
Reported: 2010-07-20 15:14 PDT by Sam Weinig
Modified: 2012-05-23 10:51 PDT (History)
12 users (show)

See Also:


Attachments
Patch (73.98 KB, patch)
2012-05-17 16:12 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (890.00 KB, application/zip)
2012-05-17 19:58 PDT, WebKit Review Bot
no flags Details
Patch (74.67 KB, patch)
2012-05-18 13:30 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (95.50 KB, patch)
2012-05-18 16:03 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (88.73 KB, patch)
2012-05-18 16:12 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch (88.73 KB, patch)
2012-05-21 04:40 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sam Weinig 2010-07-20 15:14:51 PDT
WebKitTestRunner needs layoutTestController.setEditingBehavior
Comment 1 Sam Weinig 2010-07-20 15:31:52 PDT
<rdar://problem/8213852>
Comment 2 Simon Fraser (smfr) 2011-09-08 14:10:05 PDT
Seems like a good candidate to move to window.internal.
Comment 3 mitz 2012-03-20 21:45:00 PDT
*** Bug 81738 has been marked as a duplicate of this bug. ***
Comment 4 mitz 2012-03-20 21:48:12 PDT
Added editing/selection/move-by-word-visually-crash-test-5.html to the WebKit2 skip list in <http://trac.webkit.org/r111498>.
Comment 5 Caio Marcelo de Oliveira Filho 2012-05-17 12:06:02 PDT
(In reply to comment #2)
> Seems like a good candidate to move to window.internal.

I'll give it a shot.
Comment 6 Caio Marcelo de Oliveira Filho 2012-05-17 16:12:00 PDT
Created attachment 142581 [details]
Patch
Comment 7 Build Bot 2012-05-17 16:59:46 PDT
Comment on attachment 142581 [details]
Patch

Attachment 142581 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12718698
Comment 8 WebKit Review Bot 2012-05-17 19:58:25 PDT
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
Comment 9 WebKit Review Bot 2012-05-17 19:58:33 PDT
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
Comment 10 Gyuyoung Kim 2012-05-17 20:04:52 PDT
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.
Comment 11 Gyuyoung Kim 2012-05-17 20:16:30 PDT
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
Comment 12 Caio Marcelo de Oliveira Filho 2012-05-17 21:14:29 PDT
Comment on attachment 142581 [details]
Patch

Removing r? until I fix the problems already pointed out.
Comment 13 Hajime Morrita 2012-05-17 21:15:14 PDT
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.
Comment 14 Alexey Proskuryakov 2012-05-18 10:06:16 PDT
> 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.
Comment 15 Caio Marcelo de Oliveira Filho 2012-05-18 13:30:03 PDT
Created attachment 142776 [details]
Patch
Comment 16 Caio Marcelo de Oliveira Filho 2012-05-18 13:32:02 PDT
(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?
Comment 17 Caio Marcelo de Oliveira Filho 2012-05-18 13:34:17 PDT
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.
Comment 18 Alexey Proskuryakov 2012-05-18 13:56:34 PDT
> 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.
Comment 19 Kenneth Rohde Christiansen 2012-05-18 14:16:02 PDT
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?
Comment 20 Caio Marcelo de Oliveira Filho 2012-05-18 16:03:21 PDT
Created attachment 142809 [details]
Patch
Comment 21 Caio Marcelo de Oliveira Filho 2012-05-18 16:04:51 PDT
Comment on attachment 142809 [details]
Patch

Checking if Mac and Win will be happy with the changes.
Comment 22 Caio Marcelo de Oliveira Filho 2012-05-18 16:12:34 PDT
Created attachment 142813 [details]
Patch

trying to make win bot apply the patch
Comment 23 Hajime Morrita 2012-05-20 17:55:44 PDT
Comment on attachment 142813 [details]
Patch

Looks good. I hope we could see the green win build.
Comment 24 Caio Marcelo de Oliveira Filho 2012-05-20 18:33:05 PDT
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.
Comment 25 Caio Marcelo de Oliveira Filho 2012-05-20 18:33:37 PDT
...and webkit-patch upload, to upload these patches.
Comment 26 Caio Marcelo de Oliveira Filho 2012-05-21 04:40:43 PDT
Created attachment 142993 [details]
Patch
Comment 27 Caio Marcelo de Oliveira Filho 2012-05-21 05:23:43 PDT
Committed r117771: <http://trac.webkit.org/changeset/117771>
Comment 28 Jesus Sanchez-Palencia 2012-05-21 11:27:00 PDT
Comment on attachment 142993 [details]
Patch

Clearing flags.
Comment 29 Darin Adler 2012-05-21 15:13:38 PDT
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.
Comment 30 Caio Marcelo de Oliveira Filho 2012-05-21 15:44:40 PDT
> 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.
Comment 31 Caio Marcelo de Oliveira Filho 2012-05-21 17:01:55 PDT
(In reply to comment #30)
> I'll fix those extra "window." that were added by this patch. Sorry for the noise.

Bug 87059.