[webkit-reviews] review canceled: [Bug 92448] Make QuotesData use a Vector of pairs : [Attachment 155028] Patch for landing

[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 155028: Patch for landing
https://bugs.webkit.org/attachment.cgi?id=155028&action=review

------- Additional Comments from Julien Chaffraix <jchaffraix at webkit.org>
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.