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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Boris Zbarsky [:bzbarsky]]

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.

Comment 1

[Emilio Cobos Álvarez (:emilio)]

(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 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1382372: Add a fast-path for matching attr selectors with the empty namespace.

Review commit: https://reviewboard.mozilla.org/r/159192/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/159192/

Comment 3

[Boris Zbarsky [:bzbarsky]]

> 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.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b2a40d10b49
Add a fast-path for matching attr selectors with the empty namespace. r=bz

Comment 7

[Cameron McCormack (:heycam)]

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.)

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/6b2a40d10b49