Bug 92918

Created attachment 155927 [details] Patch Use the built in Element::computeInheritedLanguage and add tests that we actually pick quotes from the hashmap in RenderQuote.

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)

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

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

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

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

(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.

Created attachment 156141 [details] Patch Updated to use a ref test and added new test for xml:lang.

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!)

(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.

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?

Comment on attachment 156141 [details] Patch I think that performance concern that both Eric and myself had warrants r-.

(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.

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.

Created attachment 156170 [details] Patch Use -webkit-locale.

(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!

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?

(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?

Created attachment 156178 [details] Patch Remove unneeded asserts.

> I don't understand. What do you mean? Actually never mind. I now see that quotesData() calls style()->quotes() just like old code did.

Comment on attachment 156178 [details] Patch Clearing flags on attachment: 156178 Committed r124518: <http://trac.webkit.org/changeset/124518>

All reviewed patches have been landed. Closing bug.