Closed Bug 943510 Opened 6 years ago Closed 6 years ago

Convert to Promise.jsm in the devtools framework

Categories

(DevTools :: Framework, defect, P2)

defect

Tracking

(firefox30 fixed, firefox31 fixed)

RESOLVED FIXED
Firefox 31
Tracking Status
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: bbenvie, Assigned: bbenvie)

References

Details

(Whiteboard: [qa-])

Attachments

(1 file, 9 obsolete files)

No description provided.
Attached patch promise-framework.patch (obsolete) — Splinter Review
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).
Assignee: nobody → bbenvie
Status: NEW → ASSIGNED
Attached patch promise-framework.patch (obsolete) — Splinter Review
Fix the test failures. https://tbpl.mozilla.org/?tree=Try&rev=f639943af8a0
Attachment #8338695 - Attachment is obsolete: true
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
I cannot reproduce this locally, and it's apparently spotty on the platforms tested. =(
Depends on: 962357
Attached patch promise-framework.patch (obsolete) — Splinter Review
Rip out the changes to browser_toolbox_raise.js. Fix some new bugs introduced by the highlighter.
Attachment #8338908 - Attachment is obsolete: true
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)
Attachment #8363805 - Flags: review?(rcampbell)
Attachment #8363805 - Flags: review?(pbrosset)
Attachment #8363805 - Flags: review?(bgrinstead)
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!
Attachment #8363805 - Flags: review?(pbrosset) → review+
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 on attachment 8363805 [details] [diff] [review]
promise-framework.patch

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

framework changes look ok to me.
Attachment #8363805 - Flags: review?(rcampbell) → review+
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
Attachment #8363805 - Flags: review?(bgrinstead)
Attached patch promise-framework.patch (obsolete) — Splinter Review
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.
Attachment #8363805 - Attachment is obsolete: true
Attached patch promise-framework.patch (obsolete) — Splinter Review
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.
Attachment #8365263 - Attachment is obsolete: true
Attachment #8382565 - Flags: review+
Attached patch promise-framework.patch (obsolete) — Splinter Review
Fixes the failures locally.

https://tbpl.mozilla.org/?tree=Try&rev=81989aff7562
Attachment #8382565 - Attachment is obsolete: true
(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.
(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.
Attached patch promise-framework.patch (obsolete) — Splinter Review
Rebase. One final try. https://tbpl.mozilla.org/?tree=Try&rev=5e09965cb2e9
Attachment #8382623 - Attachment is obsolete: true
Attachment #8383831 - Flags: review+
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.
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.
(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.
Ah, nevermind then. This is good to land then.
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.
Attached patch promise-framework.patch (obsolete) — Splinter Review
Rebase. https://tbpl.mozilla.org/?tree=Try&rev=b8244a054cb6
Attachment #8383831 - Attachment is obsolete: true
Attachment #8392336 - Flags: review+
Attachment #8392336 - Attachment is obsolete: true
Attachment #8392340 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/ff438bc0b151
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Blocks: 996671
Brandon, when you get a minute, can you please request Aurora uplift on this patch? Thanks :)
Flags: needinfo?(bbenvie)
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:
Attachment #8392340 - Flags: approval-mozilla-aurora?
Flags: needinfo?(bbenvie)
Attachment #8392340 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [qa-]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.