Port the rule view to the styles actor

RESOLVED FIXED in Firefox 26

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dcamp, Assigned: dcamp)

Tracking

unspecified
Firefox 26
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v1
118.29 KB, patch
miker
: review+
Details | Diff | Splinter Review
Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 787073 [details] [diff] [review]
WIP 1
(Assignee)

Updated

5 years ago
Blocks: 886039
(Assignee)

Updated

5 years ago
Depends on: 886037
(Assignee)

Updated

5 years ago
Depends on: 903573
(Assignee)

Comment 2

5 years ago
Created attachment 788326 [details] [diff] [review]
v1
Attachment #787073 - Attachment is obsolete: true
Attachment #788326 - Flags: review?(mratcliffe)
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."
Attachment #788326 - Flags: review?(mratcliffe) → review+
(Assignee)

Updated

5 years ago
Depends on: 906375
(Assignee)

Comment 4

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/2b3f75e2d346
Assignee: nobody → dcamp
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/2b3f75e2d346
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
OS: Mac OS X → All
Hardware: x86 → All
You need to log in before you can comment on or make changes to this bug.