Closed Bug 1221436 Opened 4 years ago Closed 4 years ago

stop replacing css::StyleRule objects when they have dynamic changes

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(7 files)

5.18 KB, patch
heycam
: review+
Details | Diff | Splinter Review
60.51 KB, patch
heycam
: review+
Details | Diff | Splinter Review
4.31 KB, patch
heycam
: review+
Details | Diff | Splinter Review
4.69 KB, patch
heycam
: review+
Details | Diff | Splinter Review
6.72 KB, patch
heycam
: review+
Details | Diff | Splinter Review
2.48 KB, patch
heycam
: review+
Details | Diff | Splinter Review
5.42 KB, patch
heycam
: review+
Details | Diff | Splinter Review
In bug 978833 I'm changing the object that implements nsIStyleRule for CSS rules from css::StyleRule to css::Declaration.  This is the object whose pointer identity needs to change if the style data it represents change.

As a followup to that (although I was originally working on doing it in the same bug, but it got a bit complicated), I'd like to stop replacing css::StyleRule objects when the data change dynamically.

The difficult part to that is that we need to change what we store for style attributes from the css::StyleRule to the css::Declaration (since otherwise dynamic modification of style attributes would break when we were using the same css::StyleRule for the style attribute of multiple elements that started off with the same string), which in turn requires that we have a new way to get to the HTMLCSSStyleSheet object in order to use the same css::Declaration object for the same strings.

(This work, in turn, might let us merge DOMCSSStyleRule into css::StyleRule, which I think would fix the issue that was requiring a third base class for the conversion of CSS OM to new bindings in bug 898764 / bug 851892.)
This is needed for patch 2, since we need a replacement for the
mechanism to get from rules to the style attribute sheet
(nsHTMLCSSStyleSheet) that works instead for declarations rather than
rules.

This pointer will be merged with the owning rule pointer in patch 4, but
we can't do that until after patch 2.
Attachment #8683116 - Flags: review?(cam)
Now that Declaration implements nsIStyleRule, we don't need the memory
overhead of storing a StyleRule object for style attributes.

We also need to change this prior to patch 5, because the changes in
patch 5 that will allow rules to change (but declarations not) would
otherwise break due to style attribute object merging done by
nsAttrValue::ParseStyleAttribute.
Attachment #8683117 - Flags: review?(cam)
The pointer to nsHTMLCSSStyleSheet was added in patch 1 (which was
needed before patch 2), but it was only patch 2 that created the
invariant that we'd only have one pointer or the other, and never both.
Thus this needs to be done separately, after patch 1.
Attachment #8683119 - Flags: review?(cam)
(I think the change DOMCSSDeclarationImpl::GetCSSDeclaration fixes a
purely theoretical bug that would happen if it were possible to get
ahold of a CSS rule without calling either
CSSStyleSheet::EnsureUniqueInner (which is called by
CSSRuleListImpl::IndexedGetter) or Declaration::SetImmutable (which is
called by rule matching, including that triggered by
inIDomUtils::GetCSSStyleRules).  If that were possible, then mutating
such a declaration would mutate the shared Declaration, and then call
WillDirty which would clone the now-mutated Declaration, leading to the
mutation applying to both sheets that shared it rather than just the one
that should have been modified.)

This is the simplification allowed by bug 978833 patch 11 (and part of
what we would otherwise have needed to duplicate in @page and keyframe
rules; we'd also have needed to duplicate the object split between the
internal object and the DOM-exposed object, but for style rules that's
probably worth keeping for the memory savings).
Attachment #8683120 - Flags: review?(cam)
This is no longer used, thanks to patch 5.
Attachment #8683122 - Flags: review?(cam)
This is no longer used, thanks to patch 5.
Attachment #8683123 - Flags: review?(cam)
Attachment #8683116 - Flags: review?(cam) → review+
Comment on attachment 8683117 [details] [diff] [review]
patch 2 - For style attributes, only store a css::Declaration instead of a css::StyleRule

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

