Bug 92448 – Make QuotesData use a Vector of pairs

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Elliott Sprehn]

Make QuotesData use a Vector of pairs

[Elliott Sprehn]

Created attachment 154813 [details]
Patch

Move QuotesData to use managed containers instead of being a ptr to raw memory that contains both a QuotesData and a set of String instances off the end.

[Elliott Sprehn]

Created attachment 154829 [details]
Patch

Incorporated review comments from jchaffraix on WKBug 92061.

[Elliott Sprehn]

Created attachment 155028 [details]
Patch for landing

[Julien Chaffraix]

Comment on attachment 155028 [details]
Patch for landing

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

I would like to see another round.

> Source/WebCore/css/StyleResolver.cpp:3453
> +                // item() returns null if out of bounds so this is safe

A sentence usually ends with a dot.

> Source/WebCore/css/StyleResolver.cpp:3460
> +                String startQuote = static_cast<CSSPrimitiveValue*>(first)->getStringValue();
> +                String endQuote = static_cast<CSSPrimitiveValue*>(second)->getStringValue();

I would support adding toCSSPrimitiveValue to match other objects in WebKit but maybe people working in CSS on a more regular basis would object.

> Source/WebCore/rendering/RenderQuote.cpp:139
> +typedef HashMap<AtomicString, const QuotesData*> QuotesMap;

I think you should just use a HashMap with the CaseFoldingHash here:

typedef HashMap<AtomicString, const QuotesData*, CaseFoldingHash> QuotesMap;

This would avoid the need to call lower() below for the hash computation.

> Source/WebCore/rendering/RenderQuote.cpp:199
> +        return quotes->getCloseQuote(m_depth ? m_depth - 1 : m_depth + 1).impl();

Writing this:

int closeQuoteIndex = max<int>(m_depth - 1, 1);
return quotes->getCloseQuote(closeQuoteIndex).impl();

would be a lot more readable as m_depth != -1. On top of that, you would see that you never call getCloseQuote() with a negative number.

However this is completely wrong as it doesn't match what the spec says: "A 'close-quote' or 'no-close-quote' that would make the depth negative is in error and is ignored". Let's add a FIXME about that as it was already wrong.

> Source/WebCore/rendering/style/QuotesData.cpp:42
> +PassRefPtr<QuotesData> QuotesData::create()

Nit: This could be inlined in the header as it is a trivial create().

> Source/WebCore/rendering/style/QuotesData.cpp:56
> +    if ((size_t)index >= m_quotePairs.size())

I would cast index as an unsigned to avoid this cast. It could be argued either way though so it's a nit.

> Source/WebCore/rendering/style/QuotesData.cpp:70
> +bool QuotesData::equal(const QuotesData* a, const QuotesData* b)

This function should be named equals() in proper English. Alternatively it should be replaced by an operator= to match the rest of the style code. You don't need to fix this now but at least let's put a FIXME here.

> Source/WebCore/rendering/style/QuotesData.cpp:72
> +    return a && b && a->m_quotePairs == b->m_quotePairs;

The case where both |a| and |b| are NULL is impossible or am I wrong? If that's the case, we should add the appropriate ASSERT.

> Source/WebCore/rendering/style/QuotesData.h:39
> +    ~QuotesData() { };

The empty destructor can be removed. The compiler will generate it for you with the right access specifier AFAICT.

[Elliott Sprehn]

