stylo: Attr selector matching should perhaps fast-path the empty-string namespace

RESOLVED FIXED in Firefox 56

Status

()

RESOLVED FIXED
a year ago
a month ago

People

(Reporter: bzbarsky, Assigned: emilio)

Tracking

(Blocks: 2 bugs)

53 Branch
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

In ServoBindings.cpp, DoMatch does:

  if (aNS) {
    int32_t ns = nsContentUtils::NameSpaceManager()->GetNameSpaceID(aNS,
                                                                    aElement->IsInChromeDocument());

This is the common path; hardly anyone writes attr selectors with a wildcard namespace.

In fact, hardly anyone writes attr selectors with a namespace other than "" (the default).  So in the common case we do the IsInChromeDocument() thing, and the out of line GetNameSpaceID call, which just compares aNS to nsGkAtoms::_empty and returns kNameSpaceId_None without even using the second argument.

We should consider inlining the "is this the null namespace"? check here so we can avoid the extra work of IsInChromeDocument and whatnot.
(Reporter)

Updated

a year ago
Blocks: 1289864
(Assignee)

Comment 1

a year ago
(In reply to Boris Zbarsky [:bz] from comment #0)
> In fact, hardly anyone writes attr selectors with a namespace other than ""
> (the default).  So in the common case we do the IsInChromeDocument() thing,
> and the out of line GetNameSpaceID call, which just compares aNS to
> nsGkAtoms::_empty and returns kNameSpaceId_None without even using the
> second argument.

Note that we do have stylesheets with @namespace rules that will make some namespaces appear there though...

But I think this is reasonable.
Comment hidden (mozreview-request)
> Note that we do have stylesheets with @namespace rules that will make some namespaces appear there though...

No, we don't.  In particular, the default namespace is not applied to attr selectors.
(Assignee)

Comment 4

a year ago
(In reply to Boris Zbarsky [:bz] from comment #3)
> > Note that we do have stylesheets with @namespace rules that will make some namespaces appear there though...
> 
> No, we don't.  In particular, the default namespace is not applied to attr
> selectors.

Oh, right! Then this should be even better. Feel free to stamp it if you want btw.
(Reporter)

Comment 5

a year ago
mozreview-review
Comment on attachment 8888242 [details]
Bug 1382372: Add a fast-path for matching attr selectors with the empty namespace.

https://reviewboard.mozilla.org/r/159192/#review164638

r=me
Attachment #8888242 - Flags: review+

Comment 6

a year ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6b2a40d10b49
Add a fast-path for matching attr selectors with the empty namespace. r=bz

Comment 7

a year ago
mozreview-review
Comment on attachment 8888242 [details]
Bug 1382372: Add a fast-path for matching attr selectors with the empty namespace.

https://reviewboard.mozilla.org/r/159192/#review165070

(Already landed, but looks fine.)
Attachment #8888242 - Flags: review?(cam) → review+
https://hg.mozilla.org/mozilla-central/rev/6b2a40d10b49
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Assignee: nobody → emilio
You need to log in before you can comment on or make changes to this bug.