Bug 61408 – MediaList should not inherit from StyleBase

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Julien Chaffraix]

As part of cutting down the dependency on StyleBase. MediaList does not strictly need to inherit from StyleBase. It just needs to be TreeShared<StyleBase>. Patch forthcoming.

[Julien Chaffraix]

Created attachment 94723 [details]
Proposed refactoring

[WebKit Commit Bot]

Comment on attachment 94723 [details]
Proposed refactoring

Attachment 94723 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/8729746

[Julien Chaffraix]

Created attachment 94901 [details]
Take 2: Same as previously but should make the Mac bot happy

[Eric Seidel (no email)]

I'm not sure I understand all the pieces at play here.

[Brent Fulgham]

Comment on attachment 94901 [details]
Take 2: Same as previously but should make the Mac bot happy

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

Could you please clarify the behavior of the new m_parsedMediaLists member?  How are these elements accessed/traversed now?

> Source/WebCore/bindings/js/JSDOMBinding.h:179
> +    inline void* root(MediaList* list)

You shouldn't need to declare this inline since the implementation is in the header file.

> Source/WebCore/css/CSSParser.cpp:6276
> +    m_parsedMediaLists.append(list.release());

Previously we operated on media lists and rules as equivalent entities.  Why is it better to separate them like this?  Also, I don't see where the m_parsedStyleObjects method is revised to handle the presumably new access methods to get Media Lists (as opposed to Style Rules) out of the parser?

> Source/WebCore/css/CSSParser.h:323
> +        Vector<RefPtr<MediaList> > m_parsedMediaLists;

Again -- why do we want to handle these items as separate entities?  Does it make something more efficient, or simplify code elsewhere?

[Julien Chaffraix]

Comment on attachment 94901 [details]
Take 2: Same as previously but should make the Mac bot happy

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

>> Source/WebCore/bindings/js/JSDOMBinding.h:179
>> +    inline void* root(MediaList* list)
> 
> You shouldn't need to declare this inline since the implementation is in the header file.

This is true. However the other root() methods use inline. I would rather keep the code consistent here even if it is redundant.

>> Source/WebCore/css/CSSParser.cpp:6276
>> +    m_parsedMediaLists.append(list.release());
> 
> Previously we operated on media lists and rules as equivalent entities.  Why is it better to separate them like this?  Also, I don't see where the m_parsedStyleObjects method is revised to handle the presumably new access methods to get Media Lists (as opposed to Style Rules) out of the parser?

As far as the media lists vs rules goes, I don't think there is a problem in either direction. I would argue that it makes more sense to split it between the different types of objects as it is done to some extend in CSSParser already (see m_parsedRuleList vs m_parsedStyleObjects).
m_parsedStyleObjects is never read, only appended to and cleared in the destructor so splitting the object in 2 should not have an ill effect.

>> Source/WebCore/css/CSSParser.h:323
>> +        Vector<RefPtr<MediaList> > m_parsedMediaLists;
> 
> Again -- why do we want to handle these items as separate entities?  Does it make something more efficient, or simplify code elsewhere?

I don't think this will make the code more efficient. It would simplify the code as MediaList only needed the TreeShared behavior from StyleBase not the whole logic from the class. StyleBase is the default class a lot of CSS objects inherit from but it is a fallacy as not all objects needs to use this "interface".

[Julien Chaffraix]

Comment on attachment 94901 [details]
Take 2: Same as previously but should make the Mac bot happy

Discussed this with Antti on IRC, adding a dependency on TreeShared is not the way to go. It would be better to add the parent logic directly into the class and make MediaList RefCounted.

[Julien Chaffraix]

Created attachment 96961 [details]
Updated patch, not for review until I can re-engineer the change

[Julien Chaffraix]

Andreas Kling came and during his CSS refactoring got rid of the dependency.

See bug 70203.