Bug 38603 – Expose the editing behavior setting in DRT to test all editing code paths

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Martin Robinson]

This was discussed here: https://lists.webkit.org/pipermail/webkit-dev/2010-March/012118.html

[Martin Robinson]

Created attachment 55171 [details]
Expose the editing behavior setting to DRT

I've attached a rough patch for this. A couple notes:

1. I've implemented the necessary API points and DRT support for Windows/Mac, GTK+ and Chromium ports. This is my first shot at mucking with some of these ports so the style/implementation might be wrong. Qt and Wx are just stubbed out right now. I don't see LayoutTestController implementations for EFL or Haiku.

2. I've only converted one test because fixing this bug is the bootstrapping phase of fixing many GTK+ selection and DRT bugs. The test I converted is only 1 patch away from passing on GTK+, so it's a good starting point. I plan to iteratively convert tests as I fix bugs on GTK+.

[Early Warning System Bot]

Attachment 55171 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/2183002

[Martin Robinson]

Comment on attachment 55171 [details]
Expose the editing behavior setting to DRT

Removing review flags so that I can fix the EWS build errors.

[Martin Robinson]

Created attachment 55190 [details]
Previous patch with build fix for qt (hopefully)

[Eric Seidel (no email)]

Attachment 55190 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/2227003

[Martin Robinson]

Created attachment 55192 [details]
Previous patch with build fix for Mac

[WebKit Review Bot]

Attachment 55192 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2183003

[Martin Robinson]

Created attachment 55239 [details]
Previous patch with build fix for Chromium

[WebKit Review Bot]

Attachment 55239 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2197023

[Martin Robinson]

Created attachment 55243 [details]
Patch with Chromium build fix (take 2)

[WebKit Review Bot]

Attachment 55243 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2232018

[Martin Robinson]

