Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor) Reporter
	Description • 8 years ago

Transfer persistent type to non-persistent storage data type

	Hsin-Yi Tsai (she/her) [:hsinyi]
	Comment 1 • 8 years ago

Hi Shawn, I guess this is an internal API to support the UX design for Storage project and sounds like our next step, so I set this to P2 (something b/w urgent and backlog). Please feel free to correct or adjust this if you have a different opinion. Thanks!

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor) Reporter
	Comment 2 • 8 years ago

Yes. This should be supported for front-end.

	Tom Tung [:tt, :ttung] Assignee
	Comment 3 • 8 years ago

It's follow-up bug for bug 1296591.

It would be better to implement the reverting function in originInfo.

Before we land bug 1305665, we still have the group limit for non-persistent storage. When we convert a persistent storage to non-persistent one, we need to care about the group limit. The reason is that we don't have such group limit for persistent storage. 

Thus, we need to do the eviction if we exceed the group limit. From the bug 1298329 comment #20, we can shared some code with CheckTemporaryStorageLimits() but be careful about OriginClearCompleted() can only be run in quota manager's IO thread. 

If we cannot make group usage lower than group limit by removing inactive origins, we need to remove the active origins. To do that, we can actually abort operations for active origins which should remove them from the active origin list.

	Tom Tung [:tt, :ttung] Assignee
	Comment 4 • 8 years ago

I'll work on this bug after patch P1 in bug 1298329 get r+.

	Fischer [:Fischer]
	Comment 5 • 8 years ago

Just updating the discussed API functionalities the front-end part needs to make it clear:
1. Change non-persistent to persistent (Handled in Bug 1298329)
2. Change persistent to non-persistent
3. Change persistent to non-persistent once for all sites.

The reason for #3 is because there is a "removeAll" API in nsIPermissionManager [1]. If this "removeAll" was called some where then all persistent permissions would be revoked. However, under this case, we could only observe the "perm-changed" event with the "cleared" data. No way to know which sites are revoked. So what we can do is change persistent to non-persistent once for all sites.


[1] https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIPermissionManager.idl#148

	Tom Tung [:tt, :ttung] Assignee
	Comment 6 • 8 years ago

Hi Jan,

I have two questions about this bug while I'm trying to implement it. Could you help me figuring it out? Thanks! I listed them as follow:

First, after discussion with Shawn, I'm considering to remove the 'reverting' origin when the newGroupUsage is greater than GroupLimit. The reason is that in most cases, the usage of persisted origin is big and its usage is likely to be greater than GroupLimit. Hence, if we keep using the LRU approach may lead to killing all the origins under the group. Moreover, it's reasonable to do that because this API should only trigger by users and I think 'revert' the specific origin implies they don't care about the specific origin will be removed or not.

Second, do we have a method to ensure that all the directories under the specific group are initialized? I need this method because the 'revert origin' need to ensure all the originInfos under the group is created to calculate the current group usage.

	Jan Varga [:janv]
	Comment 7 • 8 years ago

(In reply to Tom Tung [:tt] from comment #6)
> Hi Jan,
> 
> I have two questions about this bug while I'm trying to implement it. Could
> you help me figuring it out? Thanks! I listed them as follow:
> 
> First, after discussion with Shawn, I'm considering to remove the
> 'reverting' origin when the newGroupUsage is greater than GroupLimit. The
> reason is that in most cases, the usage of persisted origin is big and its
> usage is likely to be greater than GroupLimit. Hence, if we keep using the
> LRU approach may lead to killing all the origins under the group. Moreover,
> it's reasonable to do that because this API should only trigger by users and
> I think 'revert' the specific origin implies they don't care about the
> specific origin will be removed or not.

"I'm considering to remove the 'reverting' origin when the newGroupUsage is greater than GroupLimit."
What does that mean, "remove the 'reverting'" ? You don't want to implement it at all ?

Btw, the reverting to best-effort can also mean that origin is deleted in the end too, because even when we delete all origins under the group, the persisted origin which is being reverted to best-effort, is bigger than group limit.

> Second, do we have a method to ensure that all the directories under the
> specific group are initialized? I need this method because the 'revert
> origin' need to ensure all the originInfos under the group is created to
> calculate the current group usage.

This is default/temporary storage, right ? So origins can't be initialized individually, it's all or nothing.
mTemporaryStorageIsInitialized indicates if temporary storage is initialized or not.

	Tom Tung [:tt, :ttung] Assignee
	Comment 8 • 8 years ago

(In reply to Jan Varga [:janv] from comment #7)
> "I'm considering to remove the 'reverting' origin when the newGroupUsage is
> greater than GroupLimit."
> What does that mean, "remove the 'reverting'" ? You don't want to implement
> it at all ?

I mean to delete the storage of persisted origin when 'RevertOrigin' is called and the newGroupUsage is greater than the groupLimit. 

> Btw, the reverting to best-effort can also mean that origin is deleted in
> the end too, because even when we delete all origins under the group, the
> persisted origin which is being reverted to best-effort, is bigger than
> group limit.

That's what I worry about. In this case, we won't need to delete the other origins if we only delete the persisted origin which is being reverted to best-effort.

> This is default/temporary storage, right ? So origins can't be initialized
> individually, it's all or nothing.
> mTemporaryStorageIsInitialized indicates if temporary storage is initialized
> or not.

So, I need to call EnsureOriginIsInitialized() if I want to do that?

	Jan Varga [:janv]
	Comment 9 • 8 years ago

(In reply to Tom Tung [:tt] from comment #8)
> I mean to delete the storage of persisted origin when 'RevertOrigin' is
> called and the newGroupUsage is greater than the groupLimit. 

So let's say we have these origins:
host1.example.com - persisted (last access time Feb 8 2017) - usage 499 MB
host2.example.com - best-effort (last access time Jan 1 2016) - usage 100 MB
host3.example.com - best-effort (last access time Jan 1 2016) - usage 100 MB
group limit: 500 MB

Now, the user changes host1 from persisted to best-effort. You figure out that new group usage is 699 which is greater than 500. So you delete host1 immediately.
However last access time of host1 is Feb 8 2017 and host2/host3 were accessed more than a year ago. So the old data remains and "fresh" data is deleted.

> So, I need to call EnsureOriginIsInitialized() if I want to do that?

Yes.

	Tom Tung [:tt, :ttung] Assignee
	Comment 10 • 8 years ago

(In reply to Jan Varga [:janv] from comment #9)
Thanks for explanations!

> So let's say we have these origins:
> host1.example.com - persisted (last access time Feb 8 2017) - usage 499 MB
> host2.example.com - best-effort (last access time Jan 1 2016) - usage 100 MB
> host3.example.com - best-effort (last access time Jan 1 2016) - usage 100 MB
> group limit: 500 MB
> 
> Now, the user changes host1 from persisted to best-effort. You figure out
> that new group usage is 699 which is greater than 500. So you delete host1
> immediately.
> However last access time of host1 is Feb 8 2017 and host2/host3 were
> accessed more than a year ago. So the old data remains and "fresh" data is
> deleted.

Yeah, it's strange in this kinds of situations and I cannot persuade myself either. So, I'll use LRU policy.
  
But, I want to point out somthings like below will happen:
host1.example.com - persisted (last access time Feb 8 2017) - usage 501 MB
host2.example.com - best-effort (last access time Jan 1 2016) - usage 100 MB
host3.example.com - best-effort (last access time Jan 1 2016) - usage 100 MB
group limit:500MB

and host1~host3 will be deleted when the user changes host1 from persisted to best-effort.

	Jan Varga [:janv]
	Comment 11 • 8 years ago

(In reply to Tom Tung [:tt] from comment #10)
> But, I want to point out somthings like below will happen:
> host1.example.com - persisted (last access time Feb 8 2017) - usage 501 MB
> host2.example.com - best-effort (last access time Jan 1 2016) - usage 100 MB
> host3.example.com - best-effort (last access time Jan 1 2016) - usage 100 MB
> group limit:500MB
> 
> and host1~host3 will be deleted when the user changes host1 from persisted
> to best-effort.

if host1 (the host that is being changed from persisted to best-effort) usage (not the new group usage) is greater than group limit, you can optimize and delete just host1.

	Tom Tung [:tt, :ttung] Assignee
	Comment 13 • 8 years ago

Hi Jan,

Could you help me to review this patch? Thanks!

I implement an API for reverting the persisted origin. I'll implement another API for reverting all persisted origin in the other patch. Besides, I test simple functionalities (evict the other origins under group & delete itself) it in local. 

In this patch, I mainly implement an API (RevertPersisted) from QMS to QM (RevertPersistedOp). In RevertPersistedOp, I check whether the usage of the origin is greater than the group limit at |DeleteIfOverGroupLimit()|. If it is, then delete the file and quota for this origin. If it's not, then update metadata and originInfo.

I implement updating originInfo at |RevertPersistedOrigin()|. In this function, I split the state into two pieces: reverting and updating the flag. The reason is that I don't want to update persisted flag at first but change it back as failure happening. The |LockedMaybeUpdateGroupUsage()| is mainly to update the group usage. Once it fails, the reverting persisted origin will be deleted. 

Moreover, I extract some duplicate code out. 
- GroupInfo::LockedUpdateUsageCheck() do the checking whether new usage is under groupLimit.
- QuotaManager::LockedEvictOrigins() do the collecting origins for eviction, deleting files and removing quota for origins things here.
- QuotaManager::LockedGetInactiveOriginInfos() do the getting inactive originInfos for the given group.
- Add a member variable |mTargetGroupInfoPair| in class CollectOriginsHelper to make it collect origins under the specific group.

	Jan Varga [:janv]
	Comment 14 • 8 years ago

I can't get to this right now.

	Andrew Overholt [:overholt]
	Comment 15 • 8 years ago

Maybe Bevis has a bit of time to help with reviewing here? (Feel free to say no!)

	Bevis Tseng[:bevis][:btseng](Exited)
	Comment 16 • 8 years ago

Comment on attachment 8836690 [details] [diff] [review]
Bug-1301276-P1-v0-Revert the persisted origin and extract some codes in QM.

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

I'd like to resume the review again after the following issues are addressed.
Thanks!

::: dom/quota/ActorsParent.cpp
@@ +500,5 @@
>      return mPersisted;
>    }
>  
> +  bool
> +  LockedIsReverting() const

LockedIsUnpersisting()

@@ +504,5 @@
> +  LockedIsReverting() const
> +  {
> +    AssertCurrentThreadOwnsQuotaMutex();
> +
> +    return mReverting;

*mUnpersisting*

@@ +531,5 @@
>    void
>    LockedPersist();
>  
> +  void
> +  LockedStartRevertPersist();

LockedStartUnpersist()

@@ +534,5 @@
> +  void
> +  LockedStartRevertPersist();
> +
> +  void
> +  LockedStopRevertPersist();

LockedStopUnpersist()

@@ +543,5 @@
>    const nsCString mOrigin;
>    uint64_t mUsage;
>    int64_t mAccessTime;
>    bool mPersisted;
> +  bool mReverting;

ditto

@@ +1246,5 @@
>    void
>    GetResponse(RequestResponse& aResponse) override;
>  };
>  
> +class RevertPersistedOp final

class *UnpersistOp* final

@@ +3683,5 @@
> +  AssertIsOnIOThread();
> +  MOZ_ASSERT(aDirectory);
> +
> +  // Get a QuotaObject while we've not gotten the lock from QM yet.
> +  RefPtr<QuotaObject> quotaObject = GetQuotaObject(PERSISTENCE_TYPE_DEFAULT,

Get quotaObject later when needed.

@@ +3696,5 @@
> +                                                      aGroup,
> +                                                      aOrigin);
> +  MOZ_ASSERT(originInfo);
> +
> +  if (originInfo->LockedPersisted() && !originInfo->LockedIsReverting()) {

MOZ_ASSERT(!originInfo->LockedIsReverting());

if (originInfo->LockedPersisted()) {
  ...
}

@@ +3724,5 @@
> +      MutexAutoUnlock autoUnlock(mQuotaMutex);
> +
> +      quotaObject->Release();
> +
> +      return;

We didn't originInfo->LockedStopRevertPersist() before returning.
Maybe we can declare a stack class in this function and declare a stack variable to auto Start|StopUnpersisting()

@@ +7319,5 @@
> +
> +  // We don't need to update metadata & originInfo if we deleted it.
> +  if (originDeleted) {
> +    return NS_OK;
> +  }

if (bool originDeleted = aQuotaManager->DeleteIfOverGroupLimit(...) {
    return NS_OK;
  }

@@ +7359,5 @@
> +
> +  // Directory metadata has been successfully created/updated, try to update
> +  // OriginInfo too (it's ok if OriginInfo doesn't exist yet).
> +  // Note: we will delete the file and remove quota if revert fail.
> +  aQuotaManager->RevertPersistedOrigin(mGroup,

UnpersistOrigin

::: dom/quota/PQuota.ipdl
@@ +58,5 @@
>  {
>    PrincipalInfo principalInfo;
>  };
>  
> +struct RevertPersistedParams

UnpersistParams

@@ +71,5 @@
>    ClearAllParams;
>    ResetAllParams;
>    PersistedParams;
>    PersistParams;
> +  RevertPersistedParams;

ditto

::: dom/quota/PQuotaRequest.ipdl
@@ +32,5 @@
>  struct PersistResponse
>  {
>  };
>  
> +struct RevertPersistedResponse

UnpersistResponse

@@ +45,5 @@
>    ClearAllResponse;
>    ResetAllResponse;
>    PersistedResponse;
>    PersistResponse;
> +  RevertPersistedResponse;

ditto

::: dom/quota/QuotaManager.h
@@ +196,5 @@
>    void
>    PersistOrigin(const nsACString& aGroup,
>                  const nsACString& aOrigin);
>  
> +  void

bool

@@ +199,5 @@
>  
> +  void
> +  DeleteIfOverGroupLimit(const nsACString& aGroup,
> +                         const nsACString& aOrigin,
> +                         bool* aOriginDeleted);

return true if originDeleted or false, otherwise.

@@ +202,5 @@
> +                         const nsACString& aOrigin,
> +                         bool* aOriginDeleted);
> +
> +  void
> +  RevertPersistedOrigin(const nsACString& aGroup,

*UnpersistOrigin*

::: dom/quota/nsIQuotaManagerService.idl
@@ +95,5 @@
> +  /**
> +   * Make given origin be best-effort.
> +   *
> +   * @param aPrincipal
> +   *        A principal for the origin which we want to persist.

which we want to *unpersist*

@@ +98,5 @@
> +   * @param aPrincipal
> +   *        A principal for the origin which we want to persist.
> +   */
> +  [must_use] nsIQuotaRequest
> +  revertPersisted(in nsIPrincipal aPrincipal);

How about "unpersist"?

	Jan Varga [:janv]
	Comment 17 • 8 years ago

Yeah, unpersist sounds much better.

	Tom Tung [:tt, :ttung] Assignee
	Comment 18 • 8 years ago

(In reply to Bevis Tseng[:bevistseng][:btseng] from comment #16)
Thanks for the feedback! I'll address comment!

Trying to split the patch into three part and this patch is patch one. This patch is mainly to create an API in QMS and communicate with QM through IPC.

	Tom Tung [:tt, :ttung] Assignee
	Comment 19 • 8 years ago

This is the second patch for unpersist(). This patch is mainly to unpersist the given origin.

Note: I put the extracting code in the third patch. Thus, this patch may have lots of duplicate code.

	Tom Tung [:tt, :ttung] Assignee
	Comment 20 • 8 years ago

This patch is the third patch for unpersist and it's mainly to extract duplicate code out.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor) Reporter
	Comment 24 • 8 years ago

Comment on attachment 8839024 [details] [diff] [review]
P1-2 - patch for imlementing unpersist in QM.

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

::: dom/quota/ActorsParent.cpp
@@ +535,5 @@
> +  LockedStartUnpersist();
> +
> +  void
> +  LockedStopUnpersist();
> +

Shall we keep only one function here? These two functions only update flags.

@@ +3741,5 @@
> +                                 originInfo->mOrigin);
> +    }
> +
> +    // Update the persisted flag if we still have the originInfo object.
> +    originInfo->LockedStopUnpersist();

This should be moved out of if condition because you're going to delete it?

@@ +5266,5 @@
> +
> +  uint64_t delta = aOriginInfo->mUsage;
> +
> +  // Update the group usage if it's allowed.
> +  AssertNoOverflow(groupInfo->mUsage, delta);

Can you explain why this check is needed here? I thought removing this line makes more sense.

@@ +5268,5 @@
> +
> +  // Update the group usage if it's allowed.
> +  AssertNoOverflow(groupInfo->mUsage, delta);
> +
> +  uint64_t groupUsage = groupInfo->mUsage;

Can you add short comments to address the concept 'groupUsage = 'default type' group info usage + 'temporary type' group info usage?

@@ +5275,5 @@
> +    groupUsage += complementaryGroupInfo->mUsage;
> +  }
> +
> +  AssertNoOverflow(groupUsage, delta);
> +  if (groupUsage + delta <= GetGroupLimit()) {

Shall we update group usage here? Your previous patch seems to be correct. I guess something missed when you split the patchsets.

@@ +5288,5 @@
> +  AutoTArray<RefPtr<DirectoryLockImpl>, 2> locks;
> +
> +  // Note: we lock and unlock several times here.
> +  if(!LockedEvictOrigins(delta, locks, aOriginInfo)) {
> +    // The totoal usage of all inactive origins under group is not enough.

So here we don't need to call unlock?

MutexAutoUnlock autoUnlock(mQuotaMutex);
FinalizeOriginEviction(locks);

@@ +5300,5 @@
> +  delta = aOriginInfo->mUsage;
> +
> +  uint64_t newGroupUsage = groupInfo->mUsage + delta;
> +
> +  AssertNoOverflow(groupInfo->mUsage, delta);

Shall we assert first? Move this line above |uint64_t newGroupUsage = groupInfo->mUsage + delta|.

@@ +5323,5 @@
> +
> +    return false;
> +  }
> +
> +  groupInfo->mUsage = newGroupUsage;

Why are we using newGroupUsage here? I don't see |newGroupUsage| got updated.
Can we |groupInfo->mUsage += delta|?

@@ +7363,5 @@
> +
> +  // Directory metadata has been successfully created/updated, try to update
> +  // OriginInfo too.
> +  // Note: we will delete the file and remove quota if unpersist fail.
> +  aQuotaManager->UnpersistOrigin(mGroup,

Why do we still need to call |UnpersistOrigin| even if metadata persisted flag is false? If there is a special reason, can you elaborate it?

	Tom Tung [:tt, :ttung] Assignee
	Comment 25 • 8 years ago

(In reply to Shawn Huang [:shawnjohnjr] from comment #24)
Thanks for feedback!

> @@ +535,5 @@
> > +  LockedStartUnpersist();
> > +
> > +  void
> > +  LockedStopUnpersist();
> > +
> 
> Shall we keep only one function here? These two functions only update flags.

I wrote this two functions to make it much readable.
We need at least two functions here (one for setting the persisted flag, and the other for setting the un-persisting flag).

Let me think about a way to avoid using un-persisting flag...

> @@ +3741,5 @@
> > +                                 originInfo->mOrigin);
> > +    }
> > +
> > +    // Update the persisted flag if we still have the originInfo object.
> > +    originInfo->LockedStopUnpersist();
> 
> This should be moved out of if condition because you're going to delete it?

Oh, I somehow delete the early return in above if-condition.

> @@ +5266,5 @@
> > +
> > +  uint64_t delta = aOriginInfo->mUsage;
> > +
> > +  // Update the group usage if it's allowed.
> > +  AssertNoOverflow(groupInfo->mUsage, delta);
> 
> Can you explain why this check is needed here? I thought removing this line
> makes more sense.

Yeah, it should be removed.

> @@ +5268,5 @@
> > +
> > +  // Update the group usage if it's allowed.
> > +  AssertNoOverflow(groupInfo->mUsage, delta);
> > +
> > +  uint64_t groupUsage = groupInfo->mUsage;
> 
> Can you add short comments to address the concept 'groupUsage = 'default
> type' group info usage + 'temporary type' group info usage?

I wanted to follow the variable naming in MaybeUpdateSize() in order to extract them out to an another function in patch P1-3. Thus, this line will be removed in P1-3.
 
> @@ +5275,5 @@
> > +    groupUsage += complementaryGroupInfo->mUsage;
> > +  }
> > +
> > +  AssertNoOverflow(groupUsage, delta);
> > +  if (groupUsage + delta <= GetGroupLimit()) {
> 
> Shall we update group usage here? Your previous patch seems to be correct. I
> guess something missed when you split the patchsets.

You are right! I will correct it.
 
> @@ +5288,5 @@
> > +  AutoTArray<RefPtr<DirectoryLockImpl>, 2> locks;
> > +
> > +  // Note: we lock and unlock several times here.
> > +  if(!LockedEvictOrigins(delta, locks, aOriginInfo)) {
> > +    // The totoal usage of all inactive origins under group is not enough.
> 
> So here we don't need to call unlock?
> 
> MutexAutoUnlock autoUnlock(mQuotaMutex);
> FinalizeOriginEviction(locks);

I don't think we need to call FinalizeOriginEviction() because we don't delete any origins in this situation, and thus we don't need to call unlock here.

> @@ +5300,5 @@
> > +  delta = aOriginInfo->mUsage;
> > +
> > +  uint64_t newGroupUsage = groupInfo->mUsage + delta;
> > +
> > +  AssertNoOverflow(groupInfo->mUsage, delta);
> 
> Shall we assert first? Move this line above |uint64_t newGroupUsage =
> groupInfo->mUsage + delta|.

Yeah, you are right!

> @@ +5323,5 @@
> > +
> > +    return false;
> > +  }
> > +
> > +  groupInfo->mUsage = newGroupUsage;
> 
> Why are we using newGroupUsage here? I don't see |newGroupUsage| got updated.
> Can we |groupInfo->mUsage += delta|?

I wanted to follow the format in MyabeUpdateSize to make it read easiler for extracting code out. 
> 
> @@ +7363,5 @@
> > +
> > +  // Directory metadata has been successfully created/updated, try to update
> > +  // OriginInfo too.
> > +  // Note: we will delete the file and remove quota if unpersist fail.
> > +  aQuotaManager->UnpersistOrigin(mGroup,
> 
> Why do we still need to call |UnpersistOrigin| even if metadata persisted
> flag is false? If there is a special reason, can you elaborate it?

I do this for avoiding inconsistent conditions between originInfo and metadata. I will merge them into I condition and add an assertion for inconsistent.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor) Reporter
	Comment 27 • 8 years ago

Comment on attachment 8840354 [details] [diff] [review]
P1-2 - patch for imlementing unpersist in QM.

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

::: dom/quota/ActorsParent.cpp
@@ +3231,5 @@
> +    *aDeleteTargetOrigin = false;
> +
> +    RefPtr<GroupInfo> groupInfo = aTargetOriginInfo->mGroupInfo;
> +    MOZ_ASSERT(groupInfo);
> +    MOZ_ASSERT(groupInfo->mPersistenceType == PERSISTENCE_TYPE_DEFAULT);

nit:I suggest to follow the same order, temporary then default.

@@ +3253,5 @@
> +    MOZ_ASSERT(!aTargetOriginInfo->mQuotaObjects.Count(),
> +               "The unpersisting origin should not open files");
> +    inactiveOrigins.InsertElementSorted(aTargetOriginInfo,
> +                                        OriginInfoLRUComparator());
> +

nit: Remove this line.

::: dom/quota/QuotaManager.h
@@ +449,5 @@
> +
> +  bool
> +  LockedUpdateGroupUsage(OriginInfo* aOriginInfo);
> +
> +

nit: Remove this line.

	Tom Tung [:tt, :ttung] Assignee
	Comment 28 • 8 years ago

(In reply to Shawn Huang [:shawnjohnjr] from comment #27)
> ::: dom/quota/ActorsParent.cpp
> @@ +3231,5 @@
> > +    *aDeleteTargetOrigin = false;
> > +
> > +    RefPtr<GroupInfo> groupInfo = aTargetOriginInfo->mGroupInfo;
> > +    MOZ_ASSERT(groupInfo);
> > +    MOZ_ASSERT(groupInfo->mPersistenceType == PERSISTENCE_TYPE_DEFAULT);
> 
> nit:I suggest to follow the same order, temporary then default.

The other codes that have the order temporary then default because they search groupInfo via groupInfoPair.
However, I used originInfo to get the groupInfo so I got the default and then the temporary. Anyway, I will make them follow the same order in P1-3 patch since I will extract out a function here.

	Tom Tung [:tt, :ttung] Assignee
	Comment 32 • 8 years ago

Previous one is a wrong patch...

	Tom Tung [:tt, :ttung] Assignee
	Comment 33 • 8 years ago

Quick fix for a nit.

	Tom Tung [:tt, :ttung] Assignee
	Comment 34 • 8 years ago

Update merging patch for P1-1 & P1-2 & P1-3 as well.

	Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor) Reporter
	Comment 35 • 8 years ago

Comment on attachment 8840709 [details] [diff] [review]
P1-3 - patch for extracting duplicate code out.

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

::: dom/quota/ActorsParent.cpp
@@ +599,5 @@
>    void
>    LockedAddOriginInfo(OriginInfo* aOriginInfo);
>  
> +  bool
> +  LockedUpdateUsageCheck(int64_t aDelta,

Can we use uint64_t here as I explained in the implementation? And LockedUpdateUsageCheck use cases mostly negative. So how about calling LockedIsEvictionNeeded.

@@ +5822,5 @@
>    quotaManager->mTemporaryStorageUsage += aOriginInfo->mUsage;
>  }
>  
> +bool
> +GroupInfo::LockedUpdateUsageCheck(int64_t aDelta,

Why do you choose int64_t instead of uint64_t? MaybeUpdateSize actually consider delta < 0 and return early. So you suppose to use uint64_t, right?

http://searchfox.org/mozilla-central/source/dom/quota/ActorsParent.cpp#2564

@@ +5827,5 @@
> +                                  GroupInfo* aComplementaryGroupInfo)
> +{
> +  AssertCurrentThreadOwnsQuotaMutex();
> +
> +  if (aDelta < 0) {

Then you don't have to add this check for negative case, aDelta is uint64_t.

@@ +5950,5 @@
> +  AssertIsOnBackgroundThread();
> +
> +  QuotaManager* quotaManager = QuotaManager::Get();
> +  NS_ASSERTION(quotaManager, "Shouldn't be null!");
> +

CollectOriginsForEviction(locks) only used here, is it necessary?

@@ +5963,4 @@
>    // We use extra stack vars here to avoid race detector warnings (the same
>    // memory accessed with and without the lock held).
>    nsTArray<RefPtr<DirectoryLockImpl>> locks;
> +  uint64_t sizeToBeFreed = CollectOriginsForEviction(locks);

CollectOriginsForEviction(locks) only used here, is it necessary?

	Tom Tung [:tt, :ttung] Assignee
	Comment 36 • 8 years ago

(In reply to Shawn Huang [:shawnjohnjr] from comment #35)
Thanks!

> Review of attachment 8840709 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/quota/ActorsParent.cpp
> @@ +599,5 @@
> >    void
> >    LockedAddOriginInfo(OriginInfo* aOriginInfo);
> >  
> > +  bool
> > +  LockedUpdateUsageCheck(int64_t aDelta,
> 
> Can we use uint64_t here as I explained in the implementation? And

Sure, we should uint64_t rather than int64_t.

> LockedUpdateUsageCheck use cases mostly negative. So how about calling
> LockedIsEvictionNeeded.

Sure, it sounds much better!

> @@ +5950,5 @@
> > +  AssertIsOnBackgroundThread();
> > +
> > +  QuotaManager* quotaManager = QuotaManager::Get();
> > +  NS_ASSERTION(quotaManager, "Shouldn't be null!");
> > +
> 
> CollectOriginsForEviction(locks) only used here, is it necessary?

I extract the difference between class CollectOriginsHelper and class CollectGroupHelper so that we can reuse the code in Run().

	Tom Tung [:tt, :ttung] Assignee
	Comment 37 • 7 years ago

Per discussion on last Friday (3/3), we decided not to implement this for Storage API because it's too complicate for users. We would like to provide the "clean" option for users only. Thus, close this bug and mark it as Won't fix.

	Jan Varga [:janv]
	Comment 38 • 7 years ago

I'm sorry to hear that. I was expecting that the discussion could lead to some modifications to the unpersist method, but I didn't expect that we would actually drop it.

The moral of the story should be that we shouldn't focus on implementing specific details before we actually know how specific parts are connected and how the workflow looks like.