[webkit-reviews] review denied: [Bug 38603] Expose the editing behavior setting in DRT to test all editing code paths : [Attachment 55251] Patch with Chromium build fix

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Martin Robinson
<mrobinson at webkit.org>'s request for review:
Bug 38603: Expose the editing behavior setting in DRT to test all editing code
paths
https://bugs.webkit.org/show_bug.cgi?id=38603

Attachment 55251: Patch with Chromium build fix
https://bugs.webkit.org/attachment.cgi?id=55251&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
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