Closed Bug 1123637 Opened 5 years ago Closed 5 years ago

Remove all code relevant to quota prompts

Categories

(Core :: DOM: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla38

People

(Reporter: janv, Assigned: janv)

References

Details

Attachments

(1 file)

Attached patch patchSplinter Review
No description provided.
Comment on attachment 8551745 [details] [diff] [review]
patch

Ben, this patch is not small, but it shouldn't be so hard to review. Most of it is code and test removal. However, since persistent storage now gets unlimited quota from the beginning, we can adjust/simplify some structures that are used to track quota.

Try run:
https://tbpl.mozilla.org/?tree=Try&rev=5cf1bac76cdf
Attachment #8551745 - Attachment is patch: true
Attachment #8551745 - Flags: review?(bent.mozilla)
(In reply to Jan Varga [:janv] from comment #1)
> However, since persistent storage now gets
> unlimited quota from the beginning, we can adjust/simplify some structures
> that are used to track quota.

This sounds like it would have been great as a separate patch...
Comment on attachment 8551745 [details] [diff] [review]
patch

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

Thanks! This looks good to me, with these addressed:

::: browser/modules/SitePermissions.jsm
@@ -87,5 @@
>      Services.perms.add(aURI, aPermissionID, aState);
> -
> -    if (aPermissionID in gPermissionObject &&
> -        gPermissionObject[aPermissionID].onChange)
> -      gPermissionObject[aPermissionID].onChange(aURI, aState);

I'm not sure that we want to remove this... Could addons be using this feature? Please get this change reviewed by a browser peer.

::: dom/apps/PermissionsTable.jsm
@@ -311,5 @@
>                               trusted: ALLOW_ACTION,
>                               privileged: ALLOW_ACTION,
> -                             certified: ALLOW_ACTION,
> -                             substitute: [
> -                               "indexedDB-unlimited"

This whole "storage" permission can now be removed. It isn't used for anything after this.

::: dom/quota/QuotaManager.cpp
@@ +1458,5 @@
>                                       int64_t aSize)
>  {
>    MOZ_ASSERT(!NS_IsMainThread());
>  
> +  if (aPersistenceType == PERSISTENCE_TYPE_PERSISTENT) {

This can't be asserted as it is in most of the other places?

::: dom/quota/QuotaObject.cpp
@@ +308,5 @@
>    AssertCurrentThreadOwnsQuotaMutex();
>  
>    for (uint32_t index = 0; index < mOriginInfos.Length(); index++) {
>      if (mOriginInfos[index]->mOrigin == aOrigin) {
>        mUsage -= mOriginInfos[index]->mUsage;

Can you assert that this won't overflow?

  MOZ_ASSERT(mUsage >= mOriginInfos[index]->mUsage);
  ... -= ...;

@@ +311,5 @@
>      if (mOriginInfos[index]->mOrigin == aOrigin) {
>        mUsage -= mOriginInfos[index]->mUsage;
>  
> +      QuotaManager* quotaManager = QuotaManager::Get();
> +      MOZ_ASSERT(quotaManager);

Nit: Hoist this out of the loop

@@ +316,2 @@
>  
> +      quotaManager->mTemporaryStorageUsage -= mOriginInfos[index]->mUsage;

Can you assert that this won't overflow?

  MOZ_ASSERT(quotaManager->mTemporaryStorageUsage >= mOriginInfos[index]->mUsage);
  ... -= ...;
Attachment #8551745 - Flags: review?(bent.mozilla) → review+
Comment on attachment 8551745 [details] [diff] [review]
patch

(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #3)
> ::: browser/modules/SitePermissions.jsm
> @@ -87,5 @@
> >      Services.perms.add(aURI, aPermissionID, aState);
> > -
> > -    if (aPermissionID in gPermissionObject &&
> > -        gPermissionObject[aPermissionID].onChange)
> > -      gPermissionObject[aPermissionID].onChange(aURI, aState);
> 
> I'm not sure that we want to remove this... Could addons be using this
> feature? Please get this change reviewed by a browser peer.

Ehsan, can you take a look ?
Attachment #8551745 - Flags: review?(ehsan)
(In reply to ben turner [:bent] (use the needinfo? flag!) from comment #3)
> ::: dom/apps/PermissionsTable.jsm
> @@ -311,5 @@
> >                               trusted: ALLOW_ACTION,
> >                               privileged: ALLOW_ACTION,
> > -                             certified: ALLOW_ACTION,
> > -                             substitute: [
> > -                               "indexedDB-unlimited"
> 
> This whole "storage" permission can now be removed. It isn't used for
> anything after this.

Oh, you're right.

> 
> ::: dom/quota/QuotaManager.cpp
> @@ +1458,5 @@
> >                                       int64_t aSize)
> >  {
> >    MOZ_ASSERT(!NS_IsMainThread());
> >  
> > +  if (aPersistenceType == PERSISTENCE_TYPE_PERSISTENT) {
> 
> This can't be asserted as it is in most of the other places?

The original idea was to just do an early return for public QM methods, but now I see that all the callers check mQuotaEnforced before calling this specific method, so I think we're ok with the assert here.

> 
> ::: dom/quota/QuotaObject.cpp
> @@ +308,5 @@
> >    AssertCurrentThreadOwnsQuotaMutex();
> >  
> >    for (uint32_t index = 0; index < mOriginInfos.Length(); index++) {
> >      if (mOriginInfos[index]->mOrigin == aOrigin) {
> >        mUsage -= mOriginInfos[index]->mUsage;
> 
> Can you assert that this won't overflow?
> 
>   MOZ_ASSERT(mUsage >= mOriginInfos[index]->mUsage);
>   ... -= ...;

Ok

> 
> @@ +311,5 @@
> >      if (mOriginInfos[index]->mOrigin == aOrigin) {
> >        mUsage -= mOriginInfos[index]->mUsage;
> >  
> > +      QuotaManager* quotaManager = QuotaManager::Get();
> > +      MOZ_ASSERT(quotaManager);
> 
> Nit: Hoist this out of the loop

Actually, this case is a bit different, since we call return in that branch.

> 
> @@ +316,2 @@
> >  
> > +      quotaManager->mTemporaryStorageUsage -= mOriginInfos[index]->mUsage;
> 
> Can you assert that this won't overflow?
> 
>   MOZ_ASSERT(quotaManager->mTemporaryStorageUsage >=
> mOriginInfos[index]->mUsage);
>   ... -= ...;

Ok
Comment on attachment 8551745 [details] [diff] [review]
patch

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

::: browser/modules/SitePermissions.jsm
@@ -87,5 @@
>      Services.perms.add(aURI, aPermissionID, aState);
> -
> -    if (aPermissionID in gPermissionObject &&
> -        gPermissionObject[aPermissionID].onChange)
> -      gPermissionObject[aPermissionID].onChange(aURI, aState);

<https://mxr.mozilla.org/addons/search?string=gPermissionObject> seems to suggest that add-ons do not use this.
Attachment #8551745 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/ef4823baf754
Assignee: nobody → Jan.Varga
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
Depends on: 1126638
You need to log in before you can comment on or make changes to this bug.