Tom Tung [:tt, :ttung] Assignee
	Description • 7 years ago

We decide to move the test in the bug 1372116 to this bug in the storage meeting since this shouldn't block the Storage API v1. 

Note: This test is written via SimpleDB API.

	Marion Daly [:mdaly]
	Comment 2 • 7 years ago

Janv, can you take a look at this?

	Jan Varga [:janv]
	Comment 3 • 7 years ago

Yes.

	Tom Tung [:tt, :ttung] Assignee
	Comment 4 • 6 years ago

Rewriting this test, and I'm considering to move this patch to bug 1389390 since they both use simpleDB and might use the same function

	Tom Tung [:tt, :ttung] Assignee
	Comment 5 • 6 years ago

I changed my mind and decided to keep the patch here because the test is much simpler than I expected.

Note that setTemporaryLimit() is assumed to be added to dom/quota/test/unit/head.js in bug 1389390.

	Tom Tung [:tt, :ttung] Assignee
	Comment 6 • 6 years ago

Comment on attachment 9010560 [details] [diff] [review]
Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

Jan, this patch is a test for verifying persisted origins are bounded by the global limit.

	Jan Varga [:janv]
	Comment 7 • 6 years ago

Yeah, we also another test - test_persist_groupLimit.js, I already created it locally, will file a bug for that.

	Jan Varga [:janv]
	Comment 8 • 6 years ago

s/we also another test/we also need another test

	Tom Tung [:tt, :ttung] Assignee
	Comment 9 • 6 years ago

Comment on attachment 9010560 [details] [diff] [review]
Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

Drop the review request for now. I'll revise it based on the patch in bug 1493908 and then send a review request again.

	Tom Tung [:tt, :ttung] Assignee
	Comment 10 • 6 years ago

Rebase and rename the patch

	Jan Varga [:janv]
	Comment 12 • 6 years ago

Ok, bug 1493908 is now fixed, can you re-base/re-style the patch again ? Thanks.

	Tom Tung [:tt, :ttung] Assignee
	Comment 13 • 6 years ago

