[webkit-reviews] review denied: [Bug 96592] Ruby text is incorrectly positioned when its writing-mode is changed to vertical after layout is done : [Attachment 163764] Patch

Tue Oct 30 03:21:29 PDT 2012

Hajime Morrita <morrita at google.com> has denied Yuki Sekiguchi
<yuki.sekiguchi at access-company.com>'s request for review:
Bug 96592: Ruby text is incorrectly positioned when its writing-mode is changed
to vertical after layout is done
https://bugs.webkit.org/show_bug.cgi?id=96592

Attachment 163764: Patch
https://bugs.webkit.org/attachment.cgi?id=163764&action=review

------- Additional Comments from Hajime Morrita <morrita at google.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=163764&action=review

The change itself looks sane. Could you tidy a few points up?

> Source/WebCore/rendering/RenderRubyRun.cpp:238
> +    // Logical left of RenderRubyText should always be 0.

We don't need this line. The comment says something obvious from the code.

> LayoutTests/ChangeLog:8
> +	   Update LayoutTest to use testRunner not layoutTestController

This explanation doesn't looks making sense. I'd rather drop this.

> LayoutTests/ChangeLog:15
> +	   RenderRubyText::y remain old one.

We don't need to duplicate this. It's sufficient to have the explanation in 
WebCore/ChangeLog .

> LayoutTests/fast/writing-mode/ruby-text-logical-left-expected.html:3
> +<style>

Please put <sytle> in <head> unless there is reason to do it.
Also, please make CSS syntax valid by adding ";"

> LayoutTests/fast/writing-mode/ruby-text-logical-left-expected.html:10
> +</body></html>

It would be great if you have more coverage.
- What happens if the style is removed?
- What if ruby has wider width than decorated text?

> LayoutTests/fast/writing-mode/ruby-text-logical-left.html:11
> +  setTimeout(function () {

Please indent the code appropriately.

> LayoutTests/fast/writing-mode/ruby-text-logical-left.html:14
> +  if (testRunner) {

Do check window.testRunner instead of just referring testRunner.
In that way we can run the test not only on DRT but also in a plain browser.