943510 - Convert to Promise.jsm in the devtools framework

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P2)

Comment 1

[Brandon Benvie [:benvie]]

Attachment: promise-framework.patch

This patch converts devtools/toolbox to use Promise.jsm. It also fixes bugs that arose from this conversion in the inspector (but does not convert the inspector to Promise.jsm).

Comment 2

[Brandon Benvie [:benvie]]

https://tbpl.mozilla.org/?tree=Try&rev=1398502b0eaf

Comment 3

[Brandon Benvie [:benvie]]

Attachment: promise-framework.patch

Fix the test failures. https://tbpl.mozilla.org/?tree=Try&rev=f639943af8a0

Comment 4

[Brandon Benvie [:benvie]]

I suspect this is a platform issue with focus. browser_toolbox_raise.js is such a fragile test.

https://tbpl.mozilla.org/?tree=Try&rev=97a0cd333992

Comment 5

[Brandon Benvie [:benvie]]

I cannot reproduce this locally, and it's apparently spotty on the platforms tested. =(

Comment 6

[Brandon Benvie [:benvie]]

Attachment: promise-framework.patch

Rip out the changes to browser_toolbox_raise.js. Fix some new bugs introduced by the highlighter.

Comment 7

[Brandon Benvie [:benvie]]

Attachment: promise-framework.patch

https://tbpl.mozilla.org/?tree=Try&rev=df8b25e32f25

Comment 8

[Brandon Benvie [:benvie]]

Comment on attachment 8363805 [details] [diff] [review]
promise-framework.patch

Try is looking good.

