[webkit-reviews] review granted: [Bug 92918] Built in quotes don't use lang attribute : [Attachment 156170] Patch

[bugzilla-daemon at webkit.org]

Alexey Proskuryakov <ap at webkit.org> has granted Elliott Sprehn
<esprehn at gmail.com>'s request for review:
Bug 92918: Built in quotes don't use lang attribute
https://bugs.webkit.org/show_bug.cgi?id=92918

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

------- Additional Comments from Alexey Proskuryakov <ap at webkit.org>
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?