Last Comment Bug 697989 - don't use domstorage-flush-timer in dom/tests/mochitest/localstorage/test_bug624047.html
: don't use domstorage-flush-timer in dom/tests/mochitest/localstorage/test_bug...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: x86_64 Linux
: -- normal (vote)
: mozilla10
Assigned To: Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
:
Mentors:
Depends on:
Blocks: 696399 697988
  Show dependency treegraph
 
Reported: 2011-10-28 08:14 PDT by Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
Modified: 2011-11-05 03:09 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
use domstorage-flush-timer instead of profile-before-change (1.08 KB, patch)
2011-10-28 08:22 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
mak77: review-
Details | Diff | Review
Use domstorage-flush-force (4.65 KB, patch)
2011-10-31 08:55 PDT, Rafael Ávila de Espíndola (:espindola) (not reading bugmail)
mak77: review+
honzab.moz: feedback+
Details | Diff | Review

Description Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-10-28 08:14:02 PDT

    
Comment 1 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-10-28 08:22:50 PDT
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.
Comment 2 Rafael Ávila de Espíndola (:espindola) (not reading bugmail) 2011-10-28 08:23:10 PDT
https://tbpl.mozilla.org/?tree=Try&rev=82894fac2c89
Comment 3 Marco Bonardo [::mak] 2011-10-28 14:35:18 PDT
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) 2011-10-31 08:55:02 PDT
Created attachment 570726 [details] [diff] [review]
Use domstorage-flush-force

https://tbpl.mozilla.org/?tree=Try&rev=c66a1510f18e
Comment 5 Honza Bambas (:mayhemer) 2011-10-31 11:23:23 PDT
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) 2011-10-31 11:24:58 PDT
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) 2011-10-31 11:30:15 PDT
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) 2011-10-31 11:42:44 PDT
Cool. Thanks!

I can give an unofficial r+ to this bug's patch.
Comment 9 Marco Bonardo [::mak] 2011-11-03 07:19:08 PDT
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) 2011-11-04 11:09:18 PDT
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) 2011-11-04 15:28:41 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1e0a5c30b54
Comment 12 Marco Bonardo [::mak] 2011-11-05 03:09:21 PDT
https://hg.mozilla.org/mozilla-central/rev/d1e0a5c30b54

Note You need to log in before you can comment on or make changes to this bug.