Closed Bug 1339619 Opened 3 years ago Closed 3 years ago
text-size-adjust should just use a keyword table
Bug 1339619 - Refactor parsing code for '-moz-text-size-adjust' to use the (more conventional) keyword table approach to calculate computed style.
59 bytes, text/x-review-board-request
neerja discovered that text-size-adjust doesn't use a keyword table, but instead uses VARIANT_AUTO and VARIANT_NONE, and then has to hardcode specified-style-to-computed-style conversion code in nsRuleNode.cpp (and the reverse in nsComputedDOMStyle.cpp) It should just use a keyword table, for consistency. (It's a keyword-valued property; might as well just use our standard keyword-valued-property codepaths, rather than trying to be tricky and hardcoding something more direct and more verbose/error-prone than the normal codepath.) I think the files to change here are nsCSSPropList.h, nsRuleNode.cpp, and nsComputedDOMStyle.cpp, and nsCSSProps.*
I suspect the code exists in its current state (eschewing a keyword table) because it's probably a bit faster this way. However, I claim the perf benefit here doesn't matter too much, since this is a prefixed property which likely doesn't get much usage. So, better IMO to optimize for code-comprehensibility & maintainability in this case. (Particularly in the nsComputedDOMStyle::DoGetTextSizeAdjust impl -- we can make that a nsCSSProps::ValueToKeywordEnum() invocation, rather than needing a switch statement. And particularly because this seems to be the *only* strictly-keyword-valued-property that does not use a keyword table.) dbaron: are you OK with this change? If you'd prefer to leave this as-is, I'll defer to your judgement. (It looks like this is code that you originally wrote back in bug 627842.)  https://dxr.mozilla.org/mozilla-central/source/layout/style/nsComputedDOMStyle.cpp#4205  I skimmed nsComputedDOMStyle for explicit "SetIdent(eCSSKeyword" calls and didn't find any others for pure-keyword-properties-with-no-keyword-table.
Depends on: font-inflation
(I filed this bug because our current implementation created a bit of confusion, by accident -- neerja mistakenly (but entirely understandably) assumed that she could implement "column-span:all|none" in exactly the same way that we implement "-moz-text-size-ajust:auto|none". Both properties have 2 possible keyword values, one of which is "none"; seems reasonable that they could have a very similar implementation. But, they can't, because we don't have special "all" handling like we do for "auto".) So: in the interests of avoiding similar pitfalls in the future, I'd like to either: (1) bring us into alignment with all the other keyword-valued properties here ...OR (2) add a comment to the -moz-text-size-adjust section in nsCSSPropList.h to explicitly call out that it's different & hint at why. I lean towards (1) but we could do (2) instead if you prefer -- dbaron, what do you recommend?
I'm fine with (1); I don't think I have particularly strong preferences here (but in the past I've tried to avoid mixing VARIANT_AUTO and VARIANT_NONE with things that have keyword tables).
(In reply to David Baron :dbaron: ⌚️UTC-8 from comment #3) > I'm fine with (1); OK, thanks! > (but in the past I've tried to avoid mixing VARIANT_AUTO and VARIANT_NONE > with things that have keyword tables). Agreed that that's desirable, and the end result of (1) will still adhere to that. (We'll shift from VARIANT_AUTO|VARIANT_NONE to VARIANT_HK, with eCSSKeyword_None & eCSSKeyword_Auto being the two keywords in the keyword table.)
Comment on attachment 8838224 [details] Bug 1339619 - Refactor parsing code for '-moz-text-size-adjust' to use the (more conventional) keyword table approach to calculate computed style. https://reviewboard.mozilla.org/r/113180/#review114678 Thanks! Looks good -- r=me with 2 nits addressed: ::: layout/style/nsComputedDOMStyle.h:438 (Diff revision 1) > already_AddRefed<CSSValue> DoGetTextOverflow(); > + already_AddRefed<CSSValue> DoGetTextSizeAdjust(); > already_AddRefed<CSSValue> DoGetTextTransform(); > already_AddRefed<CSSValue> DoGetTextShadow(); > already_AddRefed<CSSValue> DoGetLetterSpacing(); Nit: move this a few lines further down, to be just after DoGetTextShadow. That way, it'll be in alphabetical order, & it'll be consistent with the ordering you've used in the nsComputedDOMStyle.cpp file. ::: layout/style/nsRuleNode.cpp:4934 (Diff revision 1) > // text-size-adjust: none, auto, inherit, initial > - SetValue(*aRuleData->ValueForTextSizeAdjust(), text->mTextSizeAdjust, > + SetValue(*aRuleData->ValueForTextSizeAdjust(), Comment here needs updating. It should now say "enum, inherit, initial" I guess (matching the comments for the neighboring SetValue calls).
Attachment #8838224 - Flags: review?(dholbert) → review+
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/bb0f9802f0d0 Refactor parsing code for '-moz-text-size-adjust' to use the (more conventional) keyword table approach to calculate computed style. r=dholbert
You need to log in before you can comment on or make changes to this bug.