Bug 237235 – Optimize StyleSharingResolver inert check

• Component: CSS
• Status: RESOLVED FIXED

[Tim Nguyen (:ntim)]

readonly & inert are boolean attributes, the only thing that matters is the presence of the attribute, not its value. E.g. `readonly=false` behaves the same as `readonly=readonly` or `readonly=true`.

This saves getting and comparing values for those attributes, and also allows `readonly=readonly` and `readonly=true` or `readonly=false` to start using style sharing.

Same applies for inert.

[Tim Nguyen (:ntim)]

Created attachment 453291 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/89510628>

[Tim Nguyen (:ntim)]

Looks like we can't do this for readonly because of the special isCommonAttributeSelectorAttribute optimization.

[Darin Adler]

Comment on attachment 453291 [details]
Patch

Code change looks great. Need to update tests to reflect a progression, I think? I will set review+ once there’s a patch that includes the needed changes to tests.

[Darin Adler]

(In reply to Tim Nguyen (:ntim) from comment #3)
> Looks like we can't do this for readonly because of the special
> isCommonAttributeSelectorAttribute optimization.

Oh really? That seems unfortunate. Maybe we can fix that optimization too?

[Tim Nguyen (:ntim)]

Created attachment 453327 [details]
Patch

[Tim Nguyen (:ntim)]

(In reply to Darin Adler from comment #5)
> (In reply to Tim Nguyen (:ntim) from comment #3)
> > Looks like we can't do this for readonly because of the special
> > isCommonAttributeSelectorAttribute optimization.
> 
> Oh really? That seems unfortunate. Maybe we can fix that optimization too?

The readonly/type attributes are there for different reasons than the inert attribute.

E.g. if we removed readonly/type from `isCommonAttributeSelectorAttribute`, then they wouldn't need this style sharing check.

The inert attribute is there because StyleAdjuster applies different styling based on the inert attribute's presence, and we don't want to wrongly share that styling.

I got it wrong the first time :( I added some comments in the code to avoid this confusion.

(In reply to Darin Adler from comment #4)
> Comment on attachment 453291 [details]
> Patch
> 
> Code change looks great. Need to update tests to reflect a progression, I
> think? I will set review+ once there’s a patch that includes the needed
> changes to tests.

The failures were actually a regression in readonly's style sharing, the tests actually had different styling for [readonly=foo] and [readonly] and we rendered them the same.

[youenn fablet]

Comment on attachment 453327 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        `inert=inert` and `inert=true` or `inert=false` to start sharing style.

This seems testable, can we add a test?

[Tim Nguyen (:ntim)]

Comment on attachment 453327 [details]
Patch

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

>> Source/WebCore/ChangeLog:13
>> +        `inert=inert` and `inert=true` or `inert=false` to start sharing style.
> 
> This seems testable, can we add a test?

style sharing is an optimization, so behavior before/after this patch should be the same, just with different speeds.

I could add tests to make sure the optimization does not break anything though. I'll copy the ones readonly have, and add some inert specific ones too.

[Tim Nguyen (:ntim)]

Created attachment 453335 [details]
[fast-cq] Patch

[EWS]

Committed r290559 (247837@main): <https://commits.webkit.org/247837@main>

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

[Darin Adler]

(In reply to Tim Nguyen (:ntim) from comment #9)
> style sharing is an optimization, so behavior before/after this patch should
> be the same, just with different speeds.

Important optimizations can be tested too, with tests designed to have dramatically different performance without the optimization. These are valuable and can be important, almost as important as correctness tests, even though they can be more difficult to create.