::: dom/base/FragmentOrElement.h
@@ +285,5 @@
>       */
>      nsCOMPtr<nsICSSDeclaration> mSMILOverrideStyle;
>  
>      /**
>       * Holds any SMIL override style rules for this element.

s/rules/declaration/?

::: dom/base/nsAttrValueInlines.h
@@ +19,5 @@
>    typedef nsAttrValue::ValueType ValueType;
>  
>    ValueType mType;
>    // mStringBits points to either nsIAtom* or nsStringBuffer* and is used when
> +  // mType isn't mCSSDeclaration.

eCSSDeclaration

::: dom/base/nsTreeSanitizer.cpp
@@ +1066,5 @@
>  }
>  
>  bool
> +nsTreeSanitizer::SanitizeStyleDeclaration(mozilla::css::Declaration *aDeclaration,
> +                                          nsAutoString &aRuleText)

Nit: "*"s and "&"s.

::: dom/base/nsTreeSanitizer.h
@@ +153,5 @@
>       * Checks a style rule for the presence of the 'binding' CSS property and
>       * removes that property from the rule and reserializes in case the
>       * property was found.
>       *
>       * @param aRule The style rule to check

This needs updating.

@@ +158,5 @@
>       * @param aRuleText the serialized mutated rule if the method returns true
>       * @return true if the rule was modified and false otherwise
>       */
> +    bool SanitizeStyleDeclaration(mozilla::css::Declaration* aDeclaration,
> +                                  nsAutoString &aRuleText);

Nit: move the "&" while you're here.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +2138,5 @@
>  static already_AddRefed<Declaration>
>  CreateStyleRule(nsINode* aNode,
>    const nsCSSProperty aProp1, const nsAString& aValue1, bool* aChanged1,
>    const nsCSSProperty aProp2, const nsAString& aValue2, bool* aChanged2,
>    ErrorResult& error)

Remove the ErrorResult arg now that there's no error condition?

::: layout/style/nsCSSParser.cpp
@@ +1658,4 @@
>  
>    ReleaseScanner();
>  
>    // XXX check for low level errors

This comment doesn't add much and should probably just go.

::: layout/style/nsHTMLCSSStyleSheet.h
@@ +14,5 @@
>  #include "mozilla/MemoryReporting.h"
>  
>  #include "nsDataHashtable.h"
>  #include "nsIStyleRuleProcessor.h"
> +#include "nsCSSPseudoElements.h"

Nit: please keep the #includes sorted.
Attachment #8683117 - Flags: review?(cam) → review+
Comment on attachment 8683118 [details] [diff] [review]
patch 3 - Remove Rule::SetHTMLCSSStyleSheet and related code, now unused

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

Yay.
Attachment #8683118 - Flags: review?(cam) → review+
Comment on attachment 8683119 [details] [diff] [review]
patch 4 - Use the same pointer to store the owning rule and the nsHTMLCSSStyleSheet on css::Declaration, since we never need both

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

