Remove unnecessary eCSSKeyword_UNKNOWN checks before nsCSSProps::FindKeyword calls

RESOLVED FIXED in Firefox 53

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
10 months ago
10 months ago

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

Trunk
mozilla53
Points:
---

Firefox Tracking Flags

(firefox53 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

10 months ago
At many of the callsites to nsCSSProps::FindKeyword() calls, we have a preliminary check for eCSSKeyword_UNKNOWN, and we skip the call if we've got that sentinel keyword.  This seems to be part of our standard boilerplate, to the point where I thought it was necessary during a code-review just now.

HOWEVER, this check is unnecessary, because FindKeyword() handles eCSSKeyword_UNKNOWN relatively gracefully -- it catches it right away and returns early, via its call to FindIndexOfKeyword() (which checks for _UNKNOWN as the very first thing).

So, to simplify logic/error-handling/etc, I think we should just remove hardcoded eCSSKeyword_UNKNOWN checks, at callsites to nsCSSProps::FindKeyword() -- at least, in cases where the check is only guarding the FindKeyword call.
(Assignee)

Comment 1

10 months ago
At some point in the past, these checks were useful, because FindIndexOfKeyword didn't have a special early-return for eCSSKeyword_UNKNOWN.  Back then, if we called FindKeyword/FindIndexOfKeyword with eCSSKeyword_UNKNOWN, it would spin its wheels walking the full keyword array, only to find {eCSSKeyword_UNKNOWN,-1} at the very end.

So at that point, the callsites' preliminary _UNKNOWN-checks were an optimization to skip that unnecessary full-array-walk.

But as of bug 914432, we've got eCSSKeyword_UNKNOWN as a semi-legitimate keyword value *in the middle of arrays* (to help support pref-controlled property-values), which means FindIndexOfKeyword now behaves differently from how it used to, and has to have this early-return, as explained in the comment in the commit that added it:
https://hg.mozilla.org/mozilla-central/diff/e2909a86db2d/layout/style/nsCSSProps.cpp#l1.66

SO.  All of this is to say: the callsites' checks made some sense originally, but they don't serve the same optimization purpose that they used to serve.  So we might as well get rid of them, to clean up code & share error-handling codepaths.
(Assignee)

Updated

10 months ago
Depends on: 914432
(Assignee)

Updated

10 months ago
Assignee: nobody → dholbert
Comment hidden (mozreview-request)
(Assignee)

Comment 3

10 months ago
mozreview-review
Comment on attachment 8822749 [details]
Bug 1326457: Remove redundant check for eCSSKeyword_UNKNOWN at callsites to nsCSSProps::FindKeyword().

https://reviewboard.mozilla.org/r/101552/#review102072

I'll make two notes on the patch, to hopefully assist in review:

::: layout/style/nsCSSParser.cpp:6733
(Diff revision 1)
>        else {
>          nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(tk->mIdent);
> -        if (eCSSKeyword_UNKNOWN < keyword) { // known keyword

Note: this clashes with the patch on bug 894245 -- that patch also removes this particular _UNKNOWN comparison, as a tangential simplification.

I'd like to clean up this comparison here, rather than in bug 894245, so that it happens as part of a self-contained commit that includes an explanation for the removal & is entirely about cleaning up these comparisons.  And then I'm hoping you can rebase bug 894245 on top, as I noted in bug 894245 comment 11.  (Thanks in advance -- I don't think the rebase should be too tricky.)

::: layout/style/nsCSSParser.cpp:7555
(Diff revision 1)
>    nsCSSKeyword keyword = nsCSSKeywords::LookupKeyword(*ident);
> -  if (eCSSKeyword_UNKNOWN < keyword) {
> -    int32_t value;
> +  int32_t value;
> -    if (nsCSSProps::FindKeyword(keyword, aKeywordTable, value)) {
> +  if (nsCSSProps::FindKeyword(keyword, aKeywordTable, value)) {

Note: in some of the cleanups (like this one), I'm removing a "eCSSKeyword_UNKNOWN < keyword" condition, and I'm effectively replacing it with a "eCSSKeyword_UNKNOWN != keyword" condition (via the eCSSKeyword_UNKNOWN==keyword early-return inside of FindKeywordInTable).

Strictly speaking, this isn't a valid conversion -- it's changing behavior, by allowing more values into the bulk of the FindKeyword()/FindKeywordInTable() code. Specifically: if |keyword| happens to be less than eCSSKeyword_UNKNOWN, then the old logic would skip FindKeyword, whereas my new logic will enter FindKeyword and traverse the keyword array.

However, I assert that this isn't a problem, because:
 (1) there shouldn't be any nsCSSKeyword values that are less than eCSSKeyword_UNKNOWN -- it's defined to be the smallest value in the enum definition, here:
https://dxr.mozilla.org/mozilla-central/rev/6f63f95e28ffc05c0d2f5ef6cd6e05905fe8ea5a/layout/style/nsCSSKeywords.h#22

 (2) As further proof for (1), the |keyword| variable comes from nsCSSKeywords::LookupKeyword, and that function only seems to only return eCSSKeyword_UNKNOWN & (nonnegative) array-indices taken from nsStaticNameTable.cpp.  (Specifically, nsStaticCaseInsensitiveNameTable::Lookup returns "entry->mIndex" and those members are all initialized to values that are >=0, here:
https://dxr.mozilla.org/mozilla-central/rev/6f63f95e28ffc05c0d2f5ef6cd6e05905fe8ea5a/xpcom/ds/nsStaticNameTable.cpp#149

 (3) Even if the above reasons happen to be (or become) bogus, and we ended up with a smaller-than-UNKNOWN "keyword" value here, then still nothing catastrophic will happen if we allow it to be passed into FindKeyword().  We'd simply traverse the array, fail to find the bogus keyword (assuming it's not present in the keyword-array), and we'll return false from FindKeyword().
Comment on attachment 8822749 [details]
Bug 1326457: Remove redundant check for eCSSKeyword_UNKNOWN at callsites to nsCSSProps::FindKeyword().

https://reviewboard.mozilla.org/r/101552/#review102088
Attachment #8822749 - Flags: review?(xidorn+moz) → review+

Comment 5

10 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8c810ba464c1
Remove redundant check for eCSSKeyword_UNKNOWN at callsites to nsCSSProps::FindKeyword(). r=xidorn

Comment 6

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8c810ba464c1
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.