Closed Bug 886039 Opened 8 years ago Closed 8 years ago

Port the computed view to the styles actor

Categories

(DevTools :: Inspector, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 26

People

(Reporter: dcamp, Assigned: dcamp)

References

(Depends on 1 open bug)

Details

Attachments

(1 file)

No description provided.
Depends on: 886038
Attachment #788329 - Flags: review?(mratcliffe)
Comment on attachment 788329 [details] [diff] [review]
computed-view-remote.diff

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

+1 to a lot of the refactoring, it really needed doing.

Just one nit, why use .then(null, (err) => console.error(err)) instead of just .then(null, console.error)? Maybe you are upping your fat arrow score, surely that is cheating... I don't really care, do what you want.

r+

::: browser/devtools/styleinspector/computed-view.js
@@ +360,5 @@
> +        }
> +      });
> +      this._refreshProcess.schedule();
> +      return deferred.promise;
> +    }).then(null, (err) => console.error(err));

Why not just .then(null, console.error) ?

::: browser/devtools/styleinspector/test/browser_bug683672.js
@@ +68,4 @@
>  
> +    is(propertyView.hasMatchedSelectors, true,
> +        "hasMatchedSelectors returns true");
> +  }).then(null, (err) => console.error(err));

Same again, maybe I am missing something here?

::: browser/devtools/styleinspector/test/browser_bug722196_property_view_media_queries.js
@@ +76,5 @@
> +    is(propertyView.hasMatchedSelectors, true,
> +        "hasMatchedSelectors returns true");
> +
> +    finishUp();
> +  }).then(null, (err) => console.error(err));

/me stares vacantly into space... nope, still don't get it.

::: browser/devtools/styleinspector/test/browser_styleinspector_bug_672744_search_filter.js
@@ +36,5 @@
> +  inspector.once("inspector-updated", () => {
> +    is(span, computedView.viewedElement.rawNode(),
> +      "style inspector node matches the selected node");
> +    SI_toggleDefaultStyles();
> +  }).then(null, (err) => console.error(err));

/me is now pondering the meaning of life, the universe and everything.
Attachment #788329 - Flags: review?(mratcliffe) → review+
Chrome's console functions break if they're not bound to this. But ours don't.
(In reply to Joe Walker [:jwalker] from comment #3)
> Chrome's console functions break if they're not bound to this. But ours
> don't.

So the function isn't needed?
(In reply to Joe Walker [:jwalker] from comment #3)
> Chrome's console functions break if they're not bound to this. But ours
> don't.

You're right, for our content console object.  Our Console.jsm object does break.

But I'd rather fix that and clean this patch up, so I'll do that.
https://hg.mozilla.org/integration/fx-team/rev/788a28f4b984
Assignee: nobody → dcamp
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/788a28f4b984
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 26
Depends on: 982619
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.