Last Comment Bug 743438 - Move some style rule functions from nsIContent to Element
: Move some style rule functions from nsIContent to Element
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla14
Assigned To: :Ms2ger (⌚ UTC+1/+2)
:
Mentors:
Depends on: 745551 745552 744157
Blocks:
  Show dependency treegraph
 
Reported: 2012-04-07 03:29 PDT by :Ms2ger (⌚ UTC+1/+2)
Modified: 2012-04-15 10:01 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (12.03 KB, patch)
2012-04-07 03:29 PDT, :Ms2ger (⌚ UTC+1/+2)
mounir: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description :Ms2ger (⌚ UTC+1/+2) 2012-04-07 03:29:47 PDT
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.
Comment 1 Mounir Lamouri (:mounir) 2012-04-10 02:49:05 PDT
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 [:bz] (TPAC) 2012-04-10 10:42:11 PDT
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 (⌚ UTC+1/+2) 2012-04-15 06:41:02 PDT
Due to slots, none of those could actually become non-virtual.

https://hg.mozilla.org/mozilla-central/rev/1e337bbbea58
Comment 4 Boris Zbarsky [:bz] (TPAC) 2012-04-15 09:54:02 PDT
Could you file a followup bug on that?  The slots thing should not require nsGenericElement.

Note You need to log in before you can comment on or make changes to this bug.