Closed Bug 1355523 Opened 7 years ago Closed 7 years ago

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

Categories

(DevTools :: Style Editor, enhancement, P3)

enhancement

Tracking

(firefox55 fixed)

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: utsav.saha, Assigned: stylizit, Mentored)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_4) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/57.0.2987.133 Safari/537.36
Mentor: ttromey
Severity: normal → enhancement
Component: Untriaged → Developer Tools: Style Editor
Priority: -- → P3
Assignee: nobody → utsav.saha
Blocks: 1283869
Hi,

Tests in styleeditor seem to pass locally with the patch applied.

> Save a reference to the resolver in browser_styleeditor_opentab.js because
> we can not use 'yield' within Promise arrow functions as they are not
> generators.

This seem to work but doesn't look pretty, maybe there's a more clever way to work around that?
Comment on attachment 8865931 [details]
Bug 1355523 - Convert defer into new Promise in client/styleeditor;

https://reviewboard.mozilla.org/r/137490/#review141132

Thank you for the patch.  It looks very good!  I have a few comments, though, so I am clearing the review.

::: commit-message-37a5b:2
(Diff revision 1)
> +Save a reference to the resolver in browser_styleeditor_opentab.js because we can not use 'yield' within Promise arrow functions as they are not generators.
> +Added 'yield' to fix a bug in browser_styleeditor_scroll.js, which fails tests after converting all 'defer' to 'new Promise'.

I think these lines should be removed from the commit message.  It's fine to just put comments about what changed into the bug.

