978833 - [css-animations] dynamic change to CSS keyframe rule's style declaration doesn't take effect until next rule tree GC

Status: RESOLVED FIXED

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

Description

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

A dynamic change to the style declaration of a CSS keyframe rule doesn't take effect until the next rule tree GC.  This is because the keyframe rules are used in the rule tree, but don't follow the rules about nsIStyleRule immutability.

Attachment 8384473 [details] [diff] and attachment 8384474 [details] [diff] [review] from bug 978648 fix this problem, but introduce a different problem described in bug 978648 comment 15:  that they change the object identity of an object exposed to the DOM.

The easiest fix for that problem is to add yet another additional object, but I'd sort of rather not.  The next easiest, which perhaps is due at this point, is to change the thing that implements nsIStyleRule (the source of style data) from the rule object to be the compressed data block.

There will be a todo() test added in bug 978648 that would be fixed by this bug.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

The immutability of nsIStyleRule may be further broken since many at-rules, including @font-face and @counter-style, are mutable in spec.

Comment 2

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

The immutability is only relevant to nsIStyleRule implementations that are actually used as nsIStyleRule, i.e., have their MapRuleInfoInto method called by rule nodes.  This bit of comment 0:

> The next easiest, which perhaps is due at this
> point, is to change the thing that implements nsIStyleRule (the source of
> style data) from the rule object to be the compressed data block.

is suggesting changing the object hierarchy so most rules don't inherit from nsIStyleRule (currently they inherit from it just to save a vtable pointer on the most common rule, StyleRule, but don't actually use that inheritance).

Comment 3

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

Bug 960465 patch 17 fixes the failing test here, but doesn't fix the underlying bug.  (It probably makes the bug less apparent, though... and bug 1064915 may also make the bug less apparent.)

Comment 4

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

Got an email in a thread originating from Rachel Nabors indicating that this has actually been observed by somebody other than a Gecko developer (via devtools that allow editing keyframes).

Comment 5

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

