1495687 - Have a method in nsIQuotaManagerService to initialize temporary storage

Status: RESOLVED FIXED

(Core :: Storage: Quota Manager, enhancement, P2)

Description

[Tom Tung [:tt, :ttung]]

Add a new method to nsIQuotaManagerService called initTemporaryStorage() per bug 389378 comment#c24

Comment 1

[Tom Tung [:tt, :ttung]]

(In reply to Tom Tung [:tt] from comment #0)
> Add a new method to nsIQuotaManagerService called initTemporaryStorage() per
> bug 389378 comment#c24
bug 1389378 comment#24

Comment 2

[Tom Tung [:tt, :ttung]]

Attachment: bug1495678.patch

Comment 3

[Jan Varga [:janv]]

Comment on attachment 9013611 [details] [diff] [review]
bug1495678.patch

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

::: dom/quota/ActorsParent.cpp
@@ +7437,5 @@
> +{
> +  AssertIsOnIOThread();
> +
> +  AUTO_PROFILER_LABEL("InitTemporaryStorageOp::DoDirectoryWork", OTHER);
> +

I would also call this:
aQuotaManager->AssertStorageIsInitialized();

::: dom/quota/test/unit/test_initTemporaryStorage.js
@@ +3,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +async function testSteps()
> +{

I have to think about this test a bit.

Comment 4

[Tom Tung [:tt, :ttung]]

Attachment: bug1495687-v2.patch

Addressed feedback and modifying comments in the test.

Probably need to modifying the test as well. I guess it probably needs to be verified the usage and limits are checked if it's possible or some behaviors in InitializeRepository()?

Comment 5

[Jan Varga [:janv]]

Comment on attachment 9013692 [details] [diff] [review]
bug1495687-v2.patch

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

::: dom/quota/ActorsParent.cpp
@@ +7440,5 @@
> +  AUTO_PROFILER_LABEL("InitTemporaryStorageOp::DoDirectoryWork", OTHER);
> +
> +  aQuotaManager->AssertStorageIsInitialized();
> +
> +  aQuotaManager->EnsureTemporaryStorageIsInitialized();

The rv should be checked:
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}

::: dom/quota/test/unit/test_initTemporaryStorage.js
@@ +5,5 @@
> +
> +async function testSteps()
> +{
> +  const principal = getPrincipal("https://foo.example.com");
> +  const metadataDir = "storage/default/https+++foo.example.com/.metadata-v2";

What about this:
const originDirPath = "storage/default/https+++foo.example.com";
const metadataFileName = ".metadata-v2";

then after the clearing, just create the origin directory:
let originDir = getRelativeFile(originPath);
originDir.create(Ci.nsIFile.DIRECTORY_TYPE, parseInt("0755", 8));

now call initTemporaryStorage() and check the metadata:
let metadataFile = originDir.clone();
metadataFile.append(metadataFileName);
exists = metadataFile.exists();
...

Comment 6

[Tom Tung [:tt, :ttung]]

(In reply to Jan Varga [:janv] from comment #5)
Thanks for the feedback!

> > +  aQuotaManager->EnsureTemporaryStorageIsInitialized();
> 
> The rv should be checked:
> if (NS_WARN_IF(NS_FAILED(rv))) {
>   return rv;
> }

Ah, I forgot that. Will do.

> ::: dom/quota/test/unit/test_initTemporaryStorage.js
> @@ +5,5 @@
> > +
> > +async function testSteps()
> > +{
> > +  const principal = getPrincipal("https://foo.example.com");
> > +  const metadataDir = "storage/default/https+++foo.example.com/.metadata-v2";
> 
> What about this:
> const originDirPath = "storage/default/https+++foo.example.com";
> const metadataFileName = ".metadata-v2";
> 
> then after the clearing, just create the origin directory:
> let originDir = getRelativeFile(originPath);
> originDir.create(Ci.nsIFile.DIRECTORY_TYPE, parseInt("0755", 8));
> 
> now call initTemporaryStorage() and check the metadata:
> let metadataFile = originDir.clone();
> metadataFile.append(metadataFileName);
> exists = metadataFile.exists();
> ...

I see. So that it's much faster (reducing IPC round-trip time) and I can remove the quirky code for creating an empty directory.

Comment 7

[Jan Varga [:janv]]

Comment on attachment 9013692 [details] [diff] [review]
bug1495687-v2.patch

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

::: dom/quota/test/unit/test_initTemporaryStorage.js
@@ +9,5 @@
> +  const metadataDir = "storage/default/https+++foo.example.com/.metadata-v2";
> +
> +  info("Clearing");
> +
> +  let request = clear();

Actually, the clearing is not needed here I think.

Comment 8

[Tom Tung [:tt, :ttung]]

Attachment: bug1495687-v3.patch

Addressed comments and revised the test

Comment 9

[Tom Tung [:tt, :ttung]]

Attachment: bug1495687-v4.patch

Modified comments in the test

Comment 10

[Tom Tung [:tt, :ttung]]

Attachment: bug1495687-v4.1.patch

Added an empty line back for the test

Comment 11

[Jan Varga [:janv]]

Comment on attachment 9014309 [details] [diff] [review]
bug1495687-v4.1.patch

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

Nice, thanks!

::: dom/quota/test/unit/test_initTemporaryStorage.js
@@ +4,5 @@
> + */
> +
> +/**
> + * This test is mainly to verify initTemporaryStorage() does call
> + * QuotaManager::EnsureTemporaryStorageIsInitialized() which restore the

s/restore/restores

Actually, we might want to test other aspects of EnsureTemporaryStorageIsInitialized() in future, so I would rephrase this as:
This test is mainly to verify initTemporaryStorage() does call QuotaManager::EnsureTemporaryStorageIsInitialized() which does various things, for example, it restores the directory metadata if it's broken or missing.

Comment 12

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1495687 - Introduce initTemporaryStorage to nsIQuotaManagerService for testing; r=janv

Thanks for the reviews!

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5a4d9cdfad1c9112dd148e71bbb806a45a216bcc

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75a2cc138641
Introduce initTemporaryStorage to nsIQuotaManagerService for testing; r=janv

Comment 14

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/75a2cc138641