[Webkit-unassigned] [Bug 50952] Inputs of type "text" and "search" should support interoperable "set direction" functionality

[bugzilla-daemon at webkit.org]

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


Alexey Proskuryakov <ap at webkit.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #87536|review+                     |review-
               Flag|                            |




--- Comment #22 from Alexey Proskuryakov <ap at webkit.org>  2011-03-30 08:59:58 PST ---
(From update of attachment 87536)
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?

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