Closed Bug 1260718 Opened 4 years ago Closed 4 years ago

Switch to using plain promises instead of Promise.jsm in CustomizableUI code

Categories

(Firefox :: Toolbars and Customization, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 48
Tracking Status
firefox48 --- fixed

People

(Reporter: Gijs, Assigned: gasolin, Mentored)

References

Details

(Whiteboard: [lang=js])

Attachments

(2 files, 2 obsolete files)

https://dxr.mozilla.org/mozilla-central/search?q=promise.defer+path%3Acustomizableui+-path%3Atest&redirect=true&case=false

We don't really need to bother updating tests, but we should update the main PanelUI and CUI/CustomizeMode code to no longer use Promise.defer . It's non-standard and often harder to read than code using new Promise().
Would like take it a try, file by file
Assignee: nobody → gasolin
Status: NEW → ASSIGNED
Comment on attachment 8738390 [details]
MozReview Request: Bug 1260718 - use plain promise in CustomizableUI.jsm and ScrollbarSampler.jsm; r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44497/diff/1-2/
Attachment #8738390 - Attachment description: MozReview Request: Bug 1260718 - use plain promise in CustomizableUI.jsm; r?Gijs → MozReview Request: Bug 1260718 - use plain promise in CustomizableUI.jsm and ScrollbarSampler.jsm; r?Gijs
Attachment #8738393 - Attachment is obsolete: true
Attachment #8738393 - Flags: review?(gijskruitbosch+bugs)
https://reviewboard.mozilla.org/r/44497/#review41227

::: browser/components/customizableui/CustomizableUI.jsm:4110
(Diff revision 2)
>          this._onResize(aEvent);
>      }
>    },
>  
>    show: function() {
> -    let deferred = Promise.defer();
> +    let deferred = new Promise(resolve => {

Your `deferred` here is actually a `Promise` instance.

::: browser/components/customizableui/ScrollbarSampler.jsm:18
(Diff revision 2)
>  
>  var gSystemScrollbarWidth = null;
>  
>  this.ScrollbarSampler = {
>    getSystemScrollbarWidth: function() {
> -    let deferred = Promise.defer();
> +    let deferred = new Promise(resolve => {

Same here.
Comment on attachment 8738390 [details]
MozReview Request: Bug 1260718 - use plain promise in CustomizableUI.jsm and ScrollbarSampler.jsm; r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44497/diff/2-3/
Comment on attachment 8738412 [details]
MozReview Request: Bug 1260718 - use plain promise in panelUI.js; r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44527/diff/1-2/
Comment on attachment 8738390 [details]
MozReview Request: Bug 1260718 - use plain promise in CustomizableUI.jsm and ScrollbarSampler.jsm; r?Gijs

https://reviewboard.mozilla.org/r/44497/#review41237

::: browser/components/customizableui/CustomizableUI.jsm:4111
(Diff revision 3)
>      }
>    },
>  
>    show: function() {
> -    let deferred = Promise.defer();
> -    if (this._panel.state == "open") {
> +    let promise = new Promise(resolve => {
> +      if (this._panel.state === "open") {

Please leave this alone. We prefer double over triple equals in browser code, especially when it's already clear that both sides are going to be strings anyway, the third = does not gain us anything.

::: browser/components/customizableui/CustomizableUI.jsm:4111
(Diff revision 3)
> -    if (this._panel.state == "open") {
> -      deferred.resolve();
> -      return deferred.promise;
> +      if (this._panel.state === "open") {
> +        resolve();
> +        return promise;
> -    }
> +      }

This doesn't really make sense. You're returning the variable in which we're storing the promise object from within the promise function, but the promise's function will run as part of constructing the promise, so you're really returning undefined because the promise hasn't been assigned yet, which isn't what you mean.

Instead, please use this:

```
show: function() {
  if (this._panel.state == "open") {
    return Promise.resolve();
  }
  return new Promise(resolve => {
    // Everything else goes here.
  });
```

::: browser/components/customizableui/CustomizableUI.jsm:4131
(Diff revision 3)
> -      this.removeEventListener("popupshown", onPopupShown);
> +        this.removeEventListener("popupshown", onPopupShown);
> -      this.addEventListener("dragover", overflowableToolbarInstance);
> +        this.addEventListener("dragover", overflowableToolbarInstance);
> -      this.addEventListener("dragend", overflowableToolbarInstance);
> +        this.addEventListener("dragend", overflowableToolbarInstance);
> -      deferred.resolve();
> +        resolve();
> -    });
> +      });
> +    }).catch(err => log.error("fail to show CustomizableUI: " + err));

Why did you add a catch clause when there wasn't one before? This is a change in behaviour that isn't necessary, so please get rid of it. Consumers can catch errors, and we should keep it that way.

::: browser/components/customizableui/ScrollbarSampler.jsm:19
(Diff revision 3)
> -    if (gSystemScrollbarWidth !== null) {
> +      if (gSystemScrollbarWidth !== null) {
> -      deferred.resolve(gSystemScrollbarWidth);
> -      return deferred.promise;
> +        resolve(gSystemScrollbarWidth);
> +        return promise;
> -    }
> +      }

Again, please put these synchronous checks that intend to immediately return a resolved promise outside the function we pass to the Promise constructor.

::: browser/components/customizableui/ScrollbarSampler.jsm:28
(Diff revision 3)
>  
> -    this._sampleSystemScrollbarWidth().then(function(systemScrollbarWidth) {
> +      this._sampleSystemScrollbarWidth().then(function(systemScrollbarWidth) {
> -      gSystemScrollbarWidth = systemScrollbarWidth;
> +        gSystemScrollbarWidth = systemScrollbarWidth;
> -      deferred.resolve(gSystemScrollbarWidth);
> +        resolve(gSystemScrollbarWidth);
> -    });
> +      });
> -    return deferred.promise;
> +    }).catch(err => log.error("fail to getSystemScrollbarWidth: " + err));

And please don't add catch handlers.

::: browser/components/customizableui/ScrollbarSampler.jsm:65
(Diff revision 3)
> -      // Minimum width of 10 so that we have enough padding:
> +        // Minimum width of 10 so that we have enough padding:
> -      sbWidth.value = Math.max(sbWidth.value, 10);
> +        sbWidth.value = Math.max(sbWidth.value, 10);
> -      deferred.resolve(sbWidth.value);
> +        resolve(sbWidth.value);
> -      iframe.remove();
> +        iframe.remove();
> -    });
> +      });
> -
> +    }).catch(err => log.error("fail to _sampleSystemScrollbarWidth: " + err));

