[webkit-reviews] review canceled: [Bug 92448] Make QuotesData use a Vector of pairs : [Attachment 155075] 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 92448: Make QuotesData use a Vector of pairs
https://bugs.webkit.org/show_bug.cgi?id=92448

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

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


The change looks fine, now we need some testing as you changed some behaviors
that were obvious not covered by the tests.

> Source/WebCore/ChangeLog:20
> +	   (WebCore):
> +		   (WebCore::quotesDataLanguageMap): New function that returns
the HashMap of languages.

Weird indentation.

> Source/WebCore/rendering/RenderQuote.cpp:149
> +    staticQuotesMap->set("en", QuotesData::create(U("\x201C"), U("\x201D"),
U("\x2018"), U("\x2019")).leakRef());
> +    staticQuotesMap->set("no", QuotesData::create(U("\x00AB"), U("\x00BB"),
U("\x2039"), U("\x203A")).leakRef());
> +    staticQuotesMap->set("ro", QuotesData::create(U("\x201E"),
U("\x201D")).leakRef());
> +    staticQuotesMap->set("ru", QuotesData::create(U("\x00AB"), U("\x00BB"),
U("\x201E"), U("\x201C")).leakRef());

If you have some clue about where those constants come from, it would be nice
to mention it here. The table in CSS 2.1 section 12.3.1 is informative and
doesn't have a language mapping.

> Source/WebCore/rendering/RenderQuote.cpp:174
>      const AtomicString* language;

I think it should be an AtomicString and not a pointer. We don't ever expect
|language| to be NULL: getAttribute returns nullAtom if there is no attribute.

> Source/WebCore/rendering/RenderQuote.cpp:200
> +	   return quotes->getCloseQuote(std::max<int>(m_depth - 1, 0)).impl();

You did break this code in the previous patch for m_depth == 0, yet the tests
passed with flying colors. We need a test for this case as part of this change
(even if the result is not compliant with CSS 2.1 as we return a String when we
shouldn't)

> Source/WebCore/rendering/style/QuotesData.cpp:69
> +    if (a == b)

This was also improperly handled in the previous patch and it would be nice to
have some testing about that. Not 100% sure JS can trigger that reliably though
but it's worth a try and adding some tests.