[webkit-reviews] review denied: [Bug 50952] Inputs of type "text" and "search" should support interoperable "set direction" functionality : [Attachment 87536] A quick fix v3

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has denied	review:
Bug 50952: Inputs of type "text" and "search" should support interoperable "set
direction" functionality
https://bugs.webkit.org/show_bug.cgi?id=50952

Attachment 87536: A quick fix v3
https://bugs.webkit.org/attachment.cgi?id=87536&action=review

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
I think that this needs another iteration.

+	 // Send an input event to this element as written in "4.10.7.1.2 Text
state and Search state".
+	 // (See
<http://dev.w3.org/html5/spec/Overview.html#text-state-and-search-state>.)

I don't think that we should have such comments in code. Pretty much every line
of code in WebCore could have a link to HTML5, so it's pointless.

What is important is that regression tests cover such things, and that those
tests make it clear why they require such behavior. This test doesn't explain
anything, provoking the person who breaks it to just "update the results".

+	 focusedNode->dispatchInputEvents();

What guarantees that focusedNode still exists after style update?

+	 This adds a new layout test to verify that we can reveive input events


Typo: receive.

+    // Call [NSResponder makeBaseWritingDirection*:] since WebView implements
+    // them and it calls Editor::setBaseWritingDirection().

Another comment that should be removed in my opinion. Of course we call a
function that is implemented - why would we call an unimplemented one?

+    if (JSStringIsEqualToUTF8CString(directionName, "ltr"))
+	 [[mainFrame webView] makeBaseWritingDirectionLeftToRight:0];
+    else if (JSStringIsEqualToUTF8CString(directionName, "rtl"))
+	 [[mainFrame webView] makeBaseWritingDirectionRightToLeft:0];

I suggest adding a check for invalid values.

--- LayoutTests/fast/html/script-tests/set-text-direction.js	(revision 0)

Please don't split tests into .html and .js parts.

+// This also removes the element to verify WebKit works without crashes.

Perhaps we should force garbage collection to make the test stronger?