1341086 - stylo: need to support the :-moz-system-metric pseudo-class

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

Without this, rendering for all sorts of form controls is broken, leading to a ton of reftest failures.  In particular, forms.css has:

  button,
  input[type="color"]:-moz-system-metric(color-picker-available),
  input[type="reset"],
  input[type="button"],
  input[type="submit"] {

and all sorts of styles.  Right now that rule gets ignored.  So something like this:

  data:text/html,<button>aaa</button>

doesn't look like a button.

Comment 1

[Simon Sapin (:SimonSapin)]

Per IRC discussion with Boris, we could add support for this pseudo-class in nsCSSPseudoClasses::MatchesElement . After that, adding support in Stylo is a couple lines in components/style/gecko/non_ts_pseudo_class_list.rs and components/style/gecko/wrapper.rs , since we already have bindings for MatchesElement.

Comment 2

[Manish Goregaokar [:manishearth]]

https://github.com/servo/servo/pull/15971 helps a bit here.

Comment 3

[Manish Goregaokar [:manishearth]]

Part 1 is https://github.com/servo/servo/pull/15971

Part 2 just gives us parsing support for these. This, along with bug 1340696, will make most form controls work (many of them are in declarations that use this selector in the selector list, and our inability to parse this selector makes the whole declaration not work)

I don't yet know how to match these.

Comment 4

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1341086 - Part 1: stylo: Refactor out matching of string-arg pseudos so that we can call it from servo;

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

Comment 5

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

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

Comment 6

[Manish Goregaokar [:manishearth]]

Are we okay with landing this as-is, with parsing support to make the reftests start passing, and then add matching support?

Comment 7

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120766/diff/1-2/

Comment 8

[Bobby Holley (:bholley)]

(In reply to Manish Goregaokar [:manishearth] from comment #6)
> Are we okay with landing this as-is, with parsing support to make the
> reftests start passing, and then add matching support?

SGTM, just make sure we have a followup on file for the remaining work.

Comment 9

[Cameron McCormack (:heycam)]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

https://reviewboard.mozilla.org/r/120766/#review122810

Comment 10

[Manish Goregaokar [:manishearth]]

Servo at https://github.com/servo/servo/pull/15976 . No gecko side, since there are only test fixes, which I'll push later.

I'll continue using this bug for the rest of the work here. I have a plan (and may be able to knock out all or almost all of these pseudos).

Comment 11

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847850 [details]
Bug 1341086 - Part 1: stylo: Refactor out matching of string-arg pseudos so that we can call it from servo;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120764/diff/1-2/

Comment 12

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120766/diff/2-3/

Comment 13

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120766/diff/3-4/

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8847850 [details]
Bug 1341086 - Part 1: stylo: Refactor out matching of string-arg pseudos so that we can call it from servo;

https://reviewboard.mozilla.org/r/120764/#review123646

::: layout/style/nsCSSRuleProcessor.h:142
(Diff revision 2)
> +   * Checks if a function-like ident-containing pseudo (:pseudo(ident))
> +   * matches a given element.
> +   *
> +   * Returns Some(true) if it parses and matches, Some(false) if it
> +   * parses but does not match, and Nothing() if it fails to parse.
> +   */

This needs more documentation, what is `aString`? Can it be null? What does `aDependence` stand for?

Please elaborate.

::: layout/style/nsCSSRuleProcessor.cpp:1689
(Diff revision 2)
> +      {
> +        NS_ASSERTION(aString, "Must have string!");
> +        nsIContent *child = nullptr;
> +        int32_t index = -1;
> +
> +        if (aForStyling)

While you're here, add braces here. Also, we're not going to be able to use this, which sets the flag directly on the node. Is it intended?

::: layout/style/nsCSSRuleProcessor.cpp:1699
(Diff revision 2)
> +          // inserted/removed element (as in bug 534804 for :empty).
> +          aElement->SetFlags(NODE_HAS_SLOW_SELECTOR);
> +        do {
> +          child = aElement->GetChildAt(++index);
> +        } while (child &&
> +                  (!IsSignificantChild(child, true, false) ||

This wasn't thread-safe last time I checked... Matt has relevant patches at bug 1337068.

::: layout/style/nsCSSRuleProcessor.cpp:1702
(Diff revision 2)
> +          child = aElement->GetChildAt(++index);
> +        } while (child &&
> +                  (!IsSignificantChild(child, true, false) ||
> +                  (child->GetNameSpaceID() == aElement->GetNameSpaceID() &&
> +                    child->NodeInfo()->NameAtom()->Equals(nsDependentString(aString)))));
> +        if (child != nullptr) {

nit: if !child?

::: layout/style/nsCSSRuleProcessor.cpp:1745
(Diff revision 2)
> +      }
> +      break;
> +
> +    case CSSPseudoClassType::lang:
> +      {
> +        NS_ASSERTION(nullptr != aString, "null lang parameter");

`NS_ASSERTION(aString`

::: layout/style/nsCSSRuleProcessor.cpp:1755
(Diff revision 2)
> +        // We have to determine the language of the current element.  Since
> +        // this is currently no property and since the language is inherited
> +        // from the parent we have to be prepared to look at all parent
> +        // nodes.  The language itself is encoded in the LANG attribute.
> +        nsAutoString language;
> +        if (aElement->GetLang(language)) {

After inspectioning this, it _seems_ thread-safe, have you verified it really is?

::: layout/style/nsCSSRuleProcessor.cpp:1758
(Diff revision 2)
> +        // nodes.  The language itself is encoded in the LANG attribute.
> +        nsAutoString language;
> +        if (aElement->GetLang(language)) {
> +          if (!nsStyleUtil::DashMatchCompare(language,
> +                                              nsDependentString(aString),
> +                                              nsASCIICaseInsensitiveStringComparator())) {

nit: I think indentation is off (or mozreview UI is confusing). Please check.

::: layout/style/nsCSSRuleProcessor.cpp:2142
(Diff revision 2)
>          }
>        }
>        break;
>  
>      default:
> +      {

MOZ_ASSERT(nsCSSPseudoClasses::HasStringArg(pseudoClass->mType));

And deindent the rest.

::: layout/style/nsCSSRuleProcessor.cpp:2151
(Diff revision 2)
> +                                                                pseudoClass->u.mString,
> +                                                                aTreeMatchContext.mDocument,
> +                                                                aTreeMatchContext.mForStyling,
> +                                                                aNodeMatchContext.mStateMask,
> +                                                                aDependence);
> +

```
MOZ_ASSERT(result, "Message instead of comment");

if (!*result) {
    return false;
}
break;
```

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

https://reviewboard.mozilla.org/r/120766/#review123648

::: dom/base/nsDocument.h:983
(Diff revision 4)
>  
>    virtual nsISupports* GetCurrentContentSink() override;
>  
>    virtual mozilla::EventStates GetDocumentState() override;
> +  virtual mozilla::EventStates GetPossiblyStaleDocumentState() const override;
> +  virtual void UpdatePossiblyStaleDocumentState() override;

It'd be nice for these not to be virtual I guess... Is `nsDocument` the only impl of `nsIDocument`? Anyway this is not this bug's bussiness I guess.

But do use `final` instead of `override` to save two virtual calls in `GetDocumentState`.

While you're here, also mark `GetDocumentState` as `final`, because if someone overrides it, he'll need to also override these too.

::: layout/style/ServoBindings.cpp:510
(Diff revision 4)
> +{
> +  EventStates dummyMask; // mask is never read because we pass aDependence=nullptr
> +  auto matches = nsCSSRuleProcessor::StringPseudoMatches(aElement, aType, aIdent,
> +                                                         aDocument, true,
> +                                                         dummyMask, nullptr, aSetSlowSelectorFlag);
> +  MOZ_ASSERT(matches, "Servo should never try to match against a non-string-arg pseudo");

Given the two only callers assert that they feed an expected pseudo, perhaps StringPseudoMatches should be changed to return just `bool`, and assert that the bit that returns `Nothing()` is unreachable instead.

::: layout/style/nsCSSRuleProcessor.cpp:1655
(Diff revision 4)
>  {
>    switch (aPseudo) {
>      case CSSPseudoClassType::mozLocaleDir:
>        {
>          bool docIsRTL =
> -          aDocument->GetDocumentState().
> +          aDocument->GetPossiblyStaleDocumentState().

I think this makes the `StringPseudoMatches` flacky, depending on previous state that every caller needs to remember.

Instead of this, let's keep just `GetDocumentState` in the public API, and assert there:

```
MOZ_ASSERT_IF(!NS_IsMainThread(), mGotDocumentDocumentState.Contains(BOTH_BITS));
```

WDYT?

Comment 17

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

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

Comment 18

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847850 [details]
Bug 1341086 - Part 1: stylo: Refactor out matching of string-arg pseudos so that we can call it from servo;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120764/diff/2-3/

Comment 19

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120766/diff/4-5/

Comment 20

[Manish Goregaokar [:manishearth]]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121690/diff/1-2/

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8847850 [details]
Bug 1341086 - Part 1: stylo: Refactor out matching of string-arg pseudos so that we can call it from servo;

https://reviewboard.mozilla.org/r/120764/#review123742

::: layout/style/nsCSSRuleProcessor.h:159
(Diff revision 3)
> +   * @param aStateMask Mask containing states which we should exclude.
> +   *                   Ignored if aDependence is null
> +   * @param aDependence Pointer to be set to true if we ignored a state due to
> +   *                    aStateMask. Can be null.
> +   */
> +  static mozilla::Maybe<bool> StringPseudoMatches(mozilla::dom::Element* aElement,

This doesn't compile. You're still returning Maybe, but the impl just returns bool

::: layout/style/nsCSSRuleProcessor.h:167
(Diff revision 3)
> +                                                  nsIDocument* aDocument,
> +                                                  bool aForStyling,
> +                                                  mozilla::EventStates aStateMask,
> +                                                  bool* const aDependence = nullptr);
> +
> +  // static mozilla::Maybe<bool> StringPseudoMatches(mozilla::dom::Element* aElement,

Remove?

::: layout/style/nsCSSRuleProcessor.cpp:2153
(Diff revision 3)
> +                                                              aTreeMatchContext.mDocument,
> +                                                              aTreeMatchContext.mForStyling,
> +                                                              aNodeMatchContext.mStateMask,
> +                                                              aDependence);
> +
> +        // Did we find a pseudo successfully?

This comment seems misleading. What does it mean "Did we find a pseudo successfully?". Now `result` is whether it matched. Just rename the variable to `bool matched` instead.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

https://reviewboard.mozilla.org/r/120766/#review123744

::: dom/base/nsDocument.cpp:9616
(Diff revision 5)
> +
> +EventStates
> +nsDocument::GetPossiblyStaleDocumentState() const
> +{
> +  MOZ_ASSERT(mGotDocumentState.HasState(NS_DOCUMENT_STATE_WINDOW_INACTIVE |
> +                                        NS_DOCUMENT_STATE_RTL_LOCALE));

Now this is not stale, is it?

::: dom/base/nsIDocument.h:2381
(Diff revision 5)
>     * Returns the document state.
>     * Document state bits have the form NS_DOCUMENT_STATE_* and are declared in
>     * nsIDocument.h.
>     */
>    virtual mozilla::EventStates GetDocumentState() = 0;
> +  virtual mozilla::EventStates GetPossiblyStaleDocumentState() const = 0;

I'm still unsure why we need to add this two functions.

What's the problem with an implementation of nsDocument::GetDocumentState() that looked like:

```
// The Servo parallel traversal may access the document state. We properly compute it before-hand from the main thread, so let's ensure we don't do any unsafe stuff here.
MOZ_ASSERT_IF(!NS_IsMainThread(), mGotDocumentState.Contains(NS_DOCUMENT_STATE_WINDOW_INACTIVE | NS_DOCUMENT_STATE_RTL_LOCALE));

// Then leave it as-is.
```

Then in PreTraverse() we just need to:

```
mozilla::Unused << mPresContext->Document()->GetDocumentState();
```

With an appropriate comment.

::: layout/style/nsCSSRuleProcessor.cpp:1650
(Diff revision 5)
> -                                        nsIDocument* aDocument,
> +                                        const nsIDocument* aDocument,
>                                          bool aForStyling,
>                                          EventStates aStateMask,
> +                                        bool* aSetSlowSelectorFlag,
>                                          bool* const aDependence)
>  {

MOZ_ASSERT(aSetSlowSelectorFlag)?

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

https://reviewboard.mozilla.org/r/120766/#review123748

::: dom/base/nsDocument.cpp:9616
(Diff revision 5)
> +
> +EventStates
> +nsDocument::GetPossiblyStaleDocumentState() const
> +{
> +  MOZ_ASSERT(mGotDocumentState.HasState(NS_DOCUMENT_STATE_WINDOW_INACTIVE |
> +                                        NS_DOCUMENT_STATE_RTL_LOCALE));

Oh, sorry, I just recalled this conversation from yesterday, you really really wanted to make the function `const`, right?

Maybe just `GetDocumentState() const`?

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

https://reviewboard.mozilla.org/r/121690/#review123746

::: dom/base/nsGenericDOMDataNode.cpp:997
(Diff revision 2)
>  }
>  
>  bool
>  nsGenericDOMDataNode::TextIsOnlyWhitespace()
>  {
> +  if (!ThreadSafeTextIsOnlyWhitespace()) {

Same review comments as I left in the equivalent of this I did before (I don't have the bug number handy).

This imposes a new virtual call, and I'd prefer review of a DOM peer for the slightly different behavior of the flag change.

Also, I don't love to have two different methods for the sime thing, looks like a footgun, can we rename ThreadSafeTextIsOnlyWhitespace to nsGenericDOMDataNode::TextIsOnlyWhitespaceInternal, and use it from TextIsOnlyWhitespace, then check it's the main thread for setting the flags in TextIsOnlyWhitespace?

Again, we're losing the const method, but as long as it's audited, when bug 1294915 is running on CI it'd be not an issue, which is the proper thing to do instead of relying on const == threadsafe (which is false in a lot of places).

Please, feel free to disagree and ask :heycam for review, but I think if we take this approach to every non-threadsafe member function we have to deal with the amount of duplicated functions will scale quite badly.

::: layout/style/nsCSSRuleProcessor.cpp:1687
(Diff revision 2)
>        break;
>  
>      case CSSPseudoClassType::mozEmptyExceptChildrenWithLocalname:
>        {
>          NS_ASSERTION(aString, "Must have string!");
> -        nsIContent *child = nullptr;
> +        const nsIContent *child = nullptr;

While you're toching this line, please put the star with the type.

::: layout/style/nsCSSRuleProcessor.cpp:1701
(Diff revision 2)
>            *aSetSlowSelectorFlag = true;
>          }
>          do {
>            child = aElement->GetChildAt(++index);
>          } while (child &&
> -                  (!IsSignificantChild(child, true, false) ||
> +                  (!nsStyleUtil::ThreadSafeIsSignificantChild(child, true, false) ||

It's a bit unfortunate that we lose this optimization in Gecko because of Servo.

The solution I suggest wouldn't have this problem (with the cost of checking whether we're on the main thread when we don't cache it, which seems acceptable to me).

Comment 25

[Manish Goregaokar [:manishearth]]

> Maybe just `GetDocumentState() const`?

That has the same problem with it getting implicitly called, I'd rather have an explicit opt-in to the asserty behavior. This is the API bz suggested, too. Renamed to ThreadSafeGetDocumentState

Comment 26

[Manish Goregaokar [:manishearth]]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

https://reviewboard.mozilla.org/r/121690/#review123746

> Same review comments as I left in the equivalent of this I did before (I don't have the bug number handy).
> 
> This imposes a new virtual call, and I'd prefer review of a DOM peer for the slightly different behavior of the flag change.
> 
> Also, I don't love to have two different methods for the sime thing, looks like a footgun, can we rename ThreadSafeTextIsOnlyWhitespace to nsGenericDOMDataNode::TextIsOnlyWhitespaceInternal, and use it from TextIsOnlyWhitespace, then check it's the main thread for setting the flags in TextIsOnlyWhitespace?
> 
> Again, we're losing the const method, but as long as it's audited, when bug 1294915 is running on CI it'd be not an issue, which is the proper thing to do instead of relying on const == threadsafe (which is false in a lot of places).
> 
> Please, feel free to disagree and ask :heycam for review, but I think if we take this approach to every non-threadsafe member function we have to deal with the amount of duplicated functions will scale quite badly.

I marked it final override as mentioned in the bug so it shouldn't be virtual anymore. I think I addressed all those comments.

I prefer having two functions due to the explicit opt-in.

It's not about relying on const==threadsafe (that's why "ThreadSafe" is in the title), it's about relying that in servo parallel traversal you will always have a const (this is true), so the regular function won't work for you, and you explicitly opt in to the different behavior.

I don't think it will scale badly; there's a limited number of functions being called from parallel traversal in the first place.

However, due to your other comment in this case Gecko loses out on an optimization so I'll use your method.

Comment 27

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847850 [details]
Bug 1341086 - Part 1: stylo: Refactor out matching of string-arg pseudos so that we can call it from servo;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120764/diff/3-4/

Comment 28

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120766/diff/5-6/

Comment 29

[Manish Goregaokar [:manishearth]]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121690/diff/2-3/

Comment 30

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120766/diff/6-7/

Comment 31

[Manish Goregaokar [:manishearth]]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121690/diff/3-4/

Comment 32

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120766/diff/7-8/

Comment 33

[Manish Goregaokar [:manishearth]]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121690/diff/4-5/

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

https://reviewboard.mozilla.org/r/120766/#review123816

r=me with those three comments addressed.

::: dom/base/nsDocument.h:987
(Diff revision 8)
> +  // GetDocumentState() mutates the state due to lazy resolution;
> +  // and can't be used during parallel traversal. Use this instead,
> +  // and ensure UpdatePossiblyStaleDocumentState() has been called first.
> +  // This will assert if the state is stale.
> +  virtual mozilla::EventStates ThreadSafeGetDocumentState() const final override;
> +  virtual void UpdatePossiblyStaleDocumentState() final override;

Can we remove this function (making it internal to nsDocument), and just call GetDocumentState from PreTraverse?

Also, please only `final`, not `final override`.

::: layout/style/ServoBindings.h:382
(Diff revision 8)
>                                          RawGeckoPresContextBorrowed pres_context);
>  
> +bool Gecko_MatchStringArgPseudo(RawGeckoElementBorrowed element,
> +                                mozilla::CSSPseudoClassType type,
> +                                const char16_t* ident,
> +                                RawGeckoDocumentBorrowed document,

We can get the document from Gecko, right? This is only calling `OwnerDoc()`.

Let's do that, and assert that `aElement->OwnerDoc() == aElement->GetComposedDoc()`

::: servo/components/style/gecko/wrapper.rs:679
(Diff revision 8)
> +                    let matches = Gecko_MatchStringArgPseudo(self.0,
> +                                       pseudo_class.to_gecko_pseudoclasstype().unwrap(),
> +                                       s.as_ptr(),
> +                                       self.as_node().owner_doc(), &mut set_slow_selector);
> +                    if set_slow_selector {
> +                        self.set_selector_flags(HAS_SLOW_SELECTOR);

Please don't do this here, just pass `ElementFlags` to this function and insert there.

Comment 35

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8847850 [details]
Bug 1341086 - Part 1: stylo: Refactor out matching of string-arg pseudos so that we can call it from servo;

https://reviewboard.mozilla.org/r/120764/#review123818

::: layout/style/nsCSSRuleProcessor.cpp:1641
(Diff revision 4)
>    }
>    return true;
>  }
>  
> +/* static */
> +bool

I'm not sure if the static comment should go in the same line as the return value or not, please check and adjust if necessary.

::: layout/style/nsCSSRuleProcessor.cpp:1670
(Diff revision 4)
> +            return false;
> +          }
> +        } else {
> +          // Selectors specifying other directions never match.
> +          return false;
> +        }

Seems like we could cleanup a bunch of these to return true by default, then false. But that's not this bug's business.

::: layout/style/nsCSSRuleProcessor.cpp:1798
(Diff revision 4)
> +          }
> +        }
> +      }
> +      return false;
> +
> +    default: MOZ_ASSERT_UNREACHABLE("Called StringPseudoMatches() with unknown stringlike pseudo");

nit: string-like.

Comment 36

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

https://reviewboard.mozilla.org/r/121690/#review123822

::: dom/base/nsGenericDOMDataNode.cpp:998
(Diff revision 5)
>  
>  bool
> -nsGenericDOMDataNode::TextIsOnlyWhitespace()
> +nsGenericDOMDataNode::TextIsOnlyWhitespace() const
> +{
> +  bool result = TextIsOnlyWhitespaceInternal();
> +  if (NS_IsMainThread()) {

Let's check the cached flag here before checking `NS_IsMainThread`:

```
if (HasFlag(NS_CACHED_TEXT_IS_ONLY_WHITESPACE) || !NS_IsMainThread()) {
  return result;
}
// ...
```

Actually, I believe it may be cleaner to just handle the flag here instead of in the `Internal` method, wdyt?

```
if (HasFlag(NS_CACHED_TEXT_IS_ONLY_WHITESPACE)) {
  return HasFlag(NS_TEXT_IS_ONLY_WHITESPACE);
}

// call TextIsOnlyWhitespaceInternal and cache if it's the main thread.
```

Then assert in the internal method that we don't have the CACHED flag.

Also, please ask smaug or bz to confirm that it's not problematic having multibyte text with the cached flag.

r=me with that.

Comment 37

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

https://reviewboard.mozilla.org/r/120766/#review123816

> Please don't do this here, just pass `ElementFlags` to this function and insert there.

bz said that callers of `set_selector_flags` are going to be changed to be more thread safe, so I continued using that so that it's easier to catch when that refactoring happens.

Comment 38

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847850 [details]
Bug 1341086 - Part 1: stylo: Refactor out matching of string-arg pseudos so that we can call it from servo;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120764/diff/4-5/

Comment 39

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120766/diff/8-9/

Comment 40

[Manish Goregaokar [:manishearth]]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121690/diff/5-6/

Comment 41

[Manish Goregaokar [:manishearth]]

Boris, r? on the last patch? Specifically, 

> Also, please ask smaug or bz to confirm that it's not problematic having multibyte text with the cached flag.

Comment 42

[Emilio Cobos Álvarez (:emilio)]

(In reply to Manish Goregaokar [:manishearth] from comment #41)
> Boris, r? on the last patch? Specifically, 
> 
> > Also, please ask smaug or bz to confirm that it's not problematic having multibyte text with the cached flag.

To recap and save Boris' time, since that patch moved from another bug which also had review comments:

In particular, there's a subtle behavior change, which is that we early return false before checking or setting the flag if the text is not 8bit.

I believe the change should be innocuous, and the new behavior is also correct, per the other callsites that remove the CACHED flag, but I wanted to confirm with someone that knew this code more than I, since it's so subtle.

Comment 43

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

https://reviewboard.mozilla.org/r/120766/#review124132

Manish asked for another review round, so there we go.

::: dom/base/nsDocument.h:984
(Diff revision 9)
>    virtual nsISupports* GetCurrentContentSink() override;
>  
> -  virtual mozilla::EventStates GetDocumentState() override;
> +  virtual mozilla::EventStates GetDocumentState() final;
> +  // GetDocumentState() mutates the state due to lazy resolution;
> +  // and can't be used during parallel traversal. Use this instead,
> +  // and ensure UpdatePossiblyStaleDocumentState() has been called first.

nit: UpdatePossiblyStaleDocumentState is not public. Please say something like "and ensure the document state is up to date", or something like that.

::: dom/base/nsDocument.cpp:9616
(Diff revision 9)
> +
> +EventStates
> +nsDocument::ThreadSafeGetDocumentState() const
> +{
> +  MOZ_ASSERT(mGotDocumentState.HasState(NS_DOCUMENT_STATE_WINDOW_INACTIVE |
> +                                        NS_DOCUMENT_STATE_RTL_LOCALE));

Perhaps an `NS_DOCUMENT_STATE_ALL_BITS` or something would be a good idea...

::: servo/components/selectors/matching.rs:69
(Diff revision 9)
>  
>  bitflags! {
>      /// Set of flags that are set on either the element or its parent (depending
>      /// on the flag) if the element could potentially match a selector.
>      pub flags ElementSelectorFlags: u8 {
> +        const NO_SELECTOR_FLAGS = 0,

Remove this, which also won't compile because it has no docs. Use ElementSelectorFlags::empty instead?

::: servo/components/selectors/tree.rs:142
(Diff revision 9)
>  
>      fn is_html_element_in_html_document(&self) -> bool;
>      fn get_local_name(&self) -> &<Self::Impl as SelectorImpl>::BorrowedLocalName;
>      fn get_namespace(&self) -> &<Self::Impl as SelectorImpl>::BorrowedNamespaceUrl;
>  
> -    fn match_non_ts_pseudo_class(&self, pc: &<Self::Impl as SelectorImpl>::NonTSPseudoClass) -> bool;
> +    fn match_non_ts_pseudo_class(&self, pc: &<Self::Impl as SelectorImpl>::NonTSPseudoClass, flags: &mut ElementSelectorFlags) -> bool;

nit: put the new argument and the return value on its own line? I find it relatively hard to read as-is.

::: servo/components/style/gecko/selector_parser.rs:168
(Diff revision 9)
>              (bare: [$(($css:expr, $name:ident, $gecko_type:tt, $state:tt, $flags:tt),)*],
>               string: [$(($s_css:expr, $s_name:ident, $s_gecko_type:tt, $s_state:tt, $s_flags:tt),)*]) => {
>                  match *self {
>                      $(NonTSPseudoClass::$name => concat!(":", $css),)*
>                      $(NonTSPseudoClass::$s_name(ref s) => {
> -                        return dest.write_str(&format!(":{}({})", $s_css, s))
> +                        return dest.write_str(&format!(":{}({})", $s_css, String::from_utf16(&s).unwrap()))

We need to escape the string, right? Let's use CSSStringWriter here.

Also, while we're here, let's use write!(dest, ...) instead of allocating with format!

::: servo/components/style/gecko/wrapper.rs:820
(Diff revision 9)
>  }
>  
>  impl<'le> ElementExt for GeckoElement<'le> {
>      #[inline]
>      fn is_link(&self) -> bool {
> -        self.match_non_ts_pseudo_class(&NonTSPseudoClass::AnyLink)
> +        let mut dummy_flags = Default::default();

nit: remove this flag, and use &mut ElementSelectorFlags::empty() instead.

::: servo/components/style/servo/selector_parser.rs:453
(Diff revision 9)
>      }
>  }
>  
>  impl<E: Element<Impl=SelectorImpl> + Debug> ElementExt for E {
>      fn is_link(&self) -> bool {
> -        self.match_non_ts_pseudo_class(&NonTSPseudoClass::AnyLink)
> +        let dummy_flags = Default::default();

ditto.

Comment 44

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

https://reviewboard.mozilla.org/r/121690/#review124138

::: commit-message-5561d:1
(Diff revision 6)
> +Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild(); r?emilio

nit: Reword the commit so it shows the final approach. Perhaps: "Make nsStyleUtil::IsSignificantChild thread-safe"?

::: dom/base/nsGenericDOMDataNode.h:139
(Diff revision 6)
>      return SetText(aStr.BeginReading(), aStr.Length(), aNotify);
>    }
>    virtual nsresult AppendText(const char16_t* aBuffer, uint32_t aLength,
>                                bool aNotify) override;
> -  virtual bool TextIsOnlyWhitespace() override;
> +  virtual bool TextIsOnlyWhitespace() const override;
> +  bool TextIsOnlyWhitespaceInternal() const;

Make it private, please.

::: dom/base/nsGenericDOMDataNode.cpp:1001
(Diff revision 6)
> +{
> +  if (HasFlag(NS_CACHED_TEXT_IS_ONLY_WHITESPACE)) {
> +    return HasFlag(NS_TEXT_IS_ONLY_WHITESPACE);
> +  }
> +  bool result = TextIsOnlyWhitespaceInternal();
> +  if (NS_IsMainThread()) {

nit: Consider early-returning in the non-main-thread case, and deindenting the rest.

::: dom/base/nsGenericDOMDataNode.cpp:1018
(Diff revision 6)
> +  }
> +}
> +
> +bool
> +nsGenericDOMDataNode::TextIsOnlyWhitespaceInternal() const
>  {

Assert against the cached flag here, please.

::: layout/style/nsCSSRuleProcessor.cpp:1701
(Diff revision 6)
>            *aSetSlowSelectorFlag = true;
>          }
>          do {
>            child = aElement->GetChildAt(++index);
>          } while (child &&
> -                  (!IsSignificantChild(child, true, false) ||
> +                  (!nsStyleUtil::IsSignificantChild(child, true, false) ||

nit: This change is not needed any more, right? You can update the IsSignificantChild function to take a const pointer instead.

Comment 45

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

https://reviewboard.mozilla.org/r/120766/#review124132

> nit: remove this flag, and use &mut ElementSelectorFlags::empty() instead.

Err, this variable, I mean.

Comment 46

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

https://reviewboard.mozilla.org/r/121690/#review124162

::: dom/base/nsGenericDOMDataNode.cpp:1002
(Diff revision 6)
> +  if (HasFlag(NS_CACHED_TEXT_IS_ONLY_WHITESPACE)) {
> +    return HasFlag(NS_TEXT_IS_ONLY_WHITESPACE);
> +  }
> +  bool result = TextIsOnlyWhitespaceInternal();
> +  if (NS_IsMainThread()) {
> +    nsGenericDOMDataNode* self = const_cast<nsGenericDOMDataNode*>(this);

The change for the Is2b() case is OK, but I'm very much not a fan of this const_cast.  const should mean const; otherwise you get weird bugs in consumers.

I think we should just have two separate methods.  One checks the flag and then forwards to TextIsOnlyWhitespaceInternal and is const, the other does that and then caches.  The servo traversal should call the one that is const.  We should give it some name that clearly discourages calling it from Gecko...

Yes, the duplication is annoying, but I much prefer it to having this hidden const_cast violating invariants people think they can rely on.

Comment 47

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #46)
> Comment on attachment 8848801 [details]
> Bug 1341086 - Part 3: stylo: Add thread-safe version of
> nsStyleUtil::IsSignificantChild();
> 
> https://reviewboard.mozilla.org/r/121690/#review124162
> 
> ::: dom/base/nsGenericDOMDataNode.cpp:1002
> (Diff revision 6)
> > +  if (HasFlag(NS_CACHED_TEXT_IS_ONLY_WHITESPACE)) {
> > +    return HasFlag(NS_TEXT_IS_ONLY_WHITESPACE);
> > +  }
> > +  bool result = TextIsOnlyWhitespaceInternal();
> > +  if (NS_IsMainThread()) {
> > +    nsGenericDOMDataNode* self = const_cast<nsGenericDOMDataNode*>(this);
> 
> The change for the Is2b() case is OK, but I'm very much not a fan of this
> const_cast.  const should mean const; otherwise you get weird bugs in
> consumers.
> 
> I think we should just have two separate methods.  One checks the flag and
> then forwards to TextIsOnlyWhitespaceInternal and is const, the other does
> that and then caches.  The servo traversal should call the one that is
> const.  We should give it some name that clearly discourages calling it from
> Gecko...
> 
> Yes, the duplication is annoying, but I much prefer it to having this hidden
> const_cast violating invariants people think they can rely on.

TBH, this function is still logically const, but I guess it's fine. I believe this const_cast is comparatively a smaller potential footgun than two different implementations of the same function getting out of sync.

That being said, point taken, and happy to concede.

Comment 48

[Boris Zbarsky [:bzbarsky]]

> this function is still logically const

The problems with "logically const" start if you start assuming that "const" means "yeah, I can race on it from multiple threads".  C++ const doesn't actually guarantee that because of const_cast and mutable, but if you do it right it _should_, and I would really rather people didn't need to worry about whether it does...

Comment 49

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #48)
> > this function is still logically const
> 
> The problems with "logically const" start if you start assuming that "const"
> means "yeah, I can race on it from multiple threads".  C++ const doesn't
> actually guarantee that because of const_cast and mutable, but if you do it
> right it _should_, and I would really rather people didn't need to worry
> about whether it does...

Sure. Though function actually guards the const_cast with an NS_IsMainThread check, right?

But yeah, I get your point. That's a flaw Rust's type system doesn't have :(.

Comment 50

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120766/diff/9-10/

Comment 51

[Manish Goregaokar [:manishearth]]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121690/diff/6-7/

Comment 52

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

https://reviewboard.mozilla.org/r/120766/#review124190

::: dom/base/nsIDocument.h:202
(Diff revisions 9 - 10)
>  // RTL locale: specific to the XUL localedir attribute
>  #define NS_DOCUMENT_STATE_RTL_LOCALE              NS_DEFINE_EVENT_STATE_MACRO(0)
>  // Window activation status
>  #define NS_DOCUMENT_STATE_WINDOW_INACTIVE         NS_DEFINE_EVENT_STATE_MACRO(1)
>  
> +#define NS_DOCUMENT_STATE_ALL_BITS NS_DOCUMENT_STATE_RTL_LOCALE | NS_DOCUMENT_STATE_WINDOW_INACTIVE

nit: Wrap in parenthesis, and align with the rest.

Comment 53

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

https://reviewboard.mozilla.org/r/121690/#review124192

This approach makes gecko lose the IsWhitespace optimization if it wasn't cached. r=me, but let's run it through bz too to ensure he's fine with that bit, which is what I was trying to avoid with previous reviews (with the cost of a const_cast).

::: dom/base/FragmentOrElement.h:145
(Diff revision 7)
>      return SetText(aStr.BeginReading(), aStr.Length(), aNotify);
>    }
>    virtual nsresult AppendText(const char16_t* aBuffer, uint32_t aLength,
>                                bool aNotify) override;
>    virtual bool TextIsOnlyWhitespace() override;
> +  virtual bool ThreadSafeTextIsOnlyWhitespace() const override;

const final, please.

::: dom/base/nsGenericDOMDataNode.cpp:1003
(Diff revision 7)
> +    UnsetFlags(NS_TEXT_IS_ONLY_WHITESPACE);
> +    SetFlags(NS_CACHED_TEXT_IS_ONLY_WHITESPACE);
> +    return false;
> +  }
> +
> +  MOZ_ASSERT(NS_IsMainThread());

You want this assert unconditionally. Move it to the top.

Comment 54

[Manish Goregaokar [:manishearth]]

Attachment: Bug 1341086 - Part 4: stylo: Update test expectations;

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

Comment 55

[Manish Goregaokar [:manishearth]]

Comment on attachment 8849346 [details]
Bug 1341086 - Part 4: stylo: Update test expectations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122134/diff/1-2/

Comment 56

[Manish Goregaokar [:manishearth]]

Boris, are we okay with losing the IsWhitespace optimization in the StringArgMatches case in gecko?

Comment 57

[Boris Zbarsky [:bzbarsky]]

> Boris, are we okay with losing the IsWhitespace optimization in the StringArgMatches case in gecko?

It's not great.  It means we have to keep walking strings and checking whether they're whitespace-only or not, every restyle.  It's obviously trivial to create pathological testcases where this is horrible, and that's something we try pretty hard to avoid.

The only reason we need this is because we're sharing this function with servo, right?  If we wanted to, we _could_ have an extra arg for the function that indicates whether it's called from Gecko or servo and use the non-const version for Gecko.  Of course then stylo would have the pathological case problem.

Can we do something where we remember the "add this flag to this node" information and then do it post-traversal, like we already aim to do with the slow selector flags?

Comment 58

[Manish Goregaokar [:manishearth]]

Yeah, that makes sense, however I'm not well synced up with the post traversal thing. I'll add a bool arg so that gecko doesn't lose out, and file a followup depending on the slow selector work.

Comment 59

[Boris Zbarsky [:bzbarsky]]

That sounds perfect, thank you!

Comment 60

[Manish Goregaokar [:manishearth]]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121690/diff/7-8/

Comment 61

[Manish Goregaokar [:manishearth]]

Comment on attachment 8849346 [details]
Bug 1341086 - Part 4: stylo: Update test expectations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122134/diff/2-3/

Comment 62

[Manish Goregaokar [:manishearth]]

Comment on attachment 8849346 [details]
Bug 1341086 - Part 4: stylo: Update test expectations;

https://reviewboard.mozilla.org/r/122134/#review124266

Comment 63

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120766/diff/10-11/

Comment 64

[Manish Goregaokar [:manishearth]]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121690/diff/8-9/

Comment 65

[Manish Goregaokar [:manishearth]]

Comment on attachment 8849346 [details]
Bug 1341086 - Part 4: stylo: Update test expectations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122134/diff/3-4/

Comment 66

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120766/diff/11-12/

Comment 67

[Manish Goregaokar [:manishearth]]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121690/diff/9-10/

Comment 68

[Manish Goregaokar [:manishearth]]

Comment on attachment 8849346 [details]
Bug 1341086 - Part 4: stylo: Update test expectations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122134/diff/4-5/

Comment 69

[Manish Goregaokar [:manishearth]]

Servo at https://github.com/servo/servo/pull/16066

Almost green try at https://treeherder.mozilla.org/#/jobs?repo=try&revision=c84d551a860f0b621b4dedc5c400e439746675fd&selectedJob=85410472

Comment 70

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847850 [details]
Bug 1341086 - Part 1: stylo: Refactor out matching of string-arg pseudos so that we can call it from servo;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120764/diff/5-6/

Comment 71

[Manish Goregaokar [:manishearth]]

Comment on attachment 8847851 [details]
Bug 1341086 - Part 2: stylo: Support all non-ts pseudos with an argument;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120766/diff/12-13/

Comment 72

[Manish Goregaokar [:manishearth]]

Comment on attachment 8848801 [details]
Bug 1341086 - Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild();

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/121690/diff/10-11/

Comment 73

[Manish Goregaokar [:manishearth]]

Comment on attachment 8849346 [details]
Bug 1341086 - Part 4: stylo: Update test expectations;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/122134/diff/5-6/

Comment 74

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1f047e086789
Part 1: stylo: Refactor out matching of string-arg pseudos so that we can call it from servo; r=emilio
https://hg.mozilla.org/integration/autoland/rev/3c2c57ffa0ab
Part 2: stylo: Support all non-ts pseudos with an argument; r=emilio,heycam
https://hg.mozilla.org/integration/autoland/rev/48950b1b1828
Part 3: stylo: Add thread-safe version of nsStyleUtil::IsSignificantChild(); r=emilio
https://hg.mozilla.org/integration/autoland/rev/e0be781966d4
Part 4: stylo: Update test expectations; r=manishearth

Comment 75

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b8d4e0ec2f08
followup - Update test expectations.

Comment 76

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1f047e086789
https://hg.mozilla.org/mozilla-central/rev/3c2c57ffa0ab
https://hg.mozilla.org/mozilla-central/rev/48950b1b1828
https://hg.mozilla.org/mozilla-central/rev/e0be781966d4
https://hg.mozilla.org/mozilla-central/rev/b8d4e0ec2f08