Closed Bug 1366534 Opened 3 years ago Closed Last year

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

Categories

(DevTools Graveyard :: WebGL Shader Editor, enhancement)

enhancement
Not set

Tracking

(firefox64 fixed)

RESOLVED FIXED
Firefox 64
Tracking Status
firefox64 --- fixed

People

(Reporter: mkohler, Assigned: preeti, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

No description provided.
Unassigning myself as there has been no recent action from my side and I don't see myself doing any action on this for the next few weeks.

Sorry about this, if somebody wants to take this, go for it! :)
Assignee: me → nobody
Status: ASSIGNED → NEW
Hey, I'd love to take on this bug to gain some experience with mozcentral/devtools development. Is anyone able to mentor for this bug?
Sure ! You can ask me for review when you're ready
Assignee: nobody → imadueme
Status: NEW → ASSIGNED
Product: Firefox → DevTools
Hello Israel !
Is there anything blocking you on this bug ? I would happily help you fix this :)
Mentor: nchevobbe
Flags: needinfo?(imadueme)
Hi Nicolas, I've recently managed to build FF by following https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites. My next step getting artifact builds to work by following https://docs.firefox-dev.tools/getting-started/build.html. Just haven't gotten around to it yet because of the All Hands, but, I will make some time this week.

After that I'm pretty comfortable making the changes in javascript, but, now that I think about it I actually don't know how to run the tests lol. Do you have any advice on how I can verify my changes don't break anything. Thanks!
Flags: needinfo?(imadueme) → needinfo?(nchevobbe)
Hello,

You should be able to run the tests by calling `./mach test devtools/client/shadereditor/test --headless` from the firefox repo root folder. (--headless means there won't be any window created, you can omit it if you want to see the UI, but it then requires to have the focus).
Flags: needinfo?(nchevobbe)
Sorry Israel, this fell off my radar.
Are you blocked on something ?
Flags: needinfo?(imadueme)
No blockers aside from time. I keep making the mistake of pulling down mozilla-central when I sit down to work on it (which often means I have to rebuild everything since the clobber file changes), I'll have to figure out artifact builds sometime!

One other thing is that I don't think running the tests with `./mach test devtools/client/shadereditor/test --headless` works as expected (as of like 1 month ago), at least for me. At that time it seemed to skip over the tests (I don't know what really was going on, but, even when changing the shadereditor tests that have defer with obvious errors all tests passed). Only after removing the --headless flag did I start to see the tests reflect changes in the code.

