This manager is another step to unified quota handling. It will also support permanent and shared (temporary) storages.

Jonas, we talked about specifying the storage type for .open() and .deleteDatabase() ... We can also use something like this: partial interface IDBFactory { attribute DOMString mozStorageType; // temporary or permanent }; partial interface IDBDatabase { readonly attribute DOMString mozStorageType; }; so .open() and .deleteDatabase() would check actual value of the attribute there are still the other possibilities: .deleteDatabase("myDb", { storageType: "permanent" }) or .deleteDatabase({ name: "myDb", storageType: "permanent" })

and .open("myDb", { version: 5, storageType: "permanent" }) or .open({ name: "myDb", version: 5, storageType: "permanent" })

Since this is touching localStorage (and probably DOM storage in general) I want to ask how this is going to be designed (API and roughly the implementation). Problem is that localStorage should make the quota limit check synchronously (i'm changing this btw in the new implementation). Then, what is a planed target milestone and what is the realistic one.

I think localStorage will be using two separate policies: A) Just like today, it will should have a 5MB limit for how much data is stored. I.e. the total data-size of all keys and all values shouldn't exceed 5MB. This limit needs to be synchronously enforced so that we can throw exceptions when data is written that would exceed this limit. B) We should count the filesize of the underlying sqlite database towards the storage quota tracked by the quota manager added in this bug. I don't think there's a reason to *just* count the key+value datasizes towards the quota managed by the manager in this bug. Doing so would just add a lot of complexity. The size that the user actually cares about is the space lost filesystem size anyway, so that's what we should be tracking when it comes to the quota manager. We should still have the 5MB limit in A), but only as a means of reducing the amount of data that people store in localStorage. Because localStorage is a synchronous API, we're forced to either block page loading or block the whole thread while we are loading the localStorage data for a domain. So we want that operation to be as fast as possible. So we should keep the 5MB limit for that reason. Likewise, I don't think that we should make localStorage throw if we run run into the quota manager limit for an origin. Throwing in that situation would result in webpages seeing exceptions at more or less random times. The 5MB limit in the spec was originally created to prevent such randomness and make the API reliable for webdevelopers to use. Instead I think we should allow setting the value to happen. When we asynchronously attempt to write the data to disk and realize that we're hitting the quota manager limit, we should instead simply stop writing the data to disk and just use the memory cache. I'm guessing that's how we'd behave if the asynchronous writing failed for other reasons too? Potentially we could in that case delete the data for that origin once the user leaves the page, as to make sure that we don't remain in a state when half of the page state is saved and half is not. I don't have strong opinions on that. Does this sound reasonable?

