Bug 210917

Created attachment 397349 [details]
Patch

Comment on attachment 397349 [details]
Patch

Hi,
I think the patch is ready to review, thanks:)

Comment on attachment 397349 [details]
Patch

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

> Source/WebCore/css/CSSStyleDeclaration.cpp:171
> +        // If the property is disabled, should return CSSPropertyInvalid from here.

This comment isn’t valuable. It just says what the code below does. If it said *why* then it might be valuable.

Given that we also do this check *before* putting a property into the cache, it does not seem good that this is needed; not great for performance, basically defeating the cache.

I think the real issue is this:

1) We try to use a single cache, but that results in us caching things that depend on settings, and we could get called later with a different settings object, or nullptr.

2) Even if it was the same settings object, if settings change, the cache needs to be cleared.

> Source/WebCore/css/CSSStyleDeclaration.cpp:172
> +        if (!isEnabledCSSProperty(propertyInfo.propertyID) && !isCSSPropertyEnabledBySettings(propertyInfo.propertyID, settingsPtr))

I would make a helper function rather than repeating this code twice. But I think the only reason we are repeating it twice is that we don’t know how to make this work with the cache.

> Source/WebCore/css/CSSStyleDeclaration.cpp:173
> +            propertyInfo = { CSSPropertyInvalid, false };

Could just return this, instead of assigning it to the local.

Comment on attachment 397349 [details]
Patch

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

Hi Darin,

Thanks for the review:)

>> Source/WebCore/css/CSSStyleDeclaration.cpp:171
>> +        // If the property is disabled, should return CSSPropertyInvalid from here.
> 
> This comment isn’t valuable. It just says what the code below does. If it said *why* then it might be valuable.
> 
> Given that we also do this check *before* putting a property into the cache, it does not seem good that this is needed; not great for performance, basically defeating the cache.
> 
> I think the real issue is this:
> 
> 1) We try to use a single cache, but that results in us caching things that depend on settings, and we could get called later with a different settings object, or nullptr.
> 
> 2) Even if it was the same settings object, if settings change, the cache needs to be cleared.

I see... Maybe we can cache propertyInfo anyway, because the "enable checking" needs propertyID and cache can reduce the parsing work.

>> Source/WebCore/css/CSSStyleDeclaration.cpp:172
>> +        if (!isEnabledCSSProperty(propertyInfo.propertyID) && !isCSSPropertyEnabledBySettings(propertyInfo.propertyID, settingsPtr))
> 
> I would make a helper function rather than repeating this code twice. But I think the only reason we are repeating it twice is that we don’t know how to make this work with the cache.

Done. Added isCSSPropertyDisabled().

>> Source/WebCore/css/CSSStyleDeclaration.cpp:173
>> +            propertyInfo = { CSSPropertyInvalid, false };
> 
> Could just return this, instead of assigning it to the local.

Done.

Created attachment 397420 [details]
Patch

Created attachment 397438 [details]
Patch

Comment on attachment 397438 [details]
Patch

OK. I think we could refactor this a bit more dividing into one more function that does the disabled check after parsing, so the end of the function was shared between the cached and non-cashed case.

(In reply to Darin Adler from comment #7)
> Comment on attachment 397438 [details]
> Patch
> 
> OK. I think we could refactor this a bit more dividing into one more
> function that does the disabled check after parsing, so the end of the
> function was shared between the cached and non-cashed case.

Done! Thanks:)

Created attachment 397568 [details]
Patch

Comment on attachment 397568 [details]
Patch

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

> Source/WebCore/css/CSSStyleDeclaration.cpp:265
> +static CSSPropertyInfo getCSSPropertyInfoFromJavaScriptCSSPropertyName(const AtomString& propertyName, const Settings* settingsPtr)

Three more tiny things we could do after landing this (or before if not too late), none of which are critical:

- WebKit coding style says we don’t use the word "get" in the name of a function like this one. I would give it a shorter name that doesn’t repeat CSS twice.
- I’d name the settingsPtr arguments just "settings".
- I’d move the isCSSPropertyDisabled down past the larger parseJavaScriptCSSPropertyName function. Or maybe just collapse it in here again and not leave it as a separate function.

Anyway, looks good even without these changes!

Created attachment 397607 [details]
Patch

(In reply to Darin Adler from comment #10)
> Comment on attachment 397568 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=397568&action=review
> 
> > Source/WebCore/css/CSSStyleDeclaration.cpp:265
> > +static CSSPropertyInfo getCSSPropertyInfoFromJavaScriptCSSPropertyName(const AtomString& propertyName, const Settings* settingsPtr)
> 
> Three more tiny things we could do after landing this (or before if not too
> late), none of which are critical:
> 
> - WebKit coding style says we don’t use the word "get" in the name of a
> function like this one. I would give it a shorter name that doesn’t repeat
> CSS twice.
> - I’d name the settingsPtr arguments just "settings".
> - I’d move the isCSSPropertyDisabled down past the larger
> parseJavaScriptCSSPropertyName function. Or maybe just collapse it in here
> again and not leave it as a separate function.
> 
> Anyway, looks good even without these changes!

Done! Thanks for pointing out:)

Created attachment 397613 [details]
Patch

Committed r260723: <https://trac.webkit.org/changeset/260723>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 397613 [details].

<rdar://problem/62383245>