Closed Bug 697989 Opened 12 years ago Closed 12 years ago

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


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

Not set





(Reporter: espindola, Assigned: espindola)




(1 file, 1 obsolete file)

      No description provided.
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.
Assignee: nobody → respindola
Attachment #570268 - Flags: review?(mak77)
Attachment #570268 - Flags: feedback?(honzab.moz)
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
Attachment #570268 - Flags: review?(mak77) → review-
Attachment #570268 - Attachment is obsolete: true
Attachment #570268 - Flags: feedback?(honzab.moz)
Attachment #570726 - Flags: superreview?(honzab.moz)
Attachment #570726 - Flags: review?(mak77)
Attachment #570726 - Flags: superreview?(honzab.moz) → feedback?(honzab.moz)
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 on attachment 570726 [details] [diff] [review]
Use domstorage-flush-force

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

f+, but see comment 5.
Attachment #570726 - Flags: feedback?(honzab.moz) → feedback+
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.
Blocks: 697988, 696399
Cool. Thanks!

I can give an unofficial r+ to this bug's patch.
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.
Attachment #570726 - Flags: review?(mak77) → review+
A try with the requested changes is at

I will push it if it is green.
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.