1031407 - DataStoreService doesn't need to use IDBCursor to know if a revision exists.

Status: RESOLVED FIXED

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: new.patch

Comment 2

[Jan Varga [:janv]]

Comment on attachment 8447283 [details] [diff] [review]
new.patch

Review of attachment 8447283 [details] [diff] [review]:
-----------------------------------------------------------------

looks ok, but please send a new patch

::: dom/datastore/DataStoreDB.cpp
@@ +75,5 @@
>  NS_IMPL_ISUPPORTS(DataStoreDB, nsIDOMEventListener)
>  
>  DataStoreDB::DataStoreDB(const nsAString& aManifestURL, const nsAString& aName)
>    : mState(Inactive)
> +  , mJustCreatedDB(false)

I would call it mCreatedDB or mCreatedSchema

@@ +143,5 @@
>      mState = Inactive;
>  
>      rv = DatabaseOpened();
>      if (NS_WARN_IF(NS_FAILED(rv))) {
> +      mCallback->Run(this, false, mJustCreatedDB);

It doesn't make sense to pass mJustCreatedDB in the error case.
You could convert aSuccess in the callback to aResult and define a new enum for that:
eSuccess, // success, database already existed
eCreated, // success, created new database
eError

@@ +168,5 @@
>    MOZ_CRASH("This should not happen");
>  }
>  
>  nsresult
> +DataStoreDB::UpgradeSchema(nsIDOMEvent* aEvent)

Nit: aEvent is needed only in debug builds

@@ +180,5 @@
> +#ifdef DEBUG
> +  nsCOMPtr<IDBVersionChangeEvent> event = do_QueryInterface(aEvent);
> +  MOZ_ASSERT(event);
> +
> +  Nullable<uint64_t> version = event->GetNewVersion();

do you need to assert !version.IsNull too ?

::: dom/datastore/DataStoreService.cpp
@@ +510,5 @@
>    nsString mManifestURL;
>  };
>  
>  // This DataStoreDBCallback is called when DataStoreDB opens the DataStore DB.
>  // Then the first revision will be created if it doesn't exist yet.

maybe update to reflect the fact that you now don't check existence of it.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: new.patch

Comment 4

[Jan Varga [:janv]]

Comment on attachment 8466310 [details] [diff] [review]
new.patch

Review of attachment 8466310 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/datastore/DataStoreService.cpp
@@ +527,5 @@
>      MOZ_ASSERT(NS_IsMainThread());
>    }
>  
>    void
> +  Run(DataStoreDB* aDb, DataStoreDBCallback::RunStatus aStatus)

I think you don't need to prefix it with DataStoreDBCallback:: here nor below

Comment 5

[Andrea Marchesini [:baku]]

Attachment: new.patch

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

If it's green we can land this patch.

Comment 6

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/71dad5af5c74

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

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/71dad5af5c74