886037 - Add a styles actor for the style inspectors

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Comment 1

[Dave Camp (:dcamp)]

Attachment: styles-actor.diff

Comment 2

[Dave Camp (:dcamp)]

Comment on attachment 785176 [details] [diff] [review]
styles-actor.diff

This doesn't have tests of its own (although it does pass the rule view and style inspector tests when they are ported).  Switching from review to feedback while I get those tests written.

Comment 3

[Joe Walker [:jwalker]]

Comment on attachment 785176 [details] [diff] [review]
styles-actor.diff

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

Several console/dump calls to remove. Guess you're expecting that nit.

There are a few places where I wasn't sure of your intent - like why htmlComplete went away. I'm know you get a better review if I don't have to work that out for myself, and I'm fairly sure that there is a net benefit to Mozilla too.

Not finished reviewing yet. Not looked at styles.js, and only part way through computed-view.js. Will complete when home.

::: toolkit/devtools/styleinspector/css-logic.js
@@ +63,5 @@
>   * Special values for filter, in addition to an href these values can be used
>   */
>  CssLogic.FILTER = {
> +  ALL: "user", // show properties from all user style sheets
> +  USER: "user",

Would s/ALL/USER be hard? My gut is that we should do it.

Comment 4

[Dave Camp (:dcamp)]

Attachment: styles-actor.diff

This version has tests for `getApplied` and `modifyProperties` but still needs some tests for computed styles and matched selectors.

Comment 5

[Dave Camp (:dcamp)]

Attachment: styles-actor.diff

I think I'm happy with the tests now.

Previous patches had some porting gunk, that's been removed.

Comment 6

[Dave Camp (:dcamp)]

Sorry, missed your earlier comments:

The htmlComplete stuff didn't belong in this patch, sorry.  Let me know where else you need intent explained.

s/ALL/USER/ wouldn't be hard at all, but might be clearer in an immediate followup?  Or at least a separate patch on this bug, up to you.

Comment 7

[Joe Walker [:jwalker]]

Comment on attachment 786735 [details] [diff] [review]
styles-actor.diff

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

::: toolkit/devtools/server/actors/styles.js
@@ +139,5 @@
> +    this.cssLogic.highlight(node.rawNode);
> +    let computed = this.cssLogic._computedStyle;
> +
> +    Array.prototype.forEach.call(computed, name => {
> +      let matched = undefined;

Unused?

@@ +206,5 @@
> +   *     // The full form of any sheets referenced.
> +   *     sheets: [ <domsheet>, ... ]
> +   *  }
> +   */
> +  getMatchedSelectors: method(function(node, property, options) {

got here

@@ +219,5 @@
> +    let matched = [];
> +    let propInfo = this.cssLogic.getPropertyInfo(property);
> +    for (let selectorInfo of propInfo.matchedSelectors) {
> +      let cssRule = selectorInfo.selector._cssRule;
> +      let domRule = cssRule.sourceElement || cssRule._domRule;

We should have a clear picture of what _ means. I've generally used it to mean "don't use a _ property unless it's used as this._" i.e. the common definition of private. There is an alternative definition which is "If you use this and it breaks, that's your fault". I wonder if we shouldn't make _cssRule and _domRule (and others) 'public' if we commonly need them outside of the class in which they're defined.

@@ +255,5 @@
> +  }),
> +
> +  // Get a selector source for a CssSelectorInfor relative to a given
> +  // node.
> +  // XXX: This could probably be done in the frontend?

I'm not sure I'm 100% behind Mozilla style here, but aren't we supposed to raise a bug rather than XXXing.

@@ +317,5 @@
> +    }
> +
> +    let rules = new Set;
> +    let sheets = new Set;
> +    entries.forEach(entry => rules.add(entry.rule));

entries.forEach(entry => { rules.add(entry.rule); });

For great ES6 win.

@@ +667,5 @@
> +  /**
> +   * Return a new RuleModificationList for this node.
> +   */
> +  startModifyingProperties: function() {
> +  return new RuleModificationList(this);

indentation nit

Comment 8

[Dave Camp (:dcamp)]

(In reply to Joe Walker [:jwalker] from comment #7)
> Comment on attachment 786735 [details] [diff] [review]
> styles-actor.diff
> @@ +219,5 @@
> > +    let matched = [];
> > +    let propInfo = this.cssLogic.getPropertyInfo(property);
> > +    for (let selectorInfo of propInfo.matchedSelectors) {
> > +      let cssRule = selectorInfo.selector._cssRule;
> > +      let domRule = cssRule.sourceElement || cssRule._domRule;
> 
> We should have a clear picture of what _ means. I've generally used it to
> mean "don't use a _ property unless it's used as this._" i.e. the common
> definition of private. There is an alternative definition which is "If you
> use this and it breaks, that's your fault". I wonder if we shouldn't make
> _cssRule and _domRule (and others) 'public' if we commonly need them outside
> of the class in which they're defined.

Fair enough - I do need the raw rules, so I can make those public.

> @@ +255,5 @@
> > +  }),
> > +
> > +  // Get a selector source for a CssSelectorInfor relative to a given
> > +  // node.
> > +  // XXX: This could probably be done in the frontend?
> 
> I'm not sure I'm 100% behind Mozilla style here, but aren't we supposed to
> raise a bug rather than XXXing.

I'll just remove that comment for now.  I tried to do it in the frontend and it involved a lot more work.
 
> @@ +317,5 @@
> > +    }
> > +
> > +    let rules = new Set;
> > +    let sheets = new Set;
> > +    entries.forEach(entry => rules.add(entry.rule));
> 
> entries.forEach(entry => { rules.add(entry.rule); });
> 
> For great ES6 win.

While "function() expression" is not legal ES6, "() => expression" is, see http://wiki.ecmascript.org/doku.php?id=harmony:arrow_function_syntax

Comment 9

[Dave Camp (:dcamp)]

Attachment: csslogic-filter-rename.diff

Removes the old FILTER.ALL

Comment 10

[Dave Camp (:dcamp)]

Attachment: csslogic-public-domrules.diff

Makes public the CssLogic properties we use.

Comment 11

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/986f7c642b9f

Comment 12

[Dave Camp (:dcamp)]

Comment on attachment 787148 [details] [diff] [review]
csslogic-filter-rename.diff

Moved to a followup

Comment 13

[Dave Camp (:dcamp)]

Comment on attachment 787149 [details] [diff] [review]
csslogic-public-domrules.diff

Moved to a followup

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out for mochitest-other failures.
https://hg.mozilla.org/integration/fx-team/rev/678dd0508c82

https://tbpl.mozilla.org/php/getParsedLog.php?id=26355986&tree=Fx-Team

Comment 15

[Dave Camp (:dcamp)]

https://hg.mozilla.org/integration/fx-team/rev/b14e58f0e3ab

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/b14e58f0e3ab