::: devtools/client/styleeditor/StyleEditorUI.jsm:718
(Diff revision 1)
> -    }
> -
> -    let deferred = defer();
>      let self = this;
>  
> +    return new Promise(resolve => {

This uses "Promise" but other parts of the code use "promise".  I'm a bit surprised that Promise works (I couldn't figure out where the definition would come from); but if it does work it seems bad to mix promise implementations.

One idea would be to change the require at the top to use `const Promise = require("promise");`, and if that doesn't cause problems, rename promise->Promise throughout the file.

::: devtools/client/styleeditor/StyleEditorUI.jsm:720
(Diff revision 1)
> -    let deferred = defer();
>      let self = this;
>  
> +    return new Promise(resolve => {
> +      if (editor.summary) {
> +        return resolve(editor.summary);

I wonder if eslint will complain here.  Either way, this reads a little strangely to me; I would either leave this block above (as it was before), or I would change it to call resolve(...) and then use an "else".

::: devtools/client/styleeditor/StyleEditorUI.jsm:736
(Diff revision 1)
> -    let deferred = defer();
>      let self = this;
>  
> +    return new Promise(resolve => {
> +      if (editor.details) {
> +        return resolve(editor.details);

Likewise.

::: devtools/client/styleeditor/test/browser_styleeditor_opentab.js:25
(Diff revision 1)
> -  let tabOpenedDefer = defer();
> +  let resolver;
> +  let tabOpenedDefer = new Promise(resolve => {
> +    resolver = resolve;
> +  });
>  
>    ui._window.openUILinkIn = newUrl => {
>      // Reset the actual openUILinkIn function before proceeding.
>      ui._window.openUILinkIn = originalOpenUILinkIn;
>  
>      is(newUrl, url, "The correct tab has been opened");
> -    tabOpenedDefer.resolve();
> +    resolver();
>    };

I saw your comment about this code, and I was thinking it could be done like:

```
let tabOpenedDefer = new Promise(resolve => {
   ui._window. ... etc
});

...

yield tabOpenedDefer;
```

The function passed to new Promise is run synchronously, so it seems to me that this would work fine.

::: devtools/client/styleeditor/test/browser_styleeditor_scroll.js:41
(Diff revision 1)
>  
>    // We need to wait for editor-selected if we want to check the scroll
>    // position as scrolling occurs after selectStyleSheet resolves but before the
>    // event is emitted.
>    let selectEventPromise = waitForEditorToBeSelected(longEditor, ui);
> -  ui.selectStyleSheet(longEditor.styleSheet, LINE_TO_SELECT);
> +  yield ui.selectStyleSheet(longEditor.styleSheet, LINE_TO_SELECT);

Thank you for noticing this.

::: devtools/client/styleeditor/test/browser_styleeditor_syncAlreadyOpen.js:36
(Diff revision 1)
>  
> -  let onEditorChange = defer();
> -  editor.sourceEditor.on("change", onEditorChange.resolve);
> +  let resolver;
> +  let onEditorChange = new Promise(resolve => {
> +    resolver = resolve;
> +  });
> +  editor.sourceEditor.on("change", resolver);

I think there's no need to capture `resolver`, this line can be moved up into the function passed to the Promise constructor.

::: devtools/client/styleeditor/test/head.js:45
(Diff revision 1)
>  var navigateTo = Task.async(function* (url) {
>    info(`Navigating to ${url}`);
>    let browser = gBrowser.selectedBrowser;
>  
> -  let navigating = defer();
> +  yield new Promise(resolve => {
> -  browser.addEventListener("load", function () {
> +    browser.addEventListener("load", function () {

I think there's no need for Task.async or a generator function here.  Instead this can just be an ordinary function and it can return the new promise rather than yield it.
Attachment #8865931 - Flags: review?(ttromey)
Comment on attachment 8865931 [details]
Bug 1355523 - Convert defer into new Promise in client/styleeditor;

https://reviewboard.mozilla.org/r/137490/#review141132

> This uses "Promise" but other parts of the code use "promise".  I'm a bit surprised that Promise works (I couldn't figure out where the definition would come from); but if it does work it seems bad to mix promise implementations.
> 
> One idea would be to change the require at the top to use `const Promise = require("promise");`, and if that doesn't cause problems, rename promise->Promise throughout the file.

Done. I modified all "promise" calls with Promise and no problems seem to arise, all tests are running fine. 
From what I understand, "promise" seems it is just syntactic sugar that was introduced before the native Promise API, and as you can call directly Promise.resolve, Promise.reject, Promise.all, it should be ok for most uses of these functions.

> I wonder if eslint will complain here.  Either way, this reads a little strangely to me; I would either leave this block above (as it was before), or I would change it to call resolve(...) and then use an "else".

Yes I agree it makes more sense just to have it outside the promise

> Likewise.

Done

> I saw your comment about this code, and I was thinking it could be done like:
> 
> ```
> let tabOpenedDefer = new Promise(resolve => {
>    ui._window. ... etc
> });
> 
> ...
> 
> yield tabOpenedDefer;
> ```
> 
> The function passed to new Promise is run synchronously, so it seems to me that this would work fine.

Oh yes, simply like that! Somehow I bugged on this

> I think there's no need to capture `resolver`, this line can be moved up into the function passed to the Promise constructor.

Done

> I think there's no need for Task.async or a generator function here.  Instead this can just be an ordinary function and it can return the new promise rather than yield it.

Done
Comment on attachment 8865931 [details]
Bug 1355523 - Convert defer into new Promise in client/styleeditor;

https://reviewboard.mozilla.org/r/137490/#review142096

Thank you for updating the patch.  And, I wanted to say that I appreciate your patience the last time; it took me longer to get to the first review than I ordinarily like.

I think there is still one minor change to make in this patch.

::: devtools/client/styleeditor/test/head.js:45
(Diff revisions 1 - 2)
>  /**
>   * Navigate the currently selected tab to a new URL and wait for it to load.
>   * @param {String} url The url to be loaded in the current tab.
>   * @return a promise that resolves when the page has fully loaded.
>   */
>  var navigateTo = Task.async(function* (url) {

This still has to change to remove the async, like:

```
var navigateTo = function (url) { ...
```
Attachment #8865931 - Flags: review?(ttromey)
Comment on attachment 8865931 [details]
Bug 1355523 - Convert defer into new Promise in client/styleeditor;

https://reviewboard.mozilla.org/r/137490/#review142096

No worries at all, this one was blazing fast!

> This still has to change to remove the async, like:
> 
> ```
> var navigateTo = function (url) { ...
> ```

Whoops, missed that one! Should be good now
Comment on attachment 8865931 [details]
Bug 1355523 - Convert defer into new Promise in client/styleeditor;

https://reviewboard.mozilla.org/r/137490/#review142114

Thanks once again.  Looks great.  I'll push it through try now to see what happens.
Attachment #8865931 - Flags: review?(ttromey) → review+
There was an eslint failure:

 TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/devtools/client/styleeditor/StyleSheetEditor.jsm:17:7 | 'defer' is assigned a value but never used. (no-unused-vars) [log…]
(In reply to Tom Tromey :tromey from comment #10)
> There was an eslint failure:
> 
>  TEST-UNEXPECTED-ERROR |
> /home/worker/checkouts/gecko/devtools/client/styleeditor/StyleSheetEditor.
> jsm:17:7 | 'defer' is assigned a value but never used. (no-unused-vars)
> [log…]

Removed the defer declaration
Pushed by ttromey@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3f6a1965ff0d
Convert defer into new Promise in client/styleeditor; r=tromey
https://hg.mozilla.org/mozilla-central/rev/3f6a1965ff0d
Status: UNCONFIRMED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Assignee: utsav.saha → matthieu.rigolot
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: