Closed
Bug 980447
Opened 10 years ago
Closed 10 years ago
add 'persistence type' argument to nsIQuotaManager.clearStoragesForURI
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla32
People
(Reporter: luke, Assigned: luke)
Details
Attachments
(2 files, 2 obsolete files)
16.60 KB,
patch
|
janv
:
review+
|
Details | Diff | Splinter Review |
5.27 KB,
patch
|
janv
:
review+
|
Details | Diff | Splinter Review |
There is currently nsIQuotaManager.clear() and clearStoragesForURI(), but these functions both clear temporary and persistent. A clearTemporaryStorage() (or perhaps clearTemporaryStorageForURI) would allow us to write better tests in, e.g., bug 965970.
Comment 1•10 years ago
|
||
We might want to add just a new argument, since I'm adding clearStoragesForPrincipal() and other equivalents elsewhere.
Assignee | ||
Comment 2•10 years ago
|
||
Ahh, good idea!
Assignee | ||
Comment 3•10 years ago
|
||
Hi Jan. Is there any way we could get a patch for this in the near-term? It would really help with testing bug 965970.
Assignee | ||
Comment 4•10 years ago
|
||
Actually, this is pretty simple, I'll whip up a patch.
Comment 5•10 years ago
|
||
Ok, I'll review.
Assignee | ||
Comment 6•10 years ago
|
||
Comment 7•10 years ago
|
||
Comment on attachment 8404179 [details] [diff] [review] improve-clear-storages Review of attachment 8404179 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for helping with this! Some comments ... ::: dom/quota/PersistenceType.h @@ +56,5 @@ > } > > +inline Nullable<PersistenceType> > +NullablePersistenceTypeFromText(const nsACString& aText) > +{ Hm, I would rather not allow translation of unknown strings to null persistence type. What about putting this here: if (aText.IsVoid()) { return Nullable<PersistenceType>(); } @@ +65,5 @@ > + if (aText.EqualsLiteral("temporary")) { > + return Nullable<PersistenceType>(PERSISTENCE_TYPE_TEMPORARY); > + } > + > + return Nullable<PersistenceType>(); and throw an error here instead of returning nullable persistence type. The method signature would need to be updated: inline Nullable<PersistenceType> NullablePersistenceTypeFromText(const nsACString& aText, mozilla::ErrorResult& aRv) ::: dom/quota/QuotaManager.cpp @@ +2331,5 @@ > matches.Find(mLiveStorages, pattern); > > for (uint32_t index = 0; index < matches.Length(); index++) { > + if (persistenceType.IsNull() || > + matches[index]->Type() == persistenceType.Value()) { Hm, this should be done in StorageMatcher.h in theory, but I don't think it's worth it. Some of this stuff is going away with IndexedDB and QuotaManager on PBackground anyway. @@ +2662,5 @@ > // Find the right SynchronizedOp. > SynchronizedOp* op; > if (aStorage) { > op = FindSynchronizedOp(aPattern, > Nullable<PersistenceType>(aStorage->Type()), Just use aPersistenceType here too. I think it can be rewritten to: op = FindSynchronizedOp(aPattern, aPersistenceType, aStorage ? aStorage->Id() : EmptyCString()); @@ +2849,5 @@ > > nsAutoCString pattern; > GetOriginPatternStringMaybeIgnoreBrowser(aAppId, aBrowserOnly, pattern); > > + // Clear both temporary and persistence storages. Nit: persistent ? ::: dom/quota/QuotaManager.h @@ +198,3 @@ > nsresult > AcquireExclusiveAccess(nsIOfflineStorage* aStorage, > const nsACString& aOrigin, I would add aPersistenceType here too. OpenDatabaseHelper.cpp then needs to pass |mDatabase->Type()| ::: dom/quota/nsIQuotaManager.idl @@ +51,5 @@ > void > clearStoragesForURI(in nsIURI aURI, > [optional] in unsigned long aAppId, > + [optional] in boolean aInMozBrowserOnly, > + [optional] in ACString aPersistenceClass); hm, I would rather not introduce another terminology for "persistence", aPersistenceType should be ok, even when it's a string, I used it like this in TelemetryVFS.cpp
Attachment #8404179 -
Flags: review?(Jan.Varga) → review+
Assignee | ||
Updated•10 years ago
|
Summary: add nsIQuotaManager.clearTemporageStorage() → add 'persistence type' argument to nsIQuotaManager.clearStoragesForURI
Assignee | ||
Comment 8•10 years ago
|
||
Thanks! https://hg.mozilla.org/integration/mozilla-inbound/rev/e28142a94b74
Comment 10•10 years ago
|
||
+inline nsresult +NullablePersistenceTypeFromText(const nsACString& aText, + Nullable<PersistenceType> *aPersistenceType) put * after the type: Nullable<PersistenceType>* aPersistenceType Also, the method shouldn't return NS_ERROR_UNEXPECTED, because I think we expect that an unknown string can be passed to clearStoragesForURI(). I would rather change it to NS_ERROR_FAILURE. Then we need to translate the error to NS_ERROR_INVALID_ARG in clearStoragesForURI() implementation. Ideally, right after |NS_ENSURE_ARG_POINTER(aURI);| The bigger issue that I missed in the review is that AcquireExclusiveAccess() implementation in QuotaManager.cpp needs to be updated too. This was done in ClearStoragesForURI(): for (uint32_t index = 0; index < matches.Length(); index++) { - // We need to grab references to any live storages here to prevent them - // from dying while we invalidate them. - nsCOMPtr<nsIOfflineStorage> storage = matches[index]; - storage->Invalidate(); + if (persistenceType.IsNull() || + matches[index]->Type() == persistenceType.Value()) { + // We need to grab references to any live storages here to prevent them + // from dying while we invalidate them. + nsCOMPtr<nsIOfflineStorage> storage = matches[index]; + storage->Invalidate(); + } } Something similar needs to be done in AcquireExclusiveAccess() when live storages are collected. I knew there was something missing in the original patch, so I didn't review it immediatelly. I just didn't know what it was :)
Assignee | ||
Comment 13•10 years ago
|
||
Somehow previous upload failed...
Attachment #8406896 -
Attachment is obsolete: true
Attachment #8406896 -
Flags: review?(Jan.Varga)
Attachment #8406897 -
Flags: review?(Jan.Varga)
Assignee | ||
Comment 14•10 years ago
|
||
D'oh, qref
Attachment #8406897 -
Attachment is obsolete: true
Attachment #8406897 -
Flags: review?(Jan.Varga)
Attachment #8406898 -
Flags: review?(Jan.Varga)
Comment 15•10 years ago
|
||
Comment on attachment 8406898 [details] [diff] [review] followup-fix Review of attachment 8406898 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/quota/QuotaManager.cpp @@ +2721,2 @@ > if (!matches.IsEmpty()) { > + for (uint32_t i = 0; i < Client::TYPE_MAX; i++) { All this extra looping and checking looks terrible (not your fault). Hopefully this stuff will be much simpler soon.
Attachment #8406898 -
Flags: review?(Jan.Varga) → review+
Assignee | ||
Comment 16•10 years ago
|
||
(In reply to Jan Varga [:janv] from comment #15) I actually started down the path of adding persistence type to StorageMatcher but hit some snags. Glad to hear something better is coming.
Assignee | ||
Comment 17•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/79924a38cc00
Keywords: leave-open
Comment 18•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/79924a38cc00
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•5 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•