Closed Bug 1368199 Opened 7 years ago Closed 6 years ago

convert uses of "defer" to "new Promise" in client/framework

Categories

(DevTools :: Framework, enhancement, P3)

Product:
DevTools
Component:
Framework
Version:
53 Branch
Type:
enhancement

Tracking

(firefox63 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: stylizit, Assigned: sreeise)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 5 obsolete files)

Bug 1368199 - Replaced uses of 'defer' with 'new Promise' in the devtools/client/framework directory.
59 bytes, text/x-review-board-request
jryans
: review+
Details 
      No description provided.
Assignee: nobody → matthieu.rigolot
Status: NEW → ASSIGNED
DevTools bug triage (filter on CLIMBING SHOES).
Priority: -- → P3
This has been quiet for 5 months, and someone else expressed interest, so I'm reassigning.
Assignee: matthieu.rigolot → dortamiguel
It seems like this has been quite for many months. I started working on this.
Hello,

I am not sure if anyone if still working on this bug or not looking at the past comments. If not I can work on a patch for the client/framework directory. I see that it is assigned, and other people have expressed interest, is there anyone still working on this?
Flags: needinfo?(ttromey)
I think it is safe for you to take.
Assignee: dortamiguel → reeisesean
Flags: needinfo?(ttromey)
This is just a partial patch. Wanted to show I havn't forgotten about this bug :). I should have a finished patch here in the next couple of days. The current changes still need some work as well.
Product: Firefox → DevTools
I wasn’t sure who to get feedback on for this bug, sorry I get it wrong, please redirect me. I have made a good bit of changes but there are some tricky parts in toolbox.js. The part for ‘this.isOpen’ is used in 6 places within toolbox.js. First when initialized in the toolbox function itself, and then in 5 other functions. From what I can tell it is resolving when the toolbox is ready? I am not sure here on this one.

I also didn’t change the part in in devtools-browser.js where it says: ‘isWebIDEInitialized: defer(),’ because it resolves in a different directory, not a part of this bug, and I am not sure if this should be changed now or wait?

Thanks.
Attachment #8985471 - Flags: feedback?(ttromey)
Attachment #8983882 - Attachment is obsolete: true
Referring to my previous comment, not sure how I didn't see this before. It looks like this.isOpenDeferred.promise is used in the 'open' function to alert the the highlighter functions if i'm not mistaken. The this.isOpen function is also used several times in autocomplete-popup.js from devtools/shared.
Looking back again, I was wrong on this.isOpen for autocomplete-popup.js, I didnt see the definitions at the top, sorry! For the this.isOpenDeferred and this.isOpen in toolbox.js, I changed it to pass a 'resolve' for a Promise constructor when the event-emitter registers as 'ready'. This seems to work and it passes all the tests for the framework directory, but i'm not sure the way I called the promise as a function inside of the toolbox function and passed the 'resolve' statment is something you guys would want to do or not, or if used correctly in the context of the event-emitters?
Attachment #8985471 - Attachment is obsolete: true
Attachment #8985471 - Flags: feedback?(ttromey)
Flags: needinfo?(ttromey)
Comment on attachment 8985823 [details] [diff] [review]
Replaced uses of 'defer' with 'new Promise' in the devtools/client/framework directory

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

Thank you for the patch.  I'm sorry about the delay in the review, I was away all last week.

I think it needs another go-around.  There were some minor issues but also one or two spots where it wasn't clear to me that the transformation was correct.

