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

RESOLVED FIXED in Firefox 55

Status

()

Core
CSS Parsing and Computation
P1
normal
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: bz, Assigned: manishearth)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(4 attachments)

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

Updated

6 months ago
Blocks: 1324348
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.
Blocks: 1345200
Priority: -- → P1
Assignee: nobody → mbrubeck
(Assignee)

Comment 2

5 months ago
https://github.com/servo/servo/pull/15971 helps a bit here.
(Assignee)

Comment 3

5 months ago
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.
Assignee: mbrubeck → manishearth
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8847850 - Flags: review?(nox)
(Assignee)

Comment 6

5 months ago
Are we okay with landing this as-is, with parsing support to make the reftests start passing, and then add matching support?
Comment hidden (mozreview-request)
(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

5 months ago
mozreview-review
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
Attachment #8847851 - Flags: review?(cam) → review+
(Assignee)

Comment 10

5 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8847850 - Flags: review?(bobbyholley) → review?(emilio+bugs)
Attachment #8847851 - Flags: review?(bobbyholley) → review?(emilio+bugs)
(Assignee)

Updated

5 months ago
Duplicate of this bug: 1348479

Comment 15

5 months ago
mozreview-review
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;
```
Attachment #8847850 - Flags: review?(emilio+bugs)

Comment 16

5 months ago
mozreview-review
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?
Attachment #8847851 - Flags: review?(emilio+bugs)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

5 months ago
mozreview-review
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.
Attachment #8847850 - Flags: review?(emilio+bugs)

Comment 22

5 months ago
mozreview-review
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)?
Attachment #8847851 - Flags: review?(emilio+bugs)

Comment 23

5 months ago
mozreview-review
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

5 months ago
mozreview-review
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).
Attachment #8848801 - Flags: review?(emilio+bugs)
(Assignee)

Comment 25

5 months ago
> 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
(Assignee)

Comment 26

5 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 34

5 months ago
mozreview-review
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.
Attachment #8847851 - Flags: review?(emilio+bugs) → review+

Comment 35

5 months ago
mozreview-review
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.
Attachment #8847850 - Flags: review?(emilio+bugs) → review+

Comment 36

5 months ago
mozreview-review
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.
Attachment #8848801 - Flags: review?(emilio+bugs) → review+
Blocks: 1337068
(Assignee)

Comment 37

5 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 41

5 months ago
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.
Flags: needinfo?(bzbarsky)
(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

5 months ago
mozreview-review
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

5 months ago
mozreview-review
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

5 months ago
mozreview-review-reply
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.
(Reporter)

Comment 46

5 months ago
mozreview-review
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.
(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.
> 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...
Flags: needinfo?(bzbarsky)
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 52

5 months ago
mozreview-review
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

5 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 56

5 months ago
Boris, are we okay with losing the IsWhitespace optimization in the StringArgMatches case in gecko?
Flags: needinfo?(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?
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 58

5 months ago
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.
That sounds perfect, thank you!
(Assignee)

Updated

5 months ago
Blocks: 1349100
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 62

5 months ago
mozreview-review
Comment on attachment 8849346 [details]
Bug 1341086 - Part 4: stylo: Update test expectations;

https://reviewboard.mozilla.org/r/122134/#review124266
Attachment #8849346 - Flags: review?(manishearth) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 69

5 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 74

5 months ago
Pushed by manishearth@gmail.com:
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

5 months ago
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b8d4e0ec2f08
followup - Update test expectations.

Comment 76

5 months ago
bugherder
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
Status: NEW → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox54: affected → ---
You need to log in before you can comment on or make changes to this bug.