[webkit-reviews] review canceled: [Bug 92918] Built in quotes don't use lang attribute : [Attachment 155927] Patch

[bugzilla-daemon at webkit.org]

Julien Chaffraix <jchaffraix at webkit.org> has canceled Elliott Sprehn
<esprehn at gmail.com>'s request for review:
Bug 92918: Built in quotes don't use lang attribute
https://bugs.webkit.org/show_bug.cgi?id=92918

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

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=155927&action=review


The code change is fine, I would like to see a revised test though.

> Source/WebCore/rendering/RenderQuote.cpp:194
> +    if (style()->quotes())
> +	   return style()->quotes();

Better way:
if (QuoteData* styleQuotes = style()->quotes())
     return styleQuotes;

> LayoutTests/ChangeLog:13
> +	   *
platform/chromium-mac/fast/css-generated-content/quotes-lang-expected.txt:
Added.

Couldn't this test be a ref-test instead of a pixel-test? It looks like the
output is super easy to reproduce without using <q> or some RenderQuote.

> LayoutTests/fast/css-generated-content/quotes-lang.html:10
> +

Please, let's add some explanation as to what is expected along with what you
test!

> LayoutTests/fast/css-generated-content/quotes-lang.html:15
> +<q lang="ru"><q>ru</q></q>

Adding <br> would make the output easier to read.

> LayoutTests/fast/css-generated-content/quotes-lang.html:22
> +</div>

I think you are fixing another bug with respect to lang vs xml:lang here:

<q lang="no" xml:lang="en">en</q>

(before xml:lang would have been ignored AFAICT)