851892 - Move CSS Rules to WebIDL

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, defect)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think post-bug 1221436 this becomes easier, and we don't need to do bug 898764.

In particular, I think instead of bug 898764, we can now use css::Rule as the class that the WebIDL uses for CSS rules rather than making a new one.  It already has virtual functions, so there's no change in size.

Then what bug 1221436 allows is that we can merge css::DOMCSSStyleRule into css::StyleRule.  It currently has a vtable pointer and a nested DOMCSSDeclarationImpl object.  We can separate the DOMCSSDeclarationImpl object from it (but perhaps still make it owned, and with reference counts aggregated).  This keeps the size of DOMCSSDeclarationImpl the same (it already has an mRule pointing to the css::StyleRule) and increases the size of css::StyleRule only by one word (the vtable pointer for nsICSSStyleRuleDOMWrapper (which is mostly nsIDOMCSSStyleRule), which I think new bindings will allow us to eliminate in a later step; there's no increase for the pointer to the lazily-allocated DOMCSSDeclarationImpl since we reuse the space that's currently occupied by the lazily-allocated mDOMRule (and probably put it inside the DOMCSSStyleRule subobject as long as it's a subobject... which might not need to be the case once we remove support for the old bindings).

Do you agree that new bindings will let us get rid of the nsIDOMCSSStyleRule/nsICSSStyleRuleDOMWrapper vtable pointer?  (I'm hoping it will let us shrink DOMCSSDeclarationImpl even more... but that's far less important since they'd still be lazily created on DOM access.)

And does this plan otherwise make sense?

Comment 2

[Boris Zbarsky [:bzbarsky]]

We could probably use css::Rule now, yes.  Unfortunately, that _will_ involve changing its size, since we'll need to make it inherit from nsWrapperCache, which is two words (or at least a word followed by a uint32_t).

> Do you agree that new bindings will let us get rid of the nsIDOMCSSStyleRule/nsICSSStyleRuleDOMWrapper vtable pointer?

Yes.  nsICSSStyleRuleDOMWrapper will become completely unnecessary.  nsIDOMCSSStyleRule will take a bit more work, because we have browser and addon code doing QI to that interface (for totally unnecessary reasons, but such is the power of cargo-culting).  I assume that we will want to handle it as follows:

1)  Make css::Rule inherit from nsIDOMCSSRule and adjust its QI impl accordingly, so QI of
    any rule to nsIDOMCSSRule works.
2)  Make nsIDOMCSS*Rule _not_ inherit from nsIDOMCSSRule, so we don't end up with multiple
    inheritance from nsIDOMCSSRule.  This might need to happen in the same changeset as #1.
3)  Make nsIDOMCSSStyleRule an empty interface, change any consumers to use StyleRule
    instead as needed.
4)  Have css::StyleRule QI to it without inheriting from it (returning itself as
    nsISupports* instead); given that it's empty this is not unreasonable because the
    vtable is just that of nsISupports.

If we're not ok with #3 because of the creepy-crawley feeling it induces (and it does, I know), we could instead do:

4)  Change mozilla::dom::QueryInterface to special-case nsIDOMCSSStyleRule and when its
    IID is passed just QI the object to mozilla::css::StyleRule instead.  Have nothing to
    do with nsIDOMCSSStyleRule in StyleRule at all.

Comment 3

[Boris Zbarsky [:bzbarsky]]

OK, before I start posting patches, a summary of the new setup:

* Steps 1 and 2 from comment 2 are done.  nsIDOMCSSRule still has a
  GetCSSRule() method to get the css::StyleRule; this just returns "this".
* Step 3 from comment 2 is complicated by ServoStyleRule.  I will see
  about doing something like it in a followup.
* I am introducing a BindingStyleRule class that's a common base class of
  ServoStyleRule and StyleRule.  We may want to give it a better name
  (mozilla::StyleRule seemed like a bad idea!) and make it QI-able or
  something; at that point it could be the thing we do for step 3.
* For now I'm leaving all the nsIDOMCSS* bits around, but we may want to
  investigate removing them.  Need to figure out how that should work with
  Stylo.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Oh, and I looked at addons dxr, and looks like I need to keep scripted QueryInterface on CSSStyleRule and CSSMozDocumentStyleRule...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Er, I meant CSSMozDocumentRule.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make all CSS rules cycle-collected

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Remove the now-unused GetExistingDOMRule method

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Give CSS rules a PreCreate hook so we can safely wrappercache them

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Make css::Rule wrappercached

Note that this increases the size of css::Rule by three words, unfortunately.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Get rid of DOMCSSStyleRule

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Make css::Rule inherit from nsIDOMCSSRule

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Push the nsIDOMCSSRule implementation up to css::Rule

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Get rid of css::Rule::GetDOMRule

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Add a CSSRule Web IDL interface

Peter, could you review he UnwrapArg bits and the dom/bindings and dom/webidl changes, for this changeset and the following ones?

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Convert CSSNamespaceRule to WebIDL

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Convert CSSImportRule to WebIDL

Andrew, can you please review the weakmap test change?

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Convert CSSStyleRule to WebIDL

Andrew, can you please review the other weakmap test change?

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: part 13. Convert media, supports, and moz-document rules to WebIDL

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: part 14. Convert CSSPageRule to WebIDL

The PutForwards bit is a new feature, but easy enough to implement here.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: part 15. Convert CSSFontFaceRule to WebIDL

Comment 21

[Boris Zbarsky [:bzbarsky]]

Attachment: part 16. Convert CSSFontFeatureValuesRule to WebIDL

Comment 22

[Boris Zbarsky [:bzbarsky]]

Attachment: part 17. Convert CSSKeyframeRule to WebIDL

Comment 23

[Boris Zbarsky [:bzbarsky]]

