697989 - don't use domstorage-flush-timer in dom/tests/mochitest/localstorage/test_bug624047.html

Status: RESOLVED FIXED

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

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: use domstorage-flush-timer instead of profile-before-change

We want to do more work on profile-before-change (finish statements and close connections), so move this test to a message that currently does the same but will be unaffected by future work.

Comment 2

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

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

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 570268 [details] [diff] [review]
use domstorage-flush-timer instead of profile-before-change

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

Honza said this test is explicitly about the true param passed to FlushAndDeleteTemporaryTables(true), domstorage-flush-timer notification instead passes false.

He suggested to add a new domstorage-flush-force notification, timilar to domstorage-flush-timer but that passes the true argument

Comment 4

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Attachment: Use domstorage-flush-force

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

Comment 5

[Honza Bambas (:mayhemer)]

What bugs are this one dep on/block it if any?  Not sure how removal of the new topic observer gets handled here and in what order you want to land the patches that fix it.

Patch looks good to me otherwise.  I'll r+ it when dependencies get properly set or explained.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 570726 [details] [diff] [review]
Use domstorage-flush-force

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

f+, but see comment 5.

Comment 7

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

This was part of 697989, so it blocks it. The observer will be remove by using weak references (bug 697988). That could go in any order, but I think we should probably push this one first, so I am also adding 697988 as blocked by this.

Comment 8

[Honza Bambas (:mayhemer)]

Cool. Thanks!

I can give an unofficial r+ to this bug's patch.

Comment 9

[Marco Bonardo [:mak]]

Comment on attachment 570726 [details] [diff] [review]
Use domstorage-flush-force

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

::: dom/src/storage/nsDOMStorage.cpp
@@ +74,5 @@
>  #include "mozilla/Preferences.h"
>  #include "nsThreadUtils.h"
>  
> +#define NS_DOMSTORAGE_FLUSH_TIMER_OBSERVER "domstorage-flush-timer"
> +#define NS_DOMSTORAGE_FLUSH_FORCE_OBSERVER "domstorage-flush-force"

A comment above these, explaining the difference between the two, may be useful.

nit: These are suffixed _OBSERVER, but really should be _TOPIC... btw...

@@ +293,5 @@
>    // Used for temporary table flushing
>    os->AddObserver(gStorageManager, "profile-before-change", false);
>    os->AddObserver(gStorageManager, NS_XPCOM_SHUTDOWN_OBSERVER_ID, false);
>    os->AddObserver(gStorageManager, NS_DOMSTORAGE_FLUSH_TIMER_OBSERVER, false);
> +  os->AddObserver(gStorageManager, NS_DOMSTORAGE_FLUSH_FORCE_OBSERVER, false);

Ideally this should not be needed, if the only use is for testing. Indeed you directly call into the Observe() method.
But if Honza thinks it may be useful in future, fine by me.

Comment 10

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

A try with the requested changes is at

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

I will push it if it is green.

Comment 11

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d1e0a5c30b54

Comment 12

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/d1e0a5c30b54