Closed Bug 851892 Opened 7 years ago Closed 3 years ago

Move CSS Rules to WebIDL

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: Ms2ger, Assigned: bzbarsky)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

(Keywords: dev-doc-complete, Whiteboard: [tw-dom])

Attachments

(23 files, 4 obsolete files)

9.25 KB, patch
heycam
: review+
Details | Diff | Splinter Review
9.33 KB, patch
peterv
: review+
Details | Diff | Splinter Review
27.17 KB, patch
heycam
: review+
Details | Diff | Splinter Review
41.67 KB, patch
heycam
: review+
Details | Diff | Splinter Review
23.92 KB, patch
heycam
: review+
Details | Diff | Splinter Review
19.15 KB, patch
peterv
: review+
heycam
: review+
Details | Diff | Splinter Review
9.39 KB, patch
peterv
: review+
heycam
: review+
Details | Diff | Splinter Review
12.66 KB, patch
peterv
: review+
heycam
: review+
mccr8
: review+
Details | Diff | Splinter Review
21.88 KB, patch
peterv
: review+
heycam
: review+
mccr8
: review+
Details | Diff | Splinter Review
33.46 KB, patch
peterv
: review+
heycam
: review+
Details | Diff | Splinter Review
11.55 KB, patch
peterv
: review+
heycam
: review+
Details | Diff | Splinter Review
10.91 KB, patch
peterv
: review+
heycam
: review+
Details | Diff | Splinter Review
11.63 KB, patch
peterv
: review+
heycam
: review+
Details | Diff | Splinter Review
11.59 KB, patch
peterv
: review+
heycam
: review+
Details | Diff | Splinter Review
11.85 KB, patch
peterv
: review+
heycam
: review+
Details | Diff | Splinter Review
11.66 KB, patch
peterv
: review+
heycam
: review+
Details | Diff | Splinter Review
5.62 KB, patch
peterv
: review+
Details | Diff | Splinter Review
28.49 KB, patch
heycam
: review+
Details | Diff | Splinter Review
32.50 KB, patch
heycam
: review+
peterv
: review+
Details | Diff | Splinter Review
17.57 KB, patch
heycam
: review+
Details | Diff | Splinter Review
25.69 KB, patch
smaug
: review+
Details | Diff | Splinter Review
4.46 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.56 KB, patch
smaug
: review+
Details | Diff | Splinter Review
No description provided.
Depends on: 896277
Depends on: 898764
Blocks: 754772
Depends on: 1069065
Blocks: 1207321
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?
Flags: needinfo?(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.
Flags: needinfo?(bzbarsky)
Depends on: 1326388
Depends on: 1326522
Depends on: 1325877
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.
Oh, and I looked at addons dxr, and looks like I need to keep scripted QueryInterface on CSSStyleRule and CSSMozDocumentStyleRule...
Er, I meant CSSMozDocumentRule.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Note that this increases the size of css::Rule by three words, unfortunately.
Attachment #8822838 - Flags: review?(peterv)
Attachment #8822838 - Flags: review?(cam)
Attachment #8822839 - Flags: review?(cam)
Peter, could you review he UnwrapArg bits and the dom/bindings and dom/webidl changes, for this changeset and the following ones?
Attachment #8822843 - Flags: review?(peterv)
Attachment #8822843 - Flags: review?(cam)
Andrew, can you please review the weakmap test change?
Attachment #8822845 - Flags: review?(peterv)
Attachment #8822845 - Flags: review?(continuation)
Attachment #8822845 - Flags: review?(cam)
Andrew, can you please review the other weakmap test change?
Attachment #8822846 - Flags: review?(peterv)
Attachment #8822846 - Flags: review?(continuation)
Attachment #8822846 - Flags: review?(cam)
The PutForwards bit is a new feature, but easy enough to implement here.
Attachment #8822848 - Flags: review?(peterv)
Attachment #8822848 - Flags: review?(cam)
Attachment #8822849 - Flags: review?(peterv)
Attachment #8822849 - Flags: review?(cam)
Attachment #8822851 - Flags: review?(peterv)
Attachment #8822851 - Flags: review?(cam)
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?
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 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.
Attachment #8822845 - Flags: review?(continuation) → review+
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
Attachment #8822846 - Flags: review?(continuation) → review+
Attachment #8822835 - Attachment is obsolete: true
Attachment #8822835 - Flags: review?(cam)
Attachment #8823517 - Attachment description: Part 1 merged on top of → Part 1 merged on top of bug 1326507
Attachment #8823518 - Flags: review?(peterv)
Attachment #8823518 - Flags: review?(cam)
Attachment #8822838 - Attachment is obsolete: true
Attachment #8822838 - Flags: review?(peterv)
Attachment #8822838 - Flags: review?(cam)
Attachment #8823519 - Flags: review?(peterv)
Attachment #8823519 - Flags: review?(cam)
Attachment #8822839 - Attachment is obsolete: true
Attachment #8822839 - Flags: review?(cam)
Attachment #8823519 - Attachment description: Part 5 merged on top of → Part 5 merged on top of bug 1326507
Attachment #8823519 - Flags: review?(peterv)
Attachment #8823518 - Attachment description: Part 4 merged on top of → Part 4 merged on top of bug 1326507
Attachment #8823519 - Attachment is obsolete: true
Attachment #8823519 - Flags: review?(cam)
Attachment #8823521 - Attachment description: Part 5 merged on top of → Part 5 merged on top of bug 1326507
Attachment #8823517 - Flags: review?(cam) → review+
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.)
Attachment #8822836 - Flags: review?(cam) → review+
> 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 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.
Attachment #8823521 - Flags: review?(cam) → review+
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.
Attachment #8823518 - Flags: review?(cam) → review+
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.
Attachment #8822840 - Flags: review?(cam) → review+
(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 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. :)
Attachment #8822841 - Flags: review?(cam) → review+
> Luckily that's what the spec says to do. :)

