1435139 - No need to mutate declarationblock and notify if the property to be removed doesn't exist at all

Status: RESOLVED FIXED

(Core :: DOM: CSS Object Model, enhancement, P1)

Description

[Xidorn Quan [:xidorn] UTC+11]

We currently always make declaration block mutable and notify the owner about its being changed when we remove a property. However, it is wasteful when the property to be removed doesn't exist at all.

One free optimization we can do for this case is to check the old value in nsDOMCSSDeclaration::RemoveProperty and return directly rather than calling into RemovePropertyInternal when the old value is empty.

There are some other callsites of RemovePropertyInternal, but we cannot check property existence for free in those cases, so probably it's not worth it.

Making this block bug 1420423 because I see such wasteful removals reach the root element which can trigger a whole document restyle. (After fixing this, bug 1435122 would become the only thing triggers such restyle in my local profiling.)

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1435139 - Don't call into RemovePropertyInternal from RemoveProperty when the property doesn't exist.

Review commit: https://reviewboard.mozilla.org/r/217418/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217418/

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Looks like it doesn't work. Probably worth investigating why.

Comment 3

[Boris Zbarsky [:bzbarsky]]

What part doesn't work?  (Are there cases when properties can validly have empty string as value, btw?)

Comment 4

[Boris Zbarsky [:bzbarsky]]

It's also not 100% clear what the MutationObserver behavior for the "style" attribute should be in this case.... I really wish the CSSOM spec were sane.  :(

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> What part doesn't work?  (Are there cases when properties can validly have
> empty string as value, btw?)

Probably shorthands, from the try run.

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #3)
> What part doesn't work?  (Are there cases when properties can validly have
> empty string as value, btw?)

There are test failures on the try push (which you can see from the MozReview). I don't currently have idea on what's going on, but from a brief look of the message, I'm guessing it may be related to shorthands.

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> It's also not 100% clear what the MutationObserver behavior for the "style"
> attribute should be in this case.... I really wish the CSSOM spec were sane.
> :(

When you are removing a non-existing property, the style is not going to change at all, so I suppose MutationObserver shouldn't be triggered either...

But maybe this would make removeProperty and setProperty with empty value behave inconsistent?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Probably shorthands, from the try run.

Ah, indeed.

> so I suppose MutationObserver shouldn't be triggered either...

MutationObserver _is_ triggered if you do element.setAttribute("foo", element.getAttribute("foo")), fwiw.  So it _really_ depends on what the spec says in terms of when the attr set happens on CSSOM mutations...

We should definitely have setProperty(x, "") and removeProperty(prop) have identical behavior.

Comment 8

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #7)
> > so I suppose MutationObserver shouldn't be triggered either...
> 
> MutationObserver _is_ triggered if you do element.setAttribute("foo",
> element.getAttribute("foo")), fwiw.  So it _really_ depends on what the spec
> says in terms of when the attr set happens on CSSOM mutations...

So... CSSOM doesn't seem to be using the wording of DOM spec, but for removeProperty, it says:
* If property is a shorthand property, for each longhand property longhand that property maps to, invoke removeProperty() with longhand as argument.
* Otherwise, if property is a case-sensitive match for a property name of a CSS declaration in the declarations, remove that CSS declaration. 

So it sounds like there should be no mutation posted to anything in case the property doesn't exist. But the current spec sounds like it doesn't queue any mutation record anyway, which we probably should fix. I can probably try to write a PR for it.

And in terms of consistency, currently if the style attribute doesn't exist at all, we don't queue mutation record either. Maybe this change would make us more consistent to ourselves at least.

> We should definitely have setProperty(x, "") and removeProperty(prop) have
> identical behavior.

Yeah, the spec says:
* If value is the empty string, invoke removeProperty() with property as argument and terminate this algorithm. 

So I guess the change should probably be done in RemovePropertyInternal... Probably we can have declaration blocks return whether anything is really removed, and in case there isn't, we pass aNotify=false into SetInlineStyleDeclaration.

Comment 9

[Xidorn Quan [:xidorn] UTC+11]

> So it sounds like there should be no mutation posted to anything in case the property doesn't exist. But the current spec sounds like it doesn't queue any mutation record anyway, which we probably should fix. I can probably try to write a PR for it.

Submitted w3c/csswg-drafts#2268.

Comment 10

[Boris Zbarsky [:bzbarsky]]

> But the current spec sounds like it doesn't queue any mutation record anyway

If only things were so simple.  https://drafts.csswg.org/cssom/#the-elementcssinlinestyle-interface says:

  Mutating the declarations must set the style content attribute on the context object
  to the serialization of the declarations.

So technically it's maybe doing an attribute set for every shorthand in the longhand, with a MutationRecord for each one.  Pretty sure no one does that.

I filed https://github.com/w3c/csswg-drafts/issues/1559 back in June last year on the spec being completely nuts around this stuff...

I would be happy if no-op changes to the decl block did not queue mutation records, assuming those no-ops are easy to detect.  Certainly for the "remove a prop that's not there" case that would be good.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Hmmm, fixing the performance issue is probably easier than fixing the spec-conformance issue...

The mutation record is created in AttributeWillChange :/

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

Since solving the performance issue for bug 1420423 doesn't require fixing the mutation observer change, I'm going to ignore that part for now.

Comment 13

[Xidorn Quan [:xidorn] UTC+11]

For fixing the mutation observer behavior, I think what we probably should do is to
1. make nsMutationReceiver::AttributeWillChange put the new mutation record aside as a pending one, rather than appending it directly, then
2. add nsMutationReceiver::AttributeChanged which checks and eventually appends the pending mutation record.

This way, if an attribute isn't actually changed, we wouldn't queue any mutation record.

Comment 14

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1435139 - Don't call SetCSSDeclaration when removing non-existing property.

Review commit: https://reviewboard.mozilla.org/r/217786/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217786/

Comment 15

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8948304 [details]
Bug 1435139 - Don't call SetCSSDeclaration when removing non-existing property.

https://reviewboard.mozilla.org/r/217786/#review224716

OK.  So this will stop dispatching mutation observer notifications where we now dispatch them...

This is probably OK, assuming we sort out the mutation observer bit in the spec to match and write some web platform tests (in a followup). r=me

::: layout/style/Declaration.h:130
(Diff revision 1)
>    void ValueAppended(nsCSSPropertyID aProperty);
>  
>    void GetPropertyValue(const nsAString& aProperty, nsAString& aValue) const;
>    void GetPropertyValueByID(nsCSSPropertyID aPropID, nsAString& aValue) const;
>    bool GetPropertyIsImportant(const nsAString& aProperty) const;
> -  void RemoveProperty(const nsAString& aProperty);
> +  bool RemoveProperty(const nsAString& aProperty);

Please document the return value.

::: layout/style/Declaration.h:161
(Diff revision 1)
>    /**
>     * Removes a custom property declaration from this object.
>     *
>     * @param aName The variable name (i.e., without the "--" prefix).
>     */
> -  void RemoveVariable(const nsAString& aName);
> +  bool RemoveVariable(const nsAString& aName);

Again, document return value.

::: layout/style/DeclarationBlock.h:135
(Diff revision 1)
>    inline void GetPropertyValue(const nsAString& aProperty,
>                                 nsAString& aValue) const;
>    inline void GetPropertyValueByID(nsCSSPropertyID aPropID,
>                                     nsAString& aValue) const;
>    inline bool GetPropertyIsImportant(const nsAString& aProperty) const;
> -  inline void RemoveProperty(const nsAString& aProperty);
> +  inline bool RemoveProperty(const nsAString& aProperty);

Document return value.

::: layout/style/ServoDeclarationBlock.h:69
(Diff revision 1)
>    }
>  
>    void GetPropertyValue(const nsAString& aProperty, nsAString& aValue) const;
>    void GetPropertyValueByID(nsCSSPropertyID aPropID, nsAString& aValue) const;
>    bool GetPropertyIsImportant(const nsAString& aProperty) const;
> -  void RemoveProperty(const nsAString& aProperty);
> +  bool RemoveProperty(const nsAString& aProperty);