(In reply to Jan Varga [:janv] from comment #12)
> Ok, bug 1493908 is now fixed, can you re-base/re-style the patch again?
> Thanks.

Sure, thanks!

	Tom Tung [:tt, :ttung] Assignee
	Comment 14 • 6 years ago

Revised the patch

	Jan Varga [:janv]
	Comment 17 • 6 years ago

Comment on attachment 9013246 [details] [diff] [review]
Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

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

Looks mostly ok, but it doesn't test the original issue reported in bug 1372116.
I attached a patch that demonstrates it, you probably need to test both situations (quota initialized and quota not initialized).

	Jan Varga [:janv]
	Comment 18 • 6 years ago

Comment on attachment 8896122 [details] [diff] [review]
Bug 1389378 - P1: Test to verify persist() do create originInfo when the temporary storage has been initialized.

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

::: dom/quota/test/unit/test_persist_trackQuota.js
@@ +17,5 @@
> +  clear(continueToNextStepSync);
> +  yield undefined;
> +
> +  info("Initialize temporary repository by initOrigin(): " + url + ".");
> +  initOrigin(getPrincipal(url), "temporary", continueToNextStepSync);

Actually, here in your old patch you do test the original issue reported in bug 1372116.

@@ +34,5 @@
> +  request = connection.open();
> +  request.callback = continueToNextStepSync;
> +  yield undefined;
> +
> +  request = connection.write(new ArrayBuffer(globalLimitKB * 1024 + 1));

Here you have just one write request, I think I like it more than writing globalLimitKB * 1024 bytes and then 1 byte

	Tom Tung [:tt, :ttung] Assignee
	Comment 19 • 6 years ago

Thanks for catching that! I somehow removed it when rewriting the test

	Tom Tung [:tt, :ttung] Assignee
	Comment 22 • 6 years ago

Revised the patch.

	Jan Varga [:janv]
	Comment 24 • 6 years ago

I think we should add a new method to nsIQuotaManagerService called initTemporaryStorage(), then we don't have to do the hack in the test to call initOrigin() for foo.com
Tom, can you do it in a separate patch ? (maybe file a new bug for it and make it block this bug)
Thanks.
I'll comment about "suggestions" later today.

	Jan Varga [:janv]
	Comment 25 • 6 years ago

Comment on attachment 9013524 [details] [diff] [review]
Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

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

::: dom/quota/test/unit/test_persist_globalLimit.js
@@ +11,5 @@
> +
> +  let request = clear();
> +  await requestFinished(request);
> +
> +  info("Persisting an origin.");

This info message doesn't belong here.

Nit: other tests omit the comman

@@ +14,5 @@
> +
> +  info("Persisting an origin.");
> +
> +  const spec = "https://persisted.example.com";
> +  const principal = getPrincipal(spec);

I prefere to declare urls, specs, stuff like this in the beginning of testSteps().
We can also just do:
const principal = getPrincipal("https://persisted.example.com");

@@ +16,5 @@
> +
> +  const spec = "https://persisted.example.com";
> +  const principal = getPrincipal(spec);
> +
> +  for (let initializeStorageBeforePersist of [true, false]) {

I think it would be more natural to test without temporary storage initialization first.

@@ +20,5 @@
> +  for (let initializeStorageBeforePersist of [true, false]) {
> +    if (initializeStorageBeforePersist) {
> +      info("Initializing an origin to initialize the temporary storage.");
> +
> +      request = initOrigin(principal, "temporary");

I guess, you use "temporary" to avoid initializing the origin that is going to be persisted.
Well, I think we should either have a dummy origin for this here and use "default" or just implement initTemporaryStorage in the QM service.

@@ +29,5 @@
> +    await requestFinished(request);
> +
> +    info("Verifying the persisted origin is bounded by global limit.");
> +
> +    let database = getSimpleDatabase(principal);

I try to keep:
"
requst = ...
await requestFinished(request)
"
in separate blocks, so I would put a blank line here

@@ +33,5 @@
> +    let database = getSimpleDatabase(principal);
> +    request = database.open("data");
> +    await requestFinished(request);
> +
> +    let buffer = getBuffer(globalLimitKB * 1024 + 1);

Nit: buffer is use only once, so you can do:
request = database.write(getBuffer(globalLimitKB * 1024 + 1));

@@ +41,5 @@
> +      ok(false, "Should have thrown");
> +    } catch (ex) {
> +      ok(true, "Should have thrown");
> +      ok(ex == NS_ERROR_FILE_NO_DEVICE_SPACE,
> +         "Persisted origin was bounded by the global limit");

I would rather have "Threw right code" here, and add another info() before the write() call.

	Tom Tung [:tt, :ttung] Assignee
	Comment 26 • 6 years ago

(In reply to Jan Varga [:janv] from comment #25)

Thanks for the review!

> > +  info("Persisting an origin.");
> 
> This info message doesn't belong here.
> 
> Nit: other tests omit the comman

Will move that to the right place and remove commas for all the info message here.
 
> > +  const spec = "https://persisted.example.com";
> > +  const principal = getPrincipal(spec);
> 
> I prefere to declare urls, specs, stuff like this in the beginning of
> testSteps().
> We can also just do:
> const principal = getPrincipal("https://persisted.example.com");

I see. I put them just in front of the place calling it. Will correct that here and the future. 
 
> > +  for (let initializeStorageBeforePersist of [true, false]) {
> 
> I think it would be more natural to test without temporary storage
> initialization first.

Will do

> > +      request = initOrigin(principal, "temporary");
> 
> I guess, you use "temporary" to avoid initializing the origin that is going
> to be persisted.
> Well, I think we should either have a dummy origin for this here and use
> "default" or just implement initTemporaryStorage in the QM service.

Sure, will complete the bug 1495687 first so that it will have initTemporaryStorage() to call.

> > +    let database = getSimpleDatabase(principal);
> 
> I try to keep:
> "
> requst = ...
> await requestFinished(request)
> "
> in separate blocks, so I would put a blank line here

I see and will do.

> > +    let buffer = getBuffer(globalLimitKB * 1024 + 1);
> 
> Nit: buffer is use only once, so you can do:
> request = database.write(getBuffer(globalLimitKB * 1024 + 1));

Will do.

> @@ +41,5 @@
> > +      ok(false, "Should have thrown");
> > +    } catch (ex) {
> > +      ok(true, "Should have thrown");
> > +      ok(ex == NS_ERROR_FILE_NO_DEVICE_SPACE,
> > +         "Persisted origin was bounded by the global limit");
> 
> I would rather have "Threw right code" here, and add another info() before
> the write() call.

Got it and will correct it and add another info before calling write().

	Tom Tung [:tt, :ttung] Assignee
	Comment 27 • 6 years ago

Addressed comment and checked whether the patch aligns with the patch of suggestions. Will check again before sending it to review.

	Jan Varga [:janv]
	Comment 28 • 6 years ago

Comment on attachment 9013641 [details] [diff] [review]
Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

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

Nice!

::: dom/quota/test/unit/test_persist_globalLimit.js
@@ +12,5 @@
> +  info("Setting limits");
> +
> +  setGlobalLimit(globalLimitKB);
> +
> +  info("Clearing");

Nit: the clear() call here is part of "Setting limits", so this extra info("Clearing") could be removed.
It would also match the final "Resetting limits"

	Tom Tung [:tt, :ttung] Assignee
	Comment 30 • 6 years ago

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

	Jan Varga [:janv]
	Comment 31 • 6 years ago

Comment on attachment 9013677 [details] [diff] [review]
Bug 1389378 - A test for verifying persisted origins bounded by the global limit;

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

::: dom/quota/test/unit/test_persist_globalLimit.js
@@ +1,5 @@
> +/**
> + * Any copyright is dedicated to the Public Domain.
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +

You can also add this comment here (just before |async function testSteps()|):
/**
 * This test is mainly to verify that persisted origins are always bounded by
 * the global limit.
 */

	Tom Tung [:tt, :ttung] Assignee
	Comment 32 • 6 years ago

Thanks for notifying! Addressed comment

	Tom Tung [:tt, :ttung] Assignee
	Comment 33 • 6 years ago

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=070c8e4a29036512f1d983f6f73a7c06c93cb72b

	Pulsebot
	Comment 34 • 6 years ago

Pushed by shes050117@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4f495d81e201
A test for verifying persisted origins bounded by the global limit; r=janv

	Natalia Csoregi [:nataliaCs]
	Comment 35 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/4f495d81e201