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

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, enhancement, P2)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

This may help with some async issues we have e.g. Bug 1295761.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Bug 1443461 - Change devtools/client/storage/ui.js to ES6 classes

Review commit: https://reviewboard.mozilla.org/r/225402/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/225402/

Comment 2

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8956484 [details]
Bug 1443461 - Change devtools/client/storage/ui.js to ES6 classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225402/diff/1-2/

Comment 3

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

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 4

[Alexandre Poirot [:ochameau]]

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.

Comment 5

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

(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.

Comment 6

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Comment on attachment 8956484 [details]
Bug 1443461 - Change devtools/client/storage/ui.js to ES6 classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/225402/diff/2-3/

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/38b1cc41779c
Change devtools/client/storage/ui.js to ES6 classes r=ochameau

Comment 8

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/38b1cc41779c