Created attachment 55245 [details]
Patch with Chromium build fix (take 3) :(

[WebKit Review Bot]

Attachment 55245 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2211027

[Martin Robinson]

Created attachment 55251 [details]
Patch with Chromium build fix

[WebKit Review Bot]

Attachment 55251 [details] did not build on win:
Build output: http://webkit-commit-queue.appspot.com/results/2189103

[Ojan Vafai]

Comment on attachment 55251 [details]
Patch with Chromium build fix

Thanks for doing this! At a high-level, this looks good to me. I have mostly cleanup comments. The only significant change I'd like to see is for the layoutTestController call throw a JS error if you pass it an invalid value.

I'm also on the fence about having a getter for editingBehavior. Having a getter is more consistent with the other preferences, but I also can't think of any case where we'd use it.

r- for now for the cleanup nits and the throwing a JS error. I'm new to being a reviewer and I don't know this code too well, so I'd prefer if another reviewer gave the final r+. Maybe that person can also comment on whether it makes sense to have a getter that we don't actually expose to layoutTestController.

> +        layoutTestController.setEditingBehavior("mac");
> +        if (!runTest("a paragraph"))
> +            return;
> +
> +        layoutTestController.setEditingBehavior("windows");
> +        if (!runTest("paragra"))
> +            return;
> +
> +        logResult("SUCCESS");

It would be better if the test ran both tests and printed out SUCCESS/FAIL for each test. Gives more information about what's going wrong when the test starts failing. It's fine to have two lines that say "PASSED". Most of the script-tests do that. The expected results for this test will change obviously, but that's OK.

While I'm nit-picking, you could make the platform string an argument of runTest and then just call setEditingBehavior once from in there.

> +++ b/WebKit/chromium/public/WebSettings.h
I'd like fishd@chromium to review this as it affects chromium's public WebKit API. Looks fine to me though.

> @@ -43,6 +43,11 @@ class WebURL;
>  // these functions have a 1:1 mapping with the methods in WebCore/page/settings.h.
>  class WebSettings {
>  public:
> +    enum EditingBehavior {
> +        EditingBehaviorMac = 1,
> +        EditingBehaviorWindows
> +    };

Why start this at 1? It's my understanding that it should match the enum in Settings.h. Also, it's a bit weird to have the names of this enum be slightly different than the one in Settings.h. Was that intentional?

> +++ b/WebKit/chromium/src/WebSettingsImpl.cpp
> +void WebSettingsImpl::setEditingBehavior(EditingBehavior behavior)
> +{
> +    if (behavior == EditingBehaviorMac) {
> +        m_settings->setEditingBehavior(WebCore::EditingMacBehavior);
> +        return;
> +    }
> +
> +    m_settings->setEditingBehavior(WebCore::EditingWindowsBehavior);
> +}

This implementation doesn't match the one in WebKit/mac/WebView/WebFrame.mm. Did you do that for a reason? It would be good to have them match, at least in behavior (e.g. they do something different if you were to pass in a new enum value). The behavior in WebKit/mac/WebView/WebFrame.mm seems more robust than this one.

> +++ b/WebKit/mac/ChangeLog
> @@ -1,3 +1,24 @@
> [phone]  Martin Robinson  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Expose the editing behavior setting in DRT to test all editing code paths
> +        https://bugs.webkit.org/show_bug.cgi?id=38603
> +
> +        Expose the EditingBehavior setting in the Mac API.

No need to have this extra line of description as it just repeats the summary line above. This section is really if you have more detailed explanation to give, which I don't think is necessary in this case.

> +typedef enum {
> +    WebKitEditingMacBehavior,
> +    WebKitEditingWindowsBehavior,
> +} WebKitEditingBehavior;

Could you use these enum names in the chromium files as well? Would be better to be consistent and I these names are more clear.

> +void LayoutTestController::setEditingBehavior(const CppArgumentList& arguments, CppVariant* results)
> +{
> +    WebSettings* settings = m_shell->webView()->settings();
> +    string key = arguments[0].toString();
> +    if (key == "mac")
> +        settings->setEditingBehavior(WebSettings::EditingBehaviorMac);
> +
> +    if (key == "windows")
> +        settings->setEditingBehavior(WebSettings::EditingBehaviorWindows);
> +}

Would be nice if this errored if you passed it an invalid key. It would be really annoying to misspell "windows" in a test and have it silently fail. Also, you should use and "else if" clause for the second if-statement since the key can't be both mac and windows. Ditto for the other implementations of this method as well.

> diff --git a/WebKitTools/DumpRenderTree/wx/LayoutTestControllerWx.cpp b/WebKitTools/DumpRenderTree/wx/LayoutTestControllerWx.cpp
> index 4614aca..b82a171 100644
> --- a/WebKitTools/DumpRenderTree/wx/LayoutTestControllerWx.cpp
> +++ b/WebKitTools/DumpRenderTree/wx/LayoutTestControllerWx.cpp
> @@ -441,3 +441,8 @@ JSValueRef LayoutTestController::computedStyleIncludingVisitedInfo(JSContextRef,
>  void LayoutTestController::authenticateSession(JSStringRef, JSStringRef, JSStringRef)
>  {
>  }
> +
> +void LayoutTestController::setEditingBehavior(JSStringRef editingBehavior)
> +{
> +    // FIXME: Implement
> +}
> -- 

Did you mean to delete this line?

> 1.6.3.3

[Ojan Vafai]

Darin, can you vet the chromium webkit API changes?

[Darin Fisher (:fishd, Google)]

Comment on attachment 55251 [details]
Patch with Chromium build fix

WebKit/chromium/public/WebSettings.h:47
 +          EditingBehaviorMac = 1,
If you do not assign 1 to EditingBehaviorMac, then it will have a value of 0,
and that will mean that EditingBehaviorMac == WebCore::EditingMacBehavior
and EditingBehaviorWindows == WebCore::EditingWindowsBehavior.

This means that in the .cpp file you can just static_cast between the two
enum types.

Then to ensure that they do not get out of sync, you can add some lines to
chromium/src/AssertMatchingEnums.cpp (note that the enum groups are alphabetized).

[Martin Robinson]

Created attachment 56026 [details]
Updated patch based on suggestions

[Martin Robinson]

(In reply to comment #16)

Thanks for the review and the great feedback!

> (From update of attachment 55251 [details])

> It would be better if the test ran both tests and printed out SUCCESS/FAIL for each test. Gives more information about what's going wrong when the test starts failing. It's fine to have two lines that say "PASSED". Most of the script-tests do that. The expected results for this test will change obviously, but that's OK.
> While I'm nit-picking, you could make the platform string an argument of runTest and then just call setEditingBehavior once from in there.

Done!

> Why start this at 1? It's my understanding that it should match the enum in Settings.h. Also, it's a bit weird to have the names of this enum be slightly different than the one in Settings.h. Was that intentional?
> This implementation doesn't match the one in WebKit/mac/WebView/WebFrame.mm. Did you do that for a reason? It would be good to have them match, at least in behavior (e.g. they do something different if you were to pass in a new enum value). The behavior in WebKit/mac/WebView/WebFrame.mm seems more robust than this one.

The enum now starts at zero and I've added a compile-time assertion for equality with the WebCore enum. I've gone with the static_cast approach that Darin suggested. This seems to match the behavior in other parts of the Chromium API.

> Could you use these enum names in the chromium files as well? Would be better to be consistent and I these names are more clear.

I'm this case I tried to follow the naming scheme of the Chromium public API. For example (from AssertMatchingEnums.cpp):

COMPILE_ASSERT_MATCHING_ENUM(WebAccessibilityRoleSliderThumb, SliderThumbRole);

The API seems to consistently use [EnumName][Value]. I can change it around if you still think I should. :)


> Would be nice if this errored if you passed it an invalid key. It would be really annoying to misspell "windows" in a test and have it silently fail. Also, you should use and "else if" clause for the second if-statement since the key can't be both mac and windows. Ditto for the other implementations of this method as well.

I agree! On JSC versions, I throw an exception and on the Chromium version I use logErrorToConsole (which seems to be the convention there). There didn't appear to be an easy way to throw an exception from CppBoundClass methods, but I'm not very familiar with it.


> > +void LayoutTestController::setEditingBehavior(JSStringRef editingBehavior)
> > +{
> > +    // FIXME: Implement
> > +}
> > -- 
> Did you mean to delete this line?

Just a reminder, which seems to match the other stubs in the file.

[Darin Fisher (:fishd, Google)]

Comment on attachment 56026 [details]
Updated patch based on suggestions

WebKit/chromium/src/WebSettingsImpl.cpp:274
 +      m_settings->setEditingBehavior(static_cast<WebCore::EditingBehavior>(behavior));
nit: no need for WebCore:: prefix given the using directive at the top of the file.

[Martin Robinson]

Created attachment 56497 [details]
Patch (de-nitted)

I've attached a patch for this issue that addresses this nit.

[WebKit Review Bot]

Attachment 56497 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2294329

[Martin Robinson]

?(In reply to comment #21)
> nit: no need for WebCore:: prefix given the using directive at the top of the file.

It looks like in this case the namespace specifier was necessary because of namespace collision. Would you prefer the original patch or something like "using WebCore::EditingBehavior" at the top

[Ojan Vafai]

Comment on attachment 56497 [details]
Patch (de-nitted)

Just a few more nits.

(In reply to comment #24)
> ?(In reply to comment #21)
> > nit: no need for WebCore:: prefix given the using directive at the top of the file.
> 
> It looks like in this case the namespace specifier was necessary because of namespace collision. Would you prefer the original patch or something like "using WebCore::EditingBehavior" at the top

I'm not sure, but I think the prefix is preferred (i.e. original patch).


> > Could you use these enum names in the chromium files as well? Would be better to be consistent and I these names are more clear.
> 
> I'm this case I tried to follow the naming scheme of the Chromium public API. For example (from AssertMatchingEnums.cpp):
> 
> COMPILE_ASSERT_MATCHING_ENUM(WebAccessibilityRoleSliderThumb, SliderThumbRole);
> 
> The API seems to consistently use [EnumName][Value]. I can change it around if you still think I should. :)

Talked to fishd. He agrees with using EditingBehaviorMac and EditingBehaviorWin in the Chromium files. To make it as self-consistent as possible, note that he also suggested abbreviating both Mac and Win. Maybe we could change the JS call to setEditingBehavior to take 'mac' and 'win' as arguments as well? Then, one day, we can make the enum in WebCore consistent as well (a separate patch).


I'm confused by the last two lines here:
+++ b/WebKitTools/DumpRenderTree/wx/LayoutTestControllerWx.cpp
@@ -446,3 +446,8 @@ JSValueRef LayoutTestController::computedStyleIncludingVisitedInfo(JSContextRef,
 void LayoutTestController::authenticateSession(JSStringRef, JSStringRef, JSStringRef)
 {
 }
+
+void LayoutTestController::setEditingBehavior(JSStringRef editingBehavior)
+{
+    // FIXME: Implement
+}
-- 
1.6.3.3

Where did the following come from?
-- 
1.6.3.3

[Martin Robinson]

Created attachment 56539 [details]
Patch with suggestions

[Martin Robinson]

(In reply to comment #25)
> I'm not sure, but I think the prefix is preferred (i.e. original patch).

Okay, I've used the prefix here.
> Talked to fishd. He agrees with using EditingBehaviorMac and EditingBehaviorWin in the Chromium files. To make it as self-consistent as possible, note that he also suggested abbreviating both Mac and Win. Maybe we could change the JS call to setEditingBehavior to take 'mac' and 'win' as arguments as well? Then, one day, we can make the enum in WebCore consistent as well (a separate patch).

Using the abbreviated version in all the new APIs now and changed
LayoutTestController's setEditingBehavior to look for "win."

> Where did the following come from?
> -- 
> 1.6.3.3

This is just an artifact of git-format-patch. It's my git version. :)

[WebKit Review Bot]

Attachment 56539 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/2313336

[Ojan Vafai]

Comment on attachment 56539 [details]
Patch with suggestions

r=me once you resolve the chromium build issue. Thanks for fixing this!

WebKit/chromium/public/WebSettings.h:48
 +          EditingBehaviorWindows
I think you're missing a comma here.

[Martin Robinson]

Created attachment 56545 [details]
Patch (with build fix)

[Martin Robinson]

Committed r59840: <http://trac.webkit.org/changeset/59840>

[WebKit Review Bot]

http://trac.webkit.org/changeset/59840 might have broken Tiger Intel Release
The following changes are on the blame list:
http://trac.webkit.org/changeset/59840
http://trac.webkit.org/changeset/59839

[Martin Robinson]

Comment on attachment 56545 [details]
Patch (with build fix)

Patch committed. Clearing flags.