(In reply to Jonas Sicking (:sicking) from comment #5) > I think localStorage will be using two separate policies: > > A) Just like today, it will should have a 5MB limit for how much data is > stored. I.e. > the total data-size of all keys and all values shouldn't exceed 5MB. This > limit > needs to be synchronously enforced so that we can throw exceptions when > data is > written that would exceed this limit. Agree, my new code will enforce this (last thing to finish properly). > B) We should count the filesize of the underlying sqlite database towards the > storage quota tracked by the quota manager added in this bug. I'd agree with this, bug problem is that the file size may have a lot of unallocated space since fragmentation. We should then from time to time do vacuum on the database. Possible with a background thread, I think, could be triggered when user cleans personal data (cookies). > Instead I think we should allow setting the value to happen. When we > asynchronously attempt to write the data to disk and realize that we're > hitting the quota manager limit, we should instead simply stop writing the > data to disk and just use the memory cache. Memory cache still has to have it's limits. > I'm guessing that's how we'd > behave if the asynchronous writing failed for other reasons too? Right now, we just internally warn and that's it, I think. The patches in bug 807021 seems to have some fail/retry logic (that I don't agree with, btw). My new code doesn't have anything like that right now, but it is very simple to introduce it ; however, since localStorage more and more becomes storage for just a cookie-like type of data, usually not critical, then I don't think we should bother that much. Quota may jitter, since it is etld+1 shared, so when foo.example.com deletes something, bar.example.com will have more space suddenly. We may enforce this semi-synchronously. I plan something like if (dataDelta > originAvailable || dataDelta > etld1Available || dataDelta > globalAvailable) -> ERROR_QUOTA_REACHED; etld1Available is determined asynchronously in a way that the storage object can be used prior etld1Available value determination. The window to determine this value however will be very small. > > Potentially we could in that case delete the data for that origin once the > user leaves the page, as to make sure that we don't remain in a state when > half of the page state is saved and half is not. I don't have strong > opinions on that. I don't know. I'd turn the behavior to just a session-only like behavior silently. Or have a storageError event?

> Quota may jitter, since it is etld+1 shared, so when foo.example.com deletes > something, bar.example.com will have more space suddenly. We may enforce > this semi-synchronously. I plan something like if (dataDelta > > originAvailable || dataDelta > etld1Available || dataDelta > > globalAvailable) -> ERROR_QUOTA_REACHED; etld1Available is determined > asynchronously in a way that the storage object can be used prior > etld1Available value determination. The window to determine this value > however will be very small. In the localStorage code, I'm not sure that we need originAvailable or globalAvailable parameters. Simply having a synchronous 5MB limit for each eTLD+1 seems enough. That's how it is right now, no? Once we integrate with the quota manager we'll have to switch to having one sqlite database file per eTLD+1, or one sqlite database per origin. If we use one sqlite database per origin it likely makes sense to adjust things so that the 5MB limit applies per origin instead of per eTLD+1. The quota manager code will enforce a limit per eTLD+1 as well as a global limit, for any data that is written to disk.

I'm going to take a look at the patch in bug 600307, especially how it affects the current quota manager (bug 787804) I think eTLD+1 could be supported even if databases were stored per origin. Anyway we would then use the size of sqlite file to count quota (not the calculated one).

I think it'd be totally fine to make localStorage apply the 5MB-data-limit on a per-origin basis rather than on a per-eTLD+1 basis if it makes things simpler. It's something that we can decide once we migrate the localStorage code to using the quota manager I think.

(In reply to Jonas Sicking (:sicking) from comment #7) > In the localStorage code, I'm not sure that we need originAvailable or > globalAvailable parameters. Simply having a synchronous 5MB limit for each > eTLD+1 seems enough. That's how it is right now, no? It is how it is implemented now, how my new implementation does it, and how it is specified, yes. > > Once we integrate with the quota manager we'll have to switch to having one > sqlite database file per eTLD+1, or one sqlite database per origin. If we > use one sqlite database per origin it likely makes sense to adjust things so > that the 5MB limit applies per origin instead of per eTLD+1. > > The quota manager code will enforce a limit per eTLD+1 as well as a global > limit, for any data that is written to disk. Yep, the tricky part is just to calculate eTLD+1 consumption in a way that it doesn't block and it doesn't make large time windows allowing consumers to go over the quota. The new localStorage impl counts eTLD+1 asynchronously. So it has to keep track per-origin as a safe check to not allow storing large data during the time eTLD+1 consumption calculation is still pending. (In reply to Jan Varga [:janv] from comment #8) > I'm going to take a look at the patch in bug 600307, especially how it affects > the current quota manager (bug 787804) I quickly took a look at your patch and the QuotaInfo object seems to be something similar to my DOMStorageQuota object in the "addition1" incremental patch. > I think eTLD+1 could be supported even if databases were stored per origin. > Anyway we would then use the size of sqlite file to count quota (not the > calculated one). Agree. (In reply to Jonas Sicking (:sicking) from comment #9) > I think it'd be totally fine to make localStorage apply the 5MB-data-limit > on a per-origin basis rather than on a per-eTLD+1 basis if it makes things > simpler. Much simpler, but is against the spec.

(In reply to Honza Bambas (:mayhemer) from comment #10) > Much simpler, but is against the spec. It's not actually. The spec says: "User agents should guard against sites storing data under their origin's other affiliated sites, e.g. storing up to the limit in a1.example.com, a2.example.com, a3.example.com, etc, circumventing the main example.com storage limit." First off, this is just a "should" level requirement. Not a "must" level one (it's quite explicit that "should" means that you don't have to do it). Additionally, as long as we have a centralized quota manager which limits total filesize per eTLD+1 we are fulfilling the above rule. In fact, the spec even says: "A mostly arbitrary limit of five megabytes per origin is suggested. Implementation feedback is welcome and will be used to update this suggestion in the future." With the word "origin" being a link to the definition of an origin. So all in all I think it's quite clear that we can use a 5MB per origin limit. Especially if we apply a file-size-per-eTLD+1 limit. See http://www.whatwg.org/specs/web-apps/current-work/multipage/webstorage.html#disk-space-0

(In reply to Jonas Sicking (:sicking) from comment #11) > (In reply to Honza Bambas (:mayhemer) from comment #10) > > Much simpler, but is against the spec. > > It's not actually. The spec says: Very nice interpretation :) I can stick with it.

(In reply to Jonas Sicking (:sicking) from comment #5) > Likewise, I don't think that we should make localStorage throw if we run run > into the quota manager limit for an origin. Throwing in that situation would > result in webpages seeing exceptions at more or less random times. The 5MB > limit in the spec was originally created to prevent such randomness and make > the API reliable for webdevelopers to use. > > Instead I think we should allow setting the value to happen. When we > asynchronously attempt to write the data to disk and realize that we're > hitting the quota manager limit, we should instead simply stop writing the > data to disk and just use the memory cache. I'm guessing that's how we'd > behave if the asynchronous writing failed for other reasons too? > > Potentially we could in that case delete the data for that origin once the > user leaves the page, as to make sure that we don't remain in a state when > half of the page state is saved and half is not. I don't have strong > opinions on that. > > Does this sound reasonable? So if I understand correctly, docs.google.com could store 3MB and google.com could then store another 3MB and once the user closes their browser, some or all of that data gets thrown out? That can't be to spec, can it?

Just to clarify, my objection is that this would create seemingly random data loss in LocalStorage

(In reply to Vladan Djeric (:vladan) from comment #13) > So if I understand correctly, docs.google.com could store 3MB and google.com > could then store another 3MB and once the user closes their browser, some or > all of that data gets thrown out? My proposal is that the 5MB limit applies per-origin and that we'd synchronously enforce this limit. However the on-disk limit that is per-eTLD+1 follows the same limits that we will have for indexedDB, appcache and other storage APIs. I.e 20% of the total temp storage per eTLD+1. That limit will usually be much larger than 5MB. So if docs.google.com stores 3MB and google.com store another 3MB we'd simply have a total of 6MB for the google.com eTLD+1. Which would likely be just fine. Though there is a risk that if google.com also uses a bunch of IndexedDB data that it could reach above the limit and then get deleted, but that seems like pretty low risk. There's also a risk that another website will use temporary storage and put us above the global limit which would cause us to delete any temporary data associated with google.com, including the 6MB of localStorage data. > That can't be to spec, can it? Acctually, the spec is quite clear that the implementation is allowed to delete data anytime it wants for resource constraints reasons. So we have a lot of freedom here. The more relevant question is how do we balance the user needs for not having left-over stuff on the harddrive, with the users desire to not have websites randomly breaking.

Comment on attachment 697316 [details] [diff] [review] Part 4: Split AutoEnterWindow, OriginInfo and QuotaObject class into separate source files I decided to put these objects into separated files because the next patch (not attached yet) will add a lot of stuff from IndexedDatabaseManager to QuotaManager. Temp storage patch will add a lot of new stuff too.

There's one more patch in my queue for this bug. It introduces a concept of a storage client. It's an abstract interface for quota manager clients. Each storage API must provide an implementation of this interface in order to participate in centralized quota and storage handling. The patch removes QuotaManager's dependency on FileManager and TransactionThreadPool. It's almost ready, will attach it in few days. I have also "almost" finished patches for generic temporary storage handling and also IDB specific patch to support permanent and temporary storages at the same time. These will be reviewed in separate bugs.

Almost complete patch. QuotaManager::AcquireExclusiveAccess() and QuotaManager::RunSynchronizedOp() need more love, especially to utilize the new ArrayCluster class and to call WaitForStoragesToComplete() for all clients, not only IDB.

Comment on attachment 697091 [details] [diff] [review] Part 1: Remove IndexedDatabaseManager's dependency on AsyncConnectionHelper and IDBDatabase Review of attachment 697091 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/indexedDB/IDBDatabase.h @@ +53,5 @@ > NS_DECL_ISUPPORTS_INHERITED > NS_DECL_NSIIDBDATABASE > + > + // nsIFileStorage > + nsIAtom* StorageId() MOZ_OVERRIDE This is a little bit dangerous and I should have seen this sooner when you added nsIFileStorage... We basically have a bunch of methods now that have both a virtual and a non-virtual implementation. Since we don't have any subclasses at the moment it's easy to call either version. However, once we have a subclass that overrides one of these methods then suddenly the two methods with similar names will do very different things. Which is the correct one to call? It's not safe to have both. One option maybe is to make nsIFileStorage and nsIOfflineStorage not be true interfaces and let them have (private) members. The you could have non-virtual inline functions for getters and such. The other option is to just remove the inlined versions and always call the virtuals. @@ +55,5 @@ > + > + // nsIFileStorage > + nsIAtom* StorageId() MOZ_OVERRIDE > + { > + return Id(); Virtual methods should always be defined in the cpp file (since they can't be inlined anyway). ::: dom/indexedDB/IDBFileHandle.cpp @@ +66,5 @@ > > already_AddRefed<nsISupports> > IDBFileHandle::CreateStream(nsIFile* aFile, bool aReadOnly) > { > + IDBDatabase* database = static_cast<IDBDatabase*>(mFileStorage.get()); This won't always be safe, right? And why do you need an IDBDatabase here? If you really need the origin then mFileStorage should either 1) be an nsIOfflineStorage, 2) be QI-able to nsIOfflineStorage, or 3) should be a IDBDatabase proper. ::: dom/indexedDB/IndexedDatabaseManager.cpp @@ +38,5 @@ > #include "nsThreadUtils.h" > #include "nsXPCOM.h" > #include "nsXPCOMPrivate.h" > > +#include "IDBRequest.h" Nit: this should be alphabetized below. @@ +602,5 @@ > } > > // Add this database to its origin array if it exists, create it otherwise. > + nsTArray<nsIOfflineStorage*>* array; > + if (!mLiveDatabases.Get(aDatabase->StorageOrigin(), &array)) { Grab the origin in a stack var so you don't call the virtual twice. @@ +605,5 @@ > + nsTArray<nsIOfflineStorage*>* array; > + if (!mLiveDatabases.Get(aDatabase->StorageOrigin(), &array)) { > + nsAutoPtr<nsTArray<nsIOfflineStorage*> > newArray( > + new nsTArray<nsIOfflineStorage*>()); > + mLiveDatabases.Put(aDatabase->StorageOrigin(), newArray); Nit: Not your fault but this autoptr is unnecessary nowadays. Just: array = new nsTArray<nsIOfflineStorage*>(); mLiveDatabases.Put(origin, newArray); @@ +625,5 @@ > > // Remove this database from its origin array, maybe remove the array if it > // is then empty. > + nsTArray<nsIOfflineStorage*>* array; > + if (mLiveDatabases.Get(aDatabase->StorageOrigin(), &array) && Stack var for origin again.

Comment on attachment 697309 [details] [diff] [review] Part 2: s/database/storage Review of attachment 697309 [details] [diff] [review]: ----------------------------------------------------------------- My eyes glazed over a little while reviewing this one...

Comment on attachment 697315 [details] [diff] [review] Part 3: Remove unused member in TransactionThreadPool Review of attachment 697315 [details] [diff] [review]: ----------------------------------------------------------------- Nice!

Comment on attachment 697316 [details] [diff] [review] Part 4: Split AutoEnterWindow, OriginInfo and QuotaObject class into separate source files Review of attachment 697316 [details] [diff] [review]: ----------------------------------------------------------------- I see the benefit of splitting QuotaObject from QuotaManager, but the rest seems unnecessary. ::: dom/quota/AutoEnterWindow.h @@ +14,5 @@ > +class nsPIDOMWindow; > + > +BEGIN_QUOTA_NAMESPACE > + > +class AutoEnterWindow Hm, I don't understand why you're moving this out of QuotaManager.h since it can't compile on its own anyway. ::: dom/quota/OriginInfo.h @@ +14,5 @@ > +BEGIN_QUOTA_NAMESPACE > + > +class QuotaObject; > + > +class OriginInfo It looks to me like this could stay as part of QuotaObject.h since you always use them together. Why split them up?

Comment on attachment 697984 [details] [diff] [review] Part 5: Move entire storage management from IndexedDatabaseManager to QuotaManager, also split some classes into separate files Review of attachment 697984 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/base/content/pageinfo/permissions.js @@ +11,4 @@ > > var gPermURI; > var gPrefs; > +var gUsageRunnable; Nit: This should be something like 'gUsageCallback' @@ +106,5 @@ > .getService(Components.interfaces.nsIObserverService); > os.removeObserver(permissionObserver, "perm-changed"); > > + if (gUsageRunnable) { > + gUsageRunnable.cancel(); Nit: You should probably null this out now too. @@ +173,5 @@ > permissionManager.remove(gPermURI.host, "indexedDB-unlimited"); > } > if (aPartId == "fullscreen" && permission == UNKNOWN) { > permissionManager.remove(gPermURI.host, "fullscreen"); > + } Nit: You don't want blame for this line. ::: content/media/webrtc/MediaEngineWebRTC.h @@ +16,5 @@ > #include "nsThreadUtils.h" > #include "nsDOMMediaStream.h" > #include "nsDirectoryServiceDefs.h" > #include "nsComponentManagerUtils.h" > +#include "nsRefPtrHashtable.h" Is this needed for anything? ::: dom/indexedDB/IndexedDatabaseManager.cpp @@ +26,3 @@ > > USING_INDEXEDDB_NAMESPACE > +using namespace mozilla; What is this needed for? If it's just StaticRefPtr you should just use that. @@ +103,3 @@ > sIsMainProcess = XRE_GetProcessType() == GeckoProcessType_Default; > > + nsAutoPtr<IndexedDatabaseManager> instance(new IndexedDatabaseManager()); This isn't safe, you're calling methods when instance has a refcount of 0. @@ +111,2 @@ > > + ClearOnShutdown(&gInstance); Wait... So what's closing down the transaction thread pool? That should stay inside IDB. @@ +142,2 @@ > > + mFileManagers.Init(); This should go in the constructor (since it's infallible) ::: dom/indexedDB/IndexedDatabaseManager.h @@ +29,2 @@ > { > + friend class nsAutoPtr<IndexedDatabaseManager>; This class should never be used with nsAutoPtr. ::: dom/indexedDB/OpenDatabaseHelper.cpp @@ +1633,5 @@ > #endif > > mState = eFiringEvents; // In case we fail somewhere along the line. > > + if (QuotaManager::IsShuttingDown()) { It's weird that this got moved to QuotaManager... You call IndexedDatabaseManager::Get() below and I don't know how you can assert it if this function has been moved. ::: dom/ipc/TabParent.cpp @@ +968,5 @@ > // to read any database. That's a security hole, but we don't ship a > // configuration which creates non browser-or-app children, so it's not a big > // deal. > if (!aASCIIOrigin.EqualsLiteral("chrome") && IsBrowserOrApp() && > + !QuotaManager::TabContextMayAccessOrigin(*this, aASCIIOrigin)) { I don't really like this change. The quota manager shouldn't have any say over whether a particular window is allowed to use IndexedDB. This should be moved back to IndexedDatabaseManager. ::: dom/quota/SynchronizedOp.h @@ +20,5 @@ > + > +// A struct that contains the information corresponding to a pending or > +// running operation that requires synchronization (e.g. opening a db, > +// clearing dbs for an origin, etc). > +struct SynchronizedOp I can't tell why you moved this outside of QuotaManager (it's not exported, seems to only be used in QuotaManager.cpp). I think you should probably move it back. ::: dom/quota/nsIQuotaManager.idl @@ +22,5 @@ > + * @param aCallback > + * The callback that will be called when the usage is available. > + */ > + [optional_argc] > + nsICancelableRunnable This should just be nsICancelable or something... You don't want JS to call run() on it. I realize nsICancelable takes an nsresult arg, so maybe we need something new. ::: js/xpconnect/src/dom_quickstubs.qsconf @@ +294,5 @@ > 'nsIBoxObject.height', > > # FileReader > 'nsIDOMFileReader.*', > + Nit: undo

Comment on attachment 700232 [details] [diff] [review] Part 6: Abstract directory initialization, usage calculation and transaction service specific tasks into a new interface called Client. This removes QuotaManager's dependency on FileManager and Transa Review of attachment 700232 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/file/FileService.h @@ +47,5 @@ > NotifyLockedFileCompleted(LockedFile* aLockedFile); > > + void > + WaitForStoragesToComplete(nsTArray<nsCOMPtr<nsIFileStorage> >& aStorages, > + nsIRunnable* aCallback); Add MOZ_OVERRIDE. ::: dom/indexedDB/Client.cpp @@ +28,5 @@ > + nsAString::size_type filenameLen = aFilename.Length(); > + nsAString::size_type sqliteLen = sqlite.Length(); > + > + if (sqliteLen > filenameLen || > + Substring(aFilename, filenameLen - sqliteLen, sqliteLen) != sqlite) { Just use StringEndsWith() ::: dom/indexedDB/Client.h @@ +12,5 @@ > +#include "mozilla/dom/quota/Client.h" > + > +BEGIN_INDEXEDDB_NAMESPACE > + > +class Client : public quota::Client I think this should be fully qualified, 'mozilla::dom::quota::Client' ::: dom/indexedDB/FileManager.h @@ +14,5 @@ > > #include "mozilla/dom/quota/StoragePrivilege.h" > #include "nsDataHashtable.h" > > +#define JOURNAL_DIRECTORY_NAME "journals" Can this be moved to Client.cpp? ::: dom/indexedDB/IDBFileHandle.cpp @@ +66,5 @@ > > already_AddRefed<nsISupports> > IDBFileHandle::CreateStream(nsIFile* aFile, bool aReadOnly) > { > + IDBDatabase* database = IDBDatabase::FromStorage(mFileStorage); Why do we need a concrete pointer here? ::: dom/indexedDB/OpenDatabaseHelper.cpp @@ +1663,5 @@ > + rv = dbDirectory->Exists(&exists); > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > + > + if (exists) { > +#ifdef DEBUG Reverse this so that the debug-only code comes second? @@ +2433,5 @@ > NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > > NS_ASSERTION(directory, "What?"); > > + rv = directory->Append(NS_LITERAL_STRING("idb")); "idb" should be #defined at the top of the file so that you aren't susceptible to typos. ::: dom/indexedDB/TransactionThreadPool.cpp @@ +468,5 @@ > NS_ASSERTION(aDatabase, "Null pointer!"); > > // Get list of transactions for this database id > DatabaseTransactionInfo* dbTransactionInfo; > + if (!mTransactionsInProgress.Get(aDatabase->Id(), Leave as StorageId(), here and below. ::: dom/quota/ArrayCluster.h @@ +18,5 @@ > +{ > +public: > + ArrayClusterBase() > + { > + mArrays = new nsTArray<ValueType>[Length]; Why is this heap allocated? |nsAutoTArray<nsTArray<ValueType>, Length>| is probably what you should use right? In fact, if you just inherit nsAutoTArray you get a bunch of these methods you redefine below for free. @@ +24,5 @@ > + > + nsTArray<ValueType>& > + ArrayAt(uint32_t aIndex) const > + { > + NS_ASSERTION(aIndex < Length, "Bad index!"); New code should use MOZ_ASSERT whenever possible (since it's actually fatal in debug builds). @@ +101,5 @@ > + nsAutoArrayPtr<nsTArray<ValueType> > mArrays; > +}; > + > +template <class ValueType> > +class ArrayCluster : public ArrayClusterBase<ValueType, CLIENT_TYPE_MAX> If you're not defining anything useful here you could just use a typedef. Or just not have a base class and add a default template argument for length. ::: dom/quota/Client.h @@ +18,5 @@ > + > +// An abstract interface for quota manager clients. > +// Each storage API must provide an implementation of this interface in order > +// to participate in centralized quota and storage handling. > +class Client This should probably be refcounted. You use nsAutoPtr to manage the lifetimes of these things but I imagine that we will have problems with this down the road. @@ +21,5 @@ > +// to participate in centralized quota and storage handling. > +class Client > +{ > +public: > + virtual ~Client() You want this to be protected @@ +27,5 @@ > + > + virtual nsresult > + InitDirectory(nsIFile* aDirectory, > + const nsACString& aOrigin, > + UsageRunnable* aUsageRunnable) = 0; I don't understand why we're initializing a directory here... Wouldn't it make more sense for the client interface to only talk about initializing an origin? Or all origins? @@ +31,5 @@ > + UsageRunnable* aUsageRunnable) = 0; > + > + virtual nsresult > + GetUsageForDirectory(nsIFile* aDirectory, > + UsageRunnable* aUsageRunnable) = 0; Same here, this seems like it should be per origin, not directory. ::: dom/quota/ClientType.h @@ +10,5 @@ > +#include "mozilla/dom/quota/QuotaCommon.h" > + > +BEGIN_QUOTA_NAMESPACE > + > +enum ClientType { It would be really great if we could figure out how to make this go away. What are we missing in the interface methods of Client that makes this necessary? ::: dom/quota/Makefile.in @@ +33,5 @@ > $(NULL) > > EXPORTS_mozilla/dom/quota = \ > AcquireListener.h \ > + ArrayCluster.h \ Why does this need to be exported? ::: dom/quota/StorageMatcher.h @@ +45,5 @@ > + > +public: > + template <class T> > + void > + Find(const T& aHashtable, You could nail this down a little by making the type nsBaseHashtable<T,U,V>... It's a little uglier but it's easier for callers to figure out what they're supposed to use. @@ +116,5 @@ > + MatchPattern(const nsACString& aKey, > + BaseType* aValue, > + void* aUserArg) > + { > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); Why does the thread matter here? @@ +191,5 @@ > +}; > + > +template <class ValueType> > +class StorageMatcher : > + public MatcherBase<ArrayCluster<nsIOfflineStorage*>, ValueType> typedef or remove base class and add default template args. ::: dom/quota/nsIOfflineStorage.h @@ +8,5 @@ > #define nsIOfflineStorage_h__ > > #include "nsIFileStorage.h" > > +#include "mozilla/dom/quota/ClientType.h" Can you forward-declare this instead?

(In reply to ben turner [:bent] from comment #27) > Comment on attachment 697091 [details] [diff] [review] > Part 1: Remove IndexedDatabaseManager's dependency on AsyncConnectionHelper > and IDBDatabase > > Review of attachment 697091 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/indexedDB/IDBDatabase.h > @@ +53,5 @@ > > NS_DECL_ISUPPORTS_INHERITED > > NS_DECL_NSIIDBDATABASE > > + > > + // nsIFileStorage > > + nsIAtom* StorageId() MOZ_OVERRIDE > > This is a little bit dangerous and I should have seen this sooner when you > added nsIFileStorage... > > We basically have a bunch of methods now that have both a virtual and a > non-virtual implementation. Since we don't have any subclasses at the moment > it's easy to call either version. However, once we have a subclass that > overrides one of these methods then suddenly the two methods with similar > names will do very different things. Which is the correct one to call? It's > not safe to have both. > > One option maybe is to make nsIFileStorage and nsIOfflineStorage not be true > interfaces and let them have (private) members. The you could have > non-virtual inline functions for getters and such. > > The other option is to just remove the inlined versions and always call the > virtuals. I chose this option. > > @@ +55,5 @@ > > + > > + // nsIFileStorage > > + nsIAtom* StorageId() MOZ_OVERRIDE > > + { > > + return Id(); > > Virtual methods should always be defined in the cpp file (since they can't > be inlined anyway). ok, fixed > > ::: dom/indexedDB/IDBFileHandle.cpp > @@ +66,5 @@ > > > > already_AddRefed<nsISupports> > > IDBFileHandle::CreateStream(nsIFile* aFile, bool aReadOnly) > > { > > + IDBDatabase* database = static_cast<IDBDatabase*>(mFileStorage.get()); > > This won't always be safe, right? And why do you need an IDBDatabase here? > If you really need the origin then mFileStorage should either 1) be an > nsIOfflineStorage, 2) be QI-able to nsIOfflineStorage, or 3) should be a > IDBDatabase proper. ok, I went for a QI > > ::: dom/indexedDB/IndexedDatabaseManager.cpp > @@ +38,5 @@ > > #include "nsThreadUtils.h" > > #include "nsXPCOM.h" > > #include "nsXPCOMPrivate.h" > > > > +#include "IDBRequest.h" > > Nit: this should be alphabetized below. ok > > @@ +602,5 @@ > > } > > > > // Add this database to its origin array if it exists, create it otherwise. > > + nsTArray<nsIOfflineStorage*>* array; > > + if (!mLiveDatabases.Get(aDatabase->StorageOrigin(), &array)) { > > Grab the origin in a stack var so you don't call the virtual twice. ok, fixed > > @@ +605,5 @@ > > + nsTArray<nsIOfflineStorage*>* array; > > + if (!mLiveDatabases.Get(aDatabase->StorageOrigin(), &array)) { > > + nsAutoPtr<nsTArray<nsIOfflineStorage*> > newArray( > > + new nsTArray<nsIOfflineStorage*>()); > > + mLiveDatabases.Put(aDatabase->StorageOrigin(), newArray); > > Nit: Not your fault but this autoptr is unnecessary nowadays. Just: > > array = new nsTArray<nsIOfflineStorage*>(); > mLiveDatabases.Put(origin, newArray); I think I did that later in one of those patches. Anyway, fixed. > > @@ +625,5 @@ > > > > // Remove this database from its origin array, maybe remove the array if it > > // is then empty. > > + nsTArray<nsIOfflineStorage*>* array; > > + if (mLiveDatabases.Get(aDatabase->StorageOrigin(), &array) && > > Stack var for origin again. ok

rebased patch

rebased patch

(In reply to ben turner [:bent] from comment #30) > Comment on attachment 697316 [details] [diff] [review] > Part 4: Split AutoEnterWindow, OriginInfo and QuotaObject class into > separate source files > > Review of attachment 697316 [details] [diff] [review]: > ----------------------------------------------------------------- > > I see the benefit of splitting QuotaObject from QuotaManager, but the rest > seems unnecessary. > > ::: dom/quota/AutoEnterWindow.h > @@ +14,5 @@ > > +class nsPIDOMWindow; > > + > > +BEGIN_QUOTA_NAMESPACE > > + > > +class AutoEnterWindow > > Hm, I don't understand why you're moving this out of QuotaManager.h since it > can't compile on its own anyway. > > ::: dom/quota/OriginInfo.h > @@ +14,5 @@ > > +BEGIN_QUOTA_NAMESPACE > > + > > +class QuotaObject; > > + > > +class OriginInfo > > It looks to me like this could stay as part of QuotaObject.h since you > always use them together. Why split them up? ok to both comments

(In reply to ben turner [:bent] from comment #31) > Comment on attachment 697984 [details] [diff] [review] > Part 5: Move entire storage management from IndexedDatabaseManager to > QuotaManager, also split some classes into separate files > > Review of attachment 697984 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: browser/base/content/pageinfo/permissions.js > @@ +11,4 @@ > > > > var gPermURI; > > var gPrefs; > > +var gUsageRunnable; > > Nit: This should be something like 'gUsageCallback' ok, renamed to gUsageRequest > > @@ +106,5 @@ > > .getService(Components.interfaces.nsIObserverService); > > os.removeObserver(permissionObserver, "perm-changed"); > > > > + if (gUsageRunnable) { > > + gUsageRunnable.cancel(); > > Nit: You should probably null this out now too. ok > > @@ +173,5 @@ > > permissionManager.remove(gPermURI.host, "indexedDB-unlimited"); > > } > > if (aPartId == "fullscreen" && permission == UNKNOWN) { > > permissionManager.remove(gPermURI.host, "fullscreen"); > > + } > > Nit: You don't want blame for this line. reverted > > ::: content/media/webrtc/MediaEngineWebRTC.h > @@ +16,5 @@ > > #include "nsThreadUtils.h" > > #include "nsDOMMediaStream.h" > > #include "nsDirectoryServiceDefs.h" > > #include "nsComponentManagerUtils.h" > > +#include "nsRefPtrHashtable.h" > > Is this needed for anything? MediaEngineWebRTC.h has nsRefPtrHashtable members and the hashtable class is indirectly included through nsDOMFile.h. nsDOMFile.h includes IndexedDatabaseManager.h which doesn't need nsRefPtrHashtable anymore. > > ::: dom/indexedDB/IndexedDatabaseManager.cpp > @@ +26,3 @@ > > > > USING_INDEXEDDB_NAMESPACE > > +using namespace mozilla; > > What is this needed for? If it's just StaticRefPtr you should just use that. yeah, removed entirely (there's only one StaticRefPtr) > > @@ +103,3 @@ > > sIsMainProcess = XRE_GetProcessType() == GeckoProcessType_Default; > > > > + nsAutoPtr<IndexedDatabaseManager> instance(new IndexedDatabaseManager()); > > This isn't safe, you're calling methods when instance has a refcount of 0. hm, I think this was needed to not call the Destroy() method (which sets the closed flag) when something fails during initialization. ok, so I added a new flag gInitialized for this > > @@ +111,2 @@ > > > > + ClearOnShutdown(&gInstance); > > Wait... So what's closing down the transaction thread pool? That should stay > inside IDB. QuotaManager is closing down the transaction thread pool in this patch. The following (final) patch moves it to IDB specific quota manager client, but that's still being called from quota manager. I think the call should be made from quota manager and impl should live in IDB, shouldn't it ? > > @@ +142,2 @@ > > > > + mFileManagers.Init(); > > This should go in the constructor (since it's infallible) yeah, moved > > ::: dom/indexedDB/IndexedDatabaseManager.h > @@ +29,2 @@ > > { > > + friend class nsAutoPtr<IndexedDatabaseManager>; > > This class should never be used with nsAutoPtr. ok, removed > > ::: dom/indexedDB/OpenDatabaseHelper.cpp > @@ +1633,5 @@ > > #endif > > > > mState = eFiringEvents; // In case we fail somewhere along the line. > > > > + if (QuotaManager::IsShuttingDown()) { > > It's weird that this got moved to QuotaManager... You call > IndexedDatabaseManager::Get() below and I don't know how you can assert it > if this function has been moved. we need to discuss about this one ... > > ::: dom/ipc/TabParent.cpp > @@ +968,5 @@ > > // to read any database. That's a security hole, but we don't ship a > > // configuration which creates non browser-or-app children, so it's not a big > > // deal. > > if (!aASCIIOrigin.EqualsLiteral("chrome") && IsBrowserOrApp() && > > + !QuotaManager::TabContextMayAccessOrigin(*this, aASCIIOrigin)) { > > I don't really like this change. The quota manager shouldn't have any say > over whether a particular window is allowed to use IndexedDB. This should be > moved back to IndexedDatabaseManager. ok, I moved it back, but some helper methods are still in QuotaManager > > ::: dom/quota/SynchronizedOp.h > @@ +20,5 @@ > > + > > +// A struct that contains the information corresponding to a pending or > > +// running operation that requires synchronization (e.g. opening a db, > > +// clearing dbs for an origin, etc). > > +struct SynchronizedOp > > I can't tell why you moved this outside of QuotaManager (it's not exported, > seems to only be used in QuotaManager.cpp). I think you should probably move > it back. ok, moved back > > ::: dom/quota/nsIQuotaManager.idl > @@ +22,5 @@ > > + * @param aCallback > > + * The callback that will be called when the usage is available. > > + */ > > + [optional_argc] > > + nsICancelableRunnable > > This should just be nsICancelable or something... You don't want JS to call > run() on it. > > I realize nsICancelable takes an nsresult arg, so maybe we need something > new. Well, I created a new interface nsIQuotaRequest with only one method cancel() for now. We can extend it in future. > > ::: js/xpconnect/src/dom_quickstubs.qsconf > @@ +294,5 @@ > > 'nsIBoxObject.height', > > > > # FileReader > > 'nsIDOMFileReader.*', > > + > > Nit: undo reverted

https://tbpl.mozilla.org/?tree=Try&rev=16e111e9210a https://tbpl.mozilla.org/?tree=Try&rev=58d4f219b59e

(In reply to ben turner [:bent] from comment #32) > Comment on attachment 700232 [details] [diff] [review] > Part 6: Abstract directory initialization, usage calculation and transaction > service specific tasks into a new interface called Client. This removes > QuotaManager's dependency on FileManager and Transa > > Review of attachment 700232 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/file/FileService.h > @@ +47,5 @@ > > NotifyLockedFileCompleted(LockedFile* aLockedFile); > > > > + void > > + WaitForStoragesToComplete(nsTArray<nsCOMPtr<nsIFileStorage> >& aStorages, > > + nsIRunnable* aCallback); > > Add MOZ_OVERRIDE. not sure about this one > > ::: dom/indexedDB/Client.cpp > @@ +28,5 @@ > > + nsAString::size_type filenameLen = aFilename.Length(); > > + nsAString::size_type sqliteLen = sqlite.Length(); > > + > > + if (sqliteLen > filenameLen || > > + Substring(aFilename, filenameLen - sqliteLen, sqliteLen) != sqlite) { > > Just use StringEndsWith() ok, fixed > > ::: dom/indexedDB/Client.h > @@ +12,5 @@ > > +#include "mozilla/dom/quota/Client.h" > > + > > +BEGIN_INDEXEDDB_NAMESPACE > > + > > +class Client : public quota::Client > > I think this should be fully qualified, 'mozilla::dom::quota::Client' ok > > ::: dom/indexedDB/FileManager.h > @@ +14,5 @@ > > > > #include "mozilla/dom/quota/StoragePrivilege.h" > > #include "nsDataHashtable.h" > > > > +#define JOURNAL_DIRECTORY_NAME "journals" > > Can this be moved to Client.cpp? hmm, I can move it to Client.h since FileManager.cpp needs it too > > ::: dom/indexedDB/IDBFileHandle.cpp > @@ +66,5 @@ > > > > already_AddRefed<nsISupports> > > IDBFileHandle::CreateStream(nsIFile* aFile, bool aReadOnly) > > { > > + IDBDatabase* database = IDBDatabase::FromStorage(mFileStorage); > > Why do we need a concrete pointer here? we don't need it anymore, I removed this change > > ::: dom/indexedDB/OpenDatabaseHelper.cpp > @@ +1663,5 @@ > > + rv = dbDirectory->Exists(&exists); > > + NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > > + > > + if (exists) { > > +#ifdef DEBUG > > Reverse this so that the debug-only code comes second? ok, done > > @@ +2433,5 @@ > > NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR); > > > > NS_ASSERTION(directory, "What?"); > > > > + rv = directory->Append(NS_LITERAL_STRING("idb")); > > "idb" should be #defined at the top of the file so that you aren't > susceptible to typos. ok, done > > ::: dom/indexedDB/TransactionThreadPool.cpp > @@ +468,5 @@ > > NS_ASSERTION(aDatabase, "Null pointer!"); > > > > // Get list of transactions for this database id > > DatabaseTransactionInfo* dbTransactionInfo; > > + if (!mTransactionsInProgress.Get(aDatabase->Id(), > > Leave as StorageId(), here and below. I changed Id() to be the virtual method. > > ::: dom/quota/ArrayCluster.h > @@ +18,5 @@ > > +{ > > +public: > > + ArrayClusterBase() > > + { > > + mArrays = new nsTArray<ValueType>[Length]; > > Why is this heap allocated? |nsAutoTArray<nsTArray<ValueType>, Length>| is > probably what you should use right? In fact, if you just inherit > nsAutoTArray you get a bunch of these methods you redefine below for free. ok, changed to |nsAutoTArray<nsTArray<ValueType>, Length>| but I'm not sure how I would get those methods for free > > @@ +24,5 @@ > > + > > + nsTArray<ValueType>& > > + ArrayAt(uint32_t aIndex) const > > + { > > + NS_ASSERTION(aIndex < Length, "Bad index!"); > > New code should use MOZ_ASSERT whenever possible (since it's actually fatal > in debug builds). ok, changed > > @@ +101,5 @@ > > + nsAutoArrayPtr<nsTArray<ValueType> > mArrays; > > +}; > > + > > +template <class ValueType> > > +class ArrayCluster : public ArrayClusterBase<ValueType, CLIENT_TYPE_MAX> > > If you're not defining anything useful here you could just use a typedef. Or > just not have a base class and add a default template argument for length. ok, I added a default template argument for length > > ::: dom/quota/Client.h > @@ +18,5 @@ > > + > > +// An abstract interface for quota manager clients. > > +// Each storage API must provide an implementation of this interface in order > > +// to participate in centralized quota and storage handling. > > +class Client > > This should probably be refcounted. You use nsAutoPtr to manage the > lifetimes of these things but I imagine that we will have problems with this > down the road. ok, changed > > @@ +21,5 @@ > > +// to participate in centralized quota and storage handling. > > +class Client > > +{ > > +public: > > + virtual ~Client() > > You want this to be protected ok, done > > @@ +27,5 @@ > > + > > + virtual nsresult > > + InitDirectory(nsIFile* aDirectory, > > + const nsACString& aOrigin, > > + UsageRunnable* aUsageRunnable) = 0; > > I don't understand why we're initializing a directory here... Wouldn't it > make more sense for the client interface to only talk about initializing an > origin? Or all origins? ok, now I have to construct the directory again in the client impl, but I think it's a good compromise (clean API vs perf) > > @@ +31,5 @@ > > + UsageRunnable* aUsageRunnable) = 0; > > + > > + virtual nsresult > > + GetUsageForDirectory(nsIFile* aDirectory, > > + UsageRunnable* aUsageRunnable) = 0; > > Same here, this seems like it should be per origin, not directory. ok > > ::: dom/quota/ClientType.h > @@ +10,5 @@ > > +#include "mozilla/dom/quota/QuotaCommon.h" > > + > > +BEGIN_QUOTA_NAMESPACE > > + > > +enum ClientType { > > It would be really great if we could figure out how to make this go away. > What are we missing in the interface methods of Client that makes this > necessary? I replaced StorageClientType() with GetClient() in nsIOfflineStorage and also added GetType() to Client. So, ClientType has been removed completely. > > ::: dom/quota/Makefile.in > @@ +33,5 @@ > > $(NULL) > > > > EXPORTS_mozilla/dom/quota = \ > > AcquireListener.h \ > > + ArrayCluster.h \ > > Why does this need to be exported? QuotaManager.h includes it > > ::: dom/quota/StorageMatcher.h > @@ +45,5 @@ > > + > > +public: > > + template <class T> > > + void > > + Find(const T& aHashtable, > > You could nail this down a little by making the type > nsBaseHashtable<T,U,V>... It's a little uglier but it's easier for callers > to figure out what they're supposed to use. ok, done > > @@ +116,5 @@ > > + MatchPattern(const nsACString& aKey, > > + BaseType* aValue, > > + void* aUserArg) > > + { > > + NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); > > Why does the thread matter here? removed (also in all similar methods) changed NS_ASSERTION() to MOZ_ASSERT() too > > @@ +191,5 @@ > > +}; > > + > > +template <class ValueType> > > +class StorageMatcher : > > + public MatcherBase<ArrayCluster<nsIOfflineStorage*>, ValueType> > > typedef or remove base class and add default template args. ok, I added a default template argument for BaseType > > ::: dom/quota/nsIOfflineStorage.h > @@ +8,5 @@ > > #define nsIOfflineStorage_h__ > > > > #include "nsIFileStorage.h" > > > > +#include "mozilla/dom/quota/ClientType.h" > > Can you forward-declare this instead? this is not needed anymore

Up to date rollup patch.

I just removed "class mozIStorageServiceQuotaManagement" from FileManager.h (which should have been removed earlier) and incorporated the fix for bug 836943.

Comment on attachment 721554 [details] [diff] [review] Rollup patch Review of attachment 721554 [details] [diff] [review]: ----------------------------------------------------------------- ::: content/base/src/nsDocument.cpp @@ +7618,5 @@ > } > > + // Check if we have running offline storage transactions > + quota::QuotaManager* quotaManager = quota::QuotaManager::Get(); > + if (quotaManager && quotaManager->HasOpenTransactions(win)) { It's not clear to me that this change is correct. IDB has some behavioral requirement here but do all other storage backends? It seems like this shouldn't be a QuotaManager function. ::: dom/base/nsGlobalWindow.cpp @@ +1344,5 @@ > > + // Close all offline storages for this window. > + quota::QuotaManager* quotaManager = quota::QuotaManager::Get(); > + if (quotaManager) { > + quotaManager->AbortCloseStoragesForWindow(this); Again, not sure about this. It makes sense for IDB but what about others (especially those without transactions and rollback)? ::: dom/file/nsIFileStorage.h @@ +19,5 @@ > { > public: > NS_DECLARE_STATIC_IID_ACCESSOR(NS_FILESTORAGE_IID) > > + NS_IMETHOD_(nsIAtom*) Hm, one of these days I bet we'll run some automatic s/NS_IMETHOD_(foo)/virtual foo/ replacement. Why are you making this change? ::: dom/indexedDB/Client.h @@ +21,5 @@ > + typedef mozilla::dom::quota::UsageRunnable UsageRunnable; > + > +public: > + NS_IMETHOD_(nsrefcnt) > + AddRef(); These need MOZ_OVERRIDE ::: dom/indexedDB/IDBDatabase.cpp @@ +205,2 @@ > > + db->mQuotaClient = quotaManager->GetClient(Client::IDB); Can this fail? If not you should assert it. @@ +219,5 @@ > +IDBDatabase* > +IDBDatabase::FromStorage(nsIOfflineStorage* aStorage) > +{ > + return aStorage->GetClient()->GetType() == Client::IDB ? > + static_cast<IDBDatabase*>(aStorage) : nullptr; I'm don't understand this. Why are we going to all the trouble of creating abstract interfaces for everything if there are cases where we still need to know the exact concrete type? ::: dom/quota/QuotaManager.cpp @@ +469,2 @@ > > + mClients.AppendElements(Client::TYPE_MAX); This looks fragile. What about calling SetCapacity here and then appending the ones you actually create. @@ +946,5 @@ > + > +already_AddRefed<mozilla::dom::quota::Client> > +QuotaManager::GetClient(Client::Type aClientType) > +{ > + nsRefPtr<Client> client = mClients[aClientType]; Since this is a direct array access from an externally visible function you should probably use SafeElementAt() here. ::: dom/quota/nsIOfflineStorage.h @@ +44,5 @@ > + NS_IMETHOD_(bool) > + IsClosed() = 0; > + > + NS_IMETHOD_(void) > + Invalidate() = 0; It would be good to document the difference between Close and Invalidate here.

try looks good https://tbpl.mozilla.org/?tree=Try&rev=41a67462740b

(In reply to ben turner [:bent] from comment #50) > Comment on attachment 721554 [details] [diff] [review] > Rollup patch > > Review of attachment 721554 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: content/base/src/nsDocument.cpp > @@ +7618,5 @@ > > } > > > > + // Check if we have running offline storage transactions > > + quota::QuotaManager* quotaManager = quota::QuotaManager::Get(); > > + if (quotaManager && quotaManager->HasOpenTransactions(win)) { > > It's not clear to me that this change is correct. IDB has some behavioral > requirement here but do all other storage backends? It seems like this > shouldn't be a QuotaManager function. > > ::: dom/base/nsGlobalWindow.cpp > @@ +1344,5 @@ > > > > + // Close all offline storages for this window. > > + quota::QuotaManager* quotaManager = quota::QuotaManager::Get(); > > + if (quotaManager) { > > + quotaManager->AbortCloseStoragesForWindow(this); > > Again, not sure about this. It makes sense for IDB but what about others > (especially those without transactions and rollback)? talked to bent on irc, this can be fixed later in a followup bug or as part of LocalStorage quota client implementation > > ::: dom/file/nsIFileStorage.h > @@ +19,5 @@ > > { > > public: > > NS_DECLARE_STATIC_IID_ACCESSOR(NS_FILESTORAGE_IID) > > > > + NS_IMETHOD_(nsIAtom*) > > Hm, one of these days I bet we'll run some automatic > s/NS_IMETHOD_(foo)/virtual foo/ replacement. Why are you making this change? the Close() method is declared in both interfaces, nsIOfflineStorage and nsIIDBDatabase, and IDBDatabase implements both So Close() needs to use NS_IMETHOD (I changed other methods too for consistency) > > ::: dom/indexedDB/Client.h > @@ +21,5 @@ > > + typedef mozilla::dom::quota::UsageRunnable UsageRunnable; > > + > > +public: > > + NS_IMETHOD_(nsrefcnt) > > + AddRef(); > > These need MOZ_OVERRIDE ok, added > > ::: dom/indexedDB/IDBDatabase.cpp > @@ +205,2 @@ > > > > + db->mQuotaClient = quotaManager->GetClient(Client::IDB); > > Can this fail? If not you should assert it. ok, added an assertion > > @@ +219,5 @@ > > +IDBDatabase* > > +IDBDatabase::FromStorage(nsIOfflineStorage* aStorage) > > +{ > > + return aStorage->GetClient()->GetType() == Client::IDB ? > > + static_cast<IDBDatabase*>(aStorage) : nullptr; > > I'm don't understand this. Why are we going to all the trouble of creating > abstract interfaces for everything if there are cases where we still need to > know the exact concrete type? Ok, so I now understand why we should remove this method (the need for such method means that the QuotaManager and Client API is not well designed. However we can fix it later as part of LocalStorage quota client, etc. > > ::: dom/quota/QuotaManager.cpp > @@ +469,2 @@ > > > > + mClients.AppendElements(Client::TYPE_MAX); > > This looks fragile. What about calling SetCapacity here and then appending > the ones you actually create. Well, the auto array should have the capacity set already. I added a new assertion for that. The rest is reworked as you suggested and it should be safer. > > @@ +946,5 @@ > > + > > +already_AddRefed<mozilla::dom::quota::Client> > > +QuotaManager::GetClient(Client::Type aClientType) > > +{ > > + nsRefPtr<Client> client = mClients[aClientType]; > > Since this is a direct array access from an externally visible function you > should probably use SafeElementAt() here. ok, fixed however, the compiler I'm using doesn't allow to pass random stuff to this function, but that's probably not true for all C++ compilers > > ::: dom/quota/nsIOfflineStorage.h > @@ +44,5 @@ > > + NS_IMETHOD_(bool) > > + IsClosed() = 0; > > + > > + NS_IMETHOD_(void) > > + Invalidate() = 0; > > It would be good to document the difference between Close and Invalidate > here. done

Comment on attachment 724366 [details] [diff] [review] Rollup patch Review of attachment 724366 [details] [diff] [review]: ----------------------------------------------------------------- I think this looks good! Thanks!

Additional fix https://hg.mozilla.org/integration/mozilla-inbound/rev/b7d5e48c5def

https://hg.mozilla.org/mozilla-central/rev/8d558e07caf4 https://hg.mozilla.org/mozilla-central/rev/b7d5e48c5def

Is there any high-level overview, documentation or spec wiki page describing how the manager works and what it does as well as how it can be interacted with (from backend as well as frontend)?

There's no such description yet, I will write one when the last bit lands, that's the temporary storage support, bug 785884. If you need some info immediately, just ping me directly or maybe in the dev-webapi mailing list.

In the interim, please add dev-doc-needed to the keywords. (I sent you an email on the 29th March, I hope it didn't get stuck in the SPAM checker …)

(In reply to Jan Varga [:janv] from comment #54) > > ::: dom/base/nsGlobalWindow.cpp > > @@ +1344,5 @@ > > > > > > + // Close all offline storages for this window. > > > + quota::QuotaManager* quotaManager = quota::QuotaManager::Get(); > > > + if (quotaManager) { > > > + quotaManager->AbortCloseStoragesForWindow(this); > > > > Again, not sure about this. It makes sense for IDB but what about others > > (especially those without transactions and rollback)? > > talked to bent on irc, this can be fixed later in a followup bug or as part > of LocalStorage quota client implementation filed bug 942542 for this

Jan, why is this stuff quickstubbed? Does it need to be?

(In reply to Boris Zbarsky [:bz] from comment #64) > Jan, why is this stuff quickstubbed? Does it need to be? Those few methods aren't called a lot, so it doesn't have to be quickstubbed. Feel free to remove them from quickstubs.