::: layout/style/Declaration.h
@@ +321,4 @@
>    }
>  
> +  Rule* GetOwningRule() {
> +    if (mContainer.mRaw & 0x1) {

Oh, the low-bit trick remains. :(

We could avoid this by looking up to the Rule and checking its type.  Not sure whether it's better than what we have here.  I guess this low-bit thing is a bit safer.
Attachment #8683119 - Flags: review?(cam) → review+
Comment on attachment 8683120 [details] [diff] [review]
patch 5 - Stop generating new style rules in DeclarationChanged, since we no longer need a new style rule for nsIStyleRule identity rules

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

::: layout/style/StyleRule.cpp
@@ +1166,5 @@
>    if (mRule) {
> +    if (aOperation != eOperation_Read) {
> +      RefPtr<CSSStyleSheet> sheet = mRule->GetStyleSheet();
> +      if (sheet) {
> +        sheet->WillDirty();

Normally I would expect WillDirty and DidDirty calls to happen in pairs, but here I think we will call WillDirty once, and then DidDirty zero or more times for a given rule's declaration.  Is that right?  if so, is it worth having differently named methods on CSSStyleSheet to call, which then do the same work that WillDirty/DidDirty do?

@@ +1217,4 @@
>    }
>  
>    if (owningDoc) {
> +    owningDoc->StyleRuleChanged(sheet, mRule, mRule);

Now that we'll never pass two different rules in, should we remove the last argument?  (That might impact addons though?)

::: layout/style/StyleRule.h
@@ +314,5 @@
>    nsCSSSelectorList* Selector() { return mSelector; }
>  
>    Declaration* GetDeclaration() const { return mDeclaration; }
>  
> +  void DeclarationChanged(Declaration* aDecl);

Should we just rename this to SetDeclaration?
Attachment #8683122 - Flags: review?(cam) → review+
Attachment #8683123 - Flags: review?(cam) → review+
This morning I had a try push that showed one web-platform-tests failure on Win7 opt from bug 978833 or this bug:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4db76510299&group_state=expanded
... which I then bisected to being this bug... and then further bisected to... just having disappeared completely:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0acf6c749b61&group_state=expanded
(In reply to Cameron McCormack (:heycam) from comment #14)
> Comment on attachment 8683120 [details] [diff] [review]
> patch 5 - Stop generating new style rules in DeclarationChanged, since we no
> longer need a new style rule for nsIStyleRule identity rules
> 
> Normally I would expect WillDirty and DidDirty calls to happen in pairs, but
> here I think we will call WillDirty once, and then DidDirty zero or more
> times for a given rule's declaration.  Is that right?  if so, is it worth
> having differently named methods on CSSStyleSheet to call, which then do the
> same work that WillDirty/DidDirty do?

I think we'll call DidDirty either zero or one time following a WillDirty.  I don't see how it would be called multiple times.  I think that's certainly OK given what the methods do, although I suppose I could document that WillDirty isn't necessarily followed by DidDirty.

I think moving the WillDirty call earlier (as this patch does) is safer; otherwise we'd call EnsureUniqueInner too late.  It just happens to be true that the inner is currently always already unique by the time we get to that point, so we've been ok.

I'd be ok with adding comments to WillDirty or renaming it to WillProbablyDirty or something like that.

> >    if (owningDoc) {
> > +    owningDoc->StyleRuleChanged(sheet, mRule, mRule);
> 
> Now that we'll never pass two different rules in, should we remove the last
> argument?  (That might impact addons though?)

I'd rather use a bigger hammer here and do bug 1221908.

> > +  void DeclarationChanged(Declaration* aDecl);
> 
> Should we just rename this to SetDeclaration?

Yeah, that seems reasonable.
(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #16)
> I'd be ok with adding comments to WillDirty or renaming it to
> WillProbablyDirty or something like that.

OK, just a comment will do then.
Attachment #8683120 - Flags: review?(cam) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/2894f200ed038736e06851432e01fbb1e3311bce
Bug 1221436 patch 1 - Have a pointer back from the css::Declaration for style attributes to the nsHTMLCSSStyleSheet.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/84c5d7320a5ccc49dff580d78486d29443e25259
Bug 1221436 patch 2 - For style attributes, only store a css::Declaration instead of a css::StyleRule.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/2638966229a846861165805fe550fa332398d895
Bug 1221436 patch 3 - Remove Rule::SetHTMLCSSStyleSheet and related code, now unused.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/b09af10d01282ddbc27a59f0417d62357ff69c84
Bug 1221436 patch 4 - Use the same pointer to store the owning rule and the nsHTMLCSSStyleSheet on css::Declaration, since we never need both.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ffbb9175ac62505f0af2c0365e9325226d7bcc0
Bug 1221436 patch 5 - Stop generating new style rules in DeclarationChanged (and rename it to SetDeclaration), since we no longer need a new style rule for nsIStyleRule identity rules.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/42d1516bf147f50e1ba61602953687cb180945cf
Bug 1221436 patch 6 - Remove mechanism for replacing style rules.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/d49ec59732d62c8973a948491d24c5dd6e2f1d12
Bug 1221436 patch 7 - Remove ReplaceStyleRule/ReplaceRuleInGroup mechanism.  r=heycam
You need to log in before you can comment on or make changes to this bug.