Bug 92061 – [META] Reimplement RenderQuote to be simpler and safer (and also to follow the spec)

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Elliott Sprehn]

RenderQuote could be made much simpler, especially for code that's so rarely executed. It also doesn't follow the spec in a number of places and could be improved.

[Elliott Sprehn]

Created attachment 153950 [details]
Patch

New implementation of RenderQuote.

[Elliott Sprehn]

Note: This needs some tests, our test coverage for this stuff is quite bad.

[Elliott Sprehn]

Comment on attachment 153950 [details]
Patch

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

> Source/WebCore/rendering/RenderObjectChildList.cpp:-211
> -            toRenderRegion(newChild)->attachRegion();

This shouldn't have been removed. Code refactoring fail.

> Source/WebCore/rendering/RenderQuote.h:-31
> -    RenderQuote(Document*, const QuoteType);

Should be const, and m_type should be const too.

[Elliott Sprehn]

Created attachment 153958 [details]
Patch

Fix accidental breakage in regions.

[Elliott Sprehn]

Created attachment 154229 [details]
Patch

Revised implementation that removes unneeded tree walking and fixes bugs related to generated content in tables or when inserting a quote using script before the first quote of an existing document.

[Elliott Sprehn]

@jchaffraix: This is +225, -329 lines (and still needs some tests). If that's too big I think I might be able to break it into a couple of patches with some shim code during the transition?

[WebKit Review Bot]

Comment on attachment 154229 [details]
Patch

Attachment 154229 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13352091

New failing tests:
fast/forms/range/slider-onchange-event.html
fast/forms/range/slider-mouse-events.html
fast/forms/range/slider-delete-while-dragging-thumb.html

[WebKit Review Bot]

Created attachment 154257 [details]
Archive of layout-test-results from gce-cr-linux-07

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-07  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Elliott Sprehn]

Created attachment 154798 [details]
Patch

Made changes from in person review and changed to calculate the depth while dirtying later nodes in the list. This is where I want to take the code, Ill see what I can do to break up the patch next into smaller pieces.

[Julien Chaffraix]

Comment on attachment 154798 [details]
Patch

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

I like the direction it is going. The change is massive and I pointed out one orthogonal change that should be moved out of the main patch, I bet other parts could be moved too. I would really like to see some testing added as part of this patch or as a preparation.

> Source/WebCore/ChangeLog:20
> +        No new tests (OOPS!).

Per our discussion, we prefer changes in behavior to be covered by tests. Here there are a couple of places where you changed the behavior that would need to be tested. The more tests the better.

> Source/WebCore/css/StyleResolver.cpp:3452
> +            if (!(list->length() & 1))
> +                return;

This is a change in behavior. We used to access odd number of items.

Also this is not the right change. To reject those values, it should be done in CSSParser::parseQuotes, it would avoid having to try to apply this property over and over to just reject it every time.

> Source/WebCore/css/StyleResolver.cpp:3457
> +                CSSValue* first = list->item(i);
> +                CSSValue* second = list->item(i + 1);

We may want to use itemWithoutBoundsCheck here as we are guaranteed to be within the bounds.

> Source/WebCore/css/StyleResolver.cpp:3458
> +                if (!first || !first->isPrimitiveValue() || !second || !second->isPrimitiveValue())

We used to ASSERT that those were primitive values. I don't see any reason not to as CSSParser::parseQuotes filters out anything that is not a <string>.

> Source/WebCore/rendering/RenderQuote.cpp:35
> +typedef HashMap<AtomicString, RefPtr<QuotesData> > QuotesMap;

We could probably use QuotesData* directly if you use .leakRef() during the initialization below.

> Source/WebCore/rendering/RenderQuote.cpp:46
> +    return staticQuotesMap;

I think this change - including the |staticBasicQuotes| part - is orthogonal and should be moved to another bug.

> Source/WebCore/rendering/RenderQuote.cpp:54
> +    DEFINE_STATIC_LOCAL(RefPtr<QuotesData>, staticBasicQuotes, ());
> +    if (!staticBasicQuotes)
> +        staticBasicQuotes = QuotesData::create(U("\""), U("\""), U("'"), U("'"));
> +    return staticBasicQuotes.get();

I think you could write that:

static RefPtr<QuotesData> staticBasicQuotes = QuotesData::create(U("\""), U("\""), U("'"), U("'"));
return staticBasicQuotes.get();

> Source/WebCore/rendering/RenderQuote.cpp:69
> +    ASSERT(!m_attached && !m_next && !m_previous);

Shorter ASSERT are usually better than longer ones, especially if the clauses don't have a relation like here. It makes debugging easier as you know which part failed.

> Source/WebCore/rendering/RenderQuote.cpp:76
> +    ASSERT(!m_next && !m_previous && previousInPreOrder());

Same comment here.

I don't think the previousInPreOrder() check adds anything: you always have a RenderView.

