886038 - Port the rule view to the styles actor

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Comment 1

[Dave Camp (:dcamp)]

Attachment: WIP 1

Comment 2

[Dave Camp (:dcamp)]

Attachment: v1

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 788326 [details] [diff] [review]
v1

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

r+ with the non-nits below addressed.

::: browser/devtools/styleinspector/rule-view.js
@@ +63,5 @@
> +    allowAuth: false
> +  });
> +  let docShell = getDocShell(frame);
> +  let eventTarget = docShell.chromeEventHandler;
> +  docShell.createAboutBlankContentViewer(Cc["@mozilla.org/nullprincipal;1"].createInstance(Ci.nsIPrincipal));

Nice

@@ +133,5 @@
>    // how their .style attribute reflects them as computed values.
> +  this.dummyElementPromise = createDummyDocument().then(document => {
> +    this.dummyElement = document.createElementNS(this.element.namespaceURI,
> +                                                 this.element.tagName);
> +    this.dummyElement.setAttribute("style", "background:green");

Is there a reason that you set the green background?

@@ +465,5 @@
>     *        Creation options.  See the Rule constructor for documentation.
>     */
>    matches: function Rule_matches(aOptions)
>    {
> +    return (this.style === (aOptions.rule));

We don't need any brackets here.

@@ +636,5 @@
>      // exposes another one.
> +    this.applyProperties(modifications);
> +  },
> +
> +  _parseCSSText: function Rule_parseProperties(aCssText)

CSS_LINE_RE matches an undefined value at the end of the result due to the ;? at the end of the regex (cssText always ends in a semicolon unless empty). This causes an extra iteration of the loop and an error that we never noticed.

We could be a little fancier here using the fixed regex and destructuring:
const CSS_LINE_RE = /(?:[^;\(]*(?:\([^\)]*?\))?[^;\(]*)*;/g; // Fixed regex

function _parseCSSText(aCssText) {
  let lines = aCssText.match(CSS_LINE_RE);
  let props = [];

  for (let line of lines) {
    let [, name, value, priority] = CSS_PROP_RE.exec(line);
    if (!name || !value) {
      continue;
    }

    props.push({
      name: name,
      value: value,
      priority: priority || ""
    });
  }
  return props;
}

@@ +1048,5 @@
> +    return this._populate().then(() => {
> +      this._elementStyle.onChanged = () => {
> +        this._changed();
> +      };
> +    }).then(null, (err) => console.error);

I'll leave you to do what you want with this.

::: browser/devtools/styleinspector/style-inspector.js
@@ +96,5 @@
> +      return;
> +    }
> +    this.view.setPageStyle(this.inspector.pageStyle);
> +    dump("HIGHLIGHTING: " + this.inspector.selection.nodeFront + "\n");
> +    console.trace();

Do we really want a stack trace every time something is selected?

::: browser/devtools/styleinspector/test/browser_ruleview_734259_style_editor_link.js
@@ +67,3 @@
>      is(inspector.selection.node, div, "selection matches the div element");
>      testInlineStyle();
> +  }).then(null, (err) => console.error);

I'll leave you to do what you want with this.

::: browser/devtools/styleinspector/test/browser_ruleview_inherit.js
@@ +43,3 @@
>  
> +    emptyInherit();
> +  }).then(null, (err) => console.error(err));

And again.

@@ +69,3 @@
>  
> +    elementStyleInherit();
> +  }).then(null, (err) => console.error(err));

And again.

@@ +93,3 @@
>  
> +    finishTest();
> +  }).then(null, (err) => console.error(err));

Okay, okay... i'll stop complaining about this one.

::: browser/devtools/styleinspector/test/browser_ruleview_update.js
@@ +45,5 @@
>    // Change the id and refresh.
>    testElement.setAttribute("id", "differentid");
> +  promiseDone(ruleView.nodeChanged().then(() => {
> +    let selectors = ruleView.doc.querySelectorAll(".ruleview-selector");
> +    is(selectors.length, 2, "Three rules visible.");

"Two rules visible."

Comment 4

[Dave Camp (:dcamp)]

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

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2b3f75e2d346