Attachment: part 18. Convert CSSKeyframesRule to WebIDL

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: part 19. Convert CSSCounterStyleRule to WebIDL

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: part 20. Remove the remaining bits we added to support a mix of WebIDL and non-WebIDL rules

Comment 26

[Jean-Yves Perrier [:teoli]]

Hi and happy new year!

Happy to see this happening :-)

A quick question: does this migration to webidl change the behavior for devs in anyway, or, in another way, does this need any developer documentation?

Comment 27

[Boris Zbarsky [:bzbarsky]]

There are all sorts of behavior changes, yes.  Properties will now appear on the correct prototypes (e.g. the CSSRule ones will only be on CSSRule.prototype and not shadowed on most-derived prototypes as they are now; CSSGroupingRule.prototype will actually have properties hanging off it, etc).  The various CSS*Rule interface objects will now look like Function objects.  The toString of the various CSS*Rule.prototype objects will be different.  We're adding wrapper caching, so we will no longer do weird stuff like lose expandos on CSS rules when GC happens.  We'll be able to use CSS rules as weakmap keys (which we can't do right now).  Plus the bits described in https://groups.google.com/d/msg/mozilla.dev.platform/baiZcqFrF50/eFAX84n6EwAJ of course.  And there may be some other pieces I can't think of off the top of my head.

In terms of documenting this, I'm not really sure what the right approach is.  Mostly this makes CSS rules act like all other DOM objects.

Comment 28

[Andrew McCreight [:mccr8]]

Comment on attachment 8822845 [details] [diff] [review]
part 11.  Convert CSSImportRule to WebIDL

Review of attachment 8822845 [details] [diff] [review]:
-----------------------------------------------------------------

r=me for the test change. Thanks for maintaining these weak map tests over the years as various things were swapped to WebIDL.

Comment 29

[Andrew McCreight [:mccr8]]

Comment on attachment 8822846 [details] [diff] [review]
part 12.  Convert CSSStyleRule to WebIDL

Review of attachment 8822846 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the weakmap test changes

Comment 30

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 1 merged on top of bug 1326507

Comment 31

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 4 merged on top of bug 1326507

Comment 32

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 merged on top of bug 1326507

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 merged on top of bug 1326507

Comment 34

[Cameron McCormack (:heycam)]

Comment on attachment 8822836 [details] [diff] [review]
part 2.  Remove the now-unused GetExistingDOMRule method

Review of attachment 8822836 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/StyleRule.cpp
@@ +1433,5 @@
>  NS_IMPL_RELEASE_INHERITED(StyleRule, Rule)
>  
> +NS_IMPL_CYCLE_COLLECTION_CLASS(StyleRule)
> +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(StyleRule, Rule)
> +  tmp->DropReferences();

Was not calling DropReference / SetOwningRule during unlinking a pre-existing problem, or did this only surface for some reason due to the changes in part 1?  (This change seems unrelated to the commit message so maybe should be in a separate patch.)

Comment 35

[Boris Zbarsky [:bzbarsky]]

> This change seems unrelated to the commit message so maybe should be in a separate patch.

It should be in part 1.  Good catch!

Comment 36

[Cameron McCormack (:heycam)]

Comment on attachment 8823521 [details] [diff] [review]
Part 5 merged on top of bug 1326507

Review of attachment 8823521 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/StyleRule.cpp
@@ -1140,5 @@
> -inline css::DOMCSSStyleRule* DOMCSSDeclarationImpl::DomRule()
> -{
> -  return reinterpret_cast<css::DOMCSSStyleRule*>
> -                         (reinterpret_cast<char*>(this) -
> -           offsetof(css::DOMCSSStyleRule, mDOMDeclaration));

Much prefer having a separate UniquePtr owned object than the fused allocation we had before!

::: layout/style/StyleRule.h
@@ +313,3 @@
>  
> +class StyleRule final : public Rule,
> +                        public nsICSSStyleRuleDOMWrapper

Nit: comma on this line.

Comment 37

[Cameron McCormack (:heycam)]

Comment on attachment 8823518 [details] [diff] [review]
Part 4 merged on top of bug 1326507

Review of attachment 8823518 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsWrapperCache.h
@@ +286,5 @@
>      }
>    }
>  
>  private:
> +  // Firend declarations for things that need to be able to call

Friend

::: layout/style/Rule.h
@@ +30,5 @@
>    DECL_STYLE_RULE_INHERIT_NO_DOMRULE                       \
>    virtual nsIDOMCSSRule* GetDOMRule() override;
>  
> +class Rule : public nsISupports,
> +             public nsWrapperCache

Nit: comma on this line.

Comment 38

[Cameron McCormack (:heycam)]

Comment on attachment 8822840 [details] [diff] [review]
part 6.  Make css::Rule inherit from nsIDOMCSSRule

Review of attachment 8822840 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/Rule.h
@@ +114,5 @@
>    // Note that this returns null for inline style rules since they aren't
>    // supposed to have a DOM rule representation (and our code wouldn't work).
>    virtual nsIDOMCSSRule* GetDOMRule() = 0;
>  
>    // to implement methods on nsIDOMCSSRule

Maybe just change this comment to "// nsIDOMCSSRule" since they are the actual implementations now.

Comment 39

[Cameron McCormack (:heycam)]