> Source/WebCore/rendering/RenderQuote.cpp:79
> +        if (predecessor->isQuote()) {

Nit: earlier returns are preferred.

> Source/WebCore/rendering/RenderQuote.cpp:88
> +    if (!m_previous) {

This means you will always walk the tree for the first quote. I think you can get away without walking by checking view()->renderQuoteHead() and special casing the NULL case.

> Source/WebCore/rendering/RenderQuote.cpp:110
> +    if (!documentBeingDestroyed())
> +        for (RenderQuote* quote = m_next; quote; quote = quote->m_next)
> +            quote->updateDepth();

For statement longer than one line, we usually put curly braces.

> Source/WebCore/rendering/RenderQuote.cpp:123
> +    RenderText::styleDidChange(diff, oldStyle);

This should be moved at the beginning of the function. It's preferred to call the base class first for styleDidChange.

> Source/WebCore/rendering/RenderQuote.cpp:172
> +        for (RenderObject* renderer = parent(); renderer && !element; renderer = renderer->parent())
> +            element = toElement(renderer->node());

I would rather that you use isAnonymous() to check here. The logic is fine as node() returns 0 for anonymous renderers but it makes the intent a lot less easy to understand.

Also you mention tables so I bet your |renderer| should be a table part or a table in this case.

> Source/WebCore/rendering/RenderQuote.cpp:174
> +    }

This is a good change but it would need some tests.

> Source/WebCore/rendering/RenderQuote.cpp:203
> +        default:
> +            ASSERT_NOT_REACHED();

Removing the default case would break the compilation if we miss one case.

[Elliott Sprehn]

(In reply to comment #10)
> (From update of attachment 154798 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154798&action=review
>
> > Source/WebCore/css/StyleResolver.cpp:3458
> > +                if (!first || !first->isPrimitiveValue() || !second || !second->isPrimitiveValue())
> 
> We used to ASSERT that those were primitive values. I don't see any reason not to as CSSParser::parseQuotes filters out anything that is not a <string>.

No other case in this file ASSERTs about this, they just skip the values so I was trying to be consistent. I'll change back to an ASSERT.

> 
> > Source/WebCore/rendering/RenderQuote.cpp:46
> > +    return staticQuotesMap;
> 
> I think this change - including the |staticBasicQuotes| part - is orthogonal and should be moved to another bug.

Created WKBug 92448 and made this a meta bug.

> 
> > Source/WebCore/rendering/RenderQuote.cpp:54
> > +    DEFINE_STATIC_LOCAL(RefPtr<QuotesData>, staticBasicQuotes, ());
> > +    if (!staticBasicQuotes)
> > +        staticBasicQuotes = QuotesData::create(U("\""), U("\""), U("'"), U("'"));
> > +    return staticBasicQuotes.get();
> 
> I think you could write that:
> 
> static RefPtr<QuotesData> staticBasicQuotes = QuotesData::create(U("\""), U("\""), U("'"), U("'"));
> return staticBasicQuotes.get();

That would run the destructors on exit which fails compilation. Instead I used leakRef() and static const QuotesData*.

> 
> > Source/WebCore/rendering/RenderQuote.cpp:79
> > +        if (predecessor->isQuote()) {
> 
> Nit: earlier returns are preferred.

Trying to return early inside the loop means lots of duplication inside the loop and outside, and I think it's more confusing to understand this code. Did you mean something else?

> 
> > Source/WebCore/rendering/RenderQuote.cpp:88
> > +    if (!m_previous) {
> 
> This means you will always walk the tree for the first quote. I think you can get away without walking by checking view()->renderQuoteHead() and special casing the NULL case.

Good idea.

[WebKit Review Bot]

Comment on attachment 154798 [details]
Patch

Attachment 154798 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13378252

New failing tests:
fast/css/content/content-quotes-04.html
fast/css/content/content-quotes-05.html
fast/css/content/content-quotes-02.html
fast/css/content/content-quotes-03.html
fast/css/content/content-quotes-01.html

[WebKit Review Bot]

Created attachment 154886 [details]
Archive of layout-test-results from gce-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Julien Chaffraix]

Comment on attachment 154798 [details]
Patch

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

>>> Source/WebCore/css/StyleResolver.cpp:3458
>>> +                if (!first || !first->isPrimitiveValue() || !second || !second->isPrimitiveValue())
>> 
>> We used to ASSERT that those were primitive values. I don't see any reason not to as CSSParser::parseQuotes filters out anything that is not a <string>.
> 
> No other case in this file ASSERTs about this, they just skip the values so I was trying to be consistent. I'll change back to an ASSERT.

OK, I think it's fine to use ASSERTs everywhere due to CSSParser::parseQuotes filtering.

>>> Source/WebCore/rendering/RenderQuote.cpp:54
>>> +    return staticBasicQuotes.get();
>> 
>> I think you could write that:
>> 
>> static RefPtr<QuotesData> staticBasicQuotes = QuotesData::create(U("\""), U("\""), U("'"), U("'"));
>> return staticBasicQuotes.get();
> 
> That would run the destructors on exit which fails compilation. Instead I used leakRef() and static const QuotesData*.

You got me. I thought we would never remove the last reference but you are right. The leakRef() pattern seems better to me as the code would be more compact but it's equivalent to the existing one AFAICT so pick whichever you prefer.

>>> Source/WebCore/rendering/RenderQuote.cpp:79
>>> +        if (predecessor->isQuote()) {
>> 
>> Nit: earlier returns are preferred.
> 
> Trying to return early inside the loop means lots of duplication inside the loop and outside, and I think it's more confusing to understand this code. Did you mean something else?

Not early return indeed, I was thinking of using |continue| to avoid a nesting level in the loop.

[Elliott Sprehn]

Hooray, this is all landed now.