::: devtools/client/framework/attach-thread.js
@@ +37,5 @@
>  
>  function attachThread(toolbox) {
> +  let promiseResolve;
> +  let promiseReject;
> +  const threadPromsie = new Promise((resolve, reject) => {

`threadPromsie` has a typo.

However here I think it would be fine to just put most of the body of `attachThread` into a `new Promise` callback.

::: devtools/client/framework/test/browser_devtools_api_destroy.js
@@ +17,5 @@
>      url: "about:blank",
>      label: "someLabel",
>      build: function(iframeWindow, toolbox) {
> +      return new Promise(resolve => {
> +        resolve({

I wonder whether the `executeSoon` was needed to put the resolution into a different tick.

::: devtools/client/framework/test/browser_toolbox_options.js
@@ +415,5 @@
> +        checkRegistered.bind(null, toolId));
> +      resolve();
> +    }
> +  });
> +

This change seems very suspect to me.  First, it's moving the await above the code that's presumably firing the event that is being listened for.  Second, it's eagerly resolving the promise rather than arranging for the resolution to be done in an event callback.

::: devtools/client/framework/test/browser_toolbox_options_disable_buttons.js
@@ +50,5 @@
>    doc = toolbox.doc;
> +
> +  let promiseResolve;
> +  const toolReady = new Promise(resolve => {
> +    promiseResolve = resolve;

Here I think hoisting the body of the function into the `new Promise` would yield a cleaner solution.

::: devtools/client/framework/test/browser_toolbox_sidebar.js
@@ +21,5 @@
>      label: "FAKE TOOL!!!",
>      isTargetSupported: () => true,
>      build: function(iframeWindow, toolbox) {
> +      return new Promise(resolve => {
> +        resolve({

Removal of the `executeSoon`.  This occurs in a few spots.  It might be that this isn't important, but it's good to know beforehand.  If you ran the tests, could you say if they still worked?  And if not, please give them a try if you are able.

::: devtools/client/framework/test/browser_toolbox_sidebar_events.js
@@ +17,5 @@
>      label: "Test tool",
>      isTargetSupported: () => true,
>      build: function(iframeWindow, toolbox) {
> +      return new Promise(resolve => {
> +        resolve({

`executeSoon`

::: devtools/client/framework/test/head.js
@@ +207,4 @@
>  
>  DevToolPanel.prototype = {
>    open: function() {
> +    return new Promise(resolve => {

`executeSoon`

::: devtools/client/framework/test/helper_disable_cache.js
@@ +97,3 @@
>    const browser = gBrowser.selectedBrowser;
>  
> +  const reloadTabPromise = new Promise(resolve => {

It seems to me that the new promise isn't needed here, because presumably browserLoaded returns a promise, and then `.then` is also going to return one.  So couldn't it just be:

const reloadTabPromise = BrowserTestUtils.stuff.then(function () { ... });

?

::: devtools/client/framework/toolbox-options.js
@@ +516,3 @@
>      this._removeListeners();
>  
> +    const promiseDestroyer = new Promise(resolve => {

Here the danger is that something will make a re-entering call to this `destroy` method.  The old code might have been protecting against this by setting `this.destroyPromise` early -- but after the change that mechanism would no longer work.  So either you have to be sure that this isn't happening, or rewrite this to preserve the short-circuit.

::: devtools/client/framework/toolbox.js
@@ +445,3 @@
>        const domHelper = new DOMHelpers(this.win);
> +      domHelper.onceDOMReady(async () => {
> +        await domReady;

This change looks incorrect or at least confusing.  I think it could be done more clearly by just hoisting a bit more code into the `new Promise` callback.

@@ +574,1 @@
>      }.bind(this))().catch(console.error);

I see what is going on here but I tend to think this is a situation where going back to some defer-like behavior is actually simpler to understand.

@@ +1624,5 @@
> +    let promiseResolve;
> +    let promiseReject;
> +    const loadToolPromise = new Promise((resolve, reject) => {
> +      promiseResolve = resolve;
> +      promiseReject = reject;

I think just putting most of the function body here is going to be cleaner.
Replied to the patch, clearing NI.
Flags: needinfo?(ttromey)
(In reply to Tom Tromey :tromey from comment #10)


> Thank you for the patch.  I'm sorry about the delay in the review, I was
> away all last week.
> 
> I think it needs another go-around.  There were some minor issues but also
> one or two spots where it wasn't clear to me that the transformation was
> correct.
> 

No worries, I figured it would need some more work. Thanks for the feedback, it always helps. Devtools is somewhat confusing and just getting some more info on the changes makes it much easier to understand and refactor.


> ::: devtools/client/framework/test/browser_devtools_api_destroy.js
> @@ +17,5 @@
> >      url: "about:blank",
> >      label: "someLabel",
> >      build: function(iframeWindow, toolbox) {
> > +      return new Promise(resolve => {
> > +        resolve({
> 
> I wonder whether the `executeSoon` was needed to put the resolution into a
> different tick.
> 


I actually tried leaving the executeSoon in on the ones I removed, but it was causing the tests to timeout. I may have called it wrong however, I will go back over the ones removed and see if I can get this to work, as well as make the changes you suggested and continue reworking.
I was able to add back the executeSoon calls, as well as make the changes from the last feedback. All of the tests passed with no issues. If there is anything I should change, I can do that as well.
Attachment #8987682 - Flags: feedback?(ttromey)
Attachment #8985823 - Attachment is obsolete: true
Comment on attachment 8987682 [details] [diff] [review]
Replaced uses of 'defer' with 'new Promise' in the devtools/client/framework directory

I think probably someone more actively involved with devtools should do
the final review on this one.
Attachment #8987682 - Flags: feedback?(ttromey) → review?(jryans)
Comment on attachment 8987682 [details] [diff] [review]
Replaced uses of 'defer' with 'new Promise' in the devtools/client/framework directory

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

Thanks for working on this! :)

I'd like to see a small naming change, but otherwise it seems good to go.  Once you've updated the patch with that, I can take another look and also submit to our CI.

::: devtools/client/framework/toolbox.js
@@ +171,5 @@
>  
>    this._hostType = hostType;
>  
> +  this.isOpen = new Promise(function(resolve) {
> +    this._isOpenPromise = resolve;

I think renaming `_isOpenPromise` to `_resolveIsOpen` or something else with the verb "resolve" in the name would be easier to follow.
Attachment #8987682 - Flags: review?(jryans) → review-
Changed it to what you suggested, that name does make more sense!
Attachment #8987682 - Attachment is obsolete: true
Comment on attachment 8988377 [details]
Bug 1368199 - Replaced uses of 'defer' with 'new Promise' in the devtools/client/framework directory.

https://reviewboard.mozilla.org/r/253658/#review260392

Thanks for the updated patch, this looks great! :) I have pushed it to CI, so we'll see what it says.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07563793599d591cbcb9309068a52c8287701170
Attachment #8988377 - Flags: review?(jryans) → review+
It looks like are a few issues with performance related code paths, could you try running these tests locally?

devtools/client/shared/test/browser_telemetry_misc.js
devtools/client/shared/test/browser_theme_switching.js
devtools/client/shared/test/browser_telemetry_toolboxtabs_shadereditor.js
Flags: needinfo?(reeisesean)
It seems that the tests fail when running together but pass when run individually. From what I can tell there is an issue where initPerformance() runs and attempts to call this.performance.connect(). I am still going through the stack trace, and will try to get back to you with an update patch once I figure out what is causing the failures.
Flags: needinfo?(reeisesean)
Attachment #8988377 - Attachment is obsolete: true
I had to basically revert the initPerformance() function back to defer like behavior and used a variable equal to resolve and use it to resolve this.performance. It seems that when destroy was being called the front connection was either failing or it did not get initialized in the first place. This should fix it, but I couldn't get it to pass by resolving in the promise constructor itself. If you believe there may something else I can do, then i'm all for it.
I somehow didn't think about waiting on your to review it before triggering a try build, I am sorry about that.
(In reply to Sean Reeise [:sreeise] from comment #24)
> I somehow didn't think about waiting on your to review it before triggering
> a try build, I am sorry about that.

Oh, that's totally fine! :) Sometimes that can even be better actually, as then the reviewer can see the test results as well.  As long as you end up with a green try run and r+ before landing, the order of the steps doesn't matter as much.  So, however it works best for you.
It looks like maybe you closed the first review and then submitted an entirely new one? That's fine, but for future bugs, there shouldn't be a need to do it... You should be able to submit again to the same bug and it will update the existing review. By updating the existing review, it will retain both versions, giving the reviewer the option to see an interdiff of only what you changed since last time. (I know MozReview can be quite confusing, so you are not alone in that!)
Comment on attachment 8989340 [details]
Bug 1368199 - Replaced uses of 'defer' with 'new Promise' in the devtools/client/framework directory.

https://reviewboard.mozilla.org/r/254404/#review261304

Thanks for investigating and fixing that! :) It does make sense now that I see it... In the previous version, `_performanceFrontConnection` wasn't set until the end, and there was some async behavior in there (`await this.performance.connect`), so it's possible we could re-enter `initPerformance` before the await completes.

::: devtools/client/framework/toolbox.js:3091
(Diff revision 1)
>      if (this._performanceFrontConnection) {
> -      return this._performanceFrontConnection.promise;
> +      return this._performanceFrontConnection;
>      }
>  
> -    this._performanceFrontConnection = defer();
> +    let resolvePerformance;
> +    this._performanceFrontConnection = new Promise(function(resolve) {

If you really want to eliminate this `resolvePerformance` variable, I think you could make this promise constructor an `async function` and move the rest of this function inside the promise constructor.

I would also be fine with it as is, so whatever seems best to you.
Attachment #8989340 - Flags: review?(jryans) → review+
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #26)
> It looks like maybe you closed the first review and then submitted an
> entirely new one? That's fine, but for future bugs, there shouldn't be a
> need to do it... You should be able to submit again to the same bug and it
> will update the existing review. By updating the existing review, it will
> retain both versions, giving the reviewer the option to see an interdiff of
> only what you changed since last time. (I know MozReview can be quite
> confusing, so you are not alone in that!)

Welp, you are right it is definitely confusing, I honestly did not mean to do that. I originally pushed a different review that I somehow had changed the package-lock.json on without realizing it, so I discarded it. I wonder if that may have been it, or it may be me just being confused on that as well, I will go and look at the docs on it and make sure on how to do that. I appreciate the help, there is a lot to understand.
> If you really want to eliminate this `resolvePerformance` variable, I think
> you could make this promise constructor an `async function` and move the
> rest of this function inside the promise constructor.
> 
> I would also be fine with it as is, so whatever seems best to you.

Hmm, I actually do want to try your suggestion. The only thing I don't like is that it is just an extra variable that may not be needed, and your suggestion would fix that. I am also wondering though, if it is made into a function will it give it the same performanceFrontConnection or a new one. I may very well be over thinking here though. I will go try and see what happens.
I see what you mean now, making the function of the Promise constructor async, I misread that. I think just sticking with the original is ok too, I will resolve this open issue.
Comment on attachment 8989340 [details]
Bug 1368199 - Replaced uses of 'defer' with 'new Promise' in the devtools/client/framework directory.

https://reviewboard.mozilla.org/r/254404/#review261304

> If you really want to eliminate this `resolvePerformance` variable, I think you could make this promise constructor an `async function` and move the rest of this function inside the promise constructor.
> 
> I would also be fine with it as is, so whatever seems best to you.

Commented on bugzilla as well. I am ok with leaving as is.
Comment on attachment 8989340 [details]
Bug 1368199 - Replaced uses of 'defer' with 'new Promise' in the devtools/client/framework directory.

https://reviewboard.mozilla.org/r/254404/#review261504
Didn't realize it would post the comment to Bugzilla. I changed the issues to fixed. Anything else I should do here?
Flags: needinfo?(jryans)
Scratch the need info. Forgot about doing checkin-needed.
Flags: needinfo?(jryans)
Keywords: checkin-needed
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4fbc1e8a73bd
Replaced uses of 'defer' with 'new Promise' in the devtools/client/framework directory. r=jryans
Keywords: checkin-needed
Thanks again for working on this! :) Feel free to look for other open DevTools bugs or browse through mentored bugs at https://bugs.firefox-dev.tools.
https://hg.mozilla.org/mozilla-central/rev/4fbc1e8a73bd
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #36)
> Thanks again for working on this! :) Feel free to look for other open
> DevTools bugs or browse through mentored bugs at
> https://bugs.firefox-dev.tools.

No problem, I enjoy contributing. I appreciate your help and will definitely look at working on more bugs in devtools.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: