Closed Bug 1118806 Opened 5 years ago Closed 5 years ago

Handle uncaught promise rejections in the storage inspector

Categories

(DevTools :: Storage Inspector, defect)

x86
macOS
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 38

People

(Reporter: ejpbruel, Assigned: ejpbruel)

References

Details

Attachments

(1 file, 1 obsolete file)

Attempting to replace the deprecated-sync-thenables with Promise.jsm promises in protocol.js (bug 1096490) uncovered some uncaught promise rejections in the inspector. These need to be fixed before I can land the former.
Blocks: 1096490
I don't know who owns the storage inspector so flagging pbrosset for review is a guess.

Patrick, if you're not the right person for this feel free to unassign yourself. Or even better, assign the right person instead :-)
Attachment #8545312 - Flags: review?(pbrosset)
Comment on attachment 8545312 [details] [diff] [review]
Handle uncaught promise rejections

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

Simple enough change for R+ even if I don't own this panel.
Attachment #8545312 - Flags: review?(pbrosset) → review+
For the next attempt, let's try this on try first:
https://tbpl.mozilla.org/?tree=Try&rev=c6db05fe0011
Try push was green, but discovered another uncaught rejection, so here's a second push just to make sure:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=39880ae71804
Try push looks green. Rerequesting review for the additional changes.

Apparently I accidentally squashed two commits, so there are also some style inspector changes in this patch. It's less than 3 lines though, so easiest would be to just land it as one patch.
Attachment #8545312 - Attachment is obsolete: true
Attachment #8548018 - Flags: review?(pbrosset)
Comment on attachment 8548018 [details] [diff] [review]
Handle uncaught promise rejections

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

r=me with a comment added

::: browser/devtools/styleinspector/style-inspector-overlays.js
@@ +183,5 @@
>    _hideCurrent: function() {
>      if (this.highlighterShown) {
>        this._getHighlighter(this.highlighterShown).then(highlighter => {
> +        let promise = highlighter.hide();
> +        if (promise) {

I think a comment is required before this condition. It will be otherwise hard to future readers to understand why hide sometimes returns a promise and sometimes not.
Attachment #8548018 - Flags: review?(pbrosset) → review+
Here goes nothing. Nobody tell RyanVM where I live:
https://hg.mozilla.org/integration/fx-team/rev/c9ff49636eef
https://hg.mozilla.org/mozilla-central/rev/c9ff49636eef
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Target Milestone: Firefox 37 → Firefox 38
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.