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.
Created attachment 453291 [details] Patch
<rdar://problem/89510628>
Looks like we can't do this for readonly because of the special isCommonAttributeSelectorAttribute optimization.
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.
(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?
Created attachment 453327 [details] Patch
(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.
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?
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.
Created attachment 453335 [details] [fast-cq] Patch
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].
(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.