(In reply to David Baron [:dbaron] ⌚UTC+9 [busy, returning November 2] from comment #2)
> > The next easiest, which perhaps is due at this
> > point, is to change the thing that implements nsIStyleRule (the source of
> > style data) from the rule object to be the compressed data block.
> 
> is suggesting changing the object hierarchy so most rules don't inherit from
> nsIStyleRule (currently they inherit from it just to save a vtable pointer
> on the most common rule, StyleRule, but don't actually use that inheritance).

Or it's possible we could use mozilla::css::Declaration instead, given that it has mImmutable.  We'd certainly need to rejigger stuff related to mWasMatched / mImmutable.

That said, doing this would allow removing a whole bunch of other simplifications, such as:
 * simplifying code relating to the interaction of StyleRule and DOMCSSStyleRule
 * making nsCSSRule no longer inherit from nsIStyleRule (and rename nsIStyleRule) and otherwise simplifying the inheritance hierarchy

Comment 6

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

Taking; I made a bunch of progress on this on an airplane yesterday, though it's not done yet.

Comment 7

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=28ece2bf2178

Comment 8

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=01b818a7fcf9

Comment 9

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67886ffa2d69

Comment 10

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

Attachment: patch 1 - Add mochitest for bug 978833

I confirmed that without the patch series, the tests marked todo_is do
fail.

Comment 11

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

Attachment: patch 2 - Make css::Declaration reference-counted

This is done in preparation for making it implement nsIStyleRule, which
happens in patch 3, and which is used in patch 12.

Comment 12

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

Attachment: patch 3 - Make css::Declaration implement nsIStyleRule

We switch to using this implementation instead of the one in
css::StyleRule in patch 12.

(Yes, implementing QueryInterface for a CID is ugly, but it's the same
thing StyleRule does.  Unfortunately now we'll need to have it in both
places.)

Comment 13

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

Attachment: patch 4 - Add pointer back from css::Declaration to css::Rule

This is used in patch 12, in nsStyleSet::AssertNoCSSRules and in
inDOMUtils::GetCSSStyleRules.

Comment 14

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

Attachment: patch 5 - Rename ImportantRule to ImportantStyleData

(This is part of a longer term plan to rename nsIStyleRule to StyleData
and nsIStyleRuleProcessor to StyleDataSource.  I'm not doing all of that
here, though.)

Comment 15

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

Attachment: patch 6 - Move ImportantStyleData from StyleRule.{h,cpp} to Declaration.{h,cpp}

This is needed for patch 7.

Note that this removes an unused "friend class StyleRule;" declaration.

Comment 16

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

Attachment: patch 7 - Fuse allocation of ImportantStyleData with Declaration

Note that this adds a new public API to css::Declaration; the equivalent
API is removed from css::StyleRule and nsCSSPageRule is removed in patch
13.  But the removal and addition need to be on opposite sides of patch
12.

This fused allocation is no larger than having a pointer, and it removes
having to worry about cycles.

Comment 17

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

Attachment: patch 8 - Call SetImmutable for declarations of @page and keyframe rules

This probably should have been done before, but prior to this patch
series, dynamic changes of the declarations on these rules were broken
due to rule immutability violations; now that is no longer the case, but
to benefit from that, I believe we actually need to mark the
declarations as immutable once matched so that dynamic changes will
trigger construction of a new declaration (which thus has a new
nsIStyleRule identity).

Comment 18

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

Attachment: patch 9 - Move keyframe !important data assertion to where it will continue to happen

The current location of the assertion will stop being called in patch 12
and will go away in patch 15; the new location is valid both before and
after patch 12.

Comment 19

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

Attachment: patch 10 - Raise consistent exceptions so that the exception doesn't depend on rule destruction timing

Prior to patch 12, rule destruction for rules that were matched doesn't
happen until rule tree GC.  This means that GetCSSDeclaration is less
likely to return null, but then GetCSSParsingEnvironment might fail.
With StyleRule no longer participating in the rule tree, they're more
likely to be destroyed quickly, leading to the !olddecl failure case
instead of the !env.mPrincipal failure case.

This is needed to avoid patch 12 causing:
TEST-UNEXPECTED-FAIL | layout/inspector/tests/chrome/test_bug727834.xul | original rule is not available for modification anymore - got "NS_ERROR_FAILURE", expected "NS_ERROR_NOT_AVAILABLE"

Comment 20

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

Attachment: patch 11 - Add missing #includes in preparation for nsRuleWalker.h #include change in following patch

Patch 12 changes nsRuleWalker.h from including StyleRule.h to including
Declaration.h; this fixes other headers to deal with that change based
on the include-what-you-use principle.

Comment 21

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

Attachment: patch 12 - Use the css::Declaration instead of the css::StyleRule as the matching rule

This is the key change in this patch series; it changes the object we
use for style data (currently nsIStyleRule) identity.  It allows
removing some hacks we have to deal with that for StyleRule, and avoids
having to write similar hacks for nsCSSKeyframeRule and nsCSSPageRule
(which are broken without this).

I confirmed locally that it is this patch that fixes both of the todo_is
mochitests, by building and testing with the patch queue through patch
11, and again through patch 12.

Comment 22

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

Attachment: patch 13 - Remove important rule creation from css::StyleRule

This is the removal half corresponding to the additions in patch 7; the
removal needs to happen after patch 12.

Comment 23

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

Attachment: patch 14 - Pass mozilla::css::Rule instead of nsIStyleRule to nsIDocument/nsIDocumentObserver style rule methods

This is needed for patch 15, which will make the rules passed to these
methods no longer implement nsIStyleRule.

TODO: Given the amount that these parameters are used (not at all),
perhaps we should have a followup on removing them and simplifying these
notifications?

Comment 24

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

Attachment: patch 15 - Make css::Rule no longer inherit from nsIStyleRule

This inheritance was previously needed only by a subset of the classes
derived from css::Rule (css::StyleRule, nsCSSKeyframeRule,
nsCSSPageRule).  After patch 12, it is now needed by none.

Comment 25

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

Attachment: patch 16 - Always call Declaration::SetImmutable when we match a rule

This change needs to happen before future work that would get rid of the
DeclarationChanged dance in which we make a new StyleRule, but I've
postponed that work to a later bug.  Without this, those changes would
cause a regression, because we'd only call SetImmutable on a StyleRule's
first mDeclaration.  However, we may as well do this now, as it makes
patch 17 and patch 18 possible.

Comment 26

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

Attachment: patch 17 - Remove Rule::mWasMatched

Comment 27

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

Attachment: patch 18 - Eliminate StyleRule::RuleMatched and call Declaration::SetImmutable directly for style rules (like for @page and keyframe rules)

This is just simplification.

Comment 28

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

Attachment: patch 19 - Fix ResolvedStyleCache to use Declaration rather than nsCSSKeyframeRule as keys

This isn't needed today, but it makes more sense, and if we ever gave
the cache a longer lifetime, it would be needed, since the
nsCSSKeyframeRule can maintain its identity across style changes whereas
a matched Declaration cannot.

Comment 29

[Cameron McCormack (:heycam)]

Comment on attachment 8682453 [details] [diff] [review]
patch 2 - Make css::Declaration reference-counted

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

::: layout/style/Declaration.cpp
@@ -21,5 @@
>  
>  Declaration::Declaration()
>    : mImmutable(false)
>  {
> -  MOZ_COUNT_CTOR(mozilla::css::Declaration);

Why remove these?

::: layout/style/nsCSSParser.cpp
@@ +4196,5 @@
>    if (!declaration) {
>      return false;
>    }
>  
>    // Takes ownership of declaration.

This comment looks out of date.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8682461 [details] [diff] [review]
patch 10 - Raise consistent exceptions so that the exception doesn't depend on rule destruction timing

r=me

Comment 31

[Cameron McCormack (:heycam)]

Comment on attachment 8682454 [details] [diff] [review]
patch 3 - Make css::Declaration implement nsIStyleRule

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

> (Yes, implementing QueryInterface for a CID is ugly, but it's the same
> thing StyleRule does.  Unfortunately now we'll need to have it in both
> places.)

FWIW it looks like we only do_QueryObject to a StyleRule in DEBUG code.  Not sure if it's worth making the CID/QI stuff #ifdef DEBUG.  (Maybe not.)

::: layout/style/Declaration.cpp
@@ +58,5 @@
> +NS_IMPL_ADDREF(Declaration)
> +NS_IMPL_RELEASE(Declaration)
> +
> +/* virtual */ void
> +Declaration::MapRuleInfoInto(nsRuleData *aRuleData)

Nit: may as well move the "*" while moving this code.

::: layout/style/Declaration.h
@@ +16,5 @@
>  #ifndef MOZILLA_INTERNAL_API
>  #error "This file should only be included within libxul"
>  #endif
>  
> +#include "nsIStyleRule.h"

Please keep the #includes sorted.

Comment 32

[Cameron McCormack (:heycam)]

Comment on attachment 8682456 [details] [diff] [review]
patch 5 - Rename ImportantRule to ImportantStyleData

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

::: layout/style/StyleRule.h
@@ +298,2 @@
>  public:
> +  explicit ImportantStyleData(Declaration *aDeclaration);

Nit: if you want, let's shift the "*" across and put the { on a newline, while touching these lines.

Comment 33

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

(In reply to Cameron McCormack (:heycam) from comment #29)
> > -  MOZ_COUNT_CTOR(mozilla::css::Declaration);
> 
> Why remove these?

They're used for making non-reference-counted objects show up in leak stats.  The reference counting macros contain their own logging (which tracks the reference counts), which does the same and more.

Comment 34

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #33)
> They're used for making non-reference-counted objects show up in leak stats.
> The reference counting macros contain their own logging (which tracks the
> reference counts), which does the same and more.

Ah, I think then that I have added unnecessary MOZ_COUNT{C,D}TOR calls in some classes I've written, like FontFaceSet.

Comment 35

[Cameron McCormack (:heycam)]

Comment on attachment 8682460 [details] [diff] [review]
patch 9 - Move keyframe !important data assertion to where it will continue to happen

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

::: layout/style/nsAnimationManager.cpp
@@ +624,5 @@
>    // data that we'd really like to cache (in particular, the
>    // StyleAnimationValue values in AnimationPropertySegment).
>    nsStyleContext *result = mCache.GetWeak(aKeyframe);
>    if (!result) {
> +    Declaration *declaration = aKeyframe->Declaration();

Nit (my favourite one): "*" next to type, please.

Comment 36

[Cameron McCormack (:heycam)]

Comment on attachment 8682462 [details] [diff] [review]
patch 11 - Add missing #includes in preparation for nsRuleWalker.h #include change in following patch

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

::: layout/style/nsCSSRuleProcessor.h
@@ +23,5 @@
>  #include "nsIMediaList.h"
>  #include "nsIStyleRuleProcessor.h"
>  #include "nsRuleWalker.h"
>  #include "nsTArray.h"
> +#include "nsCSSPseudoElements.h"

Please keep the #includes sorted, since we currently are.

Comment 37

[Cameron McCormack (:heycam)]

Comment on attachment 8682463 [details] [diff] [review]
patch 12 - Use the css::Declaration instead of the css::StyleRule as the matching rule

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

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +2139,1 @@
>  CreateStyleRule(nsINode* aNode,

Maybe rename this and the other functions, s/StyleRule/Declaration/?  (Unless you want to interpret "StyleRule" in the function name as meaning "an nsIStyleRule implementation" and not "a StyleRule object'.)

@@ +2244,5 @@
>                      nsAString& aOutUsedFont,
>                      ErrorResult& error)
>  {
>    bool fontParsedSuccessfully = false;
> +  RefPtr<css::Declaration> rule =

Maybe rename rule to decl(aration)?  We are using it for its nsIStyleRule implementation, but we also interact with it as a Declaration (the GetValue call).

Comment 38

[Cameron McCormack (:heycam)]

Comment on attachment 8682465 [details] [diff] [review]
patch 14 - Pass mozilla::css::Rule instead of nsIStyleRule to nsIDocument/nsIDocumentObserver style rule methods

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

Does this resolve bug 980560 then?

> TODO: Given the amount that these parameters are used (not at all),
> perhaps we should have a followup on removing them and simplifying these
> notifications?

Maybe; we can always add them back if someone wants to start caring about the specific rules.

Comment 39

[Cameron McCormack (:heycam)]

Comment on attachment 8682469 [details] [diff] [review]
patch 18 - Eliminate StyleRule::RuleMatched and call Declaration::SetImmutable directly for style rules (like for @page and keyframe rules)

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

Maybe we should assert in nsRuleWalker::DoForward that if we can do_QueryObject a Declaration from aRule, that it has had SetImmutable called on it?

::: layout/style/Declaration.h
@@ +71,5 @@
>  // begins life in an invalid state which ends when InitializeEmpty or
>  // CompressFrom is called upon it.  After that, it can be attached to
>  // exactly one style rule, and will be destroyed when that style rule
> +// is destroyed.  A declaration becomes immutable (via a SetImmutable)
> +// call when it is matched (put in the rule tree); after that, it must

"call" should be in the parens.

Comment 40

[Cameron McCormack (:heycam)]

One thing I kind of don't like is how Declaration implements nsIStyleRule to expose its non-!important properties, but for !important properties we need to get a separate nsIStyleRule object.

I wonder if it would make things clearer if we made Declaration privately inherit nsIStyleRule, and added a GetNormalStyleData() method on Declaration that just returns itself (as an nsIStyleRule)?  That way it would be obvious at call sites that we're doing something with the non-!important data of the Declaration.

Comment 41

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

(In reply to Cameron McCormack (:heycam) from comment #31)
> Comment on attachment 8682454 [details] [diff] [review]
> patch 3 - Make css::Declaration implement nsIStyleRule
> > (Yes, implementing QueryInterface for a CID is ugly, but it's the same
> > thing StyleRule does.  Unfortunately now we'll need to have it in both
> > places.)
> 
> FWIW it looks like we only do_QueryObject to a StyleRule in DEBUG code.  Not
> sure if it's worth making the CID/QI stuff #ifdef DEBUG.  (Maybe not.)

(In reply to Cameron McCormack (:heycam) from comment #38)
> Comment on attachment 8682465 [details] [diff] [review]
> patch 14 - Pass mozilla::css::Rule instead of nsIStyleRule to
> nsIDocument/nsIDocumentObserver style rule methods
> 
> Does this resolve bug 980560 then?

Looks like it does.

(In reply to Cameron McCormack (:heycam) from comment #39)
> Comment on attachment 8682469 [details] [diff] [review]
> patch 18 - Eliminate StyleRule::RuleMatched and call
> Declaration::SetImmutable directly for style rules (like for @page and
> keyframe rules)
> 
> Maybe we should assert in nsRuleWalker::DoForward that if we can
> do_QueryObject a Declaration from aRule, that it has had SetImmutable called
> on it?

Good idea, assuming I can find a decent way to do it.

(In reply to Cameron McCormack (:heycam) from comment #40)
> One thing I kind of don't like is how Declaration implements nsIStyleRule to
> expose its non-!important properties, but for !important properties we need
> to get a separate nsIStyleRule object.
> 
> I wonder if it would make things clearer if we made Declaration privately
> inherit nsIStyleRule, and added a GetNormalStyleData() method on Declaration
> that just returns itself (as an nsIStyleRule)?  That way it would be obvious
> at call sites that we're doing something with the non-!important data of the
> Declaration.

I'm not a fan of this; there are a bunch of places where we need to do_QueryObject an nsIStyleRule back to a Declaration, and it's not really consistent with that.  QueryInterface is certainly intended to represent a concept of object identity (in which ability to go from interface A to B implies the ability to go from B to A), and I'd rather not mess with that here.

It's also the same concept we've lived with for the past decade and a half with StyleRule.

Comment 42

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

(In reply to Cameron McCormack (:heycam) from comment #31)
> Comment on attachment 8682454 [details] [diff] [review]
> patch 3 - Make css::Declaration implement nsIStyleRule
> 
> > (Yes, implementing QueryInterface for a CID is ugly, but it's the same
> > thing StyleRule does.  Unfortunately now we'll need to have it in both
> > places.)
> 
> FWIW it looks like we only do_QueryObject to a StyleRule in DEBUG code.  Not
> sure if it's worth making the CID/QI stuff #ifdef DEBUG.  (Maybe not.)

Oops, forgot to actually reply to this comment the last time.

It's not DEBUG code; the one place we need it is in inDOMUtils::GetCSSStyleRules, which is used by developer tools.  We do need the do_QueryObject, since otherwise we could risk returning a keyframe rule or an @page rule in certain edge cases... maybe... although it may well be provable that that couldn't happen, I'd rather not risk it.

Comment 43

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

(In reply to Cameron McCormack (:heycam) from comment #39)
> Comment on attachment 8682469 [details] [diff] [review]
> patch 18 - Eliminate StyleRule::RuleMatched and call
> Declaration::SetImmutable directly for style rules (like for @page and
> keyframe rules)
> 
> Maybe we should assert in nsRuleWalker::DoForward that if we can
> do_QueryObject a Declaration from aRule, that it has had SetImmutable called
> on it?

I added this, but to nsRuleNode::Transition instead, since it may as well be in the non-inline code in the .cpp file rather than in the inline code in the .h file.

Comment 44

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

I did a fuller try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4db76510299&group_state=expanded
which did show one problem (on Win7 opt web-platform-tests-4), but this try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa2df3084360&group_state=expanded
shows that the problem is related to bug 1221436 rather than this bug.

Comment 45

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/a377963bf9bbc0a9c658a513137d778b838d0b94
Bug 978833 patch 1 - Add mochitest for bug 978833.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/65c8c4d27598055a1255d24f96eafa1850dccba7
Bug 978833 patch 2 - Make css::Declaration reference-counted.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/d04634d822423fcde66c7dc5a120de9e5e2be167
Bug 978833 patch 3 - Make css::Declaration implement nsIStyleRule.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/4d51af920be6db324ef06f980bc7ba7520f7ea55
Bug 978833 patch 4 - Add pointer back from css::Declaration to css::Rule.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/3b535acc6b79600f26f60aaf6ab724573fb46c88
Bug 978833 patch 5 - Rename ImportantRule to ImportantStyleData.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/e62f0b7f0a02d3d6b7be90cef29075a56894738d
Bug 978833 patch 6 - Move ImportantStyleData from StyleRule.{h,cpp} to Declaration.{h,cpp}  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/4b633979383a8174c450ed7f77c0f06b6095f404
Bug 978833 patch 7 - Fuse allocation of ImportantStyleData with Declaration.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/0adcd9f3fac0358d859575727b3febeaf420c512
Bug 978833 patch 8 - Call SetImmutable for declarations of @page and keyframe rules.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a1bf2c602114a72dfd99a6aeb737f3cb116076b
Bug 978833 patch 9 - Move keyframe !important data assertion to where it will continue to happen.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/da3bf914effe192549fc81b44363fa9ef36a0e69
Bug 978833 patch 10 - Raise consistent exceptions so that the exception doesn't depend on rule destruction timing.  r=bzbarsky

https://hg.mozilla.org/integration/mozilla-inbound/rev/e69922893211b78e5593fbd1c795e66248a357a1
Bug 978833 patch 11 - Add missing #includes in preparation for nsRuleWalker.h #include change in following patch.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/f340cdf67edb870645ddf499da690e60eff7f73f
Bug 978833 patch 12 - Use the css::Declaration instead of the css::StyleRule as the matching rule.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2470877279bdaeed2128a527b81183a3d71bbd
Bug 978833 patch 13 - Remove important rule creation from css::StyleRule.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a8bb99e8c87d317306213d6ac600e32266edc58
Bug 978833 patch 14 - Pass mozilla::css::Rule instead of nsIStyleRule to nsIDocument/nsIDocumentObserver style rule methods.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed25ca545c1f7fe788c10c8d1a29feccb86e2dad
Bug 978833 patch 15 - Make css::Rule no longer inherit from nsIStyleRule.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/888d7ce53c3e032f6c4f6a70675a8775d710325d
Bug 978833 patch 16 - Always call Declaration::SetImmutable when we match a rule.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/6882c096a68365f6e4e3ae49a19541e68b96ff84
Bug 978833 patch 17 - Remove Rule::mWasMatched.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ab5ed0dd72f293f4d5ec9e16df136f48f9db4aa
Bug 978833 patch 18 - Eliminate StyleRule::RuleMatched and call Declaration::SetImmutable directly for style rules (like for @page and keyframe rules).  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf480f83f25d7bb46d1e174d4c15320d55b8eb1c
Bug 978833 patch 19 - Fix ResolvedStyleCache to use Declaration rather than nsCSSKeyframeRule as keys.  r=heycam

Comment 46

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0803928b5f1ccf5a7aa0dc46aa6f2be6bd02ca2
Bug 978833 patch 20 - Rev IIDs that I should have revised in Bug 978833 patch 14.

Comment 47

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a377963bf9bb
https://hg.mozilla.org/mozilla-central/rev/65c8c4d27598
https://hg.mozilla.org/mozilla-central/rev/d04634d82242
https://hg.mozilla.org/mozilla-central/rev/4d51af920be6
https://hg.mozilla.org/mozilla-central/rev/3b535acc6b79
https://hg.mozilla.org/mozilla-central/rev/e62f0b7f0a02
https://hg.mozilla.org/mozilla-central/rev/4b633979383a
https://hg.mozilla.org/mozilla-central/rev/0adcd9f3fac0
https://hg.mozilla.org/mozilla-central/rev/3a1bf2c60211
https://hg.mozilla.org/mozilla-central/rev/da3bf914effe
https://hg.mozilla.org/mozilla-central/rev/e69922893211
https://hg.mozilla.org/mozilla-central/rev/f340cdf67edb
https://hg.mozilla.org/mozilla-central/rev/9f2470877279
https://hg.mozilla.org/mozilla-central/rev/5a8bb99e8c87
https://hg.mozilla.org/mozilla-central/rev/ed25ca545c1f
https://hg.mozilla.org/mozilla-central/rev/888d7ce53c3e
https://hg.mozilla.org/mozilla-central/rev/6882c096a683
https://hg.mozilla.org/mozilla-central/rev/0ab5ed0dd72f
https://hg.mozilla.org/mozilla-central/rev/cf480f83f25d
https://hg.mozilla.org/mozilla-central/rev/f0803928b5f1