Closed Bug 1267941 Opened 4 years ago Closed 3 years ago

Implement Storage API estimate() feature

Categories

(Core :: DOM: Core & HTML, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete, Whiteboard: [storage-v1])

Attachments

(2 files, 56 obsolete files)

2.53 KB, patch
Details | Diff | Splinter Review
35.73 KB, patch
Details | Diff | Splinter Review
Implement Storage API estimate() feature
Whiteboard: btpp-fixlater
Assignee: nobody → shuang
Shall we put StorageManager related code under dom/quota? Do you suggest we shall create an another folder?
Flags: needinfo?(jvarga)
I don't think we need a new directory, but I need to see how new files are named before saying my final word.
Flags: needinfo?(jvarga)
(In reply to Jan Varga [:janv] from comment #2)
> I don't think we need a new directory, but I need to see how new files are
> named before saying my final word.

I will name it something like StorageManager.
Flags: needinfo?(jvarga)
Yeah, yeah, but I'd like to see actual files .h .cpp before saying it can live together with the original dom/quota
Flags: needinfo?(jvarga)
Record the discussion about the implementation on IRC [1]-[2] to keep ourselves remembering that.

09:37	shawnjohnjr	janv:why do we have 'group limit' instead of quota for each origin? is there any special reason?
09:52	janv	shawnjohnjr: that's the eTLD+1 grouping
10:10	tt	janv: But why don't we have a quota limit for a origin?
10:11	janv	tt: that would be a problem for some sites, like wikipedia.org
10:11	janv	err
10:11	janv	never mind
10:12	janv	tt: once you enable the limit just for origins, then one can create multiple subdomains and fill entire storage area
10:13	janv	tt: http://www.filldisk.com/
10:19	tt	janv:Yes, but having multiple limitations(origin limit, group limit and global limit together) can avoid that?
10:25	janv	tt: group limit and the global limit works just fine
10:25	janv	tt: we are immune to the filldisk hack
10:26	tt	janv: OK, I see.
10:26	tt	janv: thanks
10:26	janv	sure
10:30	shawnjohnjr	janv: the reason i asked is because storage api specification said "site storage unit" ties to a origin. so i wonder the quota we return should be limit just for origins.
10:30	shawnjohnjr	janv: "The site storage quota of an origin origin is a conserviate estimate of the amount of bytes available to origin’s site storage unit."
10:31	shawnjohnjr	https://storage.spec.whatwg.org/#site-storage-unit
10:31	janv	shawnjohnjr: I guess it should return how many bytes can be used until a quota exceeded is returned
10:32	shawnjohnjr	janv: oh, i see.
10:32	janv	shawnjohnjr: and the global limit actually doesn't apply usually
10:33	janv	when we reach the global limit we try to delete some unused origins
10:33	shawnjohnjr	janv: i see
10:34	janv	shawnjohnjr: not sure how we should account the global limit
10:35	janv	estimate should probably only check the group limit
10:53	shawnjohnjr	janv:if we only return group limit as quota, then I guess one single origin won't reflect the true quota per a origin. ex: www1.mozilla.org gets group limit 2GB, www1 current usage per origin is 1GB, www2.mozilla.org also gets group limit 2GB, www2 current usage per origin is 500MB. Both origins consider they can consume total 2GB quota. But actually they
10:53	shawnjohnjr	can't. If I understand the specification correctly.
10:59	shawnjohnjr	annevk: is it ok to return group limit as a quota value
11:00	shawnjohnjr	annevk: if we enable quota per a origin, we will have problem that janv just mentioned
11:03	janv	shawnjohnjr: yeah, discuss with annevk please
11:03	shawnjohnjr	janv: thanks for pointing out the problem
11:04	janv	shawnjohnjr: anyway, they call it "estimate" by a reason, since the value is dynamic
11:04	janv	there can be multiple workers writing to databases even of the same origin
11:04	shawnjohnjr	janv: you're right
13:01	annevk	shawnjohnjr: it would be better if we did away with groups I think
13:01	annevk	shawnjohnjr: walking around today though so not working
13:05	shawnjohnjr	annevk: so you think we should get rid of group limit?
13:08	shawnjohnjr	annevk: we still need to consider diskfill hack.
13:10	annevk	shawnjohnjr: yeah, but that is rather easy anyway
13:11	annevk	shawnjohnjr: maybe somewhat more expensive today…
13:12	annevk	shawnjohnjr: I think basing things around origins and thinking of more general protections to abuse will serve us better
13:13	shawnjohnjr	annevk: okay
13:16	annevk	shawnjohnjr: I believe Chrome plans to experiment with having close to no limit
13:16	annevk	Just let a site use as much as it needs
13:16	annevk	And then kick it out when the user switches
13:21	shawnjohnjr	annevk: site means 'origin', right?
13:22	shawnjohnjr	Not eTLD+1 here
13:24	annevk	shawnjohnjr: yeah, I think only we do eTLD+1 

[1] http://logs.glob.uno/?c=mozilla%23content&s=6+May+2016&e=6+May+2016#c372742
[2] http://logs.glob.uno/?c=mozilla%23content&s=6+May+2016&e=6+May+2016#c372788
Hi Janv,
I have some doubts on the ipc architecture for Storage API.
I'm wondering what kind of architecture is correct for Storage API case.

(1) in content process, implement one StorageManagerChild actor just like QuotaChild in QuotaManagerService, directly send IPC messages to PQuotaParent to call |GetUsageForPrincipal|. Ignore existing XPCOM QuotaManagerService interface. The benefit we only need to do IPC once but we probably may have redundant code in StorageManager. 

or 
(2) in content process, implement StorageManagerChild/StorageManagerParent actor and StorageManagerChild sends IPC messages to StorageManagerParent in webapi code, StorageManagerParent call QuotaManagerService::GetUsageForPrincipal via XPCOM interface in chrome process. The benefit is to leverage existing XPCOM service in chrome process but we need to do things in PBackground twice.
Flags: needinfo?(jvarga)
I hoped we could just call QuotaManagerService methods in content process. QuotaManagerService is a client for PQuotaParent. If we need to change QuotaManagerService methods or add new ones, I think that's fine.
Flags: needinfo?(jvarga)
Hi Jan,

Just make sure I understand. We need to create another client for PQuotaParent (communicate with QuotaManager directly rather than through QuotaManagerService) or change functions in QuotaManagerService and make it can be called in content process? Thanks!
Flags: needinfo?(jvarga)
(In reply to Tom Tung [:tt] from comment #9)
> Hi Jan,
> 
> Just make sure I understand. We need to create another client for
> PQuotaParent (communicate with QuotaManager directly rather than through
> QuotaManagerService) or change functions in QuotaManagerService and make it
> can be called in content process? Thanks!

The latter, QuotaManagerService is already available in content processes.
Flags: needinfo?(jvarga)
(In reply to Jan Varga [:janv] from comment #10)
> (In reply to Tom Tung [:tt] from comment #9)
> > Hi Jan,
> > 
> > Just make sure I understand. We need to create another client for
> > PQuotaParent (communicate with QuotaManager directly rather than through
> > QuotaManagerService) or change functions in QuotaManagerService and make it
> > can be called in content process? Thanks!
> 
> The latter, QuotaManagerService is already available in content processes.

Thanks, Jan. Then I will stop working on adding PBackgroundChild and try to modify QuotaManagerService.
(In reply to Shawn Huang [:shawnjohnjr] from comment #12)
> Created attachment 8759067 [details] [diff] [review]
> bug1267941.patch

Yeah, I believe it doesn't make sense to create a separate parent/child for this.
(In reply to Jan Varga [:janv] from comment #10)
> (In reply to Tom Tung [:tt] from comment #9)
> > Hi Jan,
> > 
> > Just make sure I understand. We need to create another client for
> > PQuotaParent (communicate with QuotaManager directly rather than through
> > QuotaManagerService) or change functions in QuotaManagerService and make it
> > can be called in content process? Thanks!
> 
> The latter, QuotaManagerService is already available in content processes.
https://dxr.mozilla.org/mozilla-central/source/dom/quota/QuotaManagerService.cpp#505
https://dxr.mozilla.org/mozilla-central/source/dom/quota/QuotaManagerService.cpp#566

I thought QuotaManagerService is for Chrome process only because of the assertion. It looks like I was wrong.
Whiteboard: btpp-fixlater → btpp-active
(In reply to Shawn Huang [:shawnjohnjr] from comment #14)
> (In reply to Jan Varga [:janv] from comment #10)
> > (In reply to Tom Tung [:tt] from comment #9)
> > > Hi Jan,
> > > 
> > > Just make sure I understand. We need to create another client for
> > > PQuotaParent (communicate with QuotaManager directly rather than through
> > > QuotaManagerService) or change functions in QuotaManagerService and make it
> > > can be called in content process? Thanks!
> > 
> > The latter, QuotaManagerService is already available in content processes.
> https://dxr.mozilla.org/mozilla-central/source/dom/quota/QuotaManagerService.
> cpp#505
> https://dxr.mozilla.org/mozilla-central/source/dom/quota/QuotaManagerService.
> cpp#566
> 
> I thought QuotaManagerService is for Chrome process only because of the
> assertion. It looks like I was wrong.

Chrome process and caller chrome is not the same. The service is definitely used in content processes.
It was actually created for this purpose.
Attached patch bug1267941.patch (WIP) (obsolete) — Splinter Review
Adding estimate() for querying usage only, still need to handle web worker.
(In reply to Jan Varga [:janv] from comment #15)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #14)
> > (In reply to Jan Varga [:janv] from comment #10)
> > > (In reply to Tom Tung [:tt] from comment #9)
> > > > Hi Jan,
> > > > 
> > > > Just make sure I understand. We need to create another client for
> > > > PQuotaParent (communicate with QuotaManager directly rather than through
> > > > QuotaManagerService) or change functions in QuotaManagerService and make it
> > > > can be called in content process? Thanks!
> > > 
> > > The latter, QuotaManagerService is already available in content processes.
> > https://dxr.mozilla.org/mozilla-central/source/dom/quota/QuotaManagerService.
> > cpp#505
> > https://dxr.mozilla.org/mozilla-central/source/dom/quota/QuotaManagerService.
> > cpp#566
> > 
> > I thought QuotaManagerService is for Chrome process only because of the
> > assertion. It looks like I was wrong.
> 
> Chrome process and caller chrome is not the same. The service is definitely
> used in content processes.
> It was actually created for this purpose.

I always hit the assertion from webapi, so I just wonder if we can remove the assertion. When running mochitest to test estimate() method, webapi calls GetUsageForPrincipal(), i think caller is content window not from chrome, right?
Flags: needinfo?(jvarga)
Comment on attachment 8759609 [details] [diff] [review]
bug1267941.patch (WIP)

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

This just a quick feedback, I think you are going in the right direction now.

::: dom/quota/EstimateUsageCallback.cpp
@@ +17,5 @@
> +NS_IMPL_ISUPPORTS(EstimateUsageCallback, nsIQuotaUsageCallback)
> +
> +NS_IMETHODIMP EstimateUsageCallback::OnUsageResult(nsIQuotaUsageRequest *aRequest)
> +{
> +  UsageRequest* r = static_cast<UsageRequest*> (aRequest);

Why do you need a static_cast ?

::: dom/quota/QuotaManagerService.cpp
@@ +501,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(aPrincipal);
>    MOZ_ASSERT(aCallback);
> +//  MOZ_ASSERT(nsContentUtils::IsCallerChrome());

Yes, I think this can now go away.

::: dom/quota/StorageManager.cpp
@@ +62,5 @@
> +    return nullptr;
> +  }
> +
> +  nsCOMPtr<nsIQuotaManagerService> qms =
> +        do_GetService(QUOTAMANAGER_SERVICE_CONTRACTID);

You don't have to go through the service manager here, just call QuotaManagerService::GetOrCreate()
However, that can only be done on the main thread. Worker implementation will have to dispatch a runnable to the main thread ...

::: dom/quota/moz.build
@@ +14,5 @@
>  
>  EXPORTS.mozilla.dom.quota += [
>      'ActorsParent.h',
>      'Client.h',
> +    'EstimateUsageCallback.h',

Why does this need to be exported ?

Actually, why do we need a separate .h/.cpp pair for this ?
It can just live in StorageManager.cpp, no ?
(In reply to Jan Varga [:janv] from comment #18)
> Comment on attachment 8759609 [details] [diff] [review]
> bug1267941.patch (WIP)
> 
> Review of attachment 8759609 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This just a quick feedback, I think you are going in the right direction now.
Thanks! It's a good starting point.
> 
> ::: dom/quota/EstimateUsageCallback.cpp
> @@ +17,5 @@
> > +NS_IMPL_ISUPPORTS(EstimateUsageCallback, nsIQuotaUsageCallback)
> > +
> > +NS_IMETHODIMP EstimateUsageCallback::OnUsageResult(nsIQuotaUsageRequest *aRequest)
> > +{
> > +  UsageRequest* r = static_cast<UsageRequest*> (aRequest);
> 
> Why do you need a static_cast ?
Hmm... I shouldn't do it.
> 
> ::: dom/quota/QuotaManagerService.cpp
> @@ +501,5 @@
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >    MOZ_ASSERT(aPrincipal);
> >    MOZ_ASSERT(aCallback);
> > +//  MOZ_ASSERT(nsContentUtils::IsCallerChrome());
> 
> Yes, I think this can now go away.
Great!
> 
> ::: dom/quota/StorageManager.cpp
> @@ +62,5 @@
> > +    return nullptr;
> > +  }
> > +
> > +  nsCOMPtr<nsIQuotaManagerService> qms =
> > +        do_GetService(QUOTAMANAGER_SERVICE_CONTRACTID);
> 
> You don't have to go through the service manager here, just call
> QuotaManagerService::GetOrCreate()
> However, that can only be done on the main thread. Worker implementation
> will have to dispatch a runnable to the main thread ...
I see, I will change it in later revision.
> 
> ::: dom/quota/moz.build
> @@ +14,5 @@
> >  
> >  EXPORTS.mozilla.dom.quota += [
> >      'ActorsParent.h',
> >      'Client.h',
> > +    'EstimateUsageCallback.h',
> 
> Why does this need to be exported ?
It's not necessary, I will remove it.
> 
> Actually, why do we need a separate .h/.cpp pair for this ?
> It can just live in StorageManager.cpp, no ?
Yeah, I will merge callback into StorageManager.
Flags: needinfo?(jvarga)
Attached patch bug-1267941.patch (WIP) (obsolete) — Splinter Review
Adding estimate() function
So right now, we can expose StorageManager to WorkerNavigator and call estimate(). Next step is to add more test cases.
So the goal (according to the spec) is to be able to do this:

navigator.storage.estimate().then(info => {
  if(info.quota - info.usage > metroidPrimeLevel.size)
    return fetch(metroidPrimeLevel.url)
  throw new Error("no space")
}).then( /* … */ )

Your patch currently returns *origin* usage and *group* quota, but our quota checking compares *group* usage and *group* quota. So the code above won't be checking right numbers and may try to fetch data when there's not enough space. Either you return group usage and group quota or origin usage and origin quota.
Since our quota handling heavily depends on having group quota (there's no such thing as origin quota), I recommend to return *group* usage and *group* quota.
(In reply to Jan Varga [:janv] from comment #27)
> So the goal (according to the spec) is to be able to do this:
> 
> navigator.storage.estimate().then(info => {
>   if(info.quota - info.usage > metroidPrimeLevel.size)
>     return fetch(metroidPrimeLevel.url)
>   throw new Error("no space")
> }).then( /* … */ )
> 
> Your patch currently returns *origin* usage and *group* quota, but our quota
> checking compares *group* usage and *group* quota. So the code above won't
> be checking right numbers and may try to fetch data when there's not enough
> space. Either you return group usage and group quota or origin usage and
> origin quota.
> Since our quota handling heavily depends on having group quota (there's no
> such thing as origin quota), I recommend to return *group* usage and *group*
> quota.

Yes, we're working on adding group usage for QuotaManagerService.
Per discussion in AllHands2016 Storage meeting, we will implement estimate method based on per site usage/quota. Bug 1281103 will implement |GetUsageForGroup|.
(In reply to Shawn Huang [:shawnjohnjr] from comment #29)
> Per discussion in AllHands2016 Storage meeting, we will implement estimate
> method based on per site usage/quota. Bug 1281103 will implement
> |GetUsageForGroup|.

As Tom suggests to name it |GetGroupUsageForPrincipal|.
(In reply to Shawn Huang [:shawnjohnjr] from comment #30)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #29)
> > Per discussion in AllHands2016 Storage meeting, we will implement estimate
> > method based on per site usage/quota. Bug 1281103 will implement
> > |GetUsageForGroup|.
> 
> As Tom suggests to name it |GetGroupUsageForPrincipal|.

Maybe GetGroupUsageQuotaForPrincipal?
Also note that current getUsageForPrincipal() counts usage for all storage types (default, temporary, persistent), but for estimates() to work correctly we need to only count default and temporary.

Information about all default and temporary storage origins is cached in GroupInfoPair/GroupInfo/OriginInfo objects, so in theory we don't have to touch the disk and just get usage/quota from these objects.

It's also questionable what should estimate() return when an origin in default storage gets persisted via navigator.persist(). I see basically two options:
1. actual usage and quota as some big number
2. zero usage and zero quota

The latter (2) would be easier to implement with regard to our current internal implementation (persisted origins are not quota tracked)

You might want to ask anne what he thinks about it.
(In reply to Jan Varga [:janv] from comment #32)
> Also note that current getUsageForPrincipal() counts usage for all storage
> types (default, temporary, persistent), but for estimates() to work
> correctly we need to only count default and temporary.
Hi Janv,
Thanks for all the hints.
Just want to confirm I understand things correctly. I remember the discussion: https://github.com/whatwg/storage/issues/25#issuecomment-215400984

So eventually 'default' internal storage type contains both temporary and persistent storage data, and site storage unit shall store data in the 'default' directory. If clients call navigator.storage.persist(), we update metadata for 'default' internal storage type. We can ignore internal storage type 'persistent', because most site storage units won't store things in our internal 'persistent' storage folder. Is it correct? Or we should consider internal 'persistent' storage type currently? Currently can sites stored data in persistent folder?

> 
> Information about all default and temporary storage origins is cached in
> GroupInfoPair/GroupInfo/OriginInfo objects, so in theory we don't have to
> touch the disk and just get usage/quota from these objects.
OK, I also saw these cached info object.
> 
> It's also questionable what should estimate() return when an origin in
> default storage gets persisted via navigator.persist(). I see basically two
You mean navigator.storage.persist()?
> options:
> 1. actual usage and quota as some big number
Do you mean returning 'actual' internal storage type "default + temporary + persistent"?
> 2. zero usage and zero quota
I don't understand why we return zero quota?
Flags: needinfo?(jvarga)
(In reply to Shawn Huang [:shawnjohnjr] from comment #33)
> (In reply to Jan Varga [:janv] from comment #32)
> > Also note that current getUsageForPrincipal() counts usage for all storage
> > types (default, temporary, persistent), but for estimates() to work
> > correctly we need to only count default and temporary.
> Hi Janv,
> Thanks for all the hints.
> Just want to confirm I understand things correctly. I remember the
> discussion:
> https://github.com/whatwg/storage/issues/25#issuecomment-215400984
> 
> So eventually 'default' internal storage type contains both temporary and
> persistent storage data, and site storage unit shall store data in the
> 'default' directory. If clients call navigator.storage.persist(), we update
> metadata for 'default' internal storage type. We can ignore internal storage
> type 'persistent', because most site storage units won't store things in our
> internal 'persistent' storage folder. Is it correct? Or we should consider
> internal 'persistent' storage type currently? Currently can sites stored
> data in persistent folder?

I mean that current getUsageForPrincipal() also includes *explicit* persistent storage.
I'm not talking about persistent origins in default storage.

> 
> > 
> > Information about all default and temporary storage origins is cached in
> > GroupInfoPair/GroupInfo/OriginInfo objects, so in theory we don't have to
> > touch the disk and just get usage/quota from these objects.
> OK, I also saw these cached info object.
> > 
> > It's also questionable what should estimate() return when an origin in
> > default storage gets persisted via navigator.persist(). I see basically two
> You mean navigator.storage.persist()?

Yes

> > options:
> > 1. actual usage and quota as some big number
> Do you mean returning 'actual' internal storage type "default + temporary +
> persistent"?

I mean we would calculate usage normally, that is, for example get it from OriginInfo, etc.


> > 2. zero usage and zero quota
> I don't understand why we return zero quota?

When a box/origin is persisted using navigator.storage.persist(), it becomes persistent, so
there's no quota anymore, right ?
So either we return some big number or zero or maybe -1
It's quite convenient to return 0, because it's easy to detect it.
Flags: needinfo?(jvarga)
(In reply to Jan Varga [:janv] from comment #34)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #33)
> > (In reply to Jan Varga [:janv] from comment #32)
> 
> > > 2. zero usage and zero quota
> > I don't understand why we return zero quota?
> 
> When a box/origin is persisted using navigator.storage.persist(), it becomes
> persistent, so
> there's no quota anymore, right ?
> So either we return some big number or zero or maybe -1
> It's quite convenient to return 0, because it's easy to detect it.

Hi Anne,
Is it true once box/origin is persisted, there's no quota anymore like janv said? From the specification, I can only see estimate() returns site storage unit usage/quota.
Flags: needinfo?(annevk)
I had imagined we would return large numbers. You can still run out of space after all and it is still useful to know how much you are using. Basically, the persistent grant might enlarge the amount of quota you get.
Flags: needinfo?(annevk)
Yeah, that's the option 1.
(In reply to Anne (:annevk) from comment #36)
> I had imagined we would return large numbers. You can still run out of space
> after all and it is still useful to know how much you are using. Basically,
> the persistent grant might enlarge the amount of quota you get.

I don't understand 'the persistent grant might enlarge the amount of quota you get', can you explain it?
Flags: needinfo?(annevk)
Say your current quota is X. You then ask the user for persistent storage permission? User agrees. The new current quota will then be X + Y (both positive integers) and the storage will be persistent.

I think our strategy for now will be that X + Y equals most of the available space. Other browsers might do things a little different. See also bug 1281416 about figuring out a better minimum and maximum.
Flags: needinfo?(annevk)
Yeah, so estimate() will be useful even for persisted sites. The quota for it will be calculated using a get free disk space function (multiplied by say 0.8-0.9).
The patch will be revised after bug 1281103 finished.
Attached patch bug-1267941.patch (obsolete) — Splinter Review
Re-base code again based on bug 1281103.
Attached patch bug-1267941.patch (obsolete) — Splinter Review
Add test cases for worker. Looking into issues after running estimate() in worker thread.
I found leakage during running test cases. I'm looking into it.
Opps! I found the leakage happens after calling new UsageRequest.                                  
    nsIQuotaUsageRequest* request = new UsageRequest(principal, callback);
Attached patch bug-1267941.patch (obsolete) — Splinter Review
Fix leakage issues.
I found the cache test case is wrong, I will fix it in the next revision.
Hi bz,
I added SecureContext annotation in Navigator.webidl based on https://storage.spec.whatwg.org/#api.

[SecureContext, NoInterfaceObject, Exposed=(Window,Worker)]                     
interface NavigatorStorage {                                                    
  readonly attribute StorageManager storage;                                    
};


WebIDL.WebIDLError: error: Interface with no interface object is exposed conditionally, /home/shawnjohnjr/code/mozilla-inbound/obj-x86_64-pc-linux-gnu/dom/bindings/Navigator.webidl line 91:0
interface NavigatorStorage {
^

In WebIDL.py, comment says "Conditional exposure makes no sense for interfaces with no           interface object, unless they're navigator properties."
                
        if (self.isExposedConditionally() and                                   
            not self.hasInterfaceObject() and                                   
            not self.isNavigatorProperty()):                                    
            raise WebIDLError("Interface with no interface object is "          
                              "exposed conditionally",                          
                              [self.location]) 

Does that mean the Storage API specification is wrong?
Flags: needinfo?(bzbarsky)
No, it means that [SecureContext] does two jobs: conditional exposure of the interface _and_ conditional exposure of the interface members.  The former job makes no sense for [NoInterfaceObject] things, but the latter does.

What we should probably do is add an optional argument to isExposedConditionally to not return true just because "SecureContext" is set and use that here along with a comment explaining that [SecureContext] is a sensible thing to do even on [NoInterfaceObject] interfaces.
Flags: needinfo?(bzbarsky)
Attached patch bug-1267941.patch (obsolete) — Splinter Review
After adding an annotation "SecureContext", storage api is not available for non-secure channel. I will check if we can make some changes to mochitests.

JavaScript error: http://mochi.test:8888/tests/dom/cache/test/mochitest/test_cache_orphaned_body.html, line 48: TypeError: navigator.storage is undefined
Marking blocked on Bug 1274315.
Bug 1274315 - Mochitest cannot be used to test [SecureContext] API
Regarding 'SecureContext' issue added for Storage API, I followed bug 1274315 comment 1, I added dummy test and try to do 'window.open("https://example.com/tests/dom/cache/test/mochitest/file_cache_orphaned_body_storage.html")'.

But now I still got 'TypeError: navigator.storage is undefined'.


0 INFO SimpleTest START
1 INFO TEST-START | dom/cache/test/mochitest/test_cache_storage.html
JavaScript error: https://example.com/tests/dom/cache/test/mochitest/file_cache_orphaned_body_storage.html, line 50: TypeError: navigator.storage is undefined

I'm not sure is there anything I missed.
Attached file interdiff.txt (obsolete) —
Hi Ted,
I followed your bug 1274315 Comment 1, I open a new tab by adding 'window.open("https://example.com/tests/dom/cache/test/mochitest/file_cache_orphaned_body_storage.html")', but still cannot access navigator.storage. Do you have any idea?
Flags: needinfo?(ted)
If you have an insecure context open a popup to an https url, that popup is still an insecure context per spec.
Right, my suggestion from that comment was to use the Marionette API somehow, but I don't think that fits very well with how Mochitest is written currently.

I think we need to get bug 1274315 sorted out, possibly by way of bug 1286312.
Flags: needinfo?(ted)
(In reply to Boris Zbarsky [:bz] from comment #61)
> If you have an insecure context open a popup to an https url, that popup is
> still an insecure context per spec.
Depends on: 1286312
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #62)
> Right, my suggestion from that comment was to use the Marionette API
> somehow, but I don't think that fits very well with how Mochitest is written
> currently.
> 
> I think we need to get bug 1274315 sorted out, possibly by way of bug
> 1286312.
So in bug 1286312, we're going to add '--use-https' option, but how do we specify the specific test case to run with https option? I guess we have to add argument '--user-https' in manifest.ini parser?
Flags: needinfo?(ted)
I proposed in that bug that we just try loading the top-level page from https and see if anything breaks. If nothing does, we can just do that. If something does we'll have to add a manifest annotation to load specific tests from specific hosts.
Flags: needinfo?(ted)
Attached patch bug-1267941.patch (obsolete) — Splinter Review
Add Indexeddb test case for StorageManager estimate method.
Based on Comment 66, I think it's better to enable SecureContext in bug 1268804.
(In reply to Shawn Huang [:shawnjohnjr] from comment #69)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e73cda202c9
I should be careful :(
* dom/quota/StorageManager.cpp:115:3: error: bad implicit conversion constructor for 'MainThreadStorageRunnable
* dom/workers/test/serviceworkers/test_serviceworker_interfaces.html
* dom/tests/mochitest/general/test_interfaces.html
(In reply to Shawn Huang [:shawnjohnjr] from comment #72)
> Created attachment 8774626 [details] [diff] [review]
> Bug 1267941 - Implement Storage API estimate() feature
> 
> Fixed test errors and build failure on the specific target
> try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=07bec679c779
* dom/workers/test/test_navigator.html 
* dom/workers/test/test_worker_interfaces.html
Fixed test_navigator.html failure.
But still failed test_worker_interfaces.html.

13 INFO TEST-UNEXPECTED-FAIL | dom/workers/test/test_worker_interfaces.html | uncaught exception - DataCloneError: The object could not be cloned. at workerTestExec/worker.onmessage@http://mochi.test:8888/tests/dom/workers/test/worker_driver.js:73:7
EventHandlerNonNull*workerTestExec@http://mochi.test:8888/tests/dom/workers/test/worker_driver.js:32:3
@http://mochi.test:8888/tests/dom/workers/test/test_worker_interfaces.html:13:1
(In reply to Shawn Huang [:shawnjohnjr] from comment #76)
> Created attachment 8775044 [details] [diff] [review]
> Bug 1267941 - Implement Storage API estimate() feature
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=09dc51eaf4f0

All the test cases of service works and workers are fixed.
Comment on attachment 8775092 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature

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

This patch exposes StorageManager estimate() to navigator. Test cases include both cache and indexeddb, both cover window and worker version. SecureContext is missing and will be handled in bug 1268804.
Given the patch status and our plan for the Storage feature, mapping btpp-active to P1. Please feel free to modify it if you have a different idea, Shawn. Thanks!
Priority: -- → P1
Whiteboard: btpp-active
Comment on attachment 8775092 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature

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

I'd like to revisit this again after these issues are addressed, thanks!

::: dom/base/Navigator.h
@@ +102,5 @@
>  class Presentation;
>  class LegacyMozTCPSocket;
> +namespace quota{
> +class StorageManager;
> +} // namespace quota

I'd prefer to expose StorageManager in mozilla::dom because it's an DOM level API instead of a class that is only used internally and is quota-related.

::: dom/bindings/Bindings.conf
@@ +941,5 @@
>  },
>  
> +'StorageManager': {
> +    'nativeType': 'mozilla::dom::quota::StorageManager',
> +},

Then, we don't need this.

::: dom/indexedDB/test/unit/test_storage_manager_estimate.js
@@ +36,5 @@
> +    testGenerator.send(estimation.usage);
> +  });
> +  let after = yield undefined;
> +
> +  ok(after > before, 'estimated usage must increase');

the "after" is always larger then "before" without objectStore.put() because a sqlite db file will be create after indexedDB.open is success.
We should check how much larger after large_value is put into objectStore.

::: dom/quota/StorageManager.cpp
@@ +50,5 @@
> +
> +  NS_IMETHODIMP
> +  OnUsageResult(nsIQuotaUsageRequest *aRequest) override
> +  {
> +    MOZ_ASSERT(mPromise);

Move this assert to its constructor and add MOZ_ASSERT(aRequest) here instead.

@@ +78,5 @@
> +public:
> +  static already_AddRefed<WorkerStorageResolver>
> +  Create(workers::WorkerPrivate* aWorkerPrivate, Promise* aPromise)
> +  {
> +    MOZ_ASSERT(aWorkerPrivate);

MOZ_ASSERT(aPromise);

@@ +132,5 @@
> +    }
> +
> +    nsCOMPtr<nsIPrincipal> principal =
> +      proxy->GetWorkerPrivate()->GetPrincipal();
> +    MOZ_ASSERT(principal);

Let the life time of the mutex as short as possible by 

nsCOMPtr<nsIPrincipal> principal;
{
  MutexAutoLock lock(proxy->Lock());
  if (proxy->CleanedUp()) {
    NS_WARNING("Aborting Storage::Estimate() because worker shut down");
    return true;
  }
  principal = proxy->GetWorkerPrivate()->GetPrincipal();
}
MOZ_ASSERT(principal);

@@ +139,5 @@
> +    MOZ_ASSERT(qms);
> +
> +    RefPtr<nsIQuotaUsageRequest> request =
> +      new UsageRequest(principal, mResolver);
> +    qms->GetUsageForPrincipal(principal, mResolver, true, getter_AddRefs(request));

You don't have to allocate it, it's will be allocated by qms->GetUsageForPrincipal() because it's an outer parameter:

RefPtr<nsIQuotaUsageRequest> request;
nsResult rv = qms->GetUsageForPrincipal(principal, callback, true, getter_AddRefs(request));

if (NS_FAILED(rv)) {
  aRv.Throw(rv);
  return true;  // or what happens if false is returned?
}

@@ +157,5 @@
> +                              const StorageEstimate& aQuotaUsage)
> +    : WorkerRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount)
> +    , mResolver(aResolver)
> +    , mQuotaUsage(aQuotaUsage)
> +  {

MOZ_ASSERT(mResolver);

@@ +195,5 @@
> +
> +NS_IMETHODIMP
> +WorkerStorageResolver::OnUsageResult(nsIQuotaUsageRequest *aRequest)
> +{
> +  AssertIsOnMainThread();

MOZ_ASSERT(aRequest);

@@ +200,5 @@
> +
> +  MutexAutoLock lock(mPromiseProxy->Lock());
> +  if (mPromiseProxy->CleanedUp()) {
> +    return NS_ERROR_FAILURE;
> +  }

Shorten the life time of the mutex lock:

WorkerPrivate* workerPrivate;
{
  MutexAutoLock lock(proxy->Lock());
  if (mPromiseProxy->CleanedUp()) {
    return NS_ERROR_FAILURE;
  }

  workerPrivate = mPromiseProxy->GetWorkerPrivate();
}

MOZ_ASSERT(workerPrivate);

@@ +251,5 @@
> +  RefPtr<Promise> promise = Promise::Create(mOwner, aRv);
> +  nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(mOwner);
> +
> +  nsCOMPtr<nsIPrincipal> principal;
> +  nsCOMPtr<nsIQuotaManagerService> qms;

Move these 3 declarations of window, principal and qms into if (NS_IsMainThread()).

@@ +272,5 @@
> +
> +    nsCOMPtr<nsIQuotaUsageCallback> callback =
> +      new MainThreadStorageResolver(promise);
> +    RefPtr<nsIQuotaUsageRequest> request =
> +      new UsageRequest(principal, callback);

You don't have to allocate it, it's will be allocated by qms->GetUsageForPrincipal() because it's an outer parameter:

RefPtr<nsIQuotaUsageRequest> request;
nsResult rv = qms->GetUsageForPrincipal(principal, callback, true, getter_AddRefs(request));

if (NS_FAILED(rv)) {
  aRv.Throw(rv);
  return nullptr;
}

::: dom/workers/WorkerNavigator.cpp
@@ +36,5 @@
> +
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(WorkerNavigator)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mStorageManager)
> +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END

NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(WorkerNavigator, mStorageManager) saves you! :)

@@ +43,5 @@
>  NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(WorkerNavigator, Release)
>  
> +NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(WorkerNavigator)
> +  NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
> +NS_IMPL_CYCLE_COLLECTION_TRACE_END

ditto

@@ +184,5 @@
> +{
> +  WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
> +  MOZ_ASSERT(workerPrivate);
> +
> +  RefPtr<nsIGlobalObject> global = workerPrivate->GlobalScope();

move these 3 lines into if (!mStorageManager) clause.

::: dom/workers/WorkerNavigator.h
@@ +17,5 @@
>  class Promise;
>  
> +namespace quota {
> +class StorageManager;
> +}

move StorageManager to mozilla::dom
Attachment #8775092 - Flags: feedback?(btseng) → feedback-
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #81)
> Comment on attachment 8775092 [details] [diff] [review]
> Bug 1267941 - Implement Storage API estimate() feature
> 
> Review of attachment 8775092 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I'd like to revisit this again after these issues are addressed, thanks!
> 
> ::: dom/base/Navigator.h
> @@ +102,5 @@
> >  class Presentation;
> >  class LegacyMozTCPSocket;
> > +namespace quota{
> > +class StorageManager;
> > +} // namespace quota
> 
> I'd prefer to expose StorageManager in mozilla::dom because it's an DOM
> level API instead of a class that is only used internally and is
> quota-related.
Hi Bevis,
First, thanks for the feedback. I will hold any changes until janv finished review, since janv already reviewed some code.

I don't have strong reason but because StorageManager is heavily using quota related code, to create an another folder for StorageManager seems to be unnecessary. Maybe janv can make the final decision.

> 
> ::: dom/bindings/Bindings.conf
> @@ +941,5 @@
> >  },
> >  
> > +'StorageManager': {
> > +    'nativeType': 'mozilla::dom::quota::StorageManager',
> > +},
> 
> Then, we don't need this.
Ok, it depends on we move StorageManager outside quota folder.
> 
> ::: dom/indexedDB/test/unit/test_storage_manager_estimate.js
> @@ +36,5 @@
> > +    testGenerator.send(estimation.usage);
> > +  });
> > +  let after = yield undefined;
> > +
> > +  ok(after > before, 'estimated usage must increase');
> 
> the "after" is always larger then "before" without objectStore.put() because
> a sqlite db file will be create after indexedDB.open is success.
> We should check how much larger after large_value is put into objectStore.
Good point. I will change it.

> 
> ::: dom/quota/StorageManager.cpp
> @@ +50,5 @@
> > +
> > +  NS_IMETHODIMP
> > +  OnUsageResult(nsIQuotaUsageRequest *aRequest) override
> > +  {
> > +    MOZ_ASSERT(mPromise);
> 
> Move this assert to its constructor and add MOZ_ASSERT(aRequest) here
> instead.
Ok
> 
> @@ +78,5 @@
> > +public:
> > +  static already_AddRefed<WorkerStorageResolver>
> > +  Create(workers::WorkerPrivate* aWorkerPrivate, Promise* aPromise)
> > +  {
> > +    MOZ_ASSERT(aWorkerPrivate);
> 
> MOZ_ASSERT(aPromise);
ok
> 
> @@ +132,5 @@
> > +    }
> > +
> > +    nsCOMPtr<nsIPrincipal> principal =
> > +      proxy->GetWorkerPrivate()->GetPrincipal();
> > +    MOZ_ASSERT(principal);
> 
> Let the life time of the mutex as short as possible by 
ok
> 
> nsCOMPtr<nsIPrincipal> principal;
> {
>   MutexAutoLock lock(proxy->Lock());
>   if (proxy->CleanedUp()) {
>     NS_WARNING("Aborting Storage::Estimate() because worker shut down");
>     return true;
>   }
>   principal = proxy->GetWorkerPrivate()->GetPrincipal();
> }
> MOZ_ASSERT(principal);
> 
> @@ +139,5 @@
> > +    MOZ_ASSERT(qms);
> > +
> > +    RefPtr<nsIQuotaUsageRequest> request =
> > +      new UsageRequest(principal, mResolver);
> > +    qms->GetUsageForPrincipal(principal, mResolver, true, getter_AddRefs(request));
> 
> You don't have to allocate it, it's will be allocated by
> qms->GetUsageForPrincipal() because it's an outer parameter:
> 
> RefPtr<nsIQuotaUsageRequest> request;
> nsResult rv = qms->GetUsageForPrincipal(principal, callback, true,
> getter_AddRefs(request));
> 
I will check.

> if (NS_FAILED(rv)) {
>   aRv.Throw(rv);
>   return true;  // or what happens if false is returned?
> }
Ah. I missed this part.
> 
> @@ +157,5 @@
> > +                              const StorageEstimate& aQuotaUsage)
> > +    : WorkerRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount)
> > +    , mResolver(aResolver)
> > +    , mQuotaUsage(aQuotaUsage)
> > +  {
> 
> MOZ_ASSERT(mResolver);
ok.
> 
> @@ +195,5 @@
> > +
> > +NS_IMETHODIMP
> > +WorkerStorageResolver::OnUsageResult(nsIQuotaUsageRequest *aRequest)
> > +{
> > +  AssertIsOnMainThread();
> 
> MOZ_ASSERT(aRequest);
> 
> @@ +200,5 @@
> > +
> > +  MutexAutoLock lock(mPromiseProxy->Lock());
> > +  if (mPromiseProxy->CleanedUp()) {
> > +    return NS_ERROR_FAILURE;
> > +  }
> 
> Shorten the life time of the mutex lock:
> 
> WorkerPrivate* workerPrivate;
> {
>   MutexAutoLock lock(proxy->Lock());
>   if (mPromiseProxy->CleanedUp()) {
>     return NS_ERROR_FAILURE;
>   }
> 
>   workerPrivate = mPromiseProxy->GetWorkerPrivate();
> }
> 
> MOZ_ASSERT(workerPrivate);
> 
> @@ +251,5 @@
> > +  RefPtr<Promise> promise = Promise::Create(mOwner, aRv);
> > +  nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(mOwner);
> > +
> > +  nsCOMPtr<nsIPrincipal> principal;
> > +  nsCOMPtr<nsIQuotaManagerService> qms;
> 
> Move these 3 declarations of window, principal and qms into if
> (NS_IsMainThread()).
ok
> 
> @@ +272,5 @@
> > +
> > +    nsCOMPtr<nsIQuotaUsageCallback> callback =
> > +      new MainThreadStorageResolver(promise);
> > +    RefPtr<nsIQuotaUsageRequest> request =
> > +      new UsageRequest(principal, callback);
> 
> You don't have to allocate it, it's will be allocated by
> qms->GetUsageForPrincipal() because it's an outer parameter:
> 
> RefPtr<nsIQuotaUsageRequest> request;
> nsResult rv = qms->GetUsageForPrincipal(principal, callback, true,
> getter_AddRefs(request));
> 
> if (NS_FAILED(rv)) {
>   aRv.Throw(rv);
>   return nullptr;
> }
> 
Ok. I will check.
> ::: dom/workers/WorkerNavigator.cpp
> @@ +36,5 @@
> > +
> > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_BEGIN(WorkerNavigator)
> > +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE(mStorageManager)
> > +  NS_IMPL_CYCLE_COLLECTION_TRAVERSE_SCRIPT_OBJECTS
> > +NS_IMPL_CYCLE_COLLECTION_TRAVERSE_END
> 
> NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(WorkerNavigator, mStorageManager)
> saves you! :)
> 
> @@ +43,5 @@
> >  NS_IMPL_CYCLE_COLLECTION_UNROOT_NATIVE(WorkerNavigator, Release)
> >  
> > +NS_IMPL_CYCLE_COLLECTION_TRACE_BEGIN(WorkerNavigator)
> > +  NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER
> > +NS_IMPL_CYCLE_COLLECTION_TRACE_END
> 
> ditto
Ok, i will check.
> 
> @@ +184,5 @@
> > +{
> > +  WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
> > +  MOZ_ASSERT(workerPrivate);
> > +
> > +  RefPtr<nsIGlobalObject> global = workerPrivate->GlobalScope();
> 
> move these 3 lines into if (!mStorageManager) clause.
> 
> ::: dom/workers/WorkerNavigator.h
> @@ +17,5 @@
> >  class Promise;
> >  
> > +namespace quota {
> > +class StorageManager;
> > +}
> 
> move StorageManager to mozilla::dom
Let's see janv's decision.
(In reply to Shawn Huang [:shawnjohnjr] from comment #82)
> I don't have strong reason but because StorageManager is heavily using quota
> related code, to create an another folder for StorageManager seems to be
> unnecessary. Maybe janv can make the final decision.
> 
It's not about creating a new folder for StorageManager but a correct name space "mozilla::dom" that StorageManager belongs to.

"mozilla::dom" is the default name space and the interface name "StorageManager" is is the default class name that StorageManagerBinding.cpp/h created by WebIDL binding looking for [1] unless you specified it with different class name or name space in dom/bindings/Bindings.conf.

[1] http://searchfox.org/mozilla-central/source/dom/bindings/Codegen.py#13121
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #83)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #82)
> > I don't have strong reason but because StorageManager is heavily using quota
> > related code, to create an another folder for StorageManager seems to be
> > unnecessary. Maybe janv can make the final decision.
> > 
> It's not about creating a new folder for StorageManager but a correct name
> space "mozilla::dom" that StorageManager belongs to.
> 
> "mozilla::dom" is the default name space and the interface name
> "StorageManager" is is the default class name that
> StorageManagerBinding.cpp/h created by WebIDL binding looking for [1] unless
> you specified it with different class name or name space in
> dom/bindings/Bindings.conf.
> 
> [1] http://searchfox.org/mozilla-central/source/dom/bindings/Codegen.py#13121

I see. I misunderstood your comments. Thanks for the explanation.
No longer depends on: 1286312
(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #81)
> Comment on attachment 8775092 [details] [diff] [review]
> Bug 1267941 - Implement Storage API estimate() feature
> ::: dom/indexedDB/test/unit/test_storage_manager_estimate.js
> @@ +36,5 @@
> > +    testGenerator.send(estimation.usage);
> > +  });
> > +  let after = yield undefined;
> > +
> > +  ok(after > before, 'estimated usage must increase');
> 
> the "after" is always larger then "before" without objectStore.put() because
> a sqlite db file will be create after indexedDB.open is success.
> We should check how much larger after large_value is put into objectStore.
> 
I think you're right.
Without adding larger object, usage is 57424, adding large object, usage is 119224.
(In reply to Shawn Huang [:shawnjohnjr] from comment #86)
> (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #81)
> > Comment on attachment 8775092 [details] [diff] [review]
> > Bug 1267941 - Implement Storage API estimate() feature
> > ::: dom/indexedDB/test/unit/test_storage_manager_estimate.js
> > @@ +36,5 @@
> > > +    testGenerator.send(estimation.usage);
> > > +  });
> > > +  let after = yield undefined;
> > > +
> > > +  ok(after > before, 'estimated usage must increase');
> > 
> > the "after" is always larger then "before" without objectStore.put() because
> > a sqlite db file will be create after indexedDB.open is success.
> > We should check how much larger after large_value is put into objectStore.
> > 
> I think you're right.
> Without adding larger object, usage is 57424, adding large object, usage is
> 119224.

I tried to modify this test case to estimate usage before/after createObject, but it turns out we found api problems on indexeddb, I'm clarifying with bevis now.
(In reply to Shawn Huang [:shawnjohnjr] from comment #87)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #86)
> > (In reply to Bevis Tseng[:bevistseng][:btseng] from comment #81)
> > > Comment on attachment 8775092 [details] [diff] [review]
> > > Bug 1267941 - Implement Storage API estimate() feature
> > > ::: dom/indexedDB/test/unit/test_storage_manager_estimate.js
> > I think you're right.
> > Without adding larger object, usage is 57424, adding large object, usage is
> > 119224.
> 
> I tried to modify this test case to estimate usage before/after
> createObject, but it turns out we found api problems on indexeddb, I'm
> clarifying with bevis now.
It should be false alarm. I should wait for event then close database.
Comment on attachment 8779272 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature - v2

It looks like I don't need to modify Bindings.conf.
Attachment #8779272 - Attachment is obsolete: true
Attachment #8779272 - Flags: feedback?(btseng)
Comment on attachment 8779626 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature - v2

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

f=me after the following issue addressed, thanks!

::: dom/base/Navigator.cpp
@@ +679,5 @@
> +{
> +  MOZ_ASSERT(mWindow);
> +
> +  nsCOMPtr<nsIGlobalObject> global = do_QueryInterface(mWindow);
> +  MOZ_ASSERT(global);

Sorry for not capturing this in previous review.
Please move these 2 lines into if (!mStorageManager), thanks!

::: dom/quota/StorageManager.cpp
@@ +144,5 @@
> +    RefPtr<nsIQuotaUsageRequest> request;
> +    nsresult rv = qms->GetUsageForPrincipal(principal, mResolver, true,
> +                                            getter_AddRefs(request));
> +    if (NS_FAILED(rv)) {
> +      return false; //TODO

I think we should make it clear now to be either true or false instead of marking it as TODO without any explanation.

::: dom/workers/WorkerNavigator.cpp
@@ +169,5 @@
> +{
> +  if (!mStorageManager) {
> +    WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
> +    MOZ_ASSERT(workerPrivate);
> +    RefPtr<nsIGlobalObject> global = workerPrivate->GlobalScope();

MOZ_ASSERT(global);
Attachment #8779626 - Flags: feedback?(btseng) → feedback+
Based on comment in bug 1286312 [1], do we need to add web-platform-test instead of mochitest in this patch? Because wpt supports https but mochitest doesn't currently. However, we might limit testing coverage without SpecialPowers, like we did in cache api mochitests.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1286312#c20
Flags: needinfo?(jvarga)
(In reply to Shawn Huang [:shawnjohnjr] from comment #94)
> Based on comment in bug 1286312 [1], do we need to add web-platform-test
> instead of mochitest in this patch? Because wpt supports https but mochitest
> doesn't currently. However, we might limit testing coverage without
> SpecialPowers, like we did in cache api mochitests.
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1286312#c20

wpt is fine as far as you are able to write good tests without SpecialPowers
Flags: needinfo?(jvarga)
Comment on attachment 8775092 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature

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

I guess it doesn't make sense to review this patch anymore. I'll take a look at v2 patch instead.
Attachment #8775092 - Flags: review?(jvarga)
Comment on attachment 8779988 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature - v2, f=btseng

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

I'm not yet done, I'll post comments for StorageManager.cpp later and additional comments for test_storage_manager_estimate.js too

::: dom/base/Navigator.cpp
@@ +45,5 @@
>  #include "mozilla/dom/InputPortManager.h"
>  #include "mozilla/dom/MobileMessageManager.h"
>  #include "mozilla/dom/Permissions.h"
>  #include "mozilla/dom/Presentation.h"
> +#include "mozilla/dom/StorageManager.h"

t comes after e

::: dom/cache/test/mochitest/mochitest.ini
@@ +3,5 @@
>    test_cache.js
>    test_cache_add.js
>    worker_driver.js
>    worker_wrapper.js
> +  worker_storage.js

This file is rather tiny, it could be done without creating a new file for the worker script. Take a look at dom/indexedDB/test and look for workerScript() to see how it is done.

::: dom/indexedDB/test/test_storage_manager_estimate.html
@@ +8,5 @@
> +
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +
> +  <script type="text/javascript;version=1.7" src="unit/test_storage_manager_estimate.js"></script>

Since this is split into separate JS file, can you also run the test in xpcshell suite ?
See xpcshell-shared.ini file.

::: dom/indexedDB/test/unit/test_storage_manager_estimate.js
@@ +7,5 @@
> +  ok(navigator.storage.estimate() instanceof Promise,
> +     'estimate() method exists and returns a Promise');
> +
> +  const name = this.window ? window.location.pathname :
> +	                     "test_storage_manager_estimate.js";

we usually put const variables in the beginning of the function

@@ +8,5 @@
> +     'estimate() method exists and returns a Promise');
> +
> +  const name = this.window ? window.location.pathname :
> +	                     "test_storage_manager_estimate.js";
> +  var large_value = new Uint8Array(1e6);

let (not var) and largeValue instead of large_value

@@ +9,5 @@
> +
> +  const name = this.window ? window.location.pathname :
> +	                     "test_storage_manager_estimate.js";
> +  var large_value = new Uint8Array(1e6);
> +  const objectStoreName = "storagesManager";

can be placed in the beginning as well

@@ +32,5 @@
> +
> +  navigator.storage.estimate().then(estimation => {
> +    testGenerator.send(estimation.usage);
> +  });
> +  let after_create = yield undefined;

we usually don't use underscore for variable names, what about just afterCreate or usageAfterCreate ?

::: dom/quota/StorageManager.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + *  * License, v. 2.0. If a copy of the MPL was not distributed with this
> + *   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +#ifndef mozilla_dom_storagemanager_h

Since this is a new file I'm going to be a little bit more strict about the style.

I think this should be mozilla_dom_StorageManager_h here and below and also in the end of the file

@@ +8,5 @@
> +#define mozilla_dom_storagemanager_h
> +
> +#include "mozilla/dom/StorageManagerBinding.h"
> +#include "mozilla/ErrorResult.h"
> +

no need for the empty line here

@@ +21,5 @@
> +class Promise;
> +struct StorageEstimate;
> +
> +class StorageManager final : public nsISupports,
> +                             public nsWrapperCache

class StorageManager final
  : public nsISupports
  , public nsWrapperCache

@@ +25,5 @@
> +                             public nsWrapperCache
> +{
> +public:
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(StorageManager)

Move these two before WrapObject()

@@ +31,5 @@
> +  explicit StorageManager(nsIGlobalObject* aGloabl) : mOwner(aGloabl)
> +  {
> +    MOZ_ASSERT(aGloabl);
> +  }
> +

Add |// WebIDL| comment here

@@ +32,5 @@
> +  {
> +    MOZ_ASSERT(aGloabl);
> +  }
> +
> +  nsIGlobalObject* GetParentObject() const { return mOwner; }

nsIGlobalObject*
GetParentObject() const
{
  return mOwner;
}

@@ +33,5 @@
> +    MOZ_ASSERT(aGloabl);
> +  }
> +
> +  nsIGlobalObject* GetParentObject() const { return mOwner; }
> +

Add |// nsWrapperCache| comment here.

@@ +38,5 @@
> +  virtual JSObject*
> +  WrapObject(JSContext* aCx, JS::Handle<JSObject*> aGivenProto) override;
> +
> +  already_AddRefed<Promise>
> +  Estimate(ErrorResult& aRv);

Move this function after GetParentObject()

@@ +43,5 @@
> +
> +private:
> +  ~StorageManager();
> +
> +  nsCOMPtr<nsIGlobalObject> mOwner;

move mOwner to the beginning, it will be private by default there

::: dom/workers/WorkerNavigator.cpp
@@ +168,5 @@
> +WorkerNavigator::Storage()
> +{
> +  if (!mStorageManager) {
> +    WorkerPrivate* workerPrivate = GetCurrentThreadWorkerPrivate();
> +    MOZ_ASSERT(workerPrivate);

I would add an empty line here.

::: dom/workers/WorkerNavigator.h
@@ +103,5 @@
>    void SetLanguages(const nsTArray<nsString>& aLanguages);
>  
>    uint64_t HardwareConcurrency() const;
> +
> +  dom::StorageManager* Storage();

Hm, is the dom:: prefix needed ?

::: dom/workers/test/serviceworkers/serviceworker_wrapper.js
@@ +98,5 @@
>    });
>  }
>  
> +function workerTestGetStorageManager(cb) {
> +  addEventListener('message', function workerTestGetStorageManager(e) {

Hm, aren't you missing the CB postfix here ?
Other functions seem to use it.

@@ +102,5 @@
> +  addEventListener('message', function workerTestGetStorageManager(e) {
> +    if (e.data.type !== 'returnStorageManager') {
> +      return;
> +    }
> +    removeEventListener('message', workerTestGetStorageManager);

here too
Comment on attachment 8779988 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature - v2, f=btseng

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

::: dom/indexedDB/test/test_storage_manager_estimate.html
@@ +8,5 @@
> +
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +
> +  <script type="text/javascript;version=1.7" src="unit/test_storage_manager_estimate.js"></script>

I thought that there is no "navigator" and "navigator.storage" properties exposed in the XPCComponents, so I didn't ask Shawn to do this.
As I know, the "indexedDB" property is available in the XPCComponents because additional work is done in http://searchfox.org/mozilla-central/source/js/xpconnect/src/Sandbox.cpp#1013
(In reply to Jan Varga [:janv] from comment #98)
> Comment on attachment 8779988 [details] [diff] [review]
> Bug 1267941 - Implement Storage API estimate() feature - v2, f=btseng
> 
> Review of attachment 8779988 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/cache/test/mochitest/mochitest.ini
> @@ +3,5 @@
> >    test_cache.js
> >    test_cache_add.js
> >    worker_driver.js
> >    worker_wrapper.js
> > +  worker_storage.js
> 
> This file is rather tiny, it could be done without creating a new file for
> the worker script. Take a look at dom/indexedDB/test and look for
> workerScript() to see how it is done.
> 
Sure. I will try to do it.
> ::: dom/indexedDB/test/test_storage_manager_estimate.html
> @@ +8,5 @@
> > +
> > +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> > +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> > +
> > +  <script type="text/javascript;version=1.7" src="unit/test_storage_manager_estimate.js"></script>
> 
> Since this is split into separate JS file, can you also run the test in
> xpcshell suite ?
> See xpcshell-shared.ini file.
I'm a bit worry about this since StorageManager exposing to window and worker not system.
I will try to check this part.

> @@ +43,5 @@
> > +
> > +private:
> > +  ~StorageManager();
> > +
> > +  nsCOMPtr<nsIGlobalObject> mOwner;
> 
> move mOwner to the beginning, it will be private by default there
You mean to move mOwner to the beginning of class? 
I thought this is ok to do like fetch api Response.
http://searchfox.org/mozilla-central/source/dom/fetch/Response.h#145

> ::: dom/workers/WorkerNavigator.h
> @@ +103,5 @@
> >    void SetLanguages(const nsTArray<nsString>& aLanguages);
> >  
> >    uint64_t HardwareConcurrency() const;
> > +
> > +  dom::StorageManager* Storage();
> 
> Hm, is the dom:: prefix needed ?
I will remove it. 
> ::: dom/workers/test/serviceworkers/serviceworker_wrapper.js
> @@ +98,5 @@
> >    });
> >  }
> >  
> > +function workerTestGetStorageManager(cb) {
> > +  addEventListener('message', function workerTestGetStorageManager(e) {
> 
> Hm, aren't you missing the CB postfix here ?
> Other functions seem to use it.
Opps, i did miss it.
(In reply to Shawn Huang [:shawnjohnjr] from comment #100)
> (In reply to Jan Varga [:janv] from comment #98)
> > Comment on attachment 8779988 [details] [diff] [review]
> > Bug 1267941 - Implement Storage API estimate() feature - v2, f=btseng
> > @@ +43,5 @@
> > > +
> > > +private:
> > > +  ~StorageManager();
> > > +
> > > +  nsCOMPtr<nsIGlobalObject> mOwner;
> > 
> > move mOwner to the beginning, it will be private by default there
> You mean to move mOwner to the beginning of class? 
> I thought this is ok to do like fetch api Response.
> http://searchfox.org/mozilla-central/source/dom/fetch/Response.h#145
I guess I now understand. I will move it to the beginning.
I fixed addressed comments, but still need to work on suggestion regarding xpcshell.
Don't worry about xpcshell, I take my comment back. It would be nice to have it, but not in this bug.
(In reply to Jan Varga [:janv] from comment #103)
> Don't worry about xpcshell, I take my comment back. It would be nice to have
> it, but not in this bug.

Thank you. Then I will wait for the rest of files to be reviewed.
Comment on attachment 8779988 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature - v2, f=btseng

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

Several comments for StorageManager.cpp, more to come.

::: dom/quota/StorageManager.cpp
@@ +1,4 @@
> +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + *  * License, v. 2.0. If a copy of the MPL was not distributed with this file,

these lines look odd, two "*" ?

@@ +9,5 @@
> +#include "mozilla/dom/PromiseWorkerProxy.h"
> +#include "mozilla/dom/quota/QuotaManagerService.h"
> +#include "mozilla/dom/WorkerPrivate.h"
> +#include "mozilla/Services.h"
> +

no need for the empty line here

@@ +12,5 @@
> +#include "mozilla/Services.h"
> +
> +#include "nsThreadUtils.h"
> +#include "nsIQuotaCallbacks.h"
> +#include "nsCycleCollectionParticipant.h"

these 3 are not in alphabet order

@@ +18,5 @@
> +using namespace mozilla::dom::workers;
> +
> +namespace mozilla {
> +namespace dom {
> +

What does this base class (EstimateObserver) give you ?
Why MainThreadStorageResolver and WorkerStorageResolver can't inherit nsIQuotaUsageCallback directly ?

@@ +19,5 @@
> +
> +namespace mozilla {
> +namespace dom {
> +
> +class EstimateObserver : public nsIQuotaUsageCallback

class EstimateObserver 
  : public nsIQuotaUsageCallback

@@ +25,5 @@
> +public:
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +
> +  EstimateObserver()
> +  { };

why the extra ";" ?
here and in the destructor too

@@ +27,5 @@
> +
> +  EstimateObserver()
> +  { };
> +
> +  virtual NS_IMETHODIMP

Why the extra "virtual" ? Take a look at NS_IMETHOD macro definition
and why NS_IMETHODIMP ? this is a declaration right ? (not an implementation)

@@ +28,5 @@
> +  EstimateObserver()
> +  { };
> +
> +  virtual NS_IMETHODIMP
> +  OnUsageResult(nsIQuotaUsageRequest *aRequest) override = 0;

and why the "= 0" ?
pure virtual method is defined in nsIQuotaUsageCallback which you inherit here, right ?

@@ +37,5 @@
> +};
> +
> +NS_IMPL_ISUPPORTS(EstimateObserver, nsIQuotaUsageCallback)
> +
> +class MainThreadStorageResolver final : public EstimateObserver

class MainThreadStorageResolver final
  : public EstimateObserver

@@ +46,5 @@
> +public:
> +  explicit MainThreadStorageResolver(Promise* aPromise) : mPromise(aPromise)
> +  { MOZ_ASSERT(mPromise); }
> +
> +  NS_IMETHODIMP

Make this just a declaration and put definition of the method outside.
Or you wanted to inline this method, if so, search for "inlining virtual methods"

@@ +91,5 @@
> +    RefPtr<WorkerStorageResolver> r = new WorkerStorageResolver(proxy);
> +    return r.forget();
> +  }
> +
> +  NS_IMETHODIMP

NS_IMETHOD

@@ +151,5 @@
> +    return true;
> +  }
> +};
> +
> +class WorkerThreadStorageRunnable final : public WorkerRunnable

class WorkerThreadStorageRunnable final
  : public WorkerRunnable

@@ +203,5 @@
> +NS_IMETHODIMP
> +WorkerStorageResolver::OnUsageResult(nsIQuotaUsageRequest *aRequest)
> +{
> +  MOZ_ASSERT(aRequest);
> +  AssertIsOnMainThread();

Assert the thread first and then arguments and then other things.

@@ +264,5 @@
> +  RefPtr<Promise> promise = Promise::Create(mOwner, aRv);
> +  if (NS_IsMainThread()) {
> +    nsCOMPtr<nsPIDOMWindowInner> window = do_QueryInterface(mOwner);
> +    nsCOMPtr<nsIPrincipal> principal;
> +    nsCOMPtr<nsIQuotaManagerService> qms;

Declare variables when you actually need them.

@@ +280,5 @@
> +    principal = doc->NodePrincipal();
> +    MOZ_ASSERT(principal);
> +
> +    qms  = QuotaManagerService::GetOrCreate();
> +    MOZ_ASSERT(qms);

GetOrCreate() can fail, you can't just assert it here.
(In reply to Jan Varga [:janv] from comment #105)
> Comment on attachment 8779988 [details] [diff] [review]
> Bug 1267941 - Implement Storage API estimate() feature - v2, f=btseng
> 
> Review of attachment 8779988 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Several comments for StorageManager.cpp, more to come.
> 
> ::: dom/quota/StorageManager.cpp
> @@ +1,4 @@
> > +/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
> > +/* vim: set ts=8 sts=2 et sw=2 tw=80: */
> > +/* This Source Code Form is subject to the terms of the Mozilla Public
> > + *  * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> 
> these lines look odd, two "*" ?
> 
> @@ +9,5 @@
> > +#include "mozilla/dom/PromiseWorkerProxy.h"
> > +#include "mozilla/dom/quota/QuotaManagerService.h"
> > +#include "mozilla/dom/WorkerPrivate.h"
> > +#include "mozilla/Services.h"
> > +
> 
> no need for the empty line here
> 
> @@ +12,5 @@
> > +#include "mozilla/Services.h"
> > +
> > +#include "nsThreadUtils.h"
> > +#include "nsIQuotaCallbacks.h"
> > +#include "nsCycleCollectionParticipant.h"
> 
> these 3 are not in alphabet order
> 
> @@ +18,5 @@
> > +using namespace mozilla::dom::workers;
> > +
> > +namespace mozilla {
> > +namespace dom {
> > +
> 
> What does this base class (EstimateObserver) give you ?
> Why MainThreadStorageResolver and WorkerStorageResolver can't inherit
> nsIQuotaUsageCallback directly ?
> 
Hi,
Thanks for the comments. Many basic error :|
It's not necessary. I will remove it.
I'm going to apply the patch locally and do some changes in StorageManager.cpp myself.
It seems there would be too many comments which would be hard to process so a patch from me should be easier to parse.
Attached file StorageManager.cpp (obsolete) —
Attachment #8782800 - Attachment mime type: text/x-c++src → text/plain
Ok, most of the changes I wanted to do are in the attached file.
I'll go through it one more time to catch some nits, but we are mostyl done here.
Attachment #8779988 - Attachment is obsolete: true
Attachment #8779988 - Flags: review?(jvarga)
Attachment #8782810 - Attachment is obsolete: true
Attachment #8782810 - Flags: review?(jvarga)
Comment on attachment 8782811 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature

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

::: dom/quota/StorageManager.h
@@ +26,5 @@
> +{
> +  nsCOMPtr<nsIGlobalObject> mOwner;
> +
> +public:
> +  explicit StorageManager(nsIGlobalObject* aGloabl) : mOwner(aGloabl)

Fix these typos here.
Attachment #8782811 - Flags: review?(jvarga)
Please apply this patch too and submit the while thing again. I'll take a look one more time and that's it, thanks!
Attachment #8782800 - Attachment is obsolete: true
Ah, actually you need fix a few XXX in OnUsageResult before submitting a new patch.
This contains the additional patch for StorageManager.cpp
Attachment #8782811 - Attachment is obsolete: true
Attachment #8782967 - Attachment is obsolete: true
Attachment #8783255 - Flags: review?(jvarga)
Comment on attachment 8783255 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature; r=janv

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

I've got some nits for StorageManager.cpp, will send it later.

::: dom/indexedDB/test/mochitest.ini
@@ +395,5 @@
>  # They currently time out on android.
>  skip-if = true ### Bug 1255339: blacklist because no more mozApps
>  [test_serviceworker.html]
>  skip-if = buildapp == 'b2g'
> +[test_storage_manager_estimate.html]

Nit: alphabetical order

@@ +396,5 @@
>  skip-if = true ### Bug 1255339: blacklist because no more mozApps
>  [test_serviceworker.html]
>  skip-if = buildapp == 'b2g'
> +[test_storage_manager_estimate.html]
> +skip-if = buildapp == 'b2g'

Shall this be ?:
skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116

::: dom/indexedDB/test/unit/test_storage_manager_estimate.js
@@ +10,5 @@
> +  is(typeof navigator.storage.estimate, 'function', 'estimate is function');
> +  ok(navigator.storage.estimate() instanceof Promise,
> +     'estimate() method exists and returns a Promise');
> +
> +  let largeValue = new Uint8Array(1e6);

Nit: this is used only once and much later in the code

I would put |const arraySize = 1e6;| after |const objectStoreName = "storagesManager";|
and then you can just |objectStore.put(new Uint8Array(arraySize), 'k');| below.

@@ +48,5 @@
> +    testGenerator.send(estimation.usage);
> +  });
> +  let usageAfterPut = yield undefined;
> +
> +  ok(usageAfterCreate > before, 'estimated usage must increase after createObjectStore');

Nit: this can go right after |let usageAfterCreate = yield undefined;|

@@ +49,5 @@
> +  });
> +  let usageAfterPut = yield undefined;
> +
> +  ok(usageAfterCreate > before, 'estimated usage must increase after createObjectStore');
> +  ok(usageAfterPut > usageAfterCreate, 'estimated usage must increase after putting large object');

Nit: this can go right after |let usageAfterPut = yield undefined;|

@@ +50,5 @@
> +  let usageAfterPut = yield undefined;
> +
> +  ok(usageAfterCreate > before, 'estimated usage must increase after createObjectStore');
> +  ok(usageAfterPut > usageAfterCreate, 'estimated usage must increase after putting large object');
> +  db.close();

Nit: add a new line here

::: dom/quota/StorageManager.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_StorageManager_h
> +#define mozilla_dom_StorageManager_h
> +
> +#include "mozilla/dom/StorageManagerBinding.h"

Nit: I think this include can go to the .cpp

@@ +7,5 @@
> +#ifndef mozilla_dom_StorageManager_h
> +#define mozilla_dom_StorageManager_h
> +
> +#include "mozilla/dom/StorageManagerBinding.h"
> +#include "mozilla/ErrorResult.h"

Nit: we don't need to access ErrorResult methods in this file, so we don't have to include the header here, but .cpp needs it, so move this line to the .cpp

@@ +10,5 @@
> +#include "mozilla/dom/StorageManagerBinding.h"
> +#include "mozilla/ErrorResult.h"
> +#include "nsCOMPtr.h"
> +#include "nsISupports.h"
> +#include "nsPIDOMWindow.h"

Nit: this include can go to the .cpp too

@@ +26,5 @@
> +{
> +  nsCOMPtr<nsIGlobalObject> mOwner;
> +
> +public:
> +  explicit StorageManager(nsIGlobalObject* aGloabl) : mOwner(aGloabl)

Nit: typos here
Attachment #8783255 - Flags: review?(jvarga)
Comment on attachment 8783255 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature; r=janv

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

::: dom/indexedDB/test/mochitest.ini
@@ +395,5 @@
>  # They currently time out on android.
>  skip-if = true ### Bug 1255339: blacklist because no more mozApps
>  [test_serviceworker.html]
>  skip-if = buildapp == 'b2g'
> +[test_storage_manager_estimate.html]

Nit: alphabetical order

@@ +396,5 @@
>  skip-if = true ### Bug 1255339: blacklist because no more mozApps
>  [test_serviceworker.html]
>  skip-if = buildapp == 'b2g'
> +[test_storage_manager_estimate.html]
> +skip-if = buildapp == 'b2g'

Shall this be ?:
skip-if = (buildapp == 'b2g' && toolkit != 'gonk') # Bug 931116

::: dom/indexedDB/test/unit/test_storage_manager_estimate.js
@@ +10,5 @@
> +  is(typeof navigator.storage.estimate, 'function', 'estimate is function');
> +  ok(navigator.storage.estimate() instanceof Promise,
> +     'estimate() method exists and returns a Promise');
> +
> +  let largeValue = new Uint8Array(1e6);

Nit: this is used only once and much later in the code

I would put |const arraySize = 1e6;| after |const objectStoreName = "storagesManager";|
and then you can just |objectStore.put(new Uint8Array(arraySize), 'k');| below.

@@ +48,5 @@
> +    testGenerator.send(estimation.usage);
> +  });
> +  let usageAfterPut = yield undefined;
> +
> +  ok(usageAfterCreate > before, 'estimated usage must increase after createObjectStore');

Nit: this can go right after |let usageAfterCreate = yield undefined;|

@@ +49,5 @@
> +  });
> +  let usageAfterPut = yield undefined;
> +
> +  ok(usageAfterCreate > before, 'estimated usage must increase after createObjectStore');
> +  ok(usageAfterPut > usageAfterCreate, 'estimated usage must increase after putting large object');

Nit: this can go right after |let usageAfterPut = yield undefined;|

@@ +50,5 @@
> +  let usageAfterPut = yield undefined;
> +
> +  ok(usageAfterCreate > before, 'estimated usage must increase after createObjectStore');
> +  ok(usageAfterPut > usageAfterCreate, 'estimated usage must increase after putting large object');
> +  db.close();

Nit: add a new line here

::: dom/quota/StorageManager.cpp
@@ +8,5 @@
> +
> +#include "mozilla/dom/PromiseWorkerProxy.h"
> +#include "mozilla/dom/quota/QuotaManagerService.h"
> +#include "mozilla/dom/WorkerPrivate.h"
> +#include "mozilla/Services.h"

Nit: I don't think this one is needed

@@ +9,5 @@
> +#include "mozilla/dom/PromiseWorkerProxy.h"
> +#include "mozilla/dom/quota/QuotaManagerService.h"
> +#include "mozilla/dom/WorkerPrivate.h"
> +#include "mozilla/Services.h"
> +#include "nsCycleCollectionParticipant.h"

Nit: This include should go to the .h file

@@ +10,5 @@
> +#include "mozilla/dom/quota/QuotaManagerService.h"
> +#include "mozilla/dom/WorkerPrivate.h"
> +#include "mozilla/Services.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsIQuotaCallbacks.h"

Nit: You also need to include nsIQuotaRequests.h

@@ +11,5 @@
> +#include "mozilla/dom/WorkerPrivate.h"
> +#include "mozilla/Services.h"
> +#include "nsCycleCollectionParticipant.h"
> +#include "nsIQuotaCallbacks.h"
> +#include "nsThreadUtils.h"

Nit: It seems this one is not needed either.

@@ +33,5 @@
> +
> +public:
> +  EstimateResolver(Promise* aPromise)
> +    : mPromise(aPromise)
> +  { }

Nit: assert the thread and aPromise here

@@ +37,5 @@
> +  { }
> +
> +  EstimateResolver(PromiseWorkerProxy* aProxy)
> +    : mProxy(aProxy)
> +  { }

Nit: assert the thread and aProxy here

@@ +61,5 @@
> +                       const StorageEstimate& aQuotaUsage)
> +    : WorkerRunnable(aWorkerPrivate, WorkerThreadUnchangedBusyCount)
> +    , mProxy(aProxy)
> +    , mQuotaUsage(aQuotaUsage)
> +  {

Nit: assert the thread here

@@ +77,5 @@
> +
> +public:
> +  explicit EstimateWorkerMainThreadRunnable(PromiseWorkerProxy* aProxy)
> +    : WorkerMainThreadRunnable(aProxy->GetWorkerPrivate(),
> +                               NS_LITERAL_CSTRING("StorageManager"))

s/StorageManager/StorageManager :: Estimate

::: dom/quota/StorageManager.h
@@ +6,5 @@
> +
> +#ifndef mozilla_dom_StorageManager_h
> +#define mozilla_dom_StorageManager_h
> +
> +#include "mozilla/dom/StorageManagerBinding.h"

Nit: I think this include can go to the .cpp

@@ +7,5 @@
> +#ifndef mozilla_dom_StorageManager_h
> +#define mozilla_dom_StorageManager_h
> +
> +#include "mozilla/dom/StorageManagerBinding.h"
> +#include "mozilla/ErrorResult.h"

Nit: we don't need to access ErrorResult methods in this file, so we don't have to include the header here, but .cpp needs it, so move this line to the .cpp

@@ +10,5 @@
> +#include "mozilla/dom/StorageManagerBinding.h"
> +#include "mozilla/ErrorResult.h"
> +#include "nsCOMPtr.h"
> +#include "nsISupports.h"
> +#include "nsPIDOMWindow.h"

Nit: this include can go to the .cpp too

@@ +26,5 @@
> +{
> +  nsCOMPtr<nsIGlobalObject> mOwner;
> +
> +public:
> +  explicit StorageManager(nsIGlobalObject* aGloabl) : mOwner(aGloabl)

Nit: typos here
Attachment #8783255 - Attachment is obsolete: false
Sorry, comment 125 contains stuff from comment 121. Ignore that part.
However, there is some new stuff for StorageManager.cpp
Comment on attachment 8783473 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature, r=janv

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

Ok, I think I'm done. I'll take one more final look when you address my last comments and that's it.

::: dom/quota/StorageManager.cpp
@@ +79,5 @@
> +  RefPtr<PromiseWorkerProxy> mProxy;
> +
> +public:
> +  explicit EstimateWorkerMainThreadRunnable(PromiseWorkerProxy* aProxy)
> +    : WorkerMainThreadRunnable(aProxy->GetWorkerPrivate(),

Nit: Add aWorkerPrivate argument and then assert it and also assert the thread in constructor body

@@ +94,5 @@
> +nsresult
> +GetUsageForPrincipal(nsIPrincipal* aPrincipal,
> +                     nsIQuotaUsageCallback* aCallback,
> +                     nsIQuotaUsageRequest** aRequest)
> +{

Nit: Assert all the arguments

@@ +124,5 @@
> +  MOZ_ASSERT(aRequest);
> +
> +  nsresult rv;
> +  aRequest->GetResultCode(&rv);
> +  if (rv != NS_OK) {

we usually use NS_FAILED(rv)

@@ +125,5 @@
> +
> +  nsresult rv;
> +  aRequest->GetResultCode(&rv);
> +  if (rv != NS_OK) {
> +    mPromise->MaybeReject(rv);

This needs to be a bit smarter, what if mPromise is null, but mProxy is not null ?
It seems that FinishWorkerRunnable needs to support the error case too, right ?

@@ +132,5 @@
> +
> +  uint64_t usage, quota;
> +  rv = aRequest->GetUsage(&usage);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    mPromise->MaybeReject(rv);

dtto

@@ +138,5 @@
> +  }
> +
> +  rv = aRequest->GetLimit(&quota);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    mPromise->MaybeReject(rv);

dtto

@@ +224,5 @@
> +StorageManager::Estimate(ErrorResult& aRv)
> +{
> +  MOZ_ASSERT(mOwner);
> +
> +  RefPtr<Promise> promise = Promise::Create(mOwner, aRv);

if (NS_WARN_IF(!promise)) {
  return nullptr;
}
Comment on attachment 8783872 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature, r=janv

In |OnUsageResult|, I check request failure via |GetResultCode| so we can return earlier, so I don't check other two places the return value via |GetUsage|[1] and |GetLimit|.

[1] http://searchfox.org/mozilla-central/source/dom/quota/QuotaRequests.cpp#152
Attachment #8783872 - Flags: review?(jvarga)
Comment on attachment 8783887 [details] [diff] [review]
bug-1267941-interdiff.patch

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

::: b/dom/quota/StorageManager.cpp
@@ +99,5 @@
>    RefPtr<PromiseWorkerProxy> mProxy;
>  
>  public:
> +  explicit EstimateWorkerMainThreadRunnable(PromiseWorkerProxy* aProxy,
> +                                            WorkerPrivate* aWorkerPrivate)

Nit: I would change the order here, aWorkerPrivate should come first

@@ +101,5 @@
>  public:
> +  explicit EstimateWorkerMainThreadRunnable(PromiseWorkerProxy* aProxy,
> +                                            WorkerPrivate* aWorkerPrivate)
> +    : WorkerMainThreadRunnable(aWorkerPrivate,
> +                               NS_LITERAL_CSTRING("StorageManager::Estimate"))

Nit: I think the style I saw in other places is "StorageManager :: Estimate"

@@ +106,5 @@
>      , mProxy(aProxy)
>    {
>      MOZ_ASSERT(aProxy);
> +    MOZ_ASSERT(aWorkerPrivate);
> +    aWorkerPrivate->AssertIsOnWorkerThread();

Nit: Follow the new order here too

@@ +151,5 @@
>    MOZ_ASSERT(aRequest);
>  
> +  nsresult errorCode, rv;
> +  rv = aRequest->GetResultCode(&errorCode);
> +  if (NS_FAILED(rv) || NS_FAILED(errorCode)) {

Ok, I see 3 problems here:
1. duplicity of code
- the whole thing, lock, new runnable and dispatch is the same
- this will become even worse once you fix 2.

2. aRequest->GetUsage() and GetLimit() both return nsresult and that should be
checked, actually you will have to do it now, since bug 1297056 already landed

3. if |rv| failed then you'll send possibly uninitialized errorCode to the runnable

You can just create a new helper method which returns nsresult and StorageEstimate. The helper method will call GetResultCode, GetUsage and GetLimit.
I would call the method GetStorageEstimate(aRequest) and it can be placed right
after GetUsageFromPrincipla() I think

@@ +214,5 @@
> +  if (NS_FAILED(mRv)) {
> +    promise->MaybeReject(mRv);
> +    mProxy->CleanUp();
> +
> +    return false;

Why do we need to return false here ?
This is a different kind of failure, the code here didn't fail and there's no pending JS exception, so I think this can just return true.
Then you can simplify it to:
if (NS_SUCCEEDED(mRv)) {
  promise->MaybeResolve(mQuotaUsage);
} else {
  promise->MaybeReject(mRv);
}

mProxy->CleanUp();

return true;

::: b/dom/quota/StorageManager.h
@@ +25,4 @@
>    nsCOMPtr<nsIGlobalObject> mOwner;
>  
>  public:
> +  explicit StorageManager(nsIGlobalObject* aGlobal) : mOwner(aGlobal)

Nit: new line here
explicit StorageManager(nsIGlobalObject* aGlobal)
  : mOwner(aGlobal)
Comment on attachment 8783872 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature, r=janv

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

::: dom/quota/StorageManager.h
@@ +24,5 @@
> +{
> +  nsCOMPtr<nsIGlobalObject> mOwner;
> +
> +public:
> +  explicit StorageManager(nsIGlobalObject* aGlobal) : mOwner(aGlobal)

Nit: new line here
explicit StorageManager(nsIGlobalObject* aGlobal)
  : mOwner(aGlobal)
Attachment #8783872 - Flags: review?(jvarga)
(In reply to Jan Varga [:janv] from comment #131)
> Comment on attachment 8783887 [details] [diff] [review]
> bug-1267941-interdiff.patch
> 
> Review of attachment 8783887 [details] [diff] [review]:
> -----------------------------------------------------------------
> You can just create a new helper method which returns nsresult and
> StorageEstimate. The helper method will call GetResultCode, GetUsage and
> GetLimit.
> I would call the method GetStorageEstimate(aRequest) and it can be placed
> right
> after GetUsageFromPrincipla() I think
I can't access EstimateResolver::FinishWorkerRunnable from a new helper method. So I added private member functions.
Attached patch patch (obsolete) — Splinter Review
Comment on attachment 8784764 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature, r=janv

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

There's some progress, but this is just too complicated. It can be much simpler.
I attached a patch which simplifies it. Let's just apply it and then I'll take a look one more time.

::: dom/quota/StorageManager.cpp
@@ +196,5 @@
> +  nsresult rv, errorCode;
> +  rv = aRequest->GetResultCode(&errorCode);
> +
> +  if (NS_FAILED(rv) || NS_FAILED(errorCode)) {
> +    rv = RejectPromise(aProxy, aPromise, errorCode);

This is still incorrect. If rv is not NS_OK then you pass errorCode to RejectPromise(), but errorCode
is just declared (not initialized) and you are not guaranteed that GetResultCode initialized errorCode
since the method failed.
Do you see the problem here ?
Attachment #8784764 - Flags: review?(jvarga)
(In reply to Jan Varga [:janv] from comment #137)
> Comment on attachment 8784764 [details] [diff] [review]
> Bug 1267941 - Implement Storage API estimate() feature, r=janv
> 
> Review of attachment 8784764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There's some progress, but this is just too complicated. It can be much
> simpler.
> I attached a patch which simplifies it. Let's just apply it and then I'll
> take a look one more time.
> 
> ::: dom/quota/StorageManager.cpp
> @@ +196,5 @@
> > +  nsresult rv, errorCode;
> > +  rv = aRequest->GetResultCode(&errorCode);
> > +
> > +  if (NS_FAILED(rv) || NS_FAILED(errorCode)) {
> > +    rv = RejectPromise(aProxy, aPromise, errorCode);
> 
> This is still incorrect. If rv is not NS_OK then you pass errorCode to
> RejectPromise(), but errorCode
> is just declared (not initialized) and you are not guaranteed that
> GetResultCode initialized errorCode
> since the method failed.
> Do you see the problem here ?

Yes. You're right. This won't work.
Comment on attachment 8785141 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature, r=janv

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

Ok, that's it.
Thanks!

::: dom/quota/StorageManager.cpp
@@ +57,5 @@
> +
> +  NS_DECL_THREADSAFE_ISUPPORTS
> +
> +  NS_IMETHOD
> +  OnUsageResult(nsIQuotaUsageRequest *aRequest) override;

This should be just NS_DECL_NSIQUOTAUSAGECALLBACK

@@ +156,5 @@
> + * Local class implementations
> + ******************************************************************************/
> +
> +void
> +EstimateResolver::ResolveOrReject(Promise* aPromise)

Sorry, forgot to add an assertion for aPromise here.
Attachment #8785141 - Flags: review?(jvarga) → review+
(In reply to Shawn Huang [:shawnjohnjr] from comment #143)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8a4d5a7fe5e

/home/worker/workspace/build/src/dom/quota/StorageManager.cpp:39:3: error: bad implicit conversion constructor for 'EstimateResolver'
/home/worker/workspace/build/src/dom/quota/StorageManager.cpp:47:3: error: bad implicit conversion constructor for 'EstimateResolver'
/home/worker/workspace/build/src/dom/quota/StorageManager.cpp:73:3: error: bad implicit conversion constructor for 'FinishWorkerRunnable'

Oh! That explicit things!
Blocks: 997786
(In reply to Shawn Huang [:shawnjohnjr] from comment #145)
> Add explicit keyword to the patch for constructors.
> 
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f68cd56cf816
I checked all the fail test cases, it seems not related to my patch.
Comment on attachment 8785206 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature, r=janv

Hi baku,
Since SecureContext is not fully ready on worker, I will add SecureContext annotation in Bug 1268804 when things are ready. Could you review webidl?

https://storage.spec.whatwg.org/#storagemanager
Attachment #8785206 - Flags: superreview?(amarchesini)
Comment on attachment 8785206 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature, r=janv

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

::: dom/quota/QuotaManagerService.cpp
@@ -502,5 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
>    MOZ_ASSERT(aPrincipal);
>    MOZ_ASSERT(aCallback);
> -  MOZ_ASSERT(nsContentUtils::IsCallerChrome());

I want janv to review this quota part.

::: dom/quota/StorageManager.cpp
@@ +21,5 @@
> +namespace dom {
> +
> +namespace {
> +
> +class EstimateResolver final

write a comment about what this class does.

@@ +63,5 @@
> +  ~EstimateResolver()
> +  { }
> +};
> +
> +class EstimateResolver::FinishWorkerRunnable final

write a comment about what this class does.

@@ +71,5 @@
> +
> +public:
> +  explicit FinishWorkerRunnable(EstimateResolver* aResolver)
> +    : WorkerRunnable(aResolver->mProxy->GetWorkerPrivate(),
> +                     WorkerThreadUnchangedBusyCount)

Why this? Remove this and use the default value.

@@ +148,5 @@
> +  aStorageEstimate.mQuota.Construct() = limit;
> +  return NS_OK;
> +}
> +
> +} // namespace

anonymous namespace

@@ +155,5 @@
> + * Local class implementations
> + ******************************************************************************/
> +
> +void
> +EstimateResolver::ResolveOrReject(Promise* aPromise)

this must be into the anonymous namespace.

@@ +183,5 @@
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      mResultCode = rv;
> +    }
> +  }
> +

Write a comment saying that here we are in a main-thread request.

@@ +184,5 @@
> +      mResultCode = rv;
> +    }
> +  }
> +
> +  if (!mProxy) {

MOZ_ASSERT(mPromise)

@@ +188,5 @@
> +  if (!mProxy) {
> +    ResolveOrReject(mPromise);
> +    return NS_OK;
> +  }
> +

... and here in a worker request.

::: dom/quota/StorageManager.h
@@ +24,5 @@
> +{
> +  nsCOMPtr<nsIGlobalObject> mOwner;
> +
> +public:
> +  explicit StorageManager(nsIGlobalObject* aGlobal)

move this too in the cpp file.

@@ +42,5 @@
> +  Estimate(ErrorResult& aRv);
> +
> +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(StorageManager)
> +

move these 2 lines at the beginning of the class definition.

::: dom/webidl/StorageManager.webidl
@@ +1,1 @@
> +[Exposed=(Window,Worker)]

Header? Please add the default comments, plus the URL of the spec.

@@ +7,5 @@
> +dictionary StorageEstimate {
> +  unsigned long long usage;
> +  unsigned long long quota;
> +};
> +

extra line.
Attachment #8785206 - Flags: superreview?(amarchesini) → review+
Attachment #8785206 - Flags: review?(jvarga)
(In reply to Andrea Marchesini [:baku] from comment #149)
> @@ -502,5 @@
> >  {
> >    MOZ_ASSERT(NS_IsMainThread());
> >    MOZ_ASSERT(aPrincipal);
> >    MOZ_ASSERT(aCallback);
> > -  MOZ_ASSERT(nsContentUtils::IsCallerChrome());
> 
> I want janv to review this quota part.

I already reviewed this.

> @@ +71,5 @@
> > +
> > +public:
> > +  explicit FinishWorkerRunnable(EstimateResolver* aResolver)
> > +    : WorkerRunnable(aResolver->mProxy->GetWorkerPrivate(),
> > +                     WorkerThreadUnchangedBusyCount)
> 
> Why this? Remove this and use the default value.

Ok, but that will use WorkerThreadModifyBusyCount, do we really want that ?

> 
> @@ +148,5 @@
> > +  aStorageEstimate.mQuota.Construct() = limit;
> > +  return NS_OK;
> > +}
> > +
> > +} // namespace
> 
> anonymous namespace

I think that was changed to just "namespace" some time ago

> @@ +155,5 @@
> > + * Local class implementations
> > + ******************************************************************************/
> > +
> > +void
> > +EstimateResolver::ResolveOrReject(Promise* aPromise)
> 
> this must be into the anonymous namespace.

hmm, why ?

> 
> @@ +183,5 @@
> > +    if (NS_WARN_IF(NS_FAILED(rv))) {
> > +      mResultCode = rv;
> > +    }
> > +  }
> > +
> 
> Write a comment saying that here we are in a main-thread request.

Yeah, the |!mProxy| case can have such a comment

> @@ +42,5 @@
> > +  Estimate(ErrorResult& aRv);
> > +
> > +  NS_DECL_CYCLE_COLLECTING_ISUPPORTS
> > +  NS_DECL_CYCLE_COLLECTION_SCRIPT_HOLDER_CLASS(StorageManager)
> > +
> 
> move these 2 lines at the beginning of the class definition.

sorry, this is actually the intended style here, I'd like to have these overrides here

> ::: dom/webidl/StorageManager.webidl
> @@ +1,1 @@
> > +[Exposed=(Window,Worker)]
> 
> Header? Please add the default comments, plus the URL of the spec.

Hm, then we could add the whole interface and comment out unimplemented methods.
Comment on attachment 8785206 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature, r=janv

r=me, but please talk to baku about issues that I commented on
Attachment #8785206 - Flags: review?(jvarga) → review+
Hi baku,
Would you please mind answer some questions in Comment 150?
Flags: needinfo?(amarchesini)
> Ok, but that will use WorkerThreadModifyBusyCount, do we really want that ?

It should be. Yes. That is the default, and I don't see why this runnable should behaves differently.

> > this must be into the anonymous namespace.
> 
> hmm, why ?

To make it clear, I would say. Many of these methods can be inline, btw.
But it's OK what janv prefers.


> Hm, then we could add the whole interface and comment out unimplemented
> methods.

Right.

For all the other comments, follow what janv says. Thanks.
Flags: needinfo?(amarchesini)
Comment on attachment 8788764 [details] [diff] [review]
Bug 1267941 - Implement Storage API estimate() feature, r=janv,baku

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

Looks good, ship it!
(In reply to Jan Varga [:janv] from comment #156)
> Comment on attachment 8788764 [details] [diff] [review]
> Bug 1267941 - Implement Storage API estimate() feature, r=janv,baku
> 
> Review of attachment 8788764 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, ship it!

Thank you for all your efforts and time.
Rebase to the latest Navigator.cpp due to some lines shifts.
Pushed by shuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e753af61eb02
Implement Storage API estimate() feature, r=janv,baku
https://hg.mozilla.org/mozilla-central/rev/e753af61eb02
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Thanks for all the efforts from janv and baku.
Sure, you are welcome.
Whiteboard: storage-v1
New documentation pages created:

https://developer.mozilla.org/en-US/docs/Web/API/Storage_API (which is somewhat rough since only the work on documenting estimates was in scope for this work).
https://developer.mozilla.org/en-US/docs/Web/API/NavigatorStorage
https://developer.mozilla.org/en-US/docs/Web/API/NavigatorStorage/storage
https://developer.mozilla.org/en-US/docs/Web/API/StorageEstimate
https://developer.mozilla.org/en-US/docs/Web/API/StorageEstimate/quota
https://developer.mozilla.org/en-US/docs/Web/API/StorageEstimate/usage

Updated:

https://developer.mozilla.org/en-US/docs/Web/API/Navigator (to mention NavigatorStorage and move storage into the standard properties list)
https://developer.mozilla.org/en-US/docs/Web/API/StorageManager
https://developer.mozilla.org/en-US/docs/Web/API/StorageManager/estimate

https://developer.mozilla.org/en-US/Firefox/Releases/51

It would be helpful if someone could review this for correctness. It’s not a complete set of docs for Storage since it’s not fully implemented, which put some stuff out of scope for this bug, but I’d like to be sure the stuff that’s there is correct.
Whiteboard: storage-v1 → [storage-v1]
Hi Shawn,

Do you have time to give the MDN documentation for Storage API a technical review, just to make sure it is correct? We would really appreciate it.

Thanks!
Flags: needinfo?(shuang)
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #166)
> Hi Shawn,
> 
> Do you have time to give the MDN documentation for Storage API a technical
> review, just to make sure it is correct? We would really appreciate it.
> 
> Thanks!

Yes. I will take a look on Friday.
(In reply to Shawn Huang [:shawnjohnjr] from comment #167)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #166)
> > Hi Shawn,
> > 
> > Do you have time to give the MDN documentation for Storage API a technical
> > review, just to make sure it is correct? We would really appreciate it.
> > 
> > Thanks!
> 
> Yes. I will take a look on Friday.

That's great - thank you very much.
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #168)
> (In reply to Shawn Huang [:shawnjohnjr] from comment #167)
> > (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #166)
> > > Hi Shawn,
> > > 
> > > Do you have time to give the MDN documentation for Storage API a technical
> > > review, just to make sure it is correct? We would really appreciate it.
> > > 
> > > Thanks!
I've updated notes and correctness. But I saw 'example tbd' in Storage API Data persistence and clearing section.
I think now you can add examples since now our Preferences UI supports this feature.

See:
https://mozilla.invisionapp.com/share/4Y87EJO39#/screens/179635747

Thank you.
Flags: needinfo?(shuang)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.