The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla10

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

unspecified
mozilla10
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Comment hidden (empty)
Created attachment 570268 [details] [diff] [review]
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.
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #570268 - Flags: review?(mak77)
Attachment #570268 - Flags: feedback?(honzab.moz)
https://tbpl.mozilla.org/?tree=Try&rev=82894fac2c89
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-
Created attachment 570726 [details] [diff] [review]
Use domstorage-flush-force

https://tbpl.mozilla.org/?tree=Try&rev=c66a1510f18e
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

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

I will push it if it is green.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1e0a5c30b54
https://hg.mozilla.org/mozilla-central/rev/d1e0a5c30b54
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
You need to log in before you can comment on or make changes to this bug.