(In reply to Cameron McCormack (:heycam) from comment #38)
> Maybe just change this comment to "// nsIDOMCSSRule" since they are the
> actual implementations now.

That changes in the next patch, so don't worry.

Comment 40

[Cameron McCormack (:heycam)]

Comment on attachment 8822841 [details] [diff] [review]
part 7.  Push the nsIDOMCSSRule implementation up to css::Rule

Review of attachment 8822841 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/NameSpaceRule.h
@@ +51,5 @@
>  
>    void GetURLSpec(nsString& aURLSpec) const { aURLSpec = mURLSpec; }
>  
> +  // WebIDL interface
> +  uint16_t Type() const override;  

Nit: trailing space.

::: layout/style/nsCSSRules.cpp
@@ +109,5 @@
> +NS_IMETHODIMP
> +Rule::SetCssText(const nsAString& aCssText)
> +{
> +  // We used to throw for some rule types, but not all.  Specifically, we did
> +  // not throw for StyleRule.  Let's just always not throw.

Luckily that's what the spec says to do. :)

Comment 41

[Boris Zbarsky [:bzbarsky]]

> Luckily that's what the spec says to do. :)

Ick spec.  Ah, well.

Comment 42

[Cameron McCormack (:heycam)]

Comment on attachment 8822843 [details] [diff] [review]
part 9.  Add a CSSRule Web IDL interface

Review of attachment 8822843 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the bits outside dom/ and not UnwrapArg-related.

::: layout/style/ServoStyleSheet.cpp
@@ +132,3 @@
>  ServoStyleSheet::GetDOMOwnerRule() const
>  {
> +  NS_ERROR("Don't know how to get DOM owner rule for ServoStyleSheet");

Nit: I've been using a prefix of "stylo: " in messages like these, so that later they'll be easy to grep for and fix.

Comment 43

[Cameron McCormack (:heycam)]

Comment on attachment 8822844 [details] [diff] [review]
part 10.  Convert CSSNamespaceRule to WebIDL

Review of attachment 8822844 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the layout/ bits.

Comment 44

[Cameron McCormack (:heycam)]

Comment on attachment 8822845 [details] [diff] [review]
part 11.  Convert CSSImportRule to WebIDL

Review of attachment 8822845 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the layout/ bits.

Comment 45

[Cameron McCormack (:heycam)]

Comment on attachment 8822846 [details] [diff] [review]
part 12.  Convert CSSStyleRule to WebIDL

Review of attachment 8822846 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the layout/ bits.

::: dom/webidl/LegacyQueryInterface.webidl
@@ +87,5 @@
>  XMLHttpRequest implements LegacyQueryInterface;
>  XMLHttpRequestUpload implements LegacyQueryInterface;
>  XMLSerializer implements LegacyQueryInterface;
>  XPathEvaluator implements LegacyQueryInterface;
> +CSSStyleRule implements LegacyQueryInterface;

Nit: This file looks pretty well sorted, so we may as well keep it that way.

Comment 46

[Cameron McCormack (:heycam)]

Comment on attachment 8822847 [details] [diff] [review]
part 13.  Convert media, supports, and moz-document rules to WebIDL

Review of attachment 8822847 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the layout/ bits.

::: layout/style/nsCSSRules.cpp
@@ +771,5 @@
> +nsMediaList*
> +MediaRule::Media() const
> +{
> +  // In practice, if we end up being parsed at all, we have non-null mMedia.  So
> +  // it's OK to claim we don't return null here.

Is it worth then changing all of the existing null checks in MediaRule's other methods into assertions?

Comment 47

[Cameron McCormack (:heycam)]

Comment on attachment 8822848 [details] [diff] [review]
part 14.  Convert CSSPageRule to WebIDL

Review of attachment 8822848 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the layout/ bits.

::: dom/bindings/Bindings.conf
@@ +233,5 @@
> +    'nativeType': 'nsCSSPageRule',
> +    'headerFile': 'nsCSSRules.h',
> +},
> +
> +

Nit: two blank lines probably not needed.

Comment 48

[Cameron McCormack (:heycam)]

Comment on attachment 8822849 [details] [diff] [review]
part 15.  Convert CSSFontFaceRule to WebIDL

Review of attachment 8822849 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the layout/ bits.

::: dom/webidl/CSSFontFaceRule.webidl
@@ +6,5 @@
> + * The origin of this IDL file is
> + * https://drafts.csswg.org/css-fonts/#om-fontface
> + */
> +
> +// https://drafts.csswg.org/cssom/#the-csspagerule-interface

Wrong link.

Comment 49

[Cameron McCormack (:heycam)]

Comment on attachment 8822850 [details] [diff] [review]
part 16.  Convert CSSFontFeatureValuesRule to WebIDL

Review of attachment 8822850 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the layout/ bits.

::: dom/webidl/CSSFontFeatureValuesRule.webidl
@@ +11,5 @@
> +// but we don't implement anything remotely resembling the spec.
> +interface CSSFontFeatureValuesRule : CSSRule {
> +  [SetterThrows]
> +  attribute DOMString fontFamily;
> +  // Gecko addition?

I guess so.  Maybe split out into a |partial interface|?

Comment 50

[Cameron McCormack (:heycam)]

Comment on attachment 8822851 [details] [diff] [review]
part 17.  Convert CSSKeyframeRule to WebIDL

Review of attachment 8822851 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the layout/ bits.

Comment 51

[Cameron McCormack (:heycam)]

Comment on attachment 8822852 [details] [diff] [review]
part 18.  Convert CSSKeyframesRule to WebIDL

Review of attachment 8822852 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the layout/ bits.

Comment 52

[Cameron McCormack (:heycam)]

Comment on attachment 8822853 [details] [diff] [review]
part 19.  Convert CSSCounterStyleRule to WebIDL

Review of attachment 8822853 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the layout/ bits.

Comment 53

[Boris Zbarsky [:bzbarsky]]

> Is it worth then changing all of the existing null checks in MediaRule's other methods into assertions?

Yeah, probably a good idea.  Not in the dtor, because we may have been cycle-collected.  Probably best done as a followup; filed bug 1328808 but stylo may make it irrelevant anyway.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Addressed all the other comments.  Thank you for the quick reviews!

Comment 55

[Peter Van der Beken [:peterv]]

Comment on attachment 8822843 [details] [diff] [review]
part 9.  Add a CSSRule Web IDL interface

Review of attachment 8822843 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/Rule.h
@@ +138,5 @@
> +// Specialization of the bindings UnwrapArg setup for css::Rule, so we can avoid
> +// adding an IID to css::Rule.  This can go away once all css::Rule subclasses
> +// are on WebIDL bindings.
> +
> +#include "js/TypeDecls.h"

Shouldn't this include be outside the mozilla namespace?

::: layout/style/nsCSSRules.cpp
@@ +46,5 @@
>  namespace mozilla {
> +
> +// Temporary code that can go away once all css::Rules are on WebIDL bindings.
> +namespace dom {
> +#include "xpcpublic.h"

Shouldn't this include be outside of the mozilla::dom namespace?

Comment 56

[Boris Zbarsky [:bzbarsky]]

> Shouldn't this include be outside the mozilla namespace?
> Shouldn't this include be outside of the mozilla::dom namespace?

Yes to both.  Good catch.  Fixed.

Comment 57

[Peter Van der Beken [:peterv]]

Comment on attachment 8822844 [details] [diff] [review]
part 10.  Convert CSSNamespaceRule to WebIDL

Review of attachment 8822844 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/tests/mochitest/general/test_interfaces.html
@@ +248,5 @@
>      "CSSMediaRule",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
>      "CSSMozDocumentRule",
>  // IMPORTANT: Do not change this list without review from a DOM peer!
> +    "CSSNamespaceRule",

Ugh!

Comment 58

[Peter Van der Beken [:peterv]]

Comment on attachment 8822846 [details] [diff] [review]
part 12.  Convert CSSStyleRule to WebIDL

Review of attachment 8822846 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/ServoStyleRule.cpp
@@ +155,5 @@
>  /* virtual */ JSObject*
>  ServoStyleRule::WrapObject(JSContext* aCx,
>                             JS::Handle<JSObject*> aGivenProto)
>  {
> +  return dom::CSSStyleRuleBinding::Wrap(aCx, this, aGivenProto);

I guess an alternative would be to put the WrapObject in BindingStyleRule, up to you.

Comment 59

[Boris Zbarsky [:bzbarsky]]

> I guess an alternative would be to put the WrapObject in BindingStyleRule

Oh, good point.  Done.

Comment 60

[Peter Van der Beken [:peterv]]

Comment on attachment 8822847 [details] [diff] [review]
part 13.  Convert media, supports, and moz-document rules to WebIDL

Review of attachment 8822847 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/CSSMediaRule.webidl
@@ +10,5 @@
> +
> +// https://drafts.csswg.org/cssom/#the-cssmediarule-interface and
> +// https://drafts.csswg.org/css-conditional/#the-cssmediarule-interface
> +// except they disagree with each other.  We're taking the inheritance from
> +// css-conditional and the PutForwards behavior from cssom.

Spec bug filed?

::: dom/webidl/LegacyQueryInterface.webidl
@@ +88,5 @@
>  XMLHttpRequestUpload implements LegacyQueryInterface;
>  XMLSerializer implements LegacyQueryInterface;
>  XPathEvaluator implements LegacyQueryInterface;
>  CSSStyleRule implements LegacyQueryInterface;
> +CSSMozDocumentRule implements LegacyQueryInterface;

Alphabetical order?

::: layout/style/nsCSSRules.cpp
@@ +586,1 @@
>    

Want to remove the trailing whitespace while you're here?

@@ +830,5 @@
>      nsresult rv = media->SetMediaText(aConditionText);
>      if (NS_SUCCEEDED(rv)) {
>        mMedia = media;
> +    } else {
> +      aRv.Throw(rv);

Hmm, why is it ok to not return here? Won't we crash below when accessing mMedia?

Comment 61

[Peter Van der Beken [:peterv]]

Comment on attachment 8822849 [details] [diff] [review]
part 15.  Convert CSSFontFaceRule to WebIDL

Review of attachment 8822849 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/CSSFontFaceRule.webidl
@@ +7,5 @@
> + * https://drafts.csswg.org/css-fonts/#om-fontface
> + */
> +
> +// https://drafts.csswg.org/cssom/#the-csspagerule-interface
> +// But we implement a very old draft, apparently....

Maybe file a bug?

Comment 62

[Boris Zbarsky [:bzbarsky]]

> Spec bug filed?

Yes, back then.  https://github.com/w3c/csswg-drafts/issues/845

> Alphabetical order?

Done.

> Want to remove the trailing whitespace while you're here?

Done.

> Hmm, why is it ok to not return here? Won't we crash below when accessing mMedia?

It's not, good catch.  We want to return whether rv succeeded or not, actually, because if it succeeded we've already done SetMediaText.  In practice, of course, mMedia is never null so this code is unreachable, but that's tracked in bug 1328808.

> Maybe file a bug?

Bug 1058408 tracks already.  Will add that to the comment.

Comment 63

[Peter Van der Beken [:peterv]]

Would it make sense to file spec bugs about adding SameObject to the style attribute of CSSKeyframeRule and PutForwards to the style attribute of CSSFontFaceRule and CSSKeyframeRule?

Comment 64

[Boris Zbarsky [:bzbarsky]]

Probably not a bad idea, good catch.  CSSFontFaceRule no longer has a .style that's a CSSStyleDeclaration in the spec, so that part is not relevant.  But you're right that CSSKeyframeRule should act like the others.  Filed https://github.com/w3c/csswg-drafts/issues/906

Comment 65

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/47fff3ed2a6c
part 1.  Make all CSS rules cycle-collected.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/4be9dd0ee8cc
part 2.  Remove the now-unused GetExistingDOMRule method.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/681ca39f5411
part 3.  Give CSS rules a PreCreate hook so we can safely wrappercache them.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/efbe11a48de7
part 4.  Make css::Rule wrappercached.  r=heycam,peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a603d30e5e1
part 5.  Get rid of DOMCSSStyleRule.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/174bb89ffd6e
part 6.  Make css::Rule inherit from nsIDOMCSSRule.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/4135091f5f31
part 7.  Push the nsIDOMCSSRule implementation up to css::Rule.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/41dca86782fa
part 8.  Get rid of css::Rule::GetDOMRule.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/78bb9ce60976
part 9.  Add a CSSRule Web IDL interface.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/cf9d73160d1a
part 10.  Convert CSSNamespaceRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a5a9437777f
part 11.  Convert CSSImportRule to WebIDL.  r=peterv,heycam,mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e48a59adc5e
part 12.  Convert CSSStyleRule to WebIDL.  r=peterv,heycam,mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d50c9372cc2e
part 13.  Convert media, supports, and moz-document rules to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/e37ac7b0c913
part 14.  Convert CSSPageRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/eda4ef1e0288
part 15.  Convert CSSFontFaceRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/224432acd298
part 16.  Convert CSSFontFeatureValuesRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/af8ee866cbed
part 17.  Convert CSSKeyframeRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/b882f9cdffa3
part 18.  Convert CSSKeyframesRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a99b184da55
part 19.  Convert CSSCounterStyleRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/ecaa5f64caf6
part 20.  Remove the remaining bits we added to support a mix of WebIDL and non-WebIDL rules.  r=peterv

Comment 66

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/855a52109378
followup to placate static analysis and reopen the CLOSED TREE.

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a82c1099a05
More bug 851892 static analysis placation on CLOSED TREE

Comment 68

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b32aaa2eec
Back out for Windows build bustage on CLOSED TREE

Comment 69

[Boris Zbarsky [:bzbarsky]]

I filed bug 1331102 on the problem that caused this to be backed out.

Comment 70

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3089dd379077
part 1.  Make all CSS rules cycle-collected.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/65422477b3a5
part 2.  Remove the now-unused GetExistingDOMRule method.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc81a167a75d
part 3.  Give CSS rules a PreCreate hook so we can safely wrappercache them.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6aa4c6192df
part 4.  Make css::Rule wrappercached.  r=heycam,peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/719bb9f41e5b
part 5.  Get rid of DOMCSSStyleRule.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/74c0ba66f108
part 6.  Make css::Rule inherit from nsIDOMCSSRule.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca4b1fb9cae4
part 7.  Push the nsIDOMCSSRule implementation up to css::Rule.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eab85b00159
part 8.  Get rid of css::Rule::GetDOMRule.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/e841a2796375
part 9.  Add a CSSRule Web IDL interface.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/c53cd7bdf8b3
part 10.  Convert CSSNamespaceRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/29cf8acbd21e
part 11.  Convert CSSImportRule to WebIDL.  r=peterv,heycam,mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8da3c34983f
part 12.  Convert CSSStyleRule to WebIDL.  r=peterv,heycam,mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8da2a3d8f10
part 13.  Convert media, supports, and moz-document rules to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/d58e0e5069ef
part 14.  Convert CSSPageRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/a25638588b6b
part 15.  Convert CSSFontFaceRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/73858e15c8c0
part 16.  Convert CSSFontFeatureValuesRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9cab46e8b45
part 17.  Convert CSSKeyframeRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/9bf5bcb3e8c5
part 18.  Convert CSSKeyframesRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/5f491bf49b85
part 19.  Convert CSSCounterStyleRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9c4115cdeac
part 20.  Remove the remaining bits we added to support a mix of WebIDL and non-WebIDL rules.  r=peterv

Comment 71

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3089dd379077
https://hg.mozilla.org/mozilla-central/rev/65422477b3a5
https://hg.mozilla.org/mozilla-central/rev/dc81a167a75d
https://hg.mozilla.org/mozilla-central/rev/d6aa4c6192df
https://hg.mozilla.org/mozilla-central/rev/719bb9f41e5b
https://hg.mozilla.org/mozilla-central/rev/74c0ba66f108
https://hg.mozilla.org/mozilla-central/rev/ca4b1fb9cae4
https://hg.mozilla.org/mozilla-central/rev/2eab85b00159
https://hg.mozilla.org/mozilla-central/rev/e841a2796375
https://hg.mozilla.org/mozilla-central/rev/c53cd7bdf8b3
https://hg.mozilla.org/mozilla-central/rev/29cf8acbd21e
https://hg.mozilla.org/mozilla-central/rev/a8da3c34983f
https://hg.mozilla.org/mozilla-central/rev/d8da2a3d8f10
https://hg.mozilla.org/mozilla-central/rev/d58e0e5069ef
https://hg.mozilla.org/mozilla-central/rev/a25638588b6b
https://hg.mozilla.org/mozilla-central/rev/73858e15c8c0
https://hg.mozilla.org/mozilla-central/rev/a9cab46e8b45
https://hg.mozilla.org/mozilla-central/rev/9bf5bcb3e8c5
https://hg.mozilla.org/mozilla-central/rev/5f491bf49b85
https://hg.mozilla.org/mozilla-central/rev/b9c4115cdeac

Comment 72

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/mozilla-central/rev/50bd08fad349 for what I fear is going to be a very inconvenient failure mode, as far as I know only Android opt, in some crashtest in crashtest-4 around 20% of the time something will fail with something like "load failed: timed out waiting for spell checks to end" or "load failed: null" or "load failed: timed out while taking snapshot (bug in harness?)" or "load failed: timed out waiting for test to complete (waiting for onload scripts to complete)" - there's a nice variety of reasons and tests in https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=5ce3882eec21be3a70e4afc050959ca2f76bfa76&filter-searchStr=crashtest-4&filter-resultStatus=testfailed&filter-resultStatus=busted

Comment 73

[Boris Zbarsky [:bzbarsky]]

Clearly this changeset just has bad karma.  ;)

Some try runs suggest that the culprit is in fact the very first part.  Compare https://treeherder.mozilla.org/#/jobs?repo=try&revision=dabc8c24f7dcc0e8705183398750dddcbd6fce5c (try push right on top of the backout) to https://treeherder.mozilla.org/#/jobs?repo=try&revision=4df4228c567f5b89f31bbbf7089b873d0a70e5a9 (try push with just part 1 on top of that).

I'll look into this on Tuesday.

Comment 74

[Boris Zbarsky [:bzbarsky]]

So just for my information, there are apparently failures in the following tests:

1) layout/style/crashtests/460209-1.html -- has a single @import rule of a sheet with no rules.  @import rules are already cycle-collected, even before my changes.
2) layout/style/crashtests/448161-1.html, layout/style/crashtests/448161-2.html -- don't contain any CSS rules (or indeed any obvious document-level style) at all, other than UA sheets.
3) layout/style/crashtests/456196.html -- only has inline style (i.e. not a rule), plus UA rules.
4) layout/style/crashtests/460323-1.html -- this has some actual StyleRule instances.
5) layout/style/crashtests/460217-1.html -- has @font-face rules only

Some of these failures (very few) are crashes, but they are highly uninformative; they just claim the crash is in libc.so somehere, with libvdm.so on the callstack for a while after that.

Comment 75

[Boris Zbarsky [:bzbarsky]]

Some more try data:

1)  Not actually telling the cycle collector about the edge from StyleRule to mDOMRule right on top of part 1 there seems to make the problem go away.  See https://treeherder.mozilla.org/#/jobs?repo=try&revision=405efca1f8dc3f6543ad77dedfef02b68046de49 (mDeclaration is not actually cycle-collected, so that edge is not likely to be relevant).

2)  With the full patch stack, and mDOMRule merged into StyleRule, but mDOMDeclaration still a separate object, not telling CC about the wrapper of the mDOMDeclaration (which is now the only thing StyleRule's CC really does, other than tracing its own wrapper) does NOT really help, except maybe in terms of frequency.  See https://treeherder.mozilla.org/#/jobs?repo=try&revision=d77d6a97d44faf929c2e77b365904deaa23b0f2c

I still have no idea why any of this makes android opt unhappy, of course.  :(

Comment 76

[Boris Zbarsky [:bzbarsky]]

And https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfd32818e9d062ca2fa53e73c8bf752056c22ba4 shows that not tracing through the actual wrapper of the StyleRule itself is also not enough to prevent the failures appearing....

Comment 77

[Boris Zbarsky [:bzbarsky]]

And https://treeherder.mozilla.org/#/jobs?repo=try&revision=7602bb4b71b3dfb43f1c768ff76464f254d48ac4 shows that even tracing nothing at all from StyleRule doesn't help, with all the patches applied.  At this point I'm at a bit of a loss for what the heck is going on.  :(

Comment 78

[Andrew McCreight [:mccr8]]

This is a total guess, but you could try setting dom.cycle_collector.incremental to false and see if that helps. In bug 1023758 comment 53, there was some weird interaction between CSS and incremental CC.

Comment 79

[Boris Zbarsky [:bzbarsky]]

No, that still fails, unfortunately...  https://treeherder.mozilla.org/#/jobs?repo=try&revision=0741b2d48a70bcc4431bc8a20a8c57658d37571a

Comment 80

[Boris Zbarsky [:bzbarsky]]

OK, with some printf debugging on try, the mystery is resolved.

Starting with that first changeset where we add rules to CC, we end up having a single CC take somewhere near 5 minutes on Android opt, which is basically the test timeout.  If that CC happens during a test, that test fails with one of the various timeout messages.  If it happens between tests, we get a green run.

The reason for that super-long CC is that it has about a million objects it's tracing and then freeing.  Those objects all come from layout/style/crashtests/439184-1.html which creates a stylesheet with 2^20 StyleRule instances in it.  Before my changes, since those rules were never touched from the CSSOM, they never ended up in the CC graph.  Now they do.

Presumably non-Android platforms have faster hardware so the CC doesn't take as long there (testing this hypothesis now).  Presumably debug Android has a higher timeout and the CC doesn't take enough longer to hit that timeout.

Still thinking about how to address this.

Comment 81

[Boris Zbarsky [:bzbarsky]]

Per discussion with smaug and mccr8, I tried adding skippability stuff to StyleRule.  That helps a lot.  The longest CC pause on the Android opt crashtests goes from 300,000ms or so to 9,000ms.

On linux64 opt, it goes from 1000ms to 318ms.  I have no idea why Android was so slow, by the way...

Oh, and as for Android debug builds... They do run layout/style/crashtests/439184-1.html but then never seem to end up with a CC that sees all those rules.  Presumably something kills off the entire sheet and all its rules (which aren't implicated in any cycles I'm aware of to start with) before CC ever manages to run.  That's why those weren't failing.

Anyway, I'll post some interdiffs for the above patches that add skippability to StyleRule and ServoStyleRule.

Comment 82

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 1 to add skippability to rule cycle collection

Comment 83

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 4 to update rule skippability to the wrappercache changes

Comment 84

[Boris Zbarsky [:bzbarsky]]

Attachment: Interdiff for part 5 to update skippability to the removal of DOMCSSStyleRule

Comment 85

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8828456 [details] [diff] [review]
Interdiff for part 4 to update rule skippability to the wrappercache changes


> NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_BEGIN(Rule)
>-  // Note that we can't make use of IsKnownLive() here directly, because we may
>-  // be subclassed by something that needs tracing.  Once we have a wrapper
>-  // cache we should be able to do better here.
>-  return tmp->IsCCLeaf();
>+  return tmp->IsCCLeaf() ||
>+    (tmp->IsKnownLive() && tmp->HasNothingToTrace(tmp));
Technically CanSkip doesn't need to ensure HasNothingToTrace, only CanSkipInCC (since CC needs to get right C++->grayJS edges to the graph).


> NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_BEGIN(Rule)
>-  // Note that we can't make use of IsKnownLive() here directly, because we may
>-  // be subclassed by something that needs tracing.  Once we have a wrapper
>-  // cache we should be able to do better here.
>-  return tmp->IsCCLeaf();
>+  return tmp->IsCCLeaf() ||
>+    (tmp->IsKnownLive() && tmp->HasNothingToTrace(tmp));
> NS_IMPL_CYCLE_COLLECTION_CAN_SKIP_IN_CC_END
So IsCCLeaf() doesn't ever return true if there is some js which might be traced, right?
In all the places we check PreservingWrapper(); and wrapper is the only thing to trace.

Comment 86

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8828455 [details] [diff] [review]
Interdiff for part 1 to add skippability to rule cycle collection

>@@ -1406,17 +1406,16 @@ StyleRule::DropReferences()
> {
>   if (mDOMRule) {
>     mDOMRule->DOMDeclaration()->DropReference();
>     mDOMRule = nullptr;
>   }
> 
>   if (mDeclaration) {
>     mDeclaration->SetOwningRule(nullptr);
>-    mDeclaration = nullptr;
>   }
Why is this ok? Why we don't want to clear the reference?


>+bool
>+StyleRule::IsCCLeaf() const
>+{
>+  if (!Rule::IsCCLeaf()) {
>+    return false;
>+  }
>+
>+  return !mDOMRule || !mDOMRule->DOMDeclaration()->PreservingWrapper();
>+}
So DOMRule doesn't keep anything else but declaration alive, right?
>+Rule::IsKnownLive() const
>+{
>+  StyleSheet* sheet = GetStyleSheet();
>+  if (!sheet) {
>+    return false;
>+  }
>+
>+  if (!sheet->IsOwnedByDocument()) {
>+    return false;
>+  }
ok, some patch in this bug is adding IsOwnedByDocument(). I assume it does what I think it does.


Could you still explain the ownership model of StyleRule and mDOMRule and mDOMRule->DOMDeclaration()?
Or perhaps how that is changed then in some patch? Which patch should I look at?

Comment 87

[Boris Zbarsky [:bzbarsky]]

> Technically CanSkip doesn't need to ensure HasNothingToTrace

Great.  I will drop that.  ;)

> So IsCCLeaf() doesn't ever return true if there is some js which might be traced, right?

That's correct, since those would be outgoing owning edges.

> Why is this ok? Why we don't want to clear the reference?

For the same reason it's OK to not cc-traverse the pointer: mDeclaration is not a CC-ed object, and hence we can't have a cycle through it.

> So DOMRule doesn't keep anything else but declaration alive, right?

That's correct.  The only member it has is a DOMCSSDeclarationImpl (as an object, not a pointer).

> ok, some patch in this bug is adding IsOwnedByDocument()

In bug 1332353 if you want to look at it.  And the idea is that it does what you think it does, yes.

> Could you still explain the ownership model of StyleRule and mDOMRule and mDOMRule->DOMDeclaration()?

In the end it doesn't matter too much, because that model is changed in part 5 in this bug.

After that change, StyleRule has an mDOMDeclaration which it owns (via UniquePtr), and they share a single refcount via aggregation.  Each has a separate nsWrapperCache, but only the StyleRule has a CC participant; the mDOMDeclaration forwards the CC-isupports and CC-participant QIs to StyleRule so from CC's point of view they're a single object.  Hence the need to trace the mDOMDeclaration's wrapper from the StyleRule's Trace, and check it in StyleRule's IsCCLeaf().

Comment 88

[Cameron McCormack (:heycam)]

Having never touched CC skippability stuff, and to the extent that the ownership description sounds OK to me, I'm happy to defer to smaug's r+es here.

Comment 89

[Boris Zbarsky [:bzbarsky]]

OK, as long as you buy my claims about IsCCLeaf and IsKnownLive working as advertised!

Comment 90

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9a4b96c10b23
part 1.  Make all CSS rules cycle-collected.  r=heycam,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/795ebf4423ba
part 2.  Remove the now-unused GetExistingDOMRule method.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca8c00f9738d
part 3.  Give CSS rules a PreCreate hook so we can safely wrappercache them.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/3be6690b9f0a
part 4.  Make css::Rule wrappercached.  r=heycam,peterv,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c2afe732553
part 5.  Get rid of DOMCSSStyleRule.  r=heycam,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/271c7cd7b59a
part 6.  Make css::Rule inherit from nsIDOMCSSRule.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/659550973d4d
part 7.  Push the nsIDOMCSSRule implementation up to css::Rule.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/9ef39356f62c
part 8.  Get rid of css::Rule::GetDOMRule.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/b975a988fb33
part 9.  Add a CSSRule Web IDL interface.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dee4f98503c
part 10.  Convert CSSNamespaceRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/eab8ccda41b8
part 11.  Convert CSSImportRule to WebIDL.  r=peterv,heycam,mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/31103a03f2c2
part 12.  Convert CSSStyleRule to WebIDL.  r=peterv,heycam,mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bbbc436690d
part 13.  Convert media, supports, and moz-document rules to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/34c280ac5f73
part 14.  Convert CSSPageRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/332bb85695a9
part 15.  Convert CSSFontFaceRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/430f2759de65
part 16.  Convert CSSFontFeatureValuesRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/76136a4cadcc
part 17.  Convert CSSKeyframeRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/663ad79f5c13
part 18.  Convert CSSKeyframesRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/f62d345e9d41
part 19.  Convert CSSCounterStyleRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/c79e8bee853e
part 20.  Remove the remaining bits we added to support a mix of WebIDL and non-WebIDL rules.  r=peterv

Comment 91

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f8a4d1c49ec
part 1.  Make all CSS rules cycle-collected.  r=heycam,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d8f9b1c8d61
part 2.  Remove the now-unused GetExistingDOMRule method.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f24650b72a1
part 3.  Give CSS rules a PreCreate hook so we can safely wrappercache them.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/b2f8a7db0858
part 4.  Make css::Rule wrappercached.  r=heycam,peterv,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0fd8df61f56
part 5.  Get rid of DOMCSSStyleRule.  r=heycam,smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8b2eaea267
part 6.  Make css::Rule inherit from nsIDOMCSSRule.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/030208c7e084
part 7.  Push the nsIDOMCSSRule implementation up to css::Rule.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/a70bbdbbdf27
part 8.  Get rid of css::Rule::GetDOMRule.  r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/b9e55a13fd3b
part 9.  Add a CSSRule Web IDL interface.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e0e575589a3
part 10.  Convert CSSNamespaceRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/aa9330ed3a07
part 11.  Convert CSSImportRule to WebIDL.  r=peterv,heycam,mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/02bd976e7157
part 12.  Convert CSSStyleRule to WebIDL.  r=peterv,heycam,mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/336c0e3ea229
part 13.  Convert media, supports, and moz-document rules to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/c70e7a0b7d29
part 14.  Convert CSSPageRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/72c7a4a0b278
part 15.  Convert CSSFontFaceRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dd6a0bbfb37
part 16.  Convert CSSFontFeatureValuesRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e899668b28b
part 17.  Convert CSSKeyframeRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e061df954b3
part 18.  Convert CSSKeyframesRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bf7bcdac8d3
part 19.  Convert CSSCounterStyleRule to WebIDL.  r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b65e31bdffb
part 20.  Remove the remaining bits we added to support a mix of WebIDL and non-WebIDL rules.  r=peterv

Comment 92

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d4dc504eea5
Backed out changeset c79e8bee853e 
https://hg.mozilla.org/integration/mozilla-inbound/rev/b01edf06d37c
Backed out changeset f62d345e9d41 
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1de38223150
Backed out changeset 663ad79f5c13 
https://hg.mozilla.org/integration/mozilla-inbound/rev/26a4cd58030a
Backed out changeset 76136a4cadcc 
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bed364016fe
Backed out changeset 430f2759de65 
https://hg.mozilla.org/integration/mozilla-inbound/rev/14f30033ee60
Backed out changeset 332bb85695a9 
https://hg.mozilla.org/integration/mozilla-inbound/rev/5523b32e5425
Backed out changeset 34c280ac5f73 
https://hg.mozilla.org/integration/mozilla-inbound/rev/633b0b55c6af
Backed out changeset 1bbbc436690d 
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c5e1faf2fdd
Backed out changeset 31103a03f2c2 
https://hg.mozilla.org/integration/mozilla-inbound/rev/7dbef218327b
Backed out changeset eab8ccda41b8 
https://hg.mozilla.org/integration/mozilla-inbound/rev/23dc67f25818
Backed out changeset 9dee4f98503c 
https://hg.mozilla.org/integration/mozilla-inbound/rev/95d8e109513e
Backed out changeset b975a988fb33 
https://hg.mozilla.org/integration/mozilla-inbound/rev/aba82d453b03
Backed out changeset 9ef39356f62c 
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a929848ba43
Backed out changeset 659550973d4d 
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee32dc6b1a12
Backed out changeset 271c7cd7b59a 
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ef461d046d5
Backed out changeset 2c2afe732553 
https://hg.mozilla.org/integration/mozilla-inbound/rev/21fd8a323aa4
Backed out changeset 3be6690b9f0a 
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1161010563e
Backed out changeset ca8c00f9738d 
https://hg.mozilla.org/integration/mozilla-inbound/rev/e345379ea857
Backed out changeset 795ebf4423ba 
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fb381da04b5
Backed out changeset 9a4b96c10b23 for build bustage. r=backout on a CLOSED TREE

Comment 93

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/621e14f5c951
followup.  Disable most of Rule::IsKnownLive for now to reopen CLOSED TREE

Comment 94

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8f8a4d1c49ec
https://hg.mozilla.org/mozilla-central/rev/0d8f9b1c8d61
https://hg.mozilla.org/mozilla-central/rev/4f24650b72a1
https://hg.mozilla.org/mozilla-central/rev/b2f8a7db0858
https://hg.mozilla.org/mozilla-central/rev/a0fd8df61f56
https://hg.mozilla.org/mozilla-central/rev/9d8b2eaea267
https://hg.mozilla.org/mozilla-central/rev/030208c7e084
https://hg.mozilla.org/mozilla-central/rev/a70bbdbbdf27
https://hg.mozilla.org/mozilla-central/rev/b9e55a13fd3b
https://hg.mozilla.org/mozilla-central/rev/5e0e575589a3
https://hg.mozilla.org/mozilla-central/rev/aa9330ed3a07
https://hg.mozilla.org/mozilla-central/rev/02bd976e7157
https://hg.mozilla.org/mozilla-central/rev/336c0e3ea229
https://hg.mozilla.org/mozilla-central/rev/c70e7a0b7d29
https://hg.mozilla.org/mozilla-central/rev/72c7a4a0b278
https://hg.mozilla.org/mozilla-central/rev/1dd6a0bbfb37
https://hg.mozilla.org/mozilla-central/rev/8e899668b28b
https://hg.mozilla.org/mozilla-central/rev/2e061df954b3
https://hg.mozilla.org/mozilla-central/rev/8bf7bcdac8d3
https://hg.mozilla.org/mozilla-central/rev/5b65e31bdffb
https://hg.mozilla.org/mozilla-central/rev/621e14f5c951

Comment 96

[Chris Mills [:cmills]]

I've had a look through the CSSOM MDN pages, and I don't think this bug really requires much in the way of docs changes. As it is, we've already got the interfaces described in the same kind of way as for any other DOM interface.

What we could probably do with is to generally update these pages, to add missing properties, etc., make sure everything is up to date. I'll file a separate docs bug for that.