The default bug view has changed. See this FAQ.

Move some style rule functions from nsIContent to Element

RESOLVED FIXED in mozilla14

Status

()

Core
DOM
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Ms2ger, Assigned: Ms2ger)

Tracking

(Depends on: 2 bugs)

Trunk
mozilla14
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
Created attachment 613090 [details] [diff] [review]
Patch v1

When calling these functions, we already have an Element*, and this allows removing some pointless implementations in nsGenericDOMDataNode.
Attachment #613090 - Flags: review?(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?
Attachment #613090 - Flags: superreview?(bzbarsky)
Attachment #613090 - Flags: review?(mounir)
Attachment #613090 - Flags: review+
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.
Attachment #613090 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Updated

5 years ago
Depends on: 744157
(Assignee)

Comment 3

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

https://hg.mozilla.org/mozilla-central/rev/1e337bbbea58
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla14
Could you file a followup bug on that?  The slots thing should not require nsGenericElement.
(Assignee)

Updated

5 years ago
Depends on: 745551
(Assignee)

Updated

5 years ago
Depends on: 745552
You need to log in before you can comment on or make changes to this bug.