1193308 - Clean up storage inspector code

Status: RESOLVED FIXED

(DevTools :: Storage Inspector, defect)

Description

[Tim Nguyen :ntim]

A lot of function waterfalls in there.

Comment 1

[Tim Nguyen :ntim]

Attachment: MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Bug 1193308 - Clean up storage inspector code. r=mratcliffe

Comment 2

[Tim Nguyen :ntim]

Comment on attachment 8646393 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

This does a bunch of early returns, and moves out some update logic to their own functions.

Comment 3

[Tim Nguyen :ntim]

Comment on attachment 8646393 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8646393 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Unified devtools loaders. Requesting feedback from jryans and ochameau for that part.

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8646393 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

https://reviewboard.mozilla.org/r/15731/#review14161

Just looked at the file headers and it looks good.

::: browser/devtools/storage/ui.js:15
(Diff revision 2)
>    () => require("devtools/shared/widgets/TableWidget").TableWidget);

For these too, you can do:
loader.lazyRequireGetter(this, "TableWidget", "devtools/shared/widgets/TableWidget", true);
// the last `true` argument tells to use destructuring in order to take the `TableWidget` attribute and put it in a `TableWidget` attribute on `this`

Comment 6

[Alexandre Poirot [:ochameau]]

(In reply to Alexandre Poirot [:ochameau] from comment #5)
> Comment on attachment 8646393 [details]
> For these too, you can do:

s/too/two/

Comment 7

[Alexandre Poirot [:ochameau]]

Comment on attachment 8646393 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Come on mozReview don't make me r+ when I was just asked for feedback!

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8646393 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

What Alex said. :)

Comment 9

[Tim Nguyen :ntim]

Comment on attachment 8646393 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Comment 10

[Tim Nguyen :ntim]

Comment on attachment 8646393 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Alex, thanks for the tip ! Applied it to the patch.

Comment 11

[Tim Nguyen :ntim]

Comment on attachment 8646393 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8646393 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Some more loader changes based on Alex's tip.

Comment 13

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

@ntim: Sorry it took me so long to get to this but we needed to land the E10S tests quickly.

Can you rebase the patch and then I will review it?

Comment 14

[Tim Nguyen :ntim]

Attachment: MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Comment 15

[Tim Nguyen :ntim]

Comment on attachment 8653447 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

I don't know if this was properly rebased (I think it was), but all the tests were passing after rebasing.

Comment 16

[Tim Nguyen :ntim]

Try push from MozReview : https://treeherder.mozilla.org/#/jobs?repo=try&revision=06e41d8429d6

Comment 17

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=958a4b3900bb

Comment 18

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

Comment on attachment 8653447 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

https://reviewboard.mozilla.org/r/17467/#review16183

r+ with theese nits addressed.

We should wait for a green try too (including E10S):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc715fbf3621

::: browser/devtools/storage/panel.js:10
(Diff revision 1)
> +const Services = require("Services");

We no longer need to require Services in this file.

::: browser/devtools/storage/panel.js:11
(Diff revision 1)
> +const promise = require("promise");

Only native promises are used in this file. There is no need for this require.

::: browser/devtools/storage/ui.js:9
(Diff revision 1)
> -const STORAGE_STRINGS = "chrome://browser/locale/devtools/storage.properties";
> +const Services = require("Services");

We no longer need to require Services in this file.

::: browser/devtools/storage/ui.js:353
(Diff revision 1)
> -            } catch(ex) {
> +          } catch(ex) {}

Our ESLint configuration disallows empty blocks. Please re-add the "// Do Nothing" comment.

Comment 19

[Tim Nguyen :ntim]

Comment on attachment 8653447 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Comment 20

[Tim Nguyen :ntim]

Comment on attachment 8653447 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Comment 21

[Tim Nguyen :ntim]

Comment on attachment 8653447 [details]
MozReview Request: Bug 1193308 - Clean up storage inspector code. r?mratcliffe

Damn it Mozreview !

Comment 22

[Tim Nguyen :ntim]

Try pushes in Comment 16 & 17. The failures seem unrelated to this patch.

Comment 23

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/9ff58e281781

Comment 24

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ff58e281781