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

RESOLVED FIXED in mozilla10

Status

()

Toolkit
Form Manager
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: espindola, Assigned: espindola)

Tracking

Trunk
mozilla10
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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

https://tbpl.mozilla.org/?tree=Try&rev=e299b84494cf
Assignee: nobody → respindola
Status: NEW → ASSIGNED
Attachment #570988 - Flags: review?(mak77)
Attachment #570988 - Flags: feedback?(honzab.moz)
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 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
Attachment #570988 - Flags: feedback?(honzab.moz) → feedback+
> 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.
(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.
Created attachment 572029 [details] [diff] [review]
Use the observer method for idle-daily and formhistory-expire-now in nsFormHistory.js

https://tbpl.mozilla.org/?tree=Try&rev=984a1d4609c0
Attachment #570988 - Attachment is obsolete: true
Attachment #570988 - Flags: review?(mak77)
Attachment #572029 - Flags: review?(mak77)
Attachment #572029 - Flags: feedback?(honzab.moz)

Updated

6 years ago
Blocks: 698570
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.
Attachment #572029 - Flags: review?(mak77) → review+
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.
Attachment #572029 - Flags: feedback?(honzab.moz) → feedback+
https://hg.mozilla.org/mozilla-central/rev/3424159bce7a
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.