[webkit-reviews] review denied: [Bug 38603] Expose the editing behavior setting in DRT to test all editing code paths : [Attachment 56497] Patch (de-nitted)

[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 56497: Patch (de-nitted)
https://bugs.webkit.org/attachment.cgi?id=56497&action=review

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