Implement a method to retrieve usage data for all origins at once

RESOLVED FIXED in Firefox -esr52

Status

()

enhancement
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: janv, Assigned: janv)

Tracking

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr5257+ fixed, firefox55 fixed)

Details

(Whiteboard: [storage-v1])

Attachments

(13 attachments, 1 obsolete attachment)

23.62 KB, patch
bevis
: review+
Details | Diff | Splinter Review
22.07 KB, patch
bevis
: review+
Details | Diff | Splinter Review
14.95 KB, patch
bevis
: review+
Details | Diff | Splinter Review
32.10 KB, patch
bevis
: review+
Details | Diff | Splinter Review
5.61 KB, patch
bevis
: review+
Details | Diff | Splinter Review
21.69 KB, patch
bevis
: review+
Details | Diff | Splinter Review
2.05 KB, patch
Details | Diff | Splinter Review
19.15 KB, patch
Details | Diff | Splinter Review
20.54 KB, patch
Details | Diff | Splinter Review
19.68 KB, patch
Details | Diff | Splinter Review
15.05 KB, patch
Details | Diff | Splinter Review
32.81 KB, patch
Details | Diff | Splinter Review
5.64 KB, patch
Details | Diff | Splinter Review
No description provided.
Andrew, if you don't have time to review this, let me know.
This new method is needed for new storage preferences to get a list of all origins.
(In reply to Jan Varga [:janv] from comment #7)
> Andrew, if you don't have time to review this, let me know.
> This new method is needed for new storage preferences to get a list of all
> origins.

Unfortunately, I do need to focus on multi-e10s and ServiceWorker issues for the near term (there's a face-2-face in 2 weeks).  Also, I don't think I have any exclusive-to-me mental-state on these changes, so this is probably a good opportunity to let Bevis or others hacking in IndexedDB/QuotaManager/Storage enjoy additional review fun ;)  Setting NI for you to redirect reviews.
Flags: needinfo?(jvarga)
Flags: needinfo?(jvarga)
Attachment #8848907 - Flags: review?(bugmail) → review?(btseng)
Attachment #8848908 - Flags: review?(bugmail) → review?(btseng)
Attachment #8848909 - Flags: review?(bugmail) → review?(btseng)
Attachment #8848910 - Flags: review?(bugmail) → review?(btseng)
Attachment #8848912 - Flags: review?(bugmail) → review?(btseng)
Attachment #8848913 - Flags: review?(bugmail) → review?(btseng)
Shawn, you attended last two meetings about new storage preferences. Can you please give Bevis some background for this bug or ask someone else involved in new storage preferences to do it ? Thanks.
Flags: needinfo?(shuang)
(In reply to Andrew Sutherland [:asuth] from comment #8)
> Unfortunately, I do need to focus on multi-e10s and ServiceWorker issues for
> the near term (there's a face-2-face in 2 weeks).

Ok, no problem.
(In reply to Jan Varga [:janv] from comment #9)
> Shawn, you attended last two meetings about new storage preferences. Can you
> please give Bevis some background for this bug or ask someone else involved
> in new storage preferences to do it ? Thanks.

Hi Jan,
Sure, I've already involved this and explain to Bevis.
Flags: needinfo?(shuang)
Comment on attachment 8848907 [details] [diff] [review]
Part 1: Convert nsIQuotaUsageRequest result related attributes to a new structure nsIQuotaUsageResult and expose it using a new result attribute of type nsIVariant

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

Please see my comments below.

Thanks!

I'll review the rest of parts tomorrow.

::: dom/quota/StorageManager.cpp
@@ +145,4 @@
>    if (NS_WARN_IF(NS_FAILED(rv))) {
>      return rv;
>    }
>  

NS_Free(iid);

@@ +158,2 @@
>    aStorageEstimate.mUsage.Construct() = usage;
>    aStorageEstimate.mQuota.Construct() = limit;

I think we could simplify these lines as followed:
  MOZ_ALWAYS_SUCCEEDS(
    usageResult->GetUsage(&aStorageEstimate.mUsage.Construct()));

  MOZ_ALWAYS_SUCCEEDS(
    usageResult->GetLimit(&aStorageEstimate.mQuota.Construct()));

::: dom/quota/nsIQuotaRequests.idl
@@ +21,5 @@
>  
>  [scriptable, uuid(166e28e6-cf6d-4927-a6d7-b51bca9d3469)]
>  interface nsIQuotaUsageRequest : nsIQuotaRequestBase
>  {
> +  [must_use] readonly attribute nsIVariant result;

It will be nice to explain that the *result* here represents various types (nsIQuotaUsageResult, nsIQuotaOriginUsageResult) to be defined in nsIQuotaUsageResult.idl.
Attachment #8848907 - Flags: review?(btseng)
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #12)
> ::: dom/quota/StorageManager.cpp
> @@ +145,4 @@
> >    if (NS_WARN_IF(NS_FAILED(rv))) {
> >      return rv;
> >    }
> >  
> 
> NS_Free(iid);

Ok, good catch, although NS_Free shouldn't be used outside the xpcom glue, just "free"

> 
> @@ +158,2 @@
> >    aStorageEstimate.mUsage.Construct() = usage;
> >    aStorageEstimate.mQuota.Construct() = limit;
> 
> I think we could simplify these lines as followed:
>   MOZ_ALWAYS_SUCCEEDS(
>     usageResult->GetUsage(&aStorageEstimate.mUsage.Construct()));
> 
>   MOZ_ALWAYS_SUCCEEDS(
>     usageResult->GetLimit(&aStorageEstimate.mQuota.Construct()));
> 

Ok, this is now possible since GetUsage() and GetLimit() are now infallible.

> ::: dom/quota/nsIQuotaRequests.idl
> @@ +21,5 @@
> >  
> >  [scriptable, uuid(166e28e6-cf6d-4927-a6d7-b51bca9d3469)]
> >  interface nsIQuotaUsageRequest : nsIQuotaRequestBase
> >  {
> > +  [must_use] readonly attribute nsIVariant result;
> 
> It will be nice to explain that the *result* here represents various types
> (nsIQuotaUsageResult, nsIQuotaOriginUsageResult) to be defined in
> nsIQuotaUsageResult.idl.

Ok, I'll add a comment.
Blocks: 1348733
Comment on attachment 8848908 [details] [diff] [review]
Part 2: Rename usage related methods and structures to express the relation to a concrete origin

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

::: dom/indexedDB/test/file.js
@@ +222,5 @@
>  {
>    testGenerator.next(request.result.fileUsage);
>  }
>  
> +function getContentUsage(usageHandler)

nit:
Is there any use case of getContentUsage() that is different from the one of getCurrentOriginUsage() in indexedDB tests?
If no, I'd prefer to rename it to GetCurrentOriginUsage() if this is the only use case for testing.

::: dom/indexedDB/test/unit/xpcshell-head-parent-process.js
@@ +518,5 @@
>  {
>    testGenerator.next(request.result.fileUsage);
>  }
>  
> +function getChromeUsage(usageHandler)

ditto:
Is there any use case of getChromeUsage() that is different from the one of getCurrentOriginUsage() in indexedDB xpcshell tests?
If no, I'd prefer to rename it to GetCurrentOriginUsage() if this is the only use case for testing.

::: dom/quota/StorageManager.cpp
@@ +155,3 @@
>  
>    uint64_t limit;
> +  MOZ_ALWAYS_SUCCEEDS(originUsageResult->GetLimit(&limit));

Same simplification as suggested in patch part 1.
Attachment #8848908 - Flags: review?(btseng) → review+
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #14)
> ::: dom/indexedDB/test/file.js
> @@ +222,5 @@
> >  {
> >    testGenerator.next(request.result.fileUsage);
> >  }
> >  
> > +function getContentUsage(usageHandler)
> 
> nit:
> Is there any use case of getContentUsage() that is different from the one of
> getCurrentOriginUsage() in indexedDB tests?
> If no, I'd prefer to rename it to GetCurrentOriginUsage() if this is the
> only use case for testing.

I plan to add getOriginUsage(principal, usageHandler) especially for xpcshell tests
Having getOriginUsage() and getChromeUsage() sounds a bit better than
getOriginUsage() and getCurrentOriginUsage().

I have to add getCurrentOriginUsage() just because there's a test which runs as
a mochitest and also as an xpcshell test.

I'm also considering to rename getCurrentOriginUsage() to getDefaultOriginUsage().
(In reply to Jan Varga [:janv] from comment #15)
> I plan to add getOriginUsage(principal, usageHandler) especially for
> xpcshell tests
> Having getOriginUsage() and getChromeUsage() sounds a bit better than
> getOriginUsage() and getCurrentOriginUsage().
This makes sense to me but I thought that these tests will be added in dom/quota/test.
Will these happen in dom/indexedDB/test as well?
If yes, we need these namings for better understanding.
> I have to add getCurrentOriginUsage() just because there's a test which runs
> as a mochitest and also as an xpcshell test.
Yes, that's also my understanding. ;)
> I'm also considering to rename getCurrentOriginUsage() to
> getDefaultOriginUsage().
Ok.
Comment on attachment 8848909 [details] [diff] [review]
Part 3: Separate the canceled state out of UsageInfo

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

::: dom/indexedDB/ActorsParent.cpp
@@ +17830,5 @@
>  {
>    AssertIsOnIOThread();
>    MOZ_ASSERT(aDirectory);
>  
> +  AtomicBool canceled;

nit:
  AtomicBool dummy(false);

::: dom/quota/ActorsParent.cpp
@@ +4196,5 @@
>        UNKNOWN_FILE_WARNING(leafName);
>        return NS_ERROR_UNEXPECTED;
>      }
>  
> +    Atomic<bool> canceled;

nit:
    Atomic<bool> dummy(false);
Attachment #8848909 - Flags: review?(btseng) → review+
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #16)
> (In reply to Jan Varga [:janv] from comment #15)
> > I plan to add getOriginUsage(principal, usageHandler) especially for
> > xpcshell tests
> > Having getOriginUsage() and getChromeUsage() sounds a bit better than
> > getOriginUsage() and getCurrentOriginUsage().
> This makes sense to me but I thought that these tests will be added in
> dom/quota/test.
> Will these happen in dom/indexedDB/test as well?
> If yes, we need these namings for better understanding.

Yes, I'm adding new tests to dom/quota/test, but I'm also renaming getUsage() in dom/indexedDB/test
to use the same naming as much as possible.
(In reply to Jan Varga [:janv] from comment #18)
> Yes, I'm adding new tests to dom/quota/test, but I'm also renaming
> getUsage() in dom/indexedDB/test
> to use the same naming as much as possible.
OK!
Comment on attachment 8848910 [details] [diff] [review]
Part 4: Extract common code from GetOriginUsageOp to a new base class QuotaUsageRequestBase

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

Looks great, thanks!
Attachment #8848910 - Flags: review?(btseng) → review+
Attachment #8848912 - Flags: review?(btseng) → review+
Attachment #8848913 - Flags: review?(btseng) → review+
Comment on attachment 8849574 [details] [diff] [review]
Part 1: Convert nsIQuotaUsageRequest result related attributes to a new structure nsIQuotaUsageResult and expose it using a new result attribute of type nsIVariant

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

::: dom/quota/nsIQuotaRequests.idl
@@ +22,5 @@
>  [scriptable, uuid(166e28e6-cf6d-4927-a6d7-b51bca9d3469)]
>  interface nsIQuotaUsageRequest : nsIQuotaRequestBase
>  {
> +  // The result can contain one of these types:
> +  //   nsIQuotaUsageResult

I'll add nsIQuotaOriginUsageResult here in Part 5 patch
Comment on attachment 8849574 [details] [diff] [review]
Part 1: Convert nsIQuotaUsageRequest result related attributes to a new structure nsIQuotaUsageResult and expose it using a new result attribute of type nsIVariant

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

Thank you!
Attachment #8849574 - Flags: review?(btseng) → review+
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea3c6e12b499
Part 1: Convert nsIQuotaUsageRequest result related attributes to a new structure nsIQuotaUsageResult and expose it using a new result attribute of type nsIVariant; r=btseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/c0d0402b8bb0
Part 2: Rename usage related methods and structures to express the relation to a concrete origin; r=btseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9f06917c82a
Part 3: Separate the canceled state out of UsageInfo; r=btseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/24244ba9a144
Part 4: Extract common code from GetOriginUsageOp to a new base class QuotaUsageRequestBase; r=btseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/2c3319501b59
Part 5: Implement a method to retrieve usage data for all origins at once; r=btseng
https://hg.mozilla.org/integration/mozilla-inbound/rev/21063e15627e
Part 6: Rename QuotaManager::IsOriginWhitelistedForPersistentStorage() to QuotaManager::IsOriginInternal(); r=btseng
Depends on: 1350564
Whiteboard: [storage-v1]
Part 6 is not needed for ESR52.
Can you request uplift to ESR? Ryan mentioned we need this for bug 1047098. Thanks.
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #34)
> Can you request uplift to ESR? Ryan mentioned we need this for bug 1047098.
> Thanks.

I think bug 1047098 shouldn't land on ESR until we solve bug 1404105.
(In reply to Jan Varga [:janv] from comment #33)
> Part 6 is not needed for ESR52.

I changed my mind, Part 6 can land on ESR52 too.
Comment on attachment 8911847 [details] [diff] [review]
Part 1 for ESR52

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is needed for bug 1047098 which already has ESR52 approval.
User impact if declined: See bug 1047098.
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Low risk
String or UUID changes made by this patch: None

The same approval request applies to all 6 patches "Part 1-6 for ESR52".
Attachment #8911847 - Flags: approval-mozilla-esr52?
Comment on attachment 8911847 [details] [diff] [review]
Part 1 for ESR52

A lot of code change for ESR but since we want to fix bug 1047098, let's risk it.
Attachment #8911847 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+
Attachment #8911849 - Flags: approval-mozilla-esr52+
Attachment #8911850 - Flags: approval-mozilla-esr52+
Attachment #8911851 - Flags: approval-mozilla-esr52+
Attachment #8911852 - Flags: approval-mozilla-esr52+
Attachment #8921849 - Flags: approval-mozilla-esr52+
See Also: → 1490784
You need to log in before you can comment on or make changes to this bug.