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>