Closed Bug 1196315 Opened 5 years ago Closed 4 years ago

[DeviceStorage] Consolidate and cache accesses to MIME service

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
FxOS-S8 (02Oct)
Tracking Status
firefox44 --- fixed

People

(Reporter: aosmond, Assigned: aosmond)

References

Details

(Whiteboard: [NG Device Storage])

Attachments

(1 file, 2 obsolete files)

Whiteboard: [NG Device Storage]
Depends on: 1171170
Target Milestone: --- → FxOS-S6 (04Sep)
Patch does not contain caching, there is actually not much point since the cursor request is the only one which benefits, and when in the child process (primary use case), it will need to post to the main thread *anyways* to send the open request. Caching can be revisited if/when cursors are updated to operate on blocks of files internally and/or when the IPC can be accessed off main thread.
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Fix how it should have returned early in ::AddOrAppendNamed.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=35f6c771b9ff
Attachment #8661881 - Attachment is obsolete: true
Whoops, patch I paired it with had trouble, try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe38d86b57d3
Attachment #8662115 - Flags: review?(dhylands)
Comment on attachment 8662115 [details] [diff] [review]
[gecko] consolidate access to MIME service, v2

Review of attachment 8662115 [details] [diff] [review]:
-----------------------------------------------------------------

Just a few cleanups. r=me with those done.

::: dom/devicestorage/nsDeviceStorage.cpp
@@ +1893,5 @@
>  class DeviceStorageCreateRequest final
>    : public DeviceStorageRequest
>  {
>  public:
> +  using DeviceStorageRequest::Initialize;

what does this do?

It's already declared public in the base class.

@@ +1967,5 @@
> +    // will post an onerror to the requestee.
> +
> +    // possible race here w/ unique filename
> +    char buffer[32];
> +    NS_MakeRandomString(buffer, ArrayLength(buffer) - 1);

Shouldn't this use DeviceStorageFile::CreateUnique? Or factor out some functionality from that so that we don't have a race/duplicate filename here.

I see you're just copying the original code here, but we might as well fix it properly.

@@ +3086,3 @@
>    nsCOMPtr<nsIRunnable> r;
>  
> +  if (!aPath.IsEmpty() && IsFullPath(aPath)) {

IsFullPath already checks for aPath.Length() > 0 so why the extra check?
Attachment #8662115 - Flags: review?(dhylands) → review+
(In reply to Dave Hylands [:dhylands] from comment #5)
> Comment on attachment 8662115 [details] [diff] [review]
> [gecko] consolidate access to MIME service, v2
> 
> Review of attachment 8662115 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Just a few cleanups. r=me with those done.
> 
> ::: dom/devicestorage/nsDeviceStorage.cpp
> @@ +1893,5 @@
> >  class DeviceStorageCreateRequest final
> >    : public DeviceStorageRequest
> >  {
> >  public:
> > +  using DeviceStorageRequest::Initialize;
> 
> what does this do?
> 
> It's already declared public in the base class.
> 

Compile breakage. I think it is because I overload it. Had to do the same thing in DeviceStorageOpenRequest.

> @@ +1967,5 @@
> > +    // will post an onerror to the requestee.
> > +
> > +    // possible race here w/ unique filename
> > +    char buffer[32];
> > +    NS_MakeRandomString(buffer, ArrayLength(buffer) - 1);
> 
> Shouldn't this use DeviceStorageFile::CreateUnique? Or factor out some
> functionality from that so that we don't have a race/duplicate filename here.
> 
> I see you're just copying the original code here, but we might as well fix
> it properly.
> 

Done. Although it still uses NS_MakeRandomString, but ::CreateUnique will ensure that is actually unique and yield a new DeviceStorageFile instead of reusing the old one.

> @@ +3086,3 @@
> >    nsCOMPtr<nsIRunnable> r;
> >  
> > +  if (!aPath.IsEmpty() && IsFullPath(aPath)) {
> 
> IsFullPath already checks for aPath.Length() > 0 so why the extra check?

Ugh, good point :).
Attachment #8662115 - Attachment is obsolete: true
Attachment #8664747 - Flags: review+
(In reply to Andrew Osmond [:aosmond] from comment #6)
> Created attachment 8664747 [details] [diff] [review]
> [gecko] consolidate access to MIME service, v3 [carries r=dhylands]
> 
> (In reply to Dave Hylands [:dhylands] from comment #5)
> > Comment on attachment 8662115 [details] [diff] [review]
> > [gecko] consolidate access to MIME service, v2
> > 
> > Review of attachment 8662115 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Just a few cleanups. r=me with those done.
> > 
> > ::: dom/devicestorage/nsDeviceStorage.cpp
> > @@ +1893,5 @@
> > >  class DeviceStorageCreateRequest final
> > >    : public DeviceStorageRequest
> > >  {
> > >  public:
> > > +  using DeviceStorageRequest::Initialize;
> > 
> > what does this do?
> > 
> > It's already declared public in the base class.
> > 
> 
> Compile breakage. I think it is because I overload it. Had to do the same
> thing in DeviceStorageOpenRequest.
> 

Err that is to say, because I *override* it *and* call the original function inside the overridden function.
https://hg.mozilla.org/mozilla-central/rev/672e0e69482e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: FxOS-S6 (04Sep) → FxOS-S8 (02Oct)
You need to log in before you can comment on or make changes to this bug.