Closed Bug 1443461 Opened 4 years ago Closed 4 years ago

Change devtools/client/storage/ui.js to ES6 classes

Categories

(DevTools :: Storage Inspector, enhancement, P2)

enhancement

Tracking

(firefox60 fixed)

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: miker, Assigned: miker)

References

Details

Attachments

(1 file)

This may help with some async issues we have e.g. Bug 1295761.
Comment on attachment 8956484 [details]
Bug 1443461 - Change devtools/client/storage/ui.js to ES6 classes

https://reviewboard.mozilla.org/r/225400/#review231318

This fixes a bunch of async issues.
Comment on attachment 8956484 [details]
Bug 1443461 - Change devtools/client/storage/ui.js to ES6 classes

https://reviewboard.mozilla.org/r/225402/#review231644

Thanks Mike, I can confirm that it fixes this STR:
  localStorage.setItem("test","true")
  localStorage.removeItem("test")

Could you provide a test to cover this?

Also, do not hesitate to split such patch in two. A first one with just the es class refactoring and another one to add the missing await and the move of handleDeletedItems within onUpdate.
As-is, this patch is harder to review as you have to review both the naive refactoring and the actual fix of bug 1295761. The naive refactoring patch could have been attached to this bug and the other patch to bug 1295761.
Attachment #8956484 - Flags: review?(poirot.alex) → review+
(In reply to Alexandre Poirot [:ochameau] from comment #4)
> Comment on attachment 8956484 [details]
> Bug 1443461 - Change devtools/client/storage/ui.js to ES6 classes
> 
> https://reviewboard.mozilla.org/r/225402/#review231644
> 
> Thanks Mike, I can confirm that it fixes this STR:
>   localStorage.setItem("test","true")
>   localStorage.removeItem("test")
> 
> Could you provide a test to cover this?
> 
> Also, do not hesitate to split such patch in two. A first one with just the
> es class refactoring and another one to add the missing await and the move
> of handleDeletedItems within onUpdate.
> As-is, this patch is harder to review as you have to review both the naive
> refactoring and the actual fix of bug 1295761. The naive refactoring patch
> could have been attached to this bug and the other patch to bug 1295761.

Yes... I didn't split it because the move over to ES6 classes with async / await caused a bunch of tests to consistently fail, which is why I included the async fixes.

Ah, I could have included it as a second patch in this bug though.

Great point, I will remember next time I have this situation.
Pushed by mratcliffe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/38b1cc41779c
Change devtools/client/storage/ui.js to ES6 classes r=ochameau
https://hg.mozilla.org/mozilla-central/rev/38b1cc41779c
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.