932119 - convert nsIAtom to nsString in QuotaManager and IndexedDB

Status: RESOLVED FIXED

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

Description

[Andrea Marchesini [:baku]]

This is needed for porting IDB to workers.

Comment 1

[Andrea Marchesini [:baku]]

Attachment: nsIAtom.patch

I don't know if bent or janv should review this patch...

Comment 2

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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?

Comment 3

[Andrea Marchesini [:baku]]

Attachment: nsIAtom.patch

https://tbpl.mozilla.org/?tree=Try&rev=da1db69fa5da

Comment 4

[Jan Varga [:janv]]

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

[Jan Varga [:janv]]

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 ?

Comment 6

[Andrea Marchesini [:baku]]

> 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

[Jan Varga [:janv]]

(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 ?

Comment 8

[Andrea Marchesini [:baku]]

Attachment: nsIAtom.patch

Comment 9

[Jan Varga [:janv]]

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;

Comment 10

[Andrea Marchesini [:baku]]

> 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

[Jan Varga [:janv]]

(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

Comment 12

[Andrea Marchesini [:baku]]

Attachment: nsIAtom.patch

Comment 13

[Andrea Marchesini [:baku]]

Attachment: nsIAtom.patch

Comment 14

[Andrea Marchesini [:baku]]

Attachment: nsIAtom.patch

Something better.

Comment 15

[Jan Varga [:janv]]

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

Comment 16

[Andrea Marchesini [:baku]]

Attachment: nsIAtom.patch

https://tbpl.mozilla.org/?tree=Try&rev=09b6df0ac0a3

do you want to land it if fully green on try?

Comment 17

[Jan Varga [:janv]]

(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.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: nsIAtom.patch

Comment 19

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6cd907704706

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/6cd907704706