[webkit-reviews] review denied: [Bug 107779] Split CSSOMWrapper data and functions out from StyleResolver into its own class : [Attachment 184953] Rename CSSOMWrapper to CSSOMWrapperForInspector.

[bugzilla-daemon at webkit.org]

Dimitri Glazkov (Google) <dglazkov at chromium.org> has denied Hayato Ito
<hayato at chromium.org>'s request for review:
Bug 107779: Split CSSOMWrapper data and functions out from StyleResolver into
its own class
https://bugs.webkit.org/show_bug.cgi?id=107779

Attachment 184953: Rename CSSOMWrapper to CSSOMWrapperForInspector.
https://bugs.webkit.org/attachment.cgi?id=184953&action=review

------- Additional Comments from Dimitri Glazkov (Google)
<dglazkov at chromium.org>
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!