886039 - Port the computed view to the styles actor

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Comment 1

[Dave Camp (:dcamp)]

Attachment: computed-view-remote.diff

Comment 2

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

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.

Comment 3

[Joe Walker [:jwalker]]

Chrome's console functions break if they're not bound to this. But ours don't.

Comment 4

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

(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?

Comment 5

[Dave Camp (:dcamp)]

(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.

Comment 6

[Dave Camp (:dcamp)]

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

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/788a28f4b984