[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





--- Comment #20 from Martin Robinson <mrobinson at webkit.org>  2010-05-13 14:55:00 PST ---
(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.

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