Factor shared/js/device_storage_utils.js

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: dhylands, Assigned: pdahiya)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
shared/device_storage_utils.js contains 2 functions related to device storage, but which wouldn't necessarily be used together.

Bug 893282 will be adding an enumerateAll function to mediadb.js

djf and I talked about this and thought that having shared/js/devicestorage with 3 separate files (one function per file) made more sense. Then an app can easily merge things together into a single file if desired, and doesn't need to pull in more than what's required.

So this bug is for splitting shared/js/device_storage_utils.js into 2 separate files, and extracting enmuerateAll from mediadb.js and creating shared/js/devicestorage as described above.
(Reporter)

Updated

5 years ago
Depends on: 893282
(Assignee)

Updated

5 years ago
Assignee: nobody → pdahiya
(Assignee)

Updated

5 years ago
Depends on: 901410
No longer depends on: 893282
(Assignee)

Updated

5 years ago
Depends on: 893282
(Assignee)

Comment 1

5 years ago
Created attachment 786582 [details] [diff] [review]
PR with the fix for Bug 900159

Attaching PR with the fix. 

Created 2 new files shared/js/device_storage/device_storage_access.js and shared/js/device_storage/device_storage_helper.js for functions inside shared/js/device_storage_utils.js.

Extracted enumerateAll() function from mediadb.js into a separate file shared/js/device_storage/device_all_storages.js.
Attachment #786582 - Flags: review?(dhylands)
Attachment #786582 - Flags: review?(dflanagan)
Comment on attachment 786582 [details] [diff] [review]
PR with the fix for Bug 900159

I don't think it makes sense to put these functions into namespace objects like you've done. If each file is going to define a single symbol in the global namespace, it seems to me that it ought to just define a single function rather than defining a single object and putting the function in it.

I'd prefer to just have these as three global functions, with filenames that match the function names.

Alternatively, we could have each of the three files define a global DeviceStorageUtils object if it doesn't already exist and then define functions as properties of that.  But really, I'd prefer to just leave them as global functions.
Attachment #786582 - Flags: review?(dflanagan) → review-
(Reporter)

Comment 3

5 years ago
Comment on attachment 786582 [details] [diff] [review]
PR with the fix for Bug 900159

I was going to pass this on to djf to review, and I see he's done it anyways...
Attachment #786582 - Flags: review?(dhylands)
(Assignee)

Comment 4

5 years ago
(In reply to David Flanagan [:djf] from comment #2)
> Comment on attachment 786582 [details] [diff] [review]
> PR with the fix for Bug 900159
> 
> I don't think it makes sense to put these functions into namespace objects
> like you've done. If each file is going to define a single symbol in the
> global namespace, it seems to me that it ought to just define a single
> function rather than defining a single object and putting the function in it.
> 
> I'd prefer to just have these as three global functions, with filenames that
> match the function names.
> 
> Alternatively, we could have each of the three files define a global
> DeviceStorageUtils object if it doesn't already exist and then define
> functions as properties of that.  But really, I'd prefer to just leave them
> as global functions.

My only thoughts behind creating name space objects was, if in future we add more function to the respective files than those will be under single global object instead of multiple global functions. I have updated PR to use global functions. Please review.
(Assignee)

Updated

5 years ago
Attachment #786582 - Flags: review- → review?(dflanagan)
Comment on attachment 786582 [details] [diff] [review]
PR with the fix for Bug 900159

Thanks, Punam. This updated pull request looks great.
Attachment #786582 - Flags: review?(dflanagan) → review+
But it is bitrotted. Please fix the merge conflict and ping me (or Dave Hylands, as I'll be OOO early next week) to land it.
(Assignee)

Comment 7

5 years ago
I have resolved merge conflicts and updated the pull request. Dave, Please land it on master. Thanks!
Flags: needinfo?(dhylands)
(Reporter)

Comment 8

5 years ago
https://github.com/mozilla-b2g/gaia/pull/11387
https://github.com/mozilla-b2g/gaia/commit/686e308a758bbbd328d52a3cf16cba697cf5f6d9

Travis was failing, but it seems to be from earlier merges and not related to these changes.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: needinfo?(dhylands)
Resolution: --- → FIXED

Updated

5 years ago
Blocks: 907052
You need to log in before you can comment on or make changes to this bug.