Bug 92918 – Built in quotes don't use lang attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Elliott Sprehn]

<q lang="ru"></q> should use Russian quotes since there's a table of quotes in RenderQuote, but we always render " and '.

Custom languages do work though which is weird:

<style>:lang(ru) { quotes: "A" "B"; }</style>
<q lang="ru">This works.</q>

[Elliott Sprehn]

Created attachment 155927 [details]
Patch

Use the built in Element::computeInheritedLanguage and add tests that we actually pick quotes from the hashmap in RenderQuote.

[Julien Chaffraix]

Comment on attachment 155927 [details]
Patch

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)

[WebKit Review Bot]

Comment on attachment 155927 [details]
Patch

Attachment 155927 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13415411

New failing tests:
fast/css-generated-content/quotes-lang.html

[WebKit Review Bot]

Created attachment 155951 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[WebKit Review Bot]

Comment on attachment 155927 [details]
Patch

Attachment 155927 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13415424

New failing tests:
fast/css-generated-content/quotes-lang.html

[WebKit Review Bot]

Created attachment 155954 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Elliott Sprehn]

(In reply to comment #2)
> (From update of attachment 155927 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155927&action=review
> ...
> 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)

In XHTML mode yeah, but how do I write a test for that with DRT? With the HTML5 parser xml:lang is entirely ignored because you get an attribute called "xml:lang" without a namespace so fastGetAttribute(XMLNames::langAttr) returns null.

[Elliott Sprehn]

Created attachment 156141 [details]
Patch

Updated to use a ref test and added new test for xml:lang.

[Eric Seidel (no email)]

Comment on attachment 156141 [details]
Patch

Do we have any performanc metrics for quotes?  My impression was computeInheritedLanguage was expensive (not that the previou tree-walking wasnt!)

[Elliott Sprehn]

(In reply to comment #9)
> (From update of attachment 156141 [details])
> Do we have any performanc metrics for quotes?  My impression was computeInheritedLanguage was expensive (not that the previou tree-walking wasnt!)

I don't think there's any performance tests for this. Is there a better way to figure out the language?

It might be good to write a perf test since the final patch is the rewritten placeQuote() algorithm, but I don't know how to do that.

[Alexey Proskuryakov]

This sounds like #2 in bug 3234, adding dependency.

Element::computeInheritedLanguage() is slow, as it performs a walk of all ancestors. I don't think that it's OK to use it in rendering.

The same performance problem existed for language-based font selection. Perhaps the same solution can be used here?

[Alexey Proskuryakov]

Comment on attachment 156141 [details]
Patch

I think that performance concern that both Eric and myself had warrants r-.

[Elliott Sprehn]

(In reply to comment #12)
> (From update of attachment 156141 [details])
> I think that performance concern that both Eric and myself had warrants r-.

The existing code already walks the whole tree though, and it's broken (doesn't actually respect the lang attribute at all even though it walks the whole tree).

It's better to land code that's correct and has same perf as before than leave code that's totally broken.

[Alexey Proskuryakov]

I don't necessarily agree - new code that uses a wrong idiom can be used as inspiration by other WebKit hackers. Even if detected in review, there is going to be an argument similar to "other code is allowed to do this, so why not me".

I hope that using -webkit-locale here won't add much complexity, although I can certainly feel the pain of revisiting a prepared patch.

[Elliott Sprehn]

Created attachment 156170 [details]
Patch

Use -webkit-locale.

[Elliott Sprehn]

(In reply to comment #14)
> I don't necessarily agree - new code that uses a wrong idiom can be used as inspiration by other WebKit hackers. Even if detected in review, there is going to be an argument similar to "other code is allowed to do this, so why not me".
> 
> I hope that using -webkit-locale here won't add much complexity, although I can certainly feel the pain of revisiting a prepared patch.

Yeah it wasn't hard, it just took me a second to realize that style()->locale() is the same as -webkit-locale.

Thanks for the perf optimization ap!

[Alexey Proskuryakov]

Comment on attachment 156170 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156170&action=review

> Source/WebCore/rendering/RenderQuote.cpp:163
> -    const QuotesData* quotes = style()->quotes();
> -    if (!quotes)
> -        quotes = defaultQuotes(this);
> +    const QuotesData* quotes = quotesData();

Hmm. Is this second-guessing what the quotes should be, instead of asking directly?

> Source/WebCore/rendering/RenderQuote.cpp:164
> +    ASSERT(quotes);

Is this ASSERT helpful? Generally, we want to assert on null for one of these reasons:

1. If it would be hard to investigate bugs caused by the pointer being null without the assertion. Here, the crash would happen immediately below the ASSERT, and it will be obvious what the cause was.

2. To document non-trivial expectations, particularly about function arguments. This doesn't seem to be so non-trivial. Also, returning a reference is IMO a better way to document the same.

> Source/WebCore/rendering/RenderQuote.cpp:188
> +    ASSERT(parent());

Why do we care?

[Elliott Sprehn]

(In reply to comment #17)
> (From update of attachment 156170 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=156170&action=review
> 
> > Source/WebCore/rendering/RenderQuote.cpp:163
> > -    const QuotesData* quotes = style()->quotes();
> > -    if (!quotes)
> > -        quotes = defaultQuotes(this);
> > +    const QuotesData* quotes = quotesData();
> 
> Hmm. Is this second-guessing what the quotes should be, instead of asking directly?
>

I don't understand. What do you mean?

[Elliott Sprehn]

Created attachment 156178 [details]
Patch

Remove unneeded asserts.

[Alexey Proskuryakov]

> I don't understand. What do you mean?

Actually never mind. I now see that quotesData() calls style()->quotes() just like old code did.

[WebKit Review Bot]

Comment on attachment 156178 [details]
Patch

Clearing flags on attachment: 156178

Committed r124518: <http://trac.webkit.org/changeset/124518>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.