1267941 - Implement Storage API estimate() feature

Status: RESOLVED FIXED

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

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Implement Storage API estimate() feature

Comment 1

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Shall we put StorageManager related code under dom/quota? Do you suggest we shall create an another folder?

Comment 2

[Jan Varga [:janv]]

I don't think we need a new directory, but I need to see how new files are named before saying my final word.

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: WIP patch

Comment 4

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 5

[Jan Varga [:janv]]

Yeah, yeah, but I'd like to see actual files .h .cpp before saying it can live together with the original dom/quota

Comment 6

[Tom Tung [:tt, :ttung]]

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

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 8

[Jan Varga [:janv]]

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.

Comment 9

[Tom Tung [:tt, :ttung]]

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!

Comment 10

[Jan Varga [:janv]]

(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.

Comment 11

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 12

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug1267941.patch

Comment 13

[Jan Varga [:janv]]

(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.

Comment 14

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 15

[Jan Varga [:janv]]

(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.

Comment 16

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug1267941.patch (WIP)

Adding estimate() for querying usage only, still need to handle web worker.

Comment 17

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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?

Comment 18

[Jan Varga [:janv]]

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 ?

Comment 19

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 20

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug1267941.patch (WIP)

Comment 21

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug1267941.patch (WIP)

Comment 22

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug1267941.patch (WIP) worker-test

Comment 23

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1267941.patch (WIP)

Adding estimate() function

Comment 24

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1267941.patch (WIP) worker-test

Comment 25

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1267941.patch (WIP) worker-test

Comment 26

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

So right now, we can expose StorageManager to WorkerNavigator and call estimate(). Next step is to add more test cases.

Comment 27

[Jan Varga [:janv]]

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.

Comment 28

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 29

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Per discussion in AllHands2016 Storage meeting, we will implement estimate method based on per site usage/quota. Bug 1281103 will implement |GetUsageForGroup|.

Comment 30

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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|.

Comment 31

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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?

Comment 32

[Jan Varga [:janv]]

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.

Comment 33

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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?

Comment 34

[Jan Varga [:janv]]

(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.

Comment 35

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 36

[Anne (: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.

Comment 37

[Jan Varga [:janv]]

Yeah, that's the option 1.

Comment 38

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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?

Comment 39

[Anne (: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.

Comment 40

[Jan Varga [:janv]]

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).

Comment 41

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

The patch will be revised after bug 1281103 finished.

Comment 42

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1267941.patch

Re-base code again based on bug 1281103.

Comment 43

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1267941.patch

Comment 44

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1267941.patch

Add test cases for worker. Looking into issues after running estimate() in worker thread.

Comment 45

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1267941.patch

Comment 46

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I found leakage during running test cases. I'm looking into it.

Comment 47

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Opps! I found the leakage happens after calling new UsageRequest.                                  
    nsIQuotaUsageRequest* request = new UsageRequest(principal, callback);

Comment 48

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1267941.patch

Fix leakage issues.

Comment 49

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I found the cache test case is wrong, I will fix it in the next revision.

Comment 50

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1267941.patch

Comment 51

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1267941.patch

Comment 53

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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?

Comment 54

[Boris Zbarsky [: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.

Comment 55

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1267941.patch

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

Comment 56

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Marking blocked on Bug 1274315.
Bug 1274315 - Mochitest cannot be used to test [SecureContext] API

Comment 57

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Following https://bugzilla.mozilla.org/show_bug.cgi?id=1274315#c1, I guess we can open a new tab to load the test.

Comment 58

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=1269052

Comment 59

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 60

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: interdiff.txt

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?

Comment 61

[Boris Zbarsky [:bzbarsky]]

If you have an insecure context open a popup to an https url, that popup is still an insecure context per spec.

Comment 62

[(not currently active) Ted Mielczarek]

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.

Comment 63

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 64

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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?

Comment 65

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1267941.patch

Rebase again

Comment 66

[(not currently active) Ted Mielczarek]

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.

Comment 67

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1267941.patch

Add Indexeddb test case for StorageManager estimate method.

Comment 68

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature

Comment 69

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2e73cda202c9

Comment 70

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Based on Comment 66, I think it's better to enable SecureContext in bug 1268804.

Comment 71

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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

Comment 72

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: 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

Comment 73

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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

Comment 74

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature

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

Comment 75

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature

Comment 76

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature

https://treeherder.mozilla.org/#/jobs?repo=try&revision=09dc51eaf4f0

Comment 77

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature

Comment 78

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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 79

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 80

[Hsin-Yi Tsai (she/her) [:hsinyi]]

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!

Comment 81

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 82

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 83

[Bevis Tseng[:bevis][:btseng](Exited)]

(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

Comment 84

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 85

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature - v2

Fix addressed comments from Bevis.

Comment 86

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 87

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 88

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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 89

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature - v2

Fixed indexeddb test case.

Comment 90

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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.

Comment 91

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature - v2

Comment 92

[Bevis Tseng[:bevis][:btseng](Exited)]

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);

Comment 93

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature - v2, f=btseng

Comment 94

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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

Comment 95

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: interdiff v1-v2

Comment 96

[Jan Varga [:janv]]

(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

Comment 97

[Jan Varga [:janv]]

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.

Comment 98

[Jan Varga [:janv]]

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 99

[Bevis Tseng[:bevis][:btseng](Exited)]

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

Comment 100

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 101

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 102

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature - v3

I fixed addressed comments, but still need to work on suggestion regarding xpcshell.

Comment 103

[Jan Varga [:janv]]

Don't worry about xpcshell, I take my comment back. It would be nice to have it, but not in this bug.

Comment 104

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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 105

[Jan Varga [:janv]]

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.

Comment 106

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 107

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature - v3

Fix comment addressed.

Comment 108

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature - v3

Comment 109

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: interdiff-v2-v3

Comment 110

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature - v3

Comment 111

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: interdiff-v2-v3

Comment 112

[Jan Varga [:janv]]

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.

Comment 113

[Jan Varga [:janv]]

Attachment: StorageManager.cpp

Comment 114

[Jan Varga [:janv]]

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.

Comment 115

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature

Comment 116

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature

Rebase Navigator.cpp/h.

Comment 117

[Jan Varga [:janv]]

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.

Comment 118

[Jan Varga [:janv]]

Attachment: additional patch for StorageManager.cpp

Please apply this patch too and submit the while thing again. I'll take a look one more time and that's it, thanks!

Comment 119

[Jan Varga [:janv]]

Ah, actually you need fix a few XXX in OnUsageResult before submitting a new patch.

Comment 120

[Jan Varga [:janv]]

Attachment: Bug 1267941 - Implement Storage API estimate() feature; r=janv

This contains the additional patch for StorageManager.cpp

Comment 121

[Jan Varga [:janv]]

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

Comment 122

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature, r=janv

Comment 123

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature, r=janv

Comment 124

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: interdiff.patch

Comment 125

[Jan Varga [:janv]]

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

Comment 126

[Jan Varga [:janv]]

Sorry, comment 125 contains stuff from comment 121. Ignore that part.
However, there is some new stuff for StorageManager.cpp

Comment 127

[Jan Varga [:janv]]

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 128

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature, r=janv

Comment 129

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: bug-1267941-interdiff.patch

Comment 130

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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

Comment 131

[Jan Varga [:janv]]

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 132

[Jan Varga [:janv]]

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)

Comment 133

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature, r=janv

Comment 134

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: interdiff.patch

Comment 135

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 136

[Jan Varga [:janv]]

Attachment: patch

Comment 137

[Jan Varga [:janv]]

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 ?

Comment 138

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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 139

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature, r=janv

Revised the patch.

Comment 140

[Jan Varga [:janv]]

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.

Comment 141

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature, r=janv

Comment 142

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature

Comment 143

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e8a4d5a7fe5e

Comment 144

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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!

Comment 145

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Add explicit keyword to the patch for constructors.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f68cd56cf816

Comment 146

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature, r=janv

Comment 147

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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 148

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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

Comment 149

[Andrea Marchesini [:baku]]

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.

Comment 150

[Jan Varga [:janv]]

(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 151

[Jan Varga [:janv]]

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

Comment 152

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature, r=janv,baku

Comment 153

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Hi baku,
Would you please mind answer some questions in Comment 150?

Comment 154

[Andrea Marchesini [:baku]]

> 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.

Comment 155

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature, r=janv,baku

Comment 156

[Jan Varga [:janv]]

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!

Comment 157

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: interdiff.patch

Comment 158

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d07145dfe912

Comment 159

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 160

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Attachment: Bug 1267941 - Implement Storage API estimate() feature, r=janv,baku

Rebase to the latest Navigator.cpp due to some lines shifts.

Comment 161

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e753af61eb02
Implement Storage API estimate() feature, r=janv,baku

Comment 162

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e753af61eb02

Comment 163

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

Thanks for all the efforts from janv and baku.

Comment 164

[Jan Varga [:janv]]

Sure, you are welcome.

Comment 165

[Eric Shepherd [:sheppy]]

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.

Comment 166

[Chris Mills [:cmills]]

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!

Comment 167

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 168

[Chris Mills [:cmills]]

(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.

Comment 169

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.