I'll give myself till this weekend to put up a patch and if not I'll unassign myself 9/17 on in case someone wants to pick it up :). Thanks for the help!
Flags: needinfo?(imadueme)
Assignee: imadueme → nobody
Status: ASSIGNED → NEW
Resigned here because I see some others in the tracking bug that would like to pick one of these up and I think this one is the last 'normal' one to grab. Thank you for the help Nicolas, I've definitely learned a few things and will try to pick up another devtools bug in the future!
Assignee: nobody → preetimukherjee98
Status: NEW → ASSIGNED
Hello everyone! Can someone please tell me how to  go about this bug?
(In reply to Preeti[:preeti] from comment #10)
> Hello everyone! Can someone please tell me how to  go about this bug?

So, you need to replace all occurences of `defer` in devtools/client/shadereditor: https://searchfox.org/mozilla-central/search?q=defer&case=false&regexp=false&path=devtools%2Fclient%2Fshadereditor

It looks like there's only a few place where we do use it.

In head.js, instead of doing `const deferred = defer()` and then calling `deffered.resolve()`, you should return a Promise in which you set the current code:
For example in https://searchfox.org/mozilla-central/rev/bdc89dfd7869e418d788b28eb60ab8d94e708a15/devtools/client/shadereditor/test/head.js#197-213 ,

it can be changed to: 

```
function getPrograms(front, count, onAdd) {
  return new Promise(resolve => {
    const actors = [];
    front.on("program-linked", function onLink(actor) {
      if (actors.length !== count) {
        actors.push(actor);
        if (typeof onAdd === "function") {
          onAdd(actors);
        }
      }

      if (actors.length === count) {
        front.off("program-linked", onLink);
        resolve(actors);
      }
    });
  });
```

So we're still returning a promise, and the function we pass as an argument is executed.
There, we set an event listener for "program-linked", which is going to call the onLink function.
And then you can see in the onLink function, we call the `resolve` function when the actors array has the expected length.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #12)
> For example in
> https://searchfox.org/mozilla-central/rev/
> bdc89dfd7869e418d788b28eb60ab8d94e708a15/devtools/client/shadereditor/test/
> head.js#197-213 ,
> 
> it can be changed to: 
> 
> ```
> function getPrograms(front, count, onAdd) {
>   return new Promise(resolve => {
>     const actors = [];
>     front.on("program-linked", function onLink(actor) {
>       if (actors.length !== count) {
>         actors.push(actor);
>         if (typeof onAdd === "function") {
>           onAdd(actors);
>         }
>       }
> 
>       if (actors.length === count) {
>         front.off("program-linked", onLink);
>         resolve(actors);
>       }
>     });
>   });
> ```
> 
> So we're still returning a promise, and the function we pass as an argument
> is executed.
> There, we set an event listener for "program-linked", which is going to call
> the onLink function.
> And then you can see in the onLink function, we call the `resolve` function
> when the actors array has the expected length.



Thank you! I have already made the changes in head.js. However, I have a few questions on other files:

1.how do I replace deferred.promise here in https://searchfox.org/mozilla-central/source/devtools/client/shadereditor/shadereditor.js#419 ?

2.what changes I need to incorporate into the test files as https://searchfox.org/mozilla-central/source/devtools/client/shadereditor/test/browser_se_shaders-edit-02.js#28    Do I delete the tests?
> 1.how do I replace deferred.promise here in https://searchfox.org/mozilla-central/source/devtools/client/shadereditor/shadereditor.js#419 ?

Hey, you're making me do all the work :D
So, here, the solution should be similar to the one in head.js, something like the following should work:

```js
const promise = new Promise(resolve => {
  // Initialize the source editor and store the newly created instance
  // in the ether of a resolved promise's value.
  const parent = $("#" + type + "-editor");
  const editor = new Editor(DEFAULT_EDITOR_CONFIG);
  editor.config.mode = Editor.modes[type];

  if (this._destroyed) {
    resolve(editor);
  } else {
    editor.appendTo(parent).then(() => resolve(editor));
  }
});

this._editorPromises.set(type, promise);
return promise;
```


> 2.what changes I need to incorporate into the test files as https://searchfox.org/mozilla-central/source/devtools/client/shadereditor/test/browser_se_shaders-edit-02.js#28    Do I delete the tests?

you don't need to change anything here. It's only a usage of the existing `defer` word, when we plan to remove the use of the given `defer` code util.
Sorry, I should have looked up for the solution for sometime, instead of promptly asking it here. I will surely keep this in mind from the next time.
convert uses of "defer" to "new Promise" in client/shadereditor
Attachment #9010121 - Attachment description: Bug#1366534-convert uses of 'defer' to 'new Promise' in client/shadereditor;r?nchevobbe → Bug 1366534-convert uses of 'defer' to 'new Promise' in client/shadereditor;r=nchevobbe
I have updated my patch earlier yesterday. Please do let me know if I need to make any further changes!
Thanks in advance!
Comment on attachment 9010121 [details]
Bug 1366534-convert uses of 'defer' to 'new Promise' in client/shadereditor;r=nchevobbe

Nicolas Chevobbe [:nchevobbe] has approved the revision.
Attachment #9010121 - Flags: review+
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/def8d4c0a39f
convert uses of 'defer' to 'new Promise' in client/shadereditor;r=nchevobbe
https://hg.mozilla.org/mozilla-central/rev/def8d4c0a39f
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 64
Product: DevTools → DevTools Graveyard
You need to log in before you can comment on or make changes to this bug.