Document.

Comment 16

[Boris Zbarsky [:bzbarsky]]

I'm sorry for the lag here.  I thought I submitted this yesterday night, but mozreview managed to time itself out submitting the publish... :(  It's been extremely slow and laggy for me recently (multi-second waits on basic UI operations like changing the review dropdown to r+).  :(

Comment 17

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #15)
> Comment on attachment 8948304 [details]
> Bug 1435139 - Don't call SetCSSDeclaration when removing non-existing
> property.
> 
> https://reviewboard.mozilla.org/r/217786/#review224716
> 
> OK.  So this will stop dispatching mutation observer notifications where we
> now dispatch them...
> 
> This is probably OK, assuming we sort out the mutation observer bit in the
> spec to match and write some web platform tests (in a followup). r=me

No, it doesn't change that behavior at all. The record is currently done in GetDeclarationBlock rather than the setter. See my comment in bug 1197705.

Comment 18

[Xidorn Quan [:xidorn] UTC+11]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #16)
> I'm sorry for the lag here.  I thought I submitted this yesterday night, but
> mozreview managed to time itself out submitting the publish... :(  It's been
> extremely slow and laggy for me recently (multi-second waits on basic UI
> operations like changing the review dropdown to r+).  :(

There may be some bug in Firefox which is making MozReview slower. I think I observed once when MozReview got extremely slow and eventually irresponsible at all, while other people told me it works fine for them and it worked after I restarted the browser.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Could be.  But network panel is claiming things like "we sent the PUT and the server took 5s to respond"...

Comment 20

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Servo PR

Comment 21

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8948304 [details]
Bug 1435139 - Don't call SetCSSDeclaration when removing non-existing property.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217786/diff/1-2/

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/03d139086ace
Don't call SetCSSDeclaration when removing non-existing property. r=bz

Comment 23

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/03d139086ace