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

[bugzilla-daemon at webkit.org]

Hironori Bono <hbono at chromium.org> has asked  for 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 87711: A quick fix v4
https://bugs.webkit.org/attachment.cgi?id=87711&action=review

------- Additional Comments from Hironori Bono <hbono at chromium.org>
Thank you for your review and comments.

(In reply to comment #22)
> 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".

Ah, right. Even though my layout test describes "WebKit sends an input event",
this test does not check if it actually sends the event. I have added a flag to
verify it. (Please feel free to blame me if I'm wrong.)
 
> +	   focusedNode->dispatchInputEvents();
> 
> What guarantees that focusedNode still exists after style update?

In my honest opinion, I'm not totally sure what the style update does, i.e. I
cannot guarantee it at all. As far as I quickly read the code,
Element::setAttribute() does not seem to delete the node. So, I have moved this
call after the Element::setAttribute(). (This analysis may be wrong, though.)
 
> +	   This adds a new layout test to verify that we can reveive input
events
>
> Typo: receive.

Thank you for noticing it.
 
> +    // 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?

OK. I have removed it.
 
> +    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.

Thank you. I have added a ASSERT_NOT_REACHED() call here.

> --- LayoutTests/fast/html/script-tests/set-text-direction.js	  (revision 0)
> 
> Please don't split tests into .html and .js parts.

Sorry. It seems I misunderstood the script tests. (I thought a script test must
consists of an .html file and a .js one.)

> +// This also removes the element to verify WebKit works without crashes.
> 
> Perhaps we should force garbage collection to make the test stronger?

It makes sense. I have copied the code that force GC from 'fast/dom/gc-1.html'.


Finally, my WebKit knowledge is rusty and I may misunderstand it. So, please
feel free to blame me if I'm wrong.

Regards,

Hironori Bono