Bug 92061 - [META] Reimplement RenderQuote to be simpler and safer (and also to follow the spec)
Summary: [META] Reimplement RenderQuote to be simpler and safer (and also to follow th...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Elliott Sprehn
URL:
Keywords:
Depends on: 92448 92690 92918 93056 93424
Blocks: 3234
  Show dependency treegraph
 
Reported: 2012-07-23 21:15 PDT by Elliott Sprehn
Modified: 2012-08-13 17:45 PDT (History)
11 users (show)

See Also:


Attachments
Patch (32.11 KB, patch)
2012-07-23 21:24 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (31.80 KB, patch)
2012-07-23 22:12 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Patch (33.53 KB, patch)
2012-07-24 20:13 PDT, Elliott Sprehn
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-07 (1018.97 KB, application/zip)
2012-07-24 22:47 PDT, WebKit Review Bot
no flags Details
Patch (33.14 KB, patch)
2012-07-26 17:57 PDT, Elliott Sprehn
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-02 (324.30 KB, application/zip)
2012-07-27 02:45 PDT, WebKit Review Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Elliott Sprehn 2012-07-23 21:15:54 PDT
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.
Comment 1 Elliott Sprehn 2012-07-23 21:24:10 PDT
Created attachment 153950 [details]
Patch

New implementation of RenderQuote.
Comment 2 Elliott Sprehn 2012-07-23 21:25:13 PDT
Note: This needs some tests, our test coverage for this stuff is quite bad.
Comment 3 Elliott Sprehn 2012-07-23 21:46:39 PDT
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.
Comment 4 Elliott Sprehn 2012-07-23 22:12:17 PDT
Created attachment 153958 [details]
Patch

Fix accidental breakage in regions.
Comment 5 Elliott Sprehn 2012-07-24 20:13:22 PDT
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.
Comment 6 Elliott Sprehn 2012-07-24 21:04:26 PDT
@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?
Comment 7 WebKit Review Bot 2012-07-24 22:47:05 PDT
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
Comment 8 WebKit Review Bot 2012-07-24 22:47:09 PDT
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
Comment 9 Elliott Sprehn 2012-07-26 17:57:20 PDT
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.
Comment 10 Julien Chaffraix 2012-07-26 19:45:34 PDT
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.
Comment 11 Elliott Sprehn 2012-07-26 21:33:45 PDT
(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.
Comment 12 WebKit Review Bot 2012-07-27 02:45:54 PDT
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
Comment 13 WebKit Review Bot 2012-07-27 02:45:58 PDT
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
Comment 14 Julien Chaffraix 2012-07-27 08:44:02 PDT
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.
Comment 15 Elliott Sprehn 2012-08-13 17:45:23 PDT
Hooray, this is all landed now.