Closed Bug 696180 Opened 10 years ago Closed 10 years ago

Add support for inherited properties to the rule view.

Categories

(DevTools :: General, defect, P2)

9 Branch
x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 10

People

(Reporter: dcamp, Assigned: dcamp)

References

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 file, 2 obsolete files)

No description provided.
Whiteboard: [styleinspector]
Is this for the style inspector?
Whiteboard: [styleinspector]
(In reply to Michael Ratcliffe from comment #1)
> Is this for the style inspector?

nope!
Assignee: nobody → dcamp
Status: NEW → ASSIGNED
Priority: -- → P2
Attachment #570883 - Flags: review?(rcampbell)
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.
Attachment #570883 - Flags: review?(rcampbell) → review+
(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.
(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.
Attached patch inIDOMUtils::isInheritedProperty (obsolete) — Splinter Review
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?
Attachment #571241 - Flags: feedback?(dbaron)
Comment on attachment 571241 [details] [diff] [review]
inIDOMUtils::isInheritedProperty

Moved this patch over to bug 699592, cancelling feedback request.
Attachment #571241 - Flags: feedback?(dbaron)
Attachment #571241 - Attachment is obsolete: true
Attached patch v2Splinter Review
Patch updated to use new inIDOMUtils::isInheritedProperty().
Attachment #570883 - Attachment is obsolete: true
Attachment #572140 - Flags: review?(rcampbell)
Depends on: 599592
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.
Attachment #572140 - Flags: review?(rcampbell) → review+
(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.
https://hg.mozilla.org/mozilla-central/rev/3246d9846071
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
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?
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.