743438 - Move some style rule functions from nsIContent to Element

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Attachment: Patch v1

When calling these functions, we already have an Element*, and this allows removing some pointless implementations in nsGenericDOMDataNode.

Comment 1

[Mounir Lamouri (:mounir)]

Comment on attachment 613090 [details] [diff] [review]
Patch v1

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

Could you update the IID of nsIContent and Element?
And, would that have been possible to make those methods not virtual and directly implement them in Element:: ?

r=me with those comments addressed (plus the nits)

However, I think this might need a sr given that you are moving stuff from a very commonly used interface to another. Boris, could you sr those method move?

::: content/base/public/Element.h
@@ +171,5 @@
>     */
>    void ClearStyleStateLocks();
>  
> +  /**
> +   * Get the inline style rule, if any, for this content node

nit: a dot at the end of the line.

@@ +176,5 @@
> +   */
> +  virtual css::StyleRule* GetInlineStyleRule() = 0;
> +
> +  /**
> +   * Set the inline style rule for this node.  This will send an

nit: there are two spaces after the dot here.

@@ +183,5 @@
> +  virtual nsresult SetInlineStyleRule(css::StyleRule* aStyleRule,
> +                                      bool aNotify) = 0;
> +
> +  /**
> +   * Get the SMIL override style rule for this content node.  If the rule

nit: ditto

@@ +190,5 @@
> +   */
> +  virtual css::StyleRule* GetSMILOverrideStyleRule() = 0;
> +
> +  /**
> +   * Set the SMIL override style rule for this node.  If aNotify is true, this

nit: ditto

@@ +213,5 @@
> +   *
> +   * Note: This method is analogous to the 'GetStyle' method in
> +   * nsGenericHTMLElement and nsStyledElement.
> +   *
> +   * TODO: All callers QI to nsICSSDeclaration.

Could you open a bug for that and add the bug number in the TODO line?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 613090 [details] [diff] [review]
Patch v1

>+   * Get the inline style rule, if any, for this content node

s/content node/element/

>+   * Set the inline style rule for this node.  This will send an

s/node/element/

>+  virtual nsresult SetInlineStyleRule(css::StyleRule* aStyleRule,
>+                                      bool aNotify) = 0;

I _think_ we could make this non-virtual and move it to styled element... But a followup for that is ok; it's not an obviously trivial change.

>+   * Get the SMIL override style rule for this content node.  If the rule

s/content node/element/

>+   * hasn't been created (or if this nsIContent object doesn't support SMIL

s/nsIContent/Element/.  But all elements support SMIL override style, so the parenthetical should just go away.  Furthermore, I believe this can be nonvirtual; just have to change the impl class from nsGenericElement to Element.

>+   * Set the SMIL override style rule for this node.  If aNotify is true, this

s/node/element/

>+   * method will notify the document's pres context, so that the style changes
>+   * will be noticed.
>+   */
>+  virtual nsresult SetSMILOverrideStyleRule(css::StyleRule* aStyleRule,

Again, I think this can be non-virtual.

>+   * Get the SMIL override style for this content node

s/content node/element/

>+  virtual nsIDOMCSSStyleDeclaration* GetSMILOverrideStyle() = 0;

And this can be nonvirtual.

sr=me with those nits.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Due to slots, none of those could actually become non-virtual.

https://hg.mozilla.org/mozilla-central/rev/1e337bbbea58

Comment 4

[Boris Zbarsky [:bzbarsky]]

Could you file a followup bug on that?  The slots thing should not require nsGenericElement.