Ick spec.  Ah, well.
Attachment #8822842 - Flags: review?(cam) → review+
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.
Attachment #8822843 - Flags: review?(cam) → review+
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.
Attachment #8822844 - Flags: review?(cam) → review+
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.
Attachment #8822845 - Flags: review?(cam) → review+
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.
Attachment #8822846 - Flags: review?(cam) → review+
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?
Attachment #8822847 - Flags: review?(cam) → review+
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.
Attachment #8822848 - Flags: review?(cam) → review+
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.
Attachment #8822849 - Flags: review?(cam) → review+
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|?
Attachment #8822850 - Flags: review?(cam) → review+
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.
Attachment #8822851 - Flags: review?(cam) → review+
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.
Attachment #8822852 - Flags: review?(cam) → review+
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.
Attachment #8822853 - Flags: review?(cam) → review+
> 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.
Addressed all the other comments.  Thank you for the quick reviews!
Attachment #8822837 - Flags: review?(peterv) → review+
Attachment #8823518 - Flags: review?(peterv) → review+
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?
Attachment #8822843 - Flags: review?(peterv) → review+
> 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 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!
Attachment #8822844 - Flags: review?(peterv) → review+
Attachment #8822845 - Flags: review?(peterv) → review+
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.
Attachment #8822846 - Flags: review?(peterv) → review+
> I guess an alternative would be to put the WrapObject in BindingStyleRule

Oh, good point.  Done.
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?
Attachment #8822847 - Flags: review?(peterv) → review+
Attachment #8822848 - Flags: review?(peterv) → review+
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?
Attachment #8822849 - Flags: review?(peterv) → review+
> 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.
Attachment #8822850 - Flags: review?(peterv) → review+
Attachment #8822851 - Flags: review?(peterv) → review+
Attachment #8822852 - Flags: review?(peterv) → review+
Attachment #8822853 - Flags: review?(peterv) → review+
Attachment #8822854 - Flags: review?(peterv) → review+
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?
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
Pushed by bzbarsky@mozilla.com:
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
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/855a52109378
followup to placate static analysis and reopen the CLOSED TREE.
Backout by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8b32aaa2eec
Back out for Windows build bustage on CLOSED TREE
I filed bug 1331102 on the problem that caused this to be backed out.
Pushed by bzbarsky@mozilla.com:
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
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla53 → ---
Flags: needinfo?(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.
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.
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.  :(
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....
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.  :(
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.
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.
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.
Depends on: 1332353
Attachment #8828458 - Flags: review?(bugs) → review+
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.
Attachment #8828456 - Flags: review?(bugs) → review+
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?
Attachment #8828455 - Flags: review?(bugs) → review+
> 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().
Flags: needinfo?(bzbarsky)
Depends on: 1332550
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.
Attachment #8828455 - Flags: review?(cam)
Attachment #8828456 - Flags: review?(cam)
Attachment #8828458 - Flags: review?(cam)
OK, as long as you buy my claims about IsCCLeaf and IsKnownLive working as advertised!
No longer depends on: 1332550
Pushed by bzbarsky@mozilla.com:
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
Pushed by bzbarsky@mozilla.com:
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
Backout by archaeopteryx@coole-files.de:
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
Blocks: 1332704
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/621e14f5c951
followup.  Disable most of Rule::IsKnownLive for now to reopen CLOSED TREE
Depends on: 1333001
Duplicate of this bug: 1337917
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.
See Also: → 1346421
You need to log in before you can comment on or make changes to this bug.