[Webkit-unassigned] [Bug 38603] Expose the editing behavior setting in DRT to test all editing code paths

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=38603


Ojan Vafai <ojan at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #55251|review?                     |review-
               Flag|                            |




--- Comment #16 from Ojan Vafai <ojan at chromium.org>  2010-05-12 16:13:38 PST ---
(From update of attachment 55251)
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 at 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 @@
> +2010-05-06  Martin Robinson  <mrobinson at webkit.org>
> +
> +        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

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.