Closed
Bug 886038
Opened 11 years ago
Closed 11 years ago
Port the rule view to the styles actor
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 26
People
(Reporter: dcamp, Assigned: dcamp)
References
Details
Attachments
(1 file, 1 obsolete file)
118.29 KB,
patch
|
miker
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•11 years ago
|
||
Assignee | ||
Comment 2•11 years ago
|
||
Attachment #787073 -
Attachment is obsolete: true
Attachment #788326 -
Flags: review?(mratcliffe)
Comment 3•11 years ago
|
||
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 | ||
Comment 4•11 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/2b3f75e2d346
Assignee: nobody → dcamp
Whiteboard: [fixed-in-fx-team]
Comment 5•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2b3f75e2d346
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Updated•11 years ago
|
OS: Mac OS X → All
Hardware: x86 → All
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•