Closed Bug 698570 Opened 8 years ago Closed 8 years ago

Use weak references in nsFormHistory.js

Categories

(Toolkit :: Form Manager, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: espindola, Assigned: espindola)

References

Details

Attachments

(1 file, 4 obsolete files)

No description provided.
Looks like I have to switch to using the observer method first. A bit obvious now that I noticed it.

While I code that, this patch changes the pref observer to be weak.

https://tbpl.mozilla.org/?tree=Try&rev=3c4098ee41ae
Attachment #570833 - Attachment is obsolete: true
Attachment #570833 - Flags: review?(mak77)
Attachment #570833 - Flags: feedback?(honzab.moz)
Attachment #570981 - Flags: review?(mak77)
Attachment #570981 - Flags: feedback?(honzab.moz)
https://tbpl.mozilla.org/?tree=Try&rev=e299b84494cf
Attachment #570981 - Attachment is obsolete: true
Attachment #570981 - Flags: review?(mak77)
Attachment #570981 - Flags: feedback?(honzab.moz)
Attachment #570989 - Flags: review?(mak77)
Attachment #570989 - Flags: feedback?(honzab.moz)
https://hg.mozilla.org/try/rev/5c03b56abe52
Attachment #570989 - Attachment is obsolete: true
Attachment #570989 - Flags: review?(mak77)
Attachment #570989 - Flags: feedback?(honzab.moz)
Attachment #570992 - Flags: review?(mak77)
Attachment #570992 - Flags: feedback?(honzab.moz)
Comment on attachment 570992 [details] [diff] [review]
Use weak references in prefs in nsFormHistory.js

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

the relation of this patch with bug 698738 is unclear, one of the two should actually convert observers to weak references... none of them is doing
unless you plan to do that in bug 696404?
This patch and bug jungle is over my understanding too, I can't keep track on this any more.  Patches and parts are seem jumping here and there, comments are spread across.  This is case where less is apparently more :)  

This should merge to a singe or up to two patches with a clear distinction, but it is probably too late.  

Rafael, thanks for your work on this, but please set the bug dependencies properly and/or give us a guide to keep track please.  Thank you.
Comment on attachment 570992 [details] [diff] [review]
Use weak references in prefs in nsFormHistory.js

Yes, this is the way to let js components be weak-reffed.

There are other addObserver calls that need to be converted too then.  I've seen changes to nsFormHistory.js also in a different bug, no idea which one it is and how exactly is related to this one.
Attachment #570992 - Flags: feedback?(honzab.moz) → feedback+
I had bad experiences changing code in this area. It is really easy to produce failures that can take one down in a long debug path. The review process also tends to add "there is something else wrong on that file, could you fix it too?", which is particularly annoying when that part the most controversial one as was the case with the "add the missing remeoveObserver/ use weak links".

That is why I am making the patches as small as possible. This is one is *just* about using weak references in nsFormHistory.js. That is one of the extra request of an old review, I assume it is a good and independent fix, no?
Comment on attachment 570992 [details] [diff] [review]
Use weak references in prefs in nsFormHistory.js

bug 698738 should land first, then this should convert all the observers in nsFormHistory.js to weak references. Otherwise we are going to lose pieces along the way.
So, once that lands, update this patch and reask for review, please.
Attachment #570992 - Flags: review?(mak77)
Depends on: 698738
(In reply to Marco Bonardo [:mak] from comment #10)
> So, once that lands, update this patch and reask for review, please.

Marco, thank for adding at least some light to dependencies.
As an additional comment, I think that "let self = this;" is now unused and can be removed. Should have been done in bug 698738 but I missed that, so please do here.
Attachment #573237 - Flags: feedback?(honzab.moz) → feedback+
Comment on attachment 573237 [details] [diff] [review]
Use weak references in nsFormHistory.js

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

thanks!
Attachment #573237 - Flags: review?(mak77) → review+
https://hg.mozilla.org/mozilla-central/rev/96123bd11904
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.