Bug 92448 - Make QuotesData use a Vector of pairs
Summary: Make QuotesData use a Vector of pairs
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords:
Depends on:
Blocks: 92061
  Show dependency treegraph
 
Reported: 2012-07-26 19:21 PDT by Elliott Sprehn
Modified: 2012-07-30 19:56 PDT (History)
9 users (show)

See Also:


Attachments
Patch (15.66 KB, patch)
2012-07-26 19:27 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (15.74 KB, patch)
2012-07-26 21:14 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (15.74 KB, patch)
2012-07-27 12:49 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (18.09 KB, patch)
2012-07-27 15:48 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (20.24 KB, patch)
2012-07-30 15:16 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch for landing (20.35 KB, patch)
2012-07-30 16:40 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2012-07-26 19:21:44 PDT
Make QuotesData use a Vector of pairs
Comment 1 Elliott Sprehn 2012-07-26 19:27:49 PDT
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.
Comment 2 Elliott Sprehn 2012-07-26 21:14:26 PDT
Created attachment 154829 [details]
Patch

Incorporated review comments from jchaffraix on WKBug 92061.
Comment 3 Elliott Sprehn 2012-07-27 12:49:53 PDT
Created attachment 155028 [details]
Patch for landing
Comment 4 Julien Chaffraix 2012-07-27 14:25:02 PDT
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.
Comment 5 Elliott Sprehn 2012-07-27 14:42:14 PDT
(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.
Comment 6 Elliott Sprehn 2012-07-27 15:08:58 PDT
(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.
Comment 7 Julien Chaffraix 2012-07-27 15:20:11 PDT
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.
Comment 8 Elliott Sprehn 2012-07-27 15:38:41 PDT
(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.
Comment 9 Elliott Sprehn 2012-07-27 15:40:20 PDT
(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)
Comment 10 Elliott Sprehn 2012-07-27 15:48:58 PDT
Created attachment 155075 [details]
Patch

Second round of review.
Comment 11 Julien Chaffraix 2012-07-30 10:16:35 PDT
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.
Comment 12 Elliott Sprehn 2012-07-30 15:10:09 PDT
(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.
Comment 13 Elliott Sprehn 2012-07-30 15:16:58 PDT
Created attachment 155380 [details]
Patch

Added test.
Comment 14 Julien Chaffraix 2012-07-30 16:25:06 PDT
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.
Comment 15 Elliott Sprehn 2012-07-30 16:40:20 PDT
Created attachment 155396 [details]
Patch for landing
Comment 16 WebKit Review Bot 2012-07-30 19:56:25 PDT
Comment on attachment 155396 [details]
Patch for landing

Clearing flags on attachment: 155396

Committed r124157: <http://trac.webkit.org/changeset/124157>
Comment 17 WebKit Review Bot 2012-07-30 19:56:30 PDT
All reviewed patches have been landed.  Closing bug.