Bug 107779 – Split CSSOMWrapper data and functions out from StyleResolver into its own class

• Component: CSS
• Status: RESOLVED FIXED

[Hayato Ito]

This will be a separated patch from https://bugs.webkit.org/show_bug.cgi?id=107777.

[Hayato Ito]

Created attachment 184407 [details]
factor CSSOMWrapper logic into a CSSOMWrapper

[Andreas Kling]

Comment on attachment 184407 [details]
factor CSSOMWrapper logic into a CSSOMWrapper

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

Why should this be in a separate class? What is the purpose of this change?
CSSOMWrapper is a bad name for this class, as we already have CSSOM wrappers in WebKit, and this class has very little to do with them.

> Source/WebCore/css/StyleResolver.cpp:296
> +    , m_cssomWrapper()

This line is not needed.

> Source/WebCore/css/StyleResolver.h:455
> +    CSSOMWrapper& cssomWrapepr() { return m_cssomWrapper; }

This name is wrong.

[Hayato Ito]

Thank you for the review.

(In reply to comment #2)
> (From update of attachment 184407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184407&action=review
> 
> Why should this be in a separate class? What is the purpose of this change?
> CSSOMWrapper is a bad name for this class, as we already have CSSOM wrappers in WebKit, and this class has very little to do with them.

Good point. Sorry for the lack of the explanation in this patch.

This patch was a separated patch from bug 107777 as mentioned in comment #0. The WIP patch in 107777 requires this change so we can pass CSSOMWrapper's instance, instead of StyleResolver, to DocumentRuleSet::appendAuthorStyleSheets().
I'd like to avoid passing StyleResolver here. I'd like to minimize the dependency from the separated class to the StyleResolver.

Another motivation is to collect related functions and related data into its own class so we can avoid having static functions in StyleResolver. I think the reason is similar to https://bugs.webkit.org/show_bug.cgi?id=89879, which is a META bug.


> 
> > Source/WebCore/css/StyleResolver.cpp:296
> > +    , m_cssomWrapper()
> 
> This line is not needed.
> 
> > Source/WebCore/css/StyleResolver.h:455
> > +    CSSOMWrapper& cssomWrapepr() { return m_cssomWrapper; }
> 
> This name is wrong.

Let me re-think good naming. If you have any ideas, please let me know it.

[Hayato Ito]

Created attachment 184953 [details]
Rename CSSOMWrapper to CSSOMWrapperForInspector.

[Dominic Cooney]

Comment on attachment 184953 [details]
Rename CSSOMWrapper to CSSOMWrapperForInspector.

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

I am not a reviewer, but I caught a mispelling noted inline. Also some comments inline.

> Source/WebCore/ChangeLog:23
> +        (WebCore::CSSOMWrapperForInspector::collectFromSheet):

I notice that a lot of inspector-related types start with Inspector, so what about

InspectorCSSOMWrappers

? It is two chars shorter :)

Also makes me wonder if we can deport it to WebCore/inspector in a future change… what do you think?

> Source/WebCore/css/StyleResolver.cpp:-229
> -static void collectCSSOMWrappers(HashMap<StyleRule*, RefPtr<CSSStyleRule> >&, ListType*);

Looks like this code already used the term "wrapper", but it does look confusing, these are CSSOM objects and not JavaScript wrappers. Might make sense to do a renaming in a follow-up patch. (Are CSSOM objects referred to as "wrappers" elsewhere? Maybe there is precedent I am just not aware of.)

> Source/WebCore/css/StyleResolver.h:139
> +    CSSStyleRule* ensureAll(StyleRule*, DocumentStyleSheetCollection*);

I think moving this to its own class is a huge win. Of course this WARNING is not visible at call sites, and

StyleResolver::ensureAll

looks innocuous;

CSSOMWrapperForInspector::ensureAll

is way more likely to be detected as bogus if used in the wrong context.

> Source/WebCore/css/StyleResolver.h:455
> +    CSSOMWrapperForInspector& cssomWrapeprForInspector() { return m_cssomWrapperForInspector; }

Wrapepr => Wrapper

[Dimitri Glazkov (Google)]

Comment on attachment 184953 [details]
Rename CSSOMWrapper to CSSOMWrapperForInspector.

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

>> Source/WebCore/css/StyleResolver.cpp:-229
>> -static void collectCSSOMWrappers(HashMap<StyleRule*, RefPtr<CSSStyleRule> >&, ListType*);
> 
> Looks like this code already used the term "wrapper", but it does look confusing, these are CSSOM objects and not JavaScript wrappers. Might make sense to do a renaming in a follow-up patch. (Are CSSOM objects referred to as "wrappers" elsewhere? Maybe there is precedent I am just not aware of.)

Kling/Antti recently made a pretty cool optimization, where CSSOM objects are actually created on demand, rather than by default, allowing most of CSS machinery function without lugging the CSSOM objects around. The "wrapper" here refers to the object that is created on demand.

> Source/WebCore/css/StyleResolver.h:135
> +class CSSOMWrapperForInspector {

Nitpicking the name: it's not really a wrapper, it's a bunch of wrappers. So, at the very least, it's CSSOMWrappersForInspector. I like Dominic's InspectorCSSOMWrappers suggestion.

>> Source/WebCore/css/StyleResolver.h:139
>> +    CSSStyleRule* ensureAll(StyleRule*, DocumentStyleSheetCollection*);
> 
> I think moving this to its own class is a huge win. Of course this WARNING is not visible at call sites, and
> 
> StyleResolver::ensureAll
> 
> looks innocuous;
> 
> CSSOMWrapperForInspector::ensureAll
> 
> is way more likely to be detected as bogus if used in the wrong context.

Yup, I agree.

>> Source/WebCore/css/StyleResolver.h:455
>> +    CSSOMWrapperForInspector& cssomWrapeprForInspector() { return m_cssomWrapperForInspector; }
> 
> Wrapepr => Wrapper

Derp!

[Hayato Ito]

Thank you for the review. Let me upload a new patch soon.

(In reply to comment #5)
> (From update of attachment 184953 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184953&action=review
> 
> I am not a reviewer, but I caught a mispelling noted inline. Also some comments inline.
> 
> > Source/WebCore/ChangeLog:23
> > +        (WebCore::CSSOMWrapperForInspector::collectFromSheet):
> 
> I notice that a lot of inspector-related types start with Inspector, so what about
> 
> InspectorCSSOMWrappers
> 
> ? It is two chars shorter :)

That makes sense to me. A nice name. Done.

> 
> Also makes me wonder if we can deport it to WebCore/inspector in a future change… what do you think?

Sounds good. I think we can. Let me do that later in a future change.

> 
> > Source/WebCore/css/StyleResolver.cpp:-229
> > -static void collectCSSOMWrappers(HashMap<StyleRule*, RefPtr<CSSStyleRule> >&, ListType*);
> 
> Looks like this code already used the term "wrapper", but it does look confusing, these are CSSOM objects and not JavaScript wrappers. Might make sense to do a renaming in a follow-up patch. (Are CSSOM objects referred to as "wrappers" elsewhere? Maybe there is precedent I am just not aware of.)
> 
> > Source/WebCore/css/StyleResolver.h:139
> > +    CSSStyleRule* ensureAll(StyleRule*, DocumentStyleSheetCollection*);
> 
> I think moving this to its own class is a huge win. Of course this WARNING is not visible at call sites, and
> 
> StyleResolver::ensureAll
> 
> looks innocuous;
> 
> CSSOMWrapperForInspector::ensureAll
> 
> is way more likely to be detected as bogus if used in the wrong context.
> 
> > Source/WebCore/css/StyleResolver.h:455
> > +    CSSOMWrapperForInspector& cssomWrapeprForInspector() { return m_cssomWrapperForInspector; }
> 
> Wrapepr => Wrapper

Ops. Fixed.

(In reply to comment #6)
> (From update of attachment 184953 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=184953&action=review
> 
> >> Source/WebCore/css/StyleResolver.cpp:-229
> >> -static void collectCSSOMWrappers(HashMap<StyleRule*, RefPtr<CSSStyleRule> >&, ListType*);
> > 
> > Looks like this code already used the term "wrapper", but it does look confusing, these are CSSOM objects and not JavaScript wrappers. Might make sense to do a renaming in a follow-up patch. (Are CSSOM objects referred to as "wrappers" elsewhere? Maybe there is precedent I am just not aware of.)
> 
> Kling/Antti recently made a pretty cool optimization, where CSSOM objects are actually created on demand, rather than by default, allowing most of CSS machinery function without lugging the CSSOM objects around. The "wrapper" here refers to the object that is created on demand.
> 
> > Source/WebCore/css/StyleResolver.h:135
> > +class CSSOMWrapperForInspector {
> 
> Nitpicking the name: it's not really a wrapper, it's a bunch of wrappers. So, at the very least, it's CSSOMWrappersForInspector. I like Dominic's InspectorCSSOMWrappers suggestion.
> 
> >> Source/WebCore/css/StyleResolver.h:139
> >> +    CSSStyleRule* ensureAll(StyleRule*, DocumentStyleSheetCollection*);
> > 
> > I think moving this to its own class is a huge win. Of course this WARNING is not visible at call sites, and
> > 
> > StyleResolver::ensureAll
> > 
> > looks innocuous;
> > 
> > CSSOMWrapperForInspector::ensureAll
> > 
> > is way more likely to be detected as bogus if used in the wrong context.
> 
> Yup, I agree.
> 
> >> Source/WebCore/css/StyleResolver.h:455
> >> +    CSSOMWrapperForInspector& cssomWrapeprForInspector() { return m_cssomWrapperForInspector; }
> > 
> > Wrapepr => Wrapper
> 
> Derp!

[Hayato Ito]

Created attachment 185180 [details]
Fixed typo. Renamed to InspectorCSSOMWrappers.

[Hajime Morrita]

Comment on attachment 185180 [details]
Fixed typo. Renamed to InspectorCSSOMWrappers.

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

> Source/WebCore/css/StyleResolver.h:137
> +    // WARNING. This will construct CSSOM wrappers for all style rules and cache then in a map for significant memory cost.

Nit: s/then/them/
Actually, this message is explaining not only this function but also this whole class.

Probably we could do this collect thing in the constructor and have this class as a OwnPtr. Then lifecycle will be clearer.

[Hayato Ito]

(In reply to comment #9)
> (From update of attachment 185180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review
> 
> > Source/WebCore/css/StyleResolver.h:137
> > +    // WARNING. This will construct CSSOM wrappers for all style rules and cache then in a map for significant memory cost.
> 
> Nit: s/then/them/
> Actually, this message is explaining not only this function but also this whole class.
> 
> Probably we could do this collect thing in the constructor and have this class as a OwnPtr. Then lifecycle will be clearer.

Good point.
Although I don't have an intention to introduce any behavior changes in this patch, the mentioned point can be another good reason to factor this into itw own class.

Let me investigate how InspectorCSSOMWrappers is used in real case later. I think that can be done in a following patch.

[Dimitri Glazkov (Google)]

Comment on attachment 185180 [details]
Fixed typo. Renamed to InspectorCSSOMWrappers.

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

> Source/WebCore/ChangeLog:21
> +        (WebCore):

It's good WebKit practice to describe changes in the ChangeLog.

> Source/WebCore/css/StyleResolver.h:139
> +    CSSStyleRule* ensureAll(StyleRule*, DocumentStyleSheetCollection*);

Still nitpicking names, sorry. ensureAll and collectFromSheet are complementary. The first one makes wrappers for all available stylesheets. The other one makes additional wrappers for a newly added stylesheet -- but only if the first one ran beforehand.

So maybe collectFromSheet's name should reflect that? ensureNewSheet?

Or at least, collectFromSheetIfNeeded?

> Source/WebCore/css/StyleResolver.h:150
> +    void collect(ListType*);
> +
> +    void collect(DocumentStyleSheetCollection*);
> +    void collect(const Vector<RefPtr<CSSStyleSheet> >&);
> +    void collect(HashSet<RefPtr<CSSStyleSheet> >& sheetWrapperSet, StyleSheetContents*);

These are probably fine, but the same name overloaded over different arguments seems sad, especially given they are all private. Overloading might be a good API strategy, but here it's just not necessary.

[Dimitri Glazkov (Google)]

Comment on attachment 185180 [details]
Fixed typo. Renamed to InspectorCSSOMWrappers.

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

> Source/WebCore/inspector/InspectorCSSAgent.cpp:974
> +        m_currentSelectorProfile->startSelector(styleResolver->inspectorCSSOMWrappers().ensureAll(rule, styleResolver->document()->styleSheetCollection()));

Looking at the callsite, "ensureAll" is the one that has the wrong name. It should just be a "get". Then what do we do with the StyleSheetCollection argument? :-\

[Hayato Ito]

Created attachment 185404 [details]
No longer overloading.

[Hayato Ito]

Thank you for the review.

(In reply to comment #11)
> (From update of attachment 185180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review
> 
> > Source/WebCore/ChangeLog:21
> > +        (WebCore):
> 
> It's good WebKit practice to describe changes in the ChangeLog.

Done.

> 
> > Source/WebCore/css/StyleResolver.h:139
> > +    CSSStyleRule* ensureAll(StyleRule*, DocumentStyleSheetCollection*);
> 
> Still nitpicking names, sorry. ensureAll and collectFromSheet are complementary. The first one makes wrappers for all available stylesheets. The other one makes additional wrappers for a newly added stylesheet -- but only if the first one ran beforehand.
> 
> So maybe collectFromSheet's name should reflect that? ensureNewSheet?
> 
> Or at least, collectFromSheetIfNeeded?

I like collectFromStyleSheet.

> 
> > Source/WebCore/css/StyleResolver.h:150
> > +    void collect(ListType*);
> > +
> > +    void collect(DocumentStyleSheetCollection*);
> > +    void collect(const Vector<RefPtr<CSSStyleSheet> >&);
> > +    void collect(HashSet<RefPtr<CSSStyleSheet> >& sheetWrapperSet, StyleSheetContents*);
> 
> These are probably fine, but the same name overloaded over different arguments seems sad, especially given they are all private. Overloading might be a good API strategy, but here it's just not necessary.

Done. I've named these functions explicitly.

I don't have a strong opinion about that. I guess the original intention about overloading is to use the same name to the template<ListType*> function.
Maybe explicit is better than implicit here.

[Hayato Ito]

(In reply to comment #14)
> Thank you for the review.
> 
> (In reply to comment #11)
> > (From update of attachment 185180 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review
> > 
> > > Source/WebCore/ChangeLog:21
> > > +        (WebCore):
> > 
> > It's good WebKit practice to describe changes in the ChangeLog.
> 
> Done.
> 
> > 
> > > Source/WebCore/css/StyleResolver.h:139
> > > +    CSSStyleRule* ensureAll(StyleRule*, DocumentStyleSheetCollection*);
> > 
> > Still nitpicking names, sorry. ensureAll and collectFromSheet are complementary. The first one makes wrappers for all available stylesheets. The other one makes additional wrappers for a newly added stylesheet -- but only if the first one ran beforehand.
> > 
> > So maybe collectFromSheet's name should reflect that? ensureNewSheet?
> > 
> > Or at least, collectFromSheetIfNeeded?
> 
> I like collectFromStyleSheet.

s/collectFromStyleSheet/collectFromStyleSheetIfNeeded/.

> 
> > 
> > > Source/WebCore/css/StyleResolver.h:150
> > > +    void collect(ListType*);
> > > +
> > > +    void collect(DocumentStyleSheetCollection*);
> > > +    void collect(const Vector<RefPtr<CSSStyleSheet> >&);
> > > +    void collect(HashSet<RefPtr<CSSStyleSheet> >& sheetWrapperSet, StyleSheetContents*);
> > 
> > These are probably fine, but the same name overloaded over different arguments seems sad, especially given they are all private. Overloading might be a good API strategy, but here it's just not necessary.
> 
> Done. I've named these functions explicitly.
> 
> I don't have a strong opinion about that. I guess the original intention about overloading is to use the same name to the template<ListType*> function.
> Maybe explicit is better than implicit here.

[Hayato Ito]

(In reply to comment #12)
> (From update of attachment 185180 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review
> 
> > Source/WebCore/inspector/InspectorCSSAgent.cpp:974
> > +        m_currentSelectorProfile->startSelector(styleResolver->inspectorCSSOMWrappers().ensureAll(rule, styleResolver->document()->styleSheetCollection()));
> 
> Looking at the callsite, "ensureAll" is the one that has the wrong name. It should just be a "get". Then what do we do with the StyleSheetCollection argument? :-\

Hmm.. 'ensureAllAndGet'? or 'getWithEnsuringAll'? What is the best name? :)

[Dominic Cooney]

Comment on attachment 185404 [details]
No longer overloading.

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

> Source/WebCore/ChangeLog:8
> +        Factored CSSOMWrapper logic and data out from StyleResolver into a

Does this sentence add anything that is not in the summary line?

> Source/WebCore/ChangeLog:15
> +        in a following patch (bug 107780), I'll move a

Nit: No need for the trailing "a" here.

[Dominic Cooney]

(In reply to comment #16)
> (In reply to comment #12)
> > (From update of attachment 185180 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review
> > 
> > > Source/WebCore/inspector/InspectorCSSAgent.cpp:974
> > > +        m_currentSelectorProfile->startSelector(styleResolver->inspectorCSSOMWrappers().ensureAll(rule, styleResolver->document()->styleSheetCollection()));
> > 
> > Looking at the callsite, "ensureAll" is the one that has the wrong name. It should just be a "get". Then what do we do with the StyleSheetCollection argument? :-\
> 
> Hmm.. 'ensureAllAndGet'? or 'getWithEnsuringAll'? What is the best name? :)

What about getWrapperForRuleInSheets?

[Dimitri Glazkov (Google)]

Comment on attachment 185404 [details]
No longer overloading.

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

> Source/WebCore/css/StyleResolver.h:135
> +class InspectorCSSOMWrappers {

The obvious thing to do here is to let this guy hold a ref to DocumentStyleSheetCollection. But that's extra weight :-\

> Source/WebCore/inspector/InspectorCSSAgent.cpp:962
> +        m_currentSelectorProfile->startSelector(styleResolver->inspectorCSSOMWrappers().ensureAll(rule, styleResolver->document()->styleSheetCollection()));

The next easiest thing is to split up this callsite to first "collectAllSheets", and then "get".

[Hayato Ito]

Thank you for the review. Let me upload a new patch soon.

(In reply to comment #17)
> (From update of attachment 185404 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185404&action=review
> 
> > Source/WebCore/ChangeLog:8
> > +        Factored CSSOMWrapper logic and data out from StyleResolver into a
> 
> Does this sentence add anything that is not in the summary line?

That's redundant. Removed.

> 
> > Source/WebCore/ChangeLog:15
> > +        in a following patch (bug 107780), I'll move a
> 
> Nit: No need for the trailing "a" here.

Done.

(In reply to comment #18)
> (In reply to comment #16)
> > (In reply to comment #12)
> > > (From update of attachment 185180 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=185180&action=review
> > > 
> > > > Source/WebCore/inspector/InspectorCSSAgent.cpp:974
> > > > +        m_currentSelectorProfile->startSelector(styleResolver->inspectorCSSOMWrappers().ensureAll(rule, styleResolver->document()->styleSheetCollection()));
> > > 
> > > Looking at the callsite, "ensureAll" is the one that has the wrong name. It should just be a "get". Then what do we do with the StyleSheetCollection argument? :-\
> > 
> > Hmm.. 'ensureAllAndGet'? or 'getWithEnsuringAll'? What is the best name? :)
> 
> What about getWrapperForRuleInSheets?

Okay. I've renamed it.


(In reply to comment #19)
> (From update of attachment 185404 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=185404&action=review
> 
> > Source/WebCore/css/StyleResolver.h:135
> > +class InspectorCSSOMWrappers {
> 
> The obvious thing to do here is to let this guy hold a ref to DocumentStyleSheetCollection. But that's extra weight :-\
> 
> > Source/WebCore/inspector/InspectorCSSAgent.cpp:962
> > +        m_currentSelectorProfile->startSelector(styleResolver->inspectorCSSOMWrappers().ensureAll(rule, styleResolver->document()->styleSheetCollection()));
> 
> The next easiest thing is to split up this callsite to first "collectAllSheets", and then "get".

I don't want to split this into two functions until there is a strong requirement.
If we split this into two functions, we have to check whether all callers call them sequentially.
I am afraid that will cause potential misusage of APIs.

[Hayato Ito]

Created attachment 185667 [details]
Addressed.

[Dimitri Glazkov (Google)]

Comment on attachment 185667 [details]
Addressed.

you wore me down.

[WebKit Review Bot]

Comment on attachment 185667 [details]
Addressed.

Clearing flags on attachment: 185667

Committed r141373: <http://trac.webkit.org/changeset/141373>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.