Closed
Bug 885753
Opened 8 years ago
Closed 8 years ago
DeviceStorage: Change getDeviceStorage to return the default storage area.
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
People
(Reporter: dhylands, Assigned: dhylands)
References
Details
Attachments
(2 files)
39.19 KB,
patch
|
janv
:
review+
|
Details | Diff | Splinter Review |
39.32 KB,
patch
|
dhylands
:
review+
|
Details | Diff | Splinter Review |
Over in bug 874313 comment 25, Jonas made a suggestion that I would like to refine a bit further. I'd like to propose that rather than having getDeviceStorage return a composite storage area, it should just return the storage area that corresponds with the default one (the storage area whose .default attribute would be true). This would imply the following: - to a get a list of all files on all storage areas, you would need to enumerate through the individual storage areas (i.e. getDeviceStorages) - to get total free space/used space, you would need to enuerate through the individual storage areas. It would make sense that the settings app would just show the individual storage areas and not show a composite one at all. - I still like the idea of being able to pass in /storagename/foo.txt to be able to manipulate a file from a particular storage area, so I think it makes sense to retain this behaviour (for all device storage areas rather than just the composite one). And I think we should continue to return fully qualified names. So if you call Add('foo.txt') it should still return /sdcard/foo.txt (if you were trying to add to the sdcard storage area).
Comment 1•8 years ago
|
||
nominate leo? because it's highly related to bug 874313 which is a leo+.
Updated•8 years ago
|
blocking-b2g: --- → leo?
Comment 2•8 years ago
|
||
It's not clear why this needs to block 1.1. What's the user impact? Why can't these changes to in bug 874313 if they're absolutely required? Can we fix this in 1.2 instead?
blocking-b2g: leo? → -
Assignee | ||
Comment 3•8 years ago
|
||
Bug 874313 is for gaia work, this bug is for gecko work (which is why the two pieces of work are in separate bugs).
Comment 4•8 years ago
|
||
(In reply to Dave Hylands [:dhylands] (PTO back July 2) from comment #3) > Bug 874313 is for gaia work, this bug is for gecko work (which is why the > two pieces of work are in separate bugs). Dave, thanks for your confirm. Yes, so we need both fixed and landed so the view will match behavior. Nominate again.
blocking-b2g: - → leo?
Updated•8 years ago
|
Comment 5•8 years ago
|
||
This leo+ is conditional upon this being the lowest risk method of fixing bug 874313
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → dhylands
Assignee | ||
Comment 6•8 years ago
|
||
This patch removes most of the support for composite device storage, and makes getDeviceStorage return the default storage area. The remnants of composite storage which remain are that AddNamed, Delete, Get allow fully qualified storage areas to be specified.
Attachment #775032 -
Flags: review?(Jan.Varga)
Comment 8•8 years ago
|
||
This bug is a month old and this is the first I've heard of it.... MediaDB still uses the composite storage object, and if I understand correctly, this patch will break Gallery, Video and Music on Leo devices or any device that has more than one device storage area. Dave: is there a corresponding bug for fixing mediadb.js to match this change?
Assignee | ||
Comment 9•8 years ago
|
||
Jan - any ETA on the review? I can find someone else if you're too busy.
Flags: needinfo?(dhylands) → needinfo?(Jan.Varga)
Comment 11•8 years ago
|
||
Comment on attachment 775032 [details] [diff] [review] Removes most of the composite storage support Review of attachment 775032 [details] [diff] [review]: ----------------------------------------------------------------- looks good ::: dom/devicestorage/DeviceStorage.h @@ +167,4 @@ > const nsAString& aVolName); > > bool IsAvailable(); > + bool IsFullPath(const nsAString& aPath) { return aPath.Length() > 0 && aPath.CharAt(0) == '/'; } Nit: 80 chars per line @@ +230,5 @@ > nsDOMDeviceStorage** aStore); > > static void CreateDeviceStoragesFor(nsPIDOMWindow* aWin, > const nsAString& aType, > + nsTArray<nsRefPtr<nsDOMDeviceStorage> >& aStores); dtto ::: dom/devicestorage/DeviceStorageRequestParent.cpp @@ +359,5 @@ > CopyASCIItoUTF16(mMimeType, mime); > > + nsString fullPath; > + mFile->GetFullPath(fullPath); > + nsCOMPtr<nsIDOMBlob> blob = new nsDOMFileFile(fullPath, mime, mLength, mFile->mFile, mLastModificationDate); dtto ::: dom/devicestorage/nsDeviceStorage.cpp @@ +1296,5 @@ > // TODO - needs janv's file handle support. > return JSVAL_NULL; > } > > + nsString fullPath; Nit: extra space @@ +2449,5 @@ > // static > void > nsDOMDeviceStorage::CreateDeviceStoragesFor(nsPIDOMWindow* aWin, > const nsAString &aType, > + nsTArray<nsRefPtr<nsDOMDeviceStorage> > &aStores) Nit: 80 chars per line @@ +2507,5 @@ > return true; > } > > already_AddRefed<nsDOMDeviceStorage> > +nsDOMDeviceStorage::GetStorage(const nsAString& aFullPath, nsAString& aOutStoragePath) dtto @@ +2555,1 @@ > nsAString& aStorageName) Nit: indentation @@ +3075,5 @@ > } > > #ifdef MOZ_WIDGET_GONK > else if (!strcmp(aTopic, NS_VOLUME_STATE_CHANGED)) { > + // We invalidate the used space cache for the volume that actually changed state. Nit: 80 chars per line
Attachment #775032 -
Flags: review?(Jan.Varga) → review+
Assignee | ||
Updated•8 years ago
|
Assignee | ||
Comment 12•8 years ago
|
||
https://hg.mozilla.org/integration/b2g-inbound/rev/046e78d8a522 https://hg.mozilla.org/integration/b2g-inbound/rev/b8bb4f63c226
Whiteboard: [fixed-in-birch]
Comment 14•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/046e78d8a522 https://hg.mozilla.org/mozilla-central/rev/b8bb4f63c226
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 15•8 years ago
|
||
Needs branch-specific patches for uplift.
status-b2g18:
--- → affected
status-b2g18-v1.0.0:
--- → wontfix
status-b2g18-v1.0.1:
--- → wontfix
status-b2g-v1.1hd:
--- → affected
status-firefox24:
--- → wontfix
status-firefox25:
--- → wontfix
status-firefox26:
--- → fixed
Flags: needinfo?(dhylands)
Keywords: branch-patch-needed
Whiteboard: [fixed-in-birch]
Assignee | ||
Comment 16•8 years ago
|
||
Carrying r+ This patch doesn't have the whitespace changes, just the functional ones.
Attachment #789014 -
Flags: review+
Flags: needinfo?(dhylands)
Comment 17•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-b2g18/rev/bfd08abe2115
Keywords: branch-patch-needed
You need to log in
before you can comment on or make changes to this bug.
Description
•