Last Comment Bug 696180 - Add support for inherited properties to the rule view.
: Add support for inherited properties to the rule view.
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools (show other bugs)
: 9 Branch
: x86 Mac OS X
: P2 normal (vote)
: Firefox 10
Assigned To: Dave Camp (:dcamp)
:
:
Mentors:
Depends on: 599592 693887
Blocks:
  Show dependency treegraph
 
Reported: 2011-10-20 12:41 PDT by Dave Camp (:dcamp)
Modified: 2012-08-29 14:48 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Inherited rules in the rule view 1 (16.16 KB, patch)
2011-10-31 16:39 PDT, Dave Camp (:dcamp)
rcampbell: review+
Details | Diff | Splinter Review
inIDOMUtils::isInheritedProperty (1.89 KB, patch)
2011-11-01 22:03 PDT, Dave Camp (:dcamp)
no flags Details | Diff | Splinter Review
v2 (9.90 KB, patch)
2011-11-04 17:35 PDT, Dave Camp (:dcamp)
rcampbell: review+
Details | Diff | Splinter Review

Description Dave Camp (:dcamp) 2011-10-20 12:41:06 PDT

    
Comment 1 Michael Ratcliffe [:miker] [:mratcliffe] 2011-10-24 01:10:29 PDT
Is this for the style inspector?
Comment 2 Rob Campbell [:rc] (:robcee) 2011-10-24 09:15:55 PDT
(In reply to Michael Ratcliffe from comment #1)
> Is this for the style inspector?

nope!
Comment 3 Dave Camp (:dcamp) 2011-10-31 16:39:23 PDT
Created attachment 570883 [details] [diff] [review]
Inherited rules in the rule view 1
Comment 4 Rob Campbell [:rc] (:robcee) 2011-11-01 20:55:03 PDT
Comment on attachment 570883 [details] [diff] [review]
Inherited rules in the rule view 1

+ * TODO: It wouldn't be too hard to expose inherited property
+ * information from the style system, that would be significantly more
+ * maintainable.
+ */
+CssLogic.INHERITED_PROPERTIES = {

this may be a dumb question, but where did you go for this list? CSS spec? Code? Maybe a link to your source in the comment would be helpful for the next person.

I hear your comment about maintainability and wonder how closely we'll have to watch for new inherited properties cropping up in the style system.

+  "-x-system-font" : true,

nit: only line with a space before the colon.

Do we need the : true parts? Wouldn't this also work as an array of strings instead of the object? I don't care either way, but it's a lot of extra typing your way. :)

Algorithm looks nice and the code is pretty.
Comment 5 Dave Camp (:dcamp) 2011-11-01 21:15:13 PDT
(In reply to Rob Campbell [:rc] (robcee) from comment #4)
> Comment on attachment 570883 [details] [diff] [review] [diff] [details] [review]
> Inherited rules in the rule view 1
> 
> + * TODO: It wouldn't be too hard to expose inherited property
> + * information from the style system, that would be significantly more
> + * maintainable.
> + */
> +CssLogic.INHERITED_PROPERTIES = {
> 
> this may be a dumb question, but where did you go for this list? CSS spec?
> Code? Maybe a link to your source in the comment would be helpful for the
> next person.

Yeah, I'll add that comment - it was a careful reading of nsCssPropList.h.

FWIW, that "careful reading of nsCssPropList.h" is essentially what we'll need to do in code to support checking this at runtime.  nsCssPropList.h can be included with different definitions for the CSS_PROP macro - we can include it in a method that builds a hashtable to check against.

Will file a followup for that.  We should do it sooner rather than later.

> 
> I hear your comment about maintainability and wonder how closely we'll have
> to watch for new inherited properties cropping up in the style system.

We'll have to watch very closely unless we implement the platform solution.

> Do we need the : true parts? Wouldn't this also work as an array of strings
> instead of the object? I don't care either way, but it's a lot of extra
> typing your way. :)

Yeah, it was a bit of extra typing.  Just preferred a hash lookup to an array lookup.
Comment 6 Dave Camp (:dcamp) 2011-11-01 22:00:44 PDT
(In reply to Dave Camp (:dcamp) from comment #5)

> FWIW, that "careful reading of nsCssPropList.h" is essentially what we'll
> need to do in code to support checking this at runtime.  nsCssPropList.h can
> be included with different definitions for the CSS_PROP macro - we can
> include it in a method that builds a hashtable to check against.

Ignore that, it's actually much simpler than that if we just want to check if a property is inherited.  Patch incoming.
Comment 7 Dave Camp (:dcamp) 2011-11-01 22:03:09 PDT
Created attachment 571241 [details] [diff] [review]
inIDOMUtils::isInheritedProperty

Still needs tests, comments, and a uuid bump.  But dbaron, is this a reasonable way to ask the style system if a given css property is inherited by default?
Comment 8 Dave Camp (:dcamp) 2011-11-04 12:29:29 PDT
Comment on attachment 571241 [details] [diff] [review]
inIDOMUtils::isInheritedProperty

Moved this patch over to bug 699592, cancelling feedback request.
Comment 9 Dave Camp (:dcamp) 2011-11-04 17:35:17 PDT
Created attachment 572140 [details] [diff] [review]
v2

Patch updated to use new inIDOMUtils::isInheritedProperty().
Comment 10 Rob Campbell [:rc] (:robcee) 2011-11-05 07:56:51 PDT
Comment on attachment 572140 [details] [diff] [review]
v2

-    }
+    var domRules = this.domUtils.getCSSStyleRules(aElement);

can this not throw anymore or do we just want the exception to bubble? 

       // XXX: non-style rules.

do we still need the XXX?

solid.
Comment 11 Dave Camp (:dcamp) 2011-11-05 08:45:08 PDT
(In reply to Rob Campbell [:rc] (robcee) from comment #10)
> Comment on attachment 572140 [details] [diff] [review] [diff] [details] [review]
> v2
> 
> -    }
> +    var domRules = this.domUtils.getCSSStyleRules(aElement);
> 
> can this not throw anymore or do we just want the exception to bubble? 

http://mxr.mozilla.org/mozilla-central/source/layout/inspector/src/inDOMUtils.cpp#187

Only exceptional returns there are invalid arguments and OOM, don't think we want to catch either of those.

> 
>        // XXX: non-style rules.
> 
> do we still need the XXX?

We have a bug on file and it's not clear that the fix belongs near that comment, so I'll pull the comment for now.


> solid.
Comment 13 Rob Campbell [:rc] (:robcee) 2011-11-06 04:51:20 PST
https://hg.mozilla.org/mozilla-central/rev/3246d9846071
Comment 14 :Ms2ger (⌚ UTC+1/+2) 2011-11-06 13:44:27 PST
Comment on attachment 572140 [details] [diff] [review]
v2

>--- a/browser/devtools/styleinspector/CssRuleView.jsm
>+++ b/browser/devtools/styleinspector/CssRuleView.jsm
>+    let element = this.element;
>+    do {
>+      this._addElementRules(element);
>+    } while ((element = element.parentNode) &&

Node.parentElement landed, no?
Comment 15 Sebastian Zartner [:sebo] 2012-08-29 14:48:41 PDT
isInheritedProperty should be documented at https://developer.mozilla.org/en-US/docs/XPCOM_Interface_Reference/inIDOMUtils.

Sebastian

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