* robcee - framework 
* pbrosset & bgrins - markupview, inspector, responsiveview, and styleinspector (you've both touched code/tests that are modified here so I wanted to make sure you both checked this over)

Comment 9

[Patrick Brosset <:pbro>]

Comment on attachment 8363805 [details] [diff] [review]
promise-framework.patch

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

Looks good to me.
Just a couple of questions/comments related to the markup-view highlight events.

::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js
@@ +57,5 @@
>      is(getHighlitNode(), doc.querySelector("h1"), "The highlighter highlights the right node");
>      return promise.resolve();
>    }
>  
>    function mouseLeaveMarkupView() {

Can you explain why you decided to introduce an event for this test? Was it intermittently failing on try with your patch?

@@ +59,5 @@
>    }
>  
>    function mouseLeaveMarkupView() {
>      let deferred = promise.defer();
> +    inspector.markup.once("highlight-hidden", deferred.resolve);

If you decide to change the event name as my comment on markup-view.js suggests, you'll need to change it here too.

::: browser/devtools/markupview/markup-view.js
@@ +200,5 @@
>      if (toolbox.isRemoteHighlightable) {
> +      return toolbox.initInspector().then(() => {
> +        return toolbox.highlighter.hideBoxModel();
> +      }).then(() => {
> +        this.emit("highlight-hidden");

Would make more sense to me if called "node-unhighlight" to be consistent with the other event, but up to you.

@@ +208,3 @@
>      // If not, no need to unhighlight as the older highlight method uses a
>      // setTimeout to hide itself
> +    return promise.resolve();

And, although it's only used in a test for now, I would also emit the event in this case. Again, just to make it consistent with the event emitted in |_showBoxModel|

::: browser/devtools/styleinspector/test/browser_bug946331_close_tooltip_on_new_selection.js
@@ +29,4 @@
>      inspector = aInspector;
>      ruleView = aRuleView;
> +    openView("computedview", aComputedView => {
> +      computedView = aComputedView;

Thanks for these "openView" refactorings! Looking good!

Comment 10

[Brandon Benvie [:benvie]]

Thanks for the review!

(In reply to Patrick Brosset [:pbrosset] from comment #9)
> ::: browser/devtools/inspector/test/browser_inspector_basic_highlighter.js
> @@ +57,5 @@
> >      is(getHighlitNode(), doc.querySelector("h1"), "The highlighter highlights the right node");
> >      return promise.resolve();
> >    }
> >  
> >    function mouseLeaveMarkupView() {
> 
> Can you explain why you decided to introduce an event for this test? Was it
> intermittently failing on try with your patch?

Yeah, it was actually a permanent failure [1]. The reason is that `toolbox.initInspector` now returns a Promise.jsm promise (which will resolve next tick at the soonest), so the executeSoon in mouseLeaveMarkupView is now too soon.

[1] https://tbpl.mozilla.org/?tree=Try&rev=9f27f915e4ea

> ::: browser/devtools/markupview/markup-view.js
> @@ +200,5 @@
> >      if (toolbox.isRemoteHighlightable) {
> > +      return toolbox.initInspector().then(() => {
> > +        return toolbox.highlighter.hideBoxModel();
> > +      }).then(() => {
> > +        this.emit("highlight-hidden");
> 
> Would make more sense to me if called "node-unhighlight" to be consistent
> with the other event, but up to you.

Yeah, I didn't really like the name either.

> @@ +208,3 @@
> >      // If not, no need to unhighlight as the older highlight method uses a
> >      // setTimeout to hide itself
> > +    return promise.resolve();
> 
> And, although it's only used in a test for now, I would also emit the event
> in this case. Again, just to make it consistent with the event emitted in
> |_showBoxModel|

Good point, I didn't think about that.

Comment 11

[Rob Campbell [:rc] (:robcee)]

Comment on attachment 8363805 [details] [diff] [review]
promise-framework.patch

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

framework changes look ok to me.

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8363805 [details] [diff] [review]
promise-framework.patch

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

I don't have anything to add beyond what Patrick has said

Comment 13

[Brandon Benvie [:benvie]]

Attachment: promise-framework.patch

Addresses Patrick's feedback. I noticed that closing the devtools after opening the style inspector causes some console spam; a last "hideBoxModel" message is trying to send after the connection is closed. Fortunately this is reproducible.

Comment 14

[Brandon Benvie [:benvie]]

Attachment: promise-framework.patch

Rebase, since the highlighter stuff moved to framework.js. I'm still seeing the issue where hideBoxModel happens after the connection is already closing. This causes windows to leak until shutdown in styleinspector/test/browser_ruleview_original_source_link.js.

Comment 15

[Brandon Benvie [:benvie]]

Attachment: promise-framework.patch

Fixes the failures locally.

https://tbpl.mozilla.org/?tree=Try&rev=81989aff7562

Comment 16

[Patrick Brosset <:pbro>]

(In reply to Brandon Benvie [:benvie] from comment #14)
> Created attachment 8382565 [details] [diff] [review]
> promise-framework.patch
> 
> Rebase, since the highlighter stuff moved to framework.js. I'm still seeing
> the issue where hideBoxModel happens after the connection is already
> closing. This causes windows to leak until shutdown in
> styleinspector/test/browser_ruleview_original_source_link.js.

Can you try removing:
inspector.selection.setNode(div);
and instead adding:
inspector.selection.setNode(div, "test");

The inspector selection triggers all sorts of server-side requests when it's assigned a new node.
One of them is the highlighter which will briefly draw the box-model outline so the user can see what's the new selection. Now, passing "test" as a second argument (the reason) will tell it not to highlight, and therefore the showBoxModel and hideBoxModel methods won't be called.

Except when actually testing the highlighter, we should always pass this "test" parameter.

Comment 17

[Brandon Benvie [:benvie]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #16)
> (In reply to Brandon Benvie [:benvie] from comment #14)
> > Created attachment 8382565 [details] [diff] [review]
> > promise-framework.patch
> > 
> > Rebase, since the highlighter stuff moved to framework.js. I'm still seeing
> > the issue where hideBoxModel happens after the connection is already
> > closing. This causes windows to leak until shutdown in
> > styleinspector/test/browser_ruleview_original_source_link.js.
> 
> Can you try removing:
> inspector.selection.setNode(div);
> and instead adding:
> inspector.selection.setNode(div, "test");
> 
> The inspector selection triggers all sorts of server-side requests when it's
> assigned a new node.
> One of them is the highlighter which will briefly draw the box-model outline
> so the user can see what's the new selection. Now, passing "test" as a
> second argument (the reason) will tell it not to highlight, and therefore
> the showBoxModel and hideBoxModel methods won't be called.
> 
> Except when actually testing the highlighter, we should always pass this
> "test" parameter.

I've made the change but that didn't fix the issue (when I removed the actual fix). The fix I made was changing ToolboxHighlighterUtils#unhilight to not use initInspector, rather just check for the existence of this.toolbox.hightlighter. Prior to the fix (i.e. as it currently is on m-c) we have to initialize the inspector just to hide the box model. This is unnecessary because we can assume if the highlighter exists then the inspector is already initialized.

It alsop introduces further complications during shutdown, because during shutdown we hide the box model, which causes the inspector to be shown. So in shutting down we attempt to initialize the inspector again, and this breaks when switching promise implementations due to it no longer being synchronous.

Comment 18

[Brandon Benvie [:benvie]]

Attachment: promise-framework.patch

Rebase. One final try. https://tbpl.mozilla.org/?tree=Try&rev=5e09965cb2e9

Comment 19

[:Paolo Amadini]

A simple tryserver run of the devtools framework patch looks green:

https://tbpl.mozilla.org/?tree=Try&rev=1989d88f02ee

I think this is ready to land, I'll go ahead unless there are reasons not to.

Comment 20

[Brandon Benvie [:benvie]]

There's a lot of oranges between this one and the last one I ran and I'm not entirely sure if this patch is causing them or not.

Comment 21

[:Paolo Amadini]

(In reply to Brandon Benvie [:benvie] from comment #20)
> There's a lot of oranges between this one and the last one I ran and I'm not
> entirely sure if this patch is causing them or not.

I'm not sure I understand what you're saying, the last run seems green except for known intermittent oranges. It is based on a recent mozilla-central revision with no other patches applied.

Comment 22

[Brandon Benvie [:benvie]]

Ah, nevermind then. This is good to land then.

Comment 23

[:Paolo Amadini]

Actually, this conflicted with another recent m-c landing in a non-obvious way.

Do you think you can take a look? I guess the patch will still work fine if adjusted in the correct way.

Comment 24

[Brandon Benvie [:benvie]]

Attachment: promise-framework.patch

Rebase. https://tbpl.mozilla.org/?tree=Try&rev=b8244a054cb6

Comment 25

[Brandon Benvie [:benvie]]

Attachment: promise-framework.patch

Comment 26

[Brandon Benvie [:benvie]]

Let's try that again... https://tbpl.mozilla.org/?tree=Try&rev=2b9c3b333716

Comment 27

[:Paolo Amadini]

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

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ff438bc0b151

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Brandon, when you get a minute, can you please request Aurora uplift on this patch? Thanks :)

Comment 30

[Brandon Benvie [:benvie]]

Comment on attachment 8392340 [details] [diff] [review]
promise-framework.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Switch from deprecated addon-sdk promises to Promise.jsm
User impact if declined: an epic uplift patch series by by RyanVM depends on this patch
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): none, it is standalone
String or IDL/UUID changes made by this patch:

Comment 31

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/76622a1dc44e