[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


Hironori Bono <hbono at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #87313|0                           |1
        is obsolete|                            |
  Attachment #87536|0                           |1
        is obsolete|                            |
  Attachment #87711|                            |review?
               Flag|                            |




--- Comment #23 from Hironori Bono <hbono at chromium.org>  2011-03-31 06:17:18 PST ---
Created an attachment (id=87711)
 --> (https://bugs.webkit.org/attachment.cgi?id=87711&action=review)
A quick fix v4

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

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