Closed Bug 932119 Opened 6 years ago Closed 6 years ago

convert nsIAtom to nsString in QuotaManager and IndexedDB

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: baku, Assigned: baku)

Details

(Whiteboard: [qa-])

Attachments

(1 file, 7 obsolete files)

This is needed for porting IDB to workers.
Attached patch nsIAtom.patch (obsolete) — Splinter Review
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)
Attached patch nsIAtom.patch (obsolete) — Splinter Review
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 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 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 ?
Attachment #824367 - Flags: review?(Jan.Varga)
Attachment #824367 - Flags: review?(Jan.Varga)
> 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...
(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 ?
Attached patch nsIAtom.patch (obsolete) — Splinter Review
Attachment #824367 - Attachment is obsolete: true
Attachment #8336168 - Flags: review?(Jan.Varga)
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;
> 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.
(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
Attached patch nsIAtom.patch (obsolete) — Splinter Review
Attachment #8336168 - Attachment is obsolete: true
Attachment #8336168 - Flags: review?(Jan.Varga)
Attachment #8336903 - Flags: review?(Jan.Varga)
Attached patch nsIAtom.patch (obsolete) — Splinter Review
Attachment #8336903 - Attachment is obsolete: true
Attachment #8336903 - Flags: review?(Jan.Varga)
Attachment #8336904 - Flags: review?(Jan.Varga)
Attached patch nsIAtom.patch (obsolete) — Splinter Review
Something better.
Attachment #8336904 - Attachment is obsolete: true
Attachment #8336904 - Flags: review?(Jan.Varga)
Attachment #8337680 - Flags: review?(Jan.Varga)
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+
Attached patch nsIAtom.patch (obsolete) — Splinter Review
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
(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.
Attached patch nsIAtom.patchSplinter Review
Attachment #8337713 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/6cd907704706
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.