Same here.
Attachment #8738390 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8738412 [details]
MozReview Request: Bug 1260718 - use plain promise in panelUI.js; r?Gijs

https://reviewboard.mozilla.org/r/44527/#review41239

Without the catch handlers, this looks OK.

::: browser/components/customizableui/content/panelUI.js:167
(Diff revision 2)
> -                                                "toolbarbutton-icon");
> +                                                  "toolbarbutton-icon");
> -      this.panel.openPopup(iconAnchor || anchor);
> +        this.panel.openPopup(iconAnchor || anchor);
> -    }, (reason) => {
> +      }, (reason) => {
> -      console.error("Error showing the PanelUI menu", reason);
> +        console.error("Error showing the PanelUI menu", reason);
> -    });
> +      });
> -
> +    }).catch(err => log.error("fail to show panelUI: " + err));

Please don't add the catch handler.

::: browser/components/customizableui/content/panelUI.js:238
(Diff revision 2)
> -            Services.obs.removeObserver(delayedStartupObserver, "browser-delayed-startup-finished");
> +              Services.obs.removeObserver(delayedStartupObserver, "browser-delayed-startup-finished");
> -            delayedStartupDeferred.resolve();
> +              resolve();
> -          }
> +            }
> -        };
> +          };
> -        Services.obs.addObserver(delayedStartupObserver, "browser-delayed-startup-finished", false);
> +          Services.obs.addObserver(delayedStartupObserver, "browser-delayed-startup-finished", false);
> -        yield delayedStartupDeferred.promise;
> +        }).catch(err => log.error("fail to yield delayedStartupObserver: " + err));

Please don't add the catch handler.
Attachment #8738412 - Flags: review?(gijskruitbosch+bugs) → review+
Please get a successful try-run before landing this.
Comment on attachment 8738412 [details]
MozReview Request: Bug 1260718 - use plain promise in panelUI.js; r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44527/diff/2-3/
Attachment #8738390 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8738390 [details]
MozReview Request: Bug 1260718 - use plain promise in CustomizableUI.jsm and ScrollbarSampler.jsm; r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44497/diff/3-4/
https://reviewboard.mozilla.org/r/44497/#review41237

> This doesn't really make sense. You're returning the variable in which we're storing the promise object from within the promise function, but the promise's function will run as part of constructing the promise, so you're really returning undefined because the promise hasn't been assigned yet, which isn't what you mean.
> 
> Instead, please use this:
> 
> ```
> show: function() {
>   if (this._panel.state == "open") {
>     return Promise.resolve();
>   }
>   return new Promise(resolve => {
>     // Everything else goes here.
>   });
> ```

make sense!

> Why did you add a catch clause when there wasn't one before? This is a change in behaviour that isn't necessary, so please get rid of it. Consumers can catch errors, and we should keep it that way.

