Closed
Bug 932119
Opened 11 years ago
Closed 11 years ago
convert nsIAtom to nsString in QuotaManager and IndexedDB
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: baku, Assigned: baku)
Details
(Whiteboard: [qa-])
Attachments
(1 file, 7 obsolete files)
43.76 KB,
patch
|
Details | Diff | Splinter Review |
This is needed for porting IDB to workers.
Assignee | ||
Comment 1•11 years ago
|
||
I don't know if bent or janv should review this patch...
Attachment #823704 -
Flags: review?(bent.mozilla)
Comment on attachment 823704 [details] [diff] [review] nsIAtom.patch Review of attachment 823704 [details] [diff] [review]: ----------------------------------------------------------------- I think Jan is a better reviewer for this one... Jan, let me know if you'd rather me do it?
Attachment #823704 -
Flags: review?(bent.mozilla) → review?(Jan.Varga)
Assignee | ||
Comment 3•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=da1db69fa5da
Attachment #823704 -
Attachment is obsolete: true
Attachment #823704 -
Flags: review?(Jan.Varga)
Attachment #824367 -
Flags: review?(Jan.Varga)
Comment 4•11 years ago
|
||
Comment on attachment 824367 [details] [diff] [review] nsIAtom.patch Review of attachment 824367 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/file/nsIFileStorage.h @@ +18,5 @@ > public: > NS_DECLARE_STATIC_IID_ACCESSOR(NS_FILESTORAGE_IID) > > + NS_IMETHOD_(void) > + Id(nsAString& aId) = 0; Hmm, it would be nicer to have: NS_IMETHOD(const nsAString& aId) Id() = 0;
Comment 5•11 years ago
|
||
Comment on attachment 824367 [details] [diff] [review] nsIAtom.patch Review of attachment 824367 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/file/nsIFileStorage.h @@ +18,5 @@ > public: > NS_DECLARE_STATIC_IID_ACCESSOR(NS_FILESTORAGE_IID) > > + NS_IMETHOD_(void) > + Id(nsAString& aId) = 0; And actually, why does this need to be an nsAString in the first place ? Can it be an nsACString ?
Updated•11 years ago
|
Attachment #824367 -
Flags: review?(Jan.Varga)
Updated•11 years ago
|
Attachment #824367 -
Flags: review?(Jan.Varga)
Assignee | ||
Comment 6•11 years ago
|
||
> And actually, why does this need to be an nsAString in the first place ? Can
> it be an nsACString ?
Because we use the same hashtable for IDs and Names.
typedef nsDataHashtable<nsISupportsHashKey, DatabaseInfo*>
DatabaseHash;
has been replaced with nsStringHashKey that doesn't support nsCString. We can change name too...
Comment 7•11 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #6) > > And actually, why does this need to be an nsAString in the first place ? Can > > it be an nsACString ? > > Because we use the same hashtable for IDs and Names. > > typedef nsDataHashtable<nsISupportsHashKey, DatabaseInfo*> > DatabaseHash; > > has been replaced with nsStringHashKey that doesn't support nsCString. We > can change name too... doesn't support nsCString ? I didn't try, but there's nsCStringHashKey, no ?
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #824367 -
Attachment is obsolete: true
Attachment #8336168 -
Flags: review?(Jan.Varga)
Comment 9•11 years ago
|
||
Comment on attachment 8336168 [details] [diff] [review] nsIAtom.patch Review of attachment 8336168 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/file/nsIFileStorage.h @@ +18,5 @@ > public: > NS_DECLARE_STATIC_IID_ACCESSOR(NS_FILESTORAGE_IID) > > + NS_IMETHOD_(void) > + Id(nsACString& aId) = 0; Is there a problem with returning a string reference ? NS_IMETHOD(const nsACString& aId) Id() = 0;
Assignee | ||
Comment 10•11 years ago
|
||
> Is there a problem with returning a string reference ?
>
> NS_IMETHOD(const nsACString& aId)
> Id() = 0;
If I remember correctly this was suggested by bent. I can revert it.
Comment 11•11 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #10) > > Is there a problem with returning a string reference ? > > > > NS_IMETHOD(const nsACString& aId) > > Id() = 0; > > If I remember correctly this was suggested by bent. I can revert it. Ok, I talked to Ben, his point is that we should follow the xpidl pattern for virtual methods. He's right, but that leads to changes like this: bool FileService::HasLockedFilesForStorage(nsIFileStorage* aFileStorage) { NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); NS_ASSERTION(aFileStorage, "Null pointer!"); + nsCString id; + aFileStorage->Id(id); + FileStorageInfo* fileStorageInfo; - if (!mFileStorageInfos.Get(aFileStorage->Id(), &fileStorageInfo)) { + if (!mFileStorageInfos.Get(id, &fileStorageInfo)) { return false; } which I don't like since we need to use stack vars every time :( It seems that we actually don't need xpidl interfaces here (the same applies to nsIOfflineStorage), but that's out of scope of this bug. So let's use what I proposed and just convert the interfaces to non-xpidl ones in a followup. You can file the bug against me. Ben says "sounds fine". Thanks
Assignee | ||
Comment 12•11 years ago
|
||
Attachment #8336168 -
Attachment is obsolete: true
Attachment #8336168 -
Flags: review?(Jan.Varga)
Attachment #8336903 -
Flags: review?(Jan.Varga)
Assignee | ||
Comment 13•11 years ago
|
||
Attachment #8336903 -
Attachment is obsolete: true
Attachment #8336903 -
Flags: review?(Jan.Varga)
Attachment #8336904 -
Flags: review?(Jan.Varga)
Assignee | ||
Comment 14•11 years ago
|
||
Something better.
Attachment #8336904 -
Attachment is obsolete: true
Attachment #8336904 -
Flags: review?(Jan.Varga)
Attachment #8337680 -
Flags: review?(Jan.Varga)
Comment 15•11 years ago
|
||
Comment on attachment 8337680 [details] [diff] [review] nsIAtom.patch Review of attachment 8337680 [details] [diff] [review]: ----------------------------------------------------------------- Touching SynchronizedOp is always scary, but I think the changes are ok ::: dom/indexedDB/IDBFactory.cpp @@ +648,5 @@ > } > else if (aDeleting) { > + nsCString databaseId; > + QuotaManager::GetStorageId(aPersistenceType, aASCIIOrigin, aName, > + databaseId); hm, this can't fail anymore, you can replace the NS_ENSURE_TRUE with MOZ_ASSERT ::: dom/indexedDB/OpenDatabaseHelper.cpp @@ +1691,5 @@ > OpenDatabaseHelper::Init() > { > + QuotaManager::GetStorageId(mPersistenceType, mASCIIOrigin, mName, > + mDatabaseId); > + NS_ENSURE_TRUE(!mDatabaseId.IsEmpty(), NS_ERROR_FAILURE); dtto ::: dom/indexedDB/ipc/IndexedDBChild.cpp @@ +293,2 @@ > } > + NS_ENSURE_TRUE(!databaseId.IsEmpty(), false); dtto ::: dom/quota/QuotaManager.cpp @@ +1628,5 @@ > } > > // If one or the other is for an origin clear, we should have matched > // solely on origin. > + NS_ASSERTION(!op->mId.IsEmpty() && !aId.IsEmpty(), "Why didn't we match earlier?"); Nit: 80 char limit @@ +3288,5 @@ > return true; > } > > // Waiting is required if either one corresponds to an origin clearing > // (a null Id). Nit: s/null/empty
Attachment #8337680 -
Flags: review?(Jan.Varga) → review+
Assignee | ||
Comment 16•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=09b6df0ac0a3 do you want to land it if fully green on try?
Attachment #8337680 -
Attachment is obsolete: true
Comment 17•11 years ago
|
||
(In reply to Andrea Marchesini (:baku) from comment #16) > Created attachment 8337713 [details] [diff] [review] > nsIAtom.patch > > https://tbpl.mozilla.org/?tree=Try&rev=09b6df0ac0a3 > > do you want to land it if fully green on try? Yes, please.
Assignee | ||
Comment 18•11 years ago
|
||
Attachment #8337713 -
Attachment is obsolete: true
Assignee | ||
Comment 19•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cd907704706
Comment 20•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6cd907704706
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
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
•