(In reply to comment #4)
> (From update of attachment 155028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155028&action=review
> ..
> I would support adding toCSSPrimitiveValue to match other objects in WebKit but maybe people working in CSS on a more regular basis would object.

Yeah this seems reasonable.

> > Source/WebCore/rendering/RenderQuote.cpp:199
> > +        return quotes->getCloseQuote(m_depth ? m_depth - 1 : m_depth + 1).impl();
> 
> Writing this:
> 
> int closeQuoteIndex = max<int>(m_depth - 1, 1);
> return quotes->getCloseQuote(closeQuoteIndex).impl();

This would be wrong.

> 
> would be a lot more readable as m_depth != -1. On top of that, you would see that you never call getCloseQuote() with a negative number.
> 
> However this is completely wrong as it doesn't match what the spec says: "A 'close-quote' or 'no-close-quote' that would make the depth negative is in error and is ignored". Let's add a FIXME about that as it was already wrong.

This isn't wrong.  Calling getCloseQuote with a negative number returns an empty string which is the correct behavior per the spec. The local depth can be negative in which case we should render as if the quote was an empty string.

> 
> > Source/WebCore/rendering/style/QuotesData.cpp:72
> > +    return a && b && a->m_quotePairs == b->m_quotePairs;
> 
> The case where both |a| and |b| are NULL is impossible or am I wrong? If that's the case, we should add the appropriate ASSERT.

It can happen if someone changes the quotes CSS property and then we go down through styleDidChange in RenderQuote.

[Elliott Sprehn]

(In reply to comment #4)
> (From update of attachment 155028 [details])
> 
> > Source/WebCore/css/StyleResolver.cpp:3460
> > +                String startQuote = static_cast<CSSPrimitiveValue*>(first)->getStringValue();
> > +                String endQuote = static_cast<CSSPrimitiveValue*>(second)->getStringValue();
> 
> I would support adding toCSSPrimitiveValue to match other objects in WebKit but maybe people working in CSS on a more regular basis would object.
> 

I'll do this in a separate patch since it makes more sense to change all of them in that file at once.

[Julien Chaffraix]

Comment on attachment 155028 [details]
Patch for landing

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

>>> Source/WebCore/rendering/RenderQuote.cpp:199
>>> +        return quotes->getCloseQuote(m_depth ? m_depth - 1 : m_depth + 1).impl();
>> 
>> Writing this:
>> 
>> int closeQuoteIndex = max<int>(m_depth - 1, 1);
>> return quotes->getCloseQuote(closeQuoteIndex).impl();
>> 
>> would be a lot more readable as m_depth != -1. On top of that, you would see that you never call getCloseQuote() with a negative number.
>> 
>> However this is completely wrong as it doesn't match what the spec says: "A 'close-quote' or 'no-close-quote' that would make the depth negative is in error and is ignored". Let's add a FIXME about that as it was already wrong.
> 
> This would be wrong.

I missed the '1' case so the max doesn't work indeed. So much for simplifying.

If I am reading correctly, the code doesn't make much sense for m_depth == 0 as we will use the 2nd pair of quotes. If you could put a FIXME comment (or an explanation comment if I missed something again) that would be cool.

[Elliott Sprehn]

(In reply to comment #7)
> (From update of attachment 155028 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=155028&action=review
> 
> I missed the '1' case so the max doesn't work indeed. So much for simplifying.
> 
> If I am reading correctly, the code doesn't make much sense for m_depth == 0 as we will use the 2nd pair of quotes. If you could put a FIXME comment (or an explanation comment if I missed something again) that would be cool.

It's just wrong, but I didn't want to change behavior in this patch.

[Elliott Sprehn]

(In reply to comment #8)
> (In reply to comment #7)
> > (From update of attachment 155028 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=155028&action=review
> > 
> > I missed the '1' case so the max doesn't work indeed. So much for simplifying.
> > 
> > If I am reading correctly, the code doesn't make much sense for m_depth == 0 as we will use the 2nd pair of quotes. If you could put a FIXME comment (or an explanation comment if I missed something again) that would be cool.
> 
> It's just wrong, but I didn't want to change behavior in this patch.

Actually, this was correct for the original code which stored all quotes in a flat array. If you were at depth 0, you should pick position 1 from the array since that'd be the closing quote of the first pair. In the new code it should be std::max<int>(m_depth - 1, 0)

[Elliott Sprehn]

Created attachment 155075 [details]
Patch

Second round of review.

[Julien Chaffraix]

Comment on attachment 155075 [details]
Patch

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.

[Elliott Sprehn]

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

Added a link. Eventually this map should include the quotes for all the languages from bug 3234's css file.

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

This is hard to test because we'd just be doing layout when we don't need to be so it's a perf thing.

[Elliott Sprehn]

Created attachment 155380 [details]
Patch

Added test.

[Julien Chaffraix]

Comment on attachment 155380 [details]
Patch

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

> Source/WebCore/css/StyleResolver.cpp:3465
> +        }
> +        if (primitiveValue) {

You don't early-return above so it should still be an:

else if (primitiveValue) {

> LayoutTests/fast/css-generated-content/close-quote-negative-depth.html:1
> +

Please don't forget a DOCTYPE as we don't want to test quirks mode behavior.

> LayoutTests/fast/css-generated-content/close-quote-negative-depth.html:8
> +<!-- There should be no quotes. -->

Let's dump this in the output along with a description of what you are testing and potentially the bug number. This would make it easier for maintainer to see what's expected and if some output is right or wrong.

[Elliott Sprehn]

Created attachment 155396 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 155396 [details]
Patch for landing

Clearing flags on attachment: 155396

Committed r124157: <http://trac.webkit.org/changeset/124157>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.