generally its a good practice to add catch clause after promise to catch up potential erros within promise. Or the error will be hard to be caught.
I will remove the catch clause if you insist.
Attachment #8738390 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8738390 [details]
MozReview Request: Bug 1260718 - use plain promise in CustomizableUI.jsm and ScrollbarSampler.jsm; r?Gijs

https://reviewboard.mozilla.org/r/44497/#review41503

Looks good, thanks!
(In reply to Fred Lin [:gasolin] from comment #14)
> > Why did you add a catch clause when there wasn't one before? This is a change in behaviour that isn't necessary, so please get rid of it. Consumers can catch errors, and we should keep it that way.
> 
> generally its a good practice to add catch clause after promise to catch up
> potential erros within promise. Or the error will be hard to be caught.
> I will remove the catch clause if you insist.

I agree that we should be cautious about catching errors from promises in general.

However, if I have a public method that returns a promise, then consumers can catch promise rejections and react appropriately. If I have a function that returns a promise but always mutes the errors then as a consumer I have no way of telling whether my call did what I wanted. So it's important to consider *where* to catch such errors.

In this case, there's also the fact that we don't really expect errors from these promises, and so leaving them uncaught is more likely to catch any, if they appear, in automated tests and so on, whereas adding .catch()es will report them to the console, but that won't fail automated tests and so we end up not realizing that there are issues. Much like wrapping all non-promise-y code in try...catch can obscure errors that you want to know about.

So I believe that here, the safest thing is to leave the status quo, and not catch any hypothetical errors. :-)
Thanks for clarify. It sound reasonable and I've removed all catch clause.


I've triggered a try run https://treeherder.mozilla.org/#/jobs?repo=try&revision=09b30388190f

There are some intermittent errors (in orange) , what is the criteria to decide if the try-run is successful?
Flags: needinfo?(gijskruitbosch+bugs)
(In reply to Fred Lin [:gasolin] from comment #17)
> Thanks for clarify. It sound reasonable and I've removed all catch clause.
> 
> 
> I've triggered a try run
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=09b30388190f
> 
> There are some intermittent errors (in orange) , what is the criteria to
> decide if the try-run is successful?

It's effectively a bit of dark magic where you try to see if any of the orange is (a) more permanent (at this point treeherder/infra will automatically re-run orange things, and most of these were green the next time) and/or (b) related to the commit you're testing.

In this particular case it looks OK (I starred most of the orange), though Windows hasn't run yet and it would be wise to wait for that, perhaps.

For future reference, you don't need to run b2g or android or reftests for changes to browser frontend code. I tend to run something like:

try: -b od -t none -p win32,win64,macosx64,linux,linux64,linux64-asan -u mochitest-bc,mochitest-e10s-bc,marionette,marionette-e10s

depending on the changes I make, I might or might not run xpcshell. None of the code in your current patch is run under xpcshell, so there's no point running them.
Flags: needinfo?(gijskruitbosch+bugs)
thanks for advice, I checked the fail cases and they seems all intemittent
Lets try to push it to inbound
Keywords: checkin-needed
CustomizeMode.jsm is not use plain promise yet, to prevent issue with mozreview,
create followup bug 1263557 to fix it.
Blocks: 1263557
Comment on attachment 8738412 [details]
MozReview Request: Bug 1260718 - use plain promise in panelUI.js; r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44527/diff/3-4/
Attachment #8738412 - Attachment description: MozReview Request: Bug 1260718 - use plain promise in panelUI.js; r?Gijs → MozReview Request: Bug 1260718 - use plain promise in CustomizeMode.jsm; r?Gijs
Attachment #8738390 - Attachment is obsolete: true
Review commit: https://reviewboard.mozilla.org/r/45421/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/45421/
Attachment #8738412 - Attachment description: MozReview Request: Bug 1260718 - use plain promise in CustomizeMode.jsm; r?Gijs → MozReview Request: Bug 1260718 - use plain promise in panelUI.js; r?Gijs
Attachment #8739895 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8738412 [details]
MozReview Request: Bug 1260718 - use plain promise in panelUI.js; r?Gijs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44527/diff/4-5/
Sorry I made some mistake operation with mozreview. The patchs should be the same now. Not sure if it still need set r+
Comment on attachment 8739895 [details]
MozReview Request: Bug 1260718 - use plain promise in CustomizableUI.jsm and ScrollbarSampler.jsm; r?Gijs

https://reviewboard.mozilla.org/r/45421/#review41979
Attachment #8739895 - Flags: review?(gijskruitbosch+bugs) → review+
https://hg.mozilla.org/mozilla-central/rev/8a367c93f87d
https://hg.mozilla.org/mozilla-central/rev/b26c9c90617b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
You need to log in before you can comment on or make changes to this bug.