698738 - Use the observer method for idle-daily and formhistory-expire-now in nsFormHistory.js

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Comment 1

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

Attachment: Use the observer method for idle-daily and formhistory-expire-now in nsFormHistory.js

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

Comment 2

[Marco Bonardo [:mak]]

Comment on attachment 570988 [details] [diff] [review]
Use the observer method for idle-daily and formhistory-expire-now in nsFormHistory.js

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

::: toolkit/components/satchel/nsFormHistory.js
@@ +394,2 @@
>          else
>              this.log("Oops! Unexpected notification: " + topic);

may you please convert this into a switch clause?

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 570988 [details] [diff] [review]
Use the observer method for idle-daily and formhistory-expire-now in nsFormHistory.js

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

::: toolkit/components/satchel/nsFormHistory.js
@@ +139,5 @@
>          this.messageManager.addMessageListener("FormHistory:FormSubmitEntries", this);
>  
>          // Add observers
> +        Services.obs.addObserver(this, "idle-daily", false);
> +        Services.obs.addObserver(this, "formhistory-expire-now", false);

You may use 'true' (add as a weak ref here) IMO, but let reviewer decide.

@@ +394,2 @@
>          else
>              this.log("Oops! Unexpected notification: " + topic);

+1

Comment 4

[Honza Bambas (:mayhemer)]

> You may use 'true' (add as a weak ref here) IMO, but let reviewer decide.

nsFormHistory is used as a service [@mozilla.org/satchel/form-history;1], to support my suggestion.

Comment 5

[Marco Bonardo [:mak]]

(In reply to Honza Bambas (:mayhemer) from comment #3)

> >          // Add observers
> > +        Services.obs.addObserver(this, "idle-daily", false);
> > +        Services.obs.addObserver(this, "formhistory-expire-now", false);
> 
> You may use 'true' (add as a weak ref here) IMO, but let reviewer decide.

fwiw, this is happening in another bug (bug 698570 exactly). It's hard to follow on all of these bugs without correct dependencies.

Comment 6

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

Attachment: Use the observer method for idle-daily and formhistory-expire-now in nsFormHistory.js

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

Comment 7

[Marco Bonardo [:mak]]

Comment on attachment 572029 [details] [diff] [review]
Use the observer method for idle-daily and formhistory-expire-now in nsFormHistory.js

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

r=me with these fixed.
The weak references will be handled in the blocked bug.

::: toolkit/components/satchel/nsFormHistory.js
@@ +400,5 @@
>              this.updatePrefs();
> +            break;
> +        case "idle-daily":
> +        case "formhistory-expire-now":
> +            this.expireOldEntries();

missing break here

@@ +406,1 @@
>              this.log("Oops! Unexpected notification: " + topic);

nit: I know it's useless, but I like to have a break in default too, mostly because default doesn't have to be the last option in a switch, so if anyone moves it around, he may miss the break.

Comment 8

[Honza Bambas (:mayhemer)]

Comment on attachment 572029 [details] [diff] [review]
Use the observer method for idle-daily and formhistory-expire-now in nsFormHistory.js

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

f+ and supporting mak's comments.

Comment 9

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

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=3424159bce7a

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/3424159bce7a