Closed
Bug 767682
Opened 12 years ago
Closed 10 years ago
Port |Bug 750454 - FUEL causes lots of leaks until shutdown, can also cause 10+minute shutdown times| to SeaMonkey
Categories
(SeaMonkey :: General, defect, P2)
SeaMonkey
General
Tracking
(Not tracked)
RESOLVED
FIXED
seamonkey2.13
People
(Reporter: sgautherie, Assigned: sgautherie)
References
()
Details
Attachments
(6 files, 2 obsolete files)
6.85 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
14.39 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
2.77 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
17.81 KB,
patch
|
mak
:
feedback+
|
Details | Diff | Splinter Review |
9.17 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
7.32 KB,
patch
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•12 years ago
|
||
Simple port. (Untested.)
Attachment #637019 -
Flags: review?(neil)
Assignee | ||
Comment 2•12 years ago
|
||
Simple port. (Untested.) NB: I'm not sure whether |.bind(this)| could still be used, so I simply copied |var self = this;|. I used |Ci|, which is explicitly defined in this file.
Attachment #637020 -
Flags: review?(neil)
Assignee | ||
Comment 3•12 years ago
|
||
Trivial copy. (Untested.)
Attachment #637021 -
Flags: review?(neil)
Assignee | ||
Comment 4•12 years ago
|
||
Simple port, and more nits. (Untested.)
Attachment #637022 -
Flags: review?(neil)
Assignee | ||
Comment 5•12 years ago
|
||
Nits, plus 2 fixes: *s/getService[Ci.mozIAsyncLivemarks]/getService(Ci.mozIAsyncLivemarks)/ *Add missing |delete this.bookmarks;|, which correctly fixes bug 758459. (Untested.)
Attachment #637024 -
Flags: review?(mak77)
Assignee | ||
Comment 6•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=37e2a400b4d9 [Mozilla/5.0 (Windows NT 5.1; rv:16.0) Gecko/16.0 Firefox/16.0 SeaMonkey/2.13a1 // Build identifier: 20120627041654] (MsVc8, sgautherie.bz@free.fr-67dbc42bd72d/try-comm-central-win32) suite/smile/test/ m-b-c still pass with the 4 SM patches applied :-)
Assignee | ||
Comment 7•12 years ago
|
||
(In reply to Serge Gautherie (:sgautherie) from comment #6) > suite/smile/test/ m-b-c still pass with the 4 SM patches applied :-) (on local Windows XP)
Comment 8•12 years ago
|
||
Comment on attachment 637019 [details] [diff] [review] (Av1) Port |Bug 750454 Part 2: Fix browser leaks| to SeaMonkey [Checked in: Comment 15] >+ this._tabbrowser.addEventListener(aType, this, >+ /* useCapture = */ true); Please can you remove the useCapture comments. Anyone who wants to patch this code had better know what the third argument means ;-) >+ } else { >+ // This tab may have moved to another window, so make sure its cached >+ // window is up-to-date. >+ smileBrowserTab._window = aSMILEWindow; Actually I don't think this is possible. Even if an extension "moves" a tab to a new window, it would need to create new <tab> and <browser> elements in the new window's document, so that a <tab> element could never exist in more than one window. r=me with this removed. (Ensure that getWindow and getBrowserTab use the same formatting.)
Attachment #637019 -
Flags: review?(neil) → review+
Updated•12 years ago
|
Attachment #637020 -
Flags: review?(neil) → review+
Comment 9•12 years ago
|
||
Comment on attachment 637021 [details] [diff] [review] (Cv1) Port |Bug 750454 Part 7: Test fixes| to SeaMonkey [Checked in: Comment 17] r=me assuming the tests pass.
Attachment #637021 -
Flags: review?(neil) → review+
Updated•12 years ago
|
Attachment #637022 -
Flags: review?(neil) → review-
Assignee | ||
Comment 10•12 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #8) > Please can you remove the useCapture comments. Anyone who wants to patch > this code had better know what the third argument means ;-) This was discussed in bug 750454 comment 60. I would rather just copy FF code, but I won't insist if you really don't want it. > >+ } else { > >+ // This tab may have moved to another window, so make sure its cached > >+ // window is up-to-date. > >+ smileBrowserTab._window = aSMILEWindow; > Actually I don't think this is possible. Even if an extension "moves" a tab > to a new window, it would need to create new <tab> and <browser> elements in > the new window's document, so that a <tab> element could never exist in more > than one window. r=me with this removed. (Ensure that getWindow and > getBrowserTab use the same formatting.) Is this a difference between SMILE and FUEL, or would you say this code is useless in FUEL too? (See bug 750454 comment 53.)
Assignee | ||
Comment 11•12 years ago
|
||
Comment on attachment 637022 [details] [diff] [review] (Dv1) Port |Bug 750454 Part 8: Reformat FUEL code| to SeaMonkey [Split into Ev1 and Fv1] Did you mean to r+, or what is wrong with this patch?
Comment 12•12 years ago
|
||
(In reply to Serge Gautherie from comment #10) > (In reply to comment #8) > > Please can you remove the useCapture comments. > This was discussed in bug 750454 comment 60. No, that was ownsWeak comments; you can leave those in if you insist. > > >+ } else { > > >+ // This tab may have moved to another window, so make sure its cached > > >+ // window is up-to-date. > > >+ smileBrowserTab._window = aSMILEWindow; > > Actually I don't think this is possible. Even if an extension "moves" a tab > > to a new window, it would need to create new <tab> and <browser> elements in > > the new window's document, so that a <tab> element could never exist in more > > than one window. r=me with this removed. (Ensure that getWindow and > > getBrowserTab use the same formatting.) > Is this a difference between SMILE and FUEL, or would you say this code is > useless in FUEL too? As far as I can tell, this code is useless in FUEL too.
Comment 13•12 years ago
|
||
(In reply to Serge Gautherie from comment #11) > Did you mean to r+, or what is wrong with this patch? I don't like the "reformatting". The whitespace changes are OK though.
Assignee | ||
Comment 14•12 years ago
|
||
Comment on attachment 637024 [details] [diff] [review] (Gv1-FF) Resync/Fix fuelApplication.js from smileApplication.js mochitest-other succeeded as https://tbpl.mozilla.org/?tree=Try&rev=9c3b516c25e6 sgautherie.bz@free.fr – Thu Jun 28 06:56:06 2012 PDT
Assignee | ||
Comment 15•12 years ago
|
||
Comment on attachment 637019 [details] [diff] [review] (Av1) Port |Bug 750454 Part 2: Fix browser leaks| to SeaMonkey [Checked in: Comment 15] http://hg.mozilla.org/comm-central/rev/88b36d0d1a85 Av1, with comment 8 suggestion(s).
Attachment #637019 -
Attachment description: (Av1) Port |Bug 750454 Part 2: Fix browser leaks| to SeaMonkey → (Av1) Port |Bug 750454 Part 2: Fix browser leaks| to SeaMonkey
[Checked in: Comment 15]
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 637020 [details] [diff] [review] (Bv1) Port |Bug 750454 Part 4: Fix bookmarks leaks| to SeaMonkey [Checked in: Comment 16] http://hg.mozilla.org/comm-central/rev/4e50453f250d
Attachment #637020 -
Attachment description: (Bv1) Port |Bug 750454 Part 4: Fix bookmarks leaks| to SeaMonkey → (Bv1) Port |Bug 750454 Part 4: Fix bookmarks leaks| to SeaMonkey
[Checked in: Comment 16]
Assignee | ||
Comment 17•12 years ago
|
||
Comment on attachment 637021 [details] [diff] [review] (Cv1) Port |Bug 750454 Part 7: Test fixes| to SeaMonkey [Checked in: Comment 17] http://hg.mozilla.org/comm-central/rev/d61f5bc92f00
Attachment #637021 -
Attachment description: (Cv1) Port |Bug 750454 Part 7: Test fixes| to SeaMonkey → (Cv1) Port |Bug 750454 Part 7: Test fixes| to SeaMonkey
[Checked in: Comment 17]
Assignee | ||
Comment 18•12 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #12) > As far as I can tell, this code is useless in FUEL too. Bug 770477 Submitted
Assignee | ||
Comment 19•12 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #13) > I don't like the "reformatting". The whitespace changes are OK though. This file already uses Ci in some places: why not use Cc/Ci everywhere?
Attachment #639032 -
Flags: review?(neil)
Comment 20•12 years ago
|
||
(In reply to Serge Gautherie from comment #19) > This file already uses Ci in some places Only because extApplication.js needs it and doesn't define it itself.
Updated•12 years ago
|
Attachment #639032 -
Flags: review?(neil) → review+
Assignee | ||
Comment 21•12 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #20) > (In reply to Serge Gautherie from comment #19) > > This file already uses Ci in some places > Only because extApplication.js needs it and doesn't define it itself. Then shouldn't we expand the few Ci which are in our file?
Comment 22•12 years ago
|
||
> Then shouldn't we expand the few Ci which are in our file?
Yes. but this is low priority.
Comment 23•12 years ago
|
||
(In reply to Serge Gautherie from comment #19) > This file already uses Ci in some places Only because I only just noticed them in your earlier attachments :-(
Comment 24•12 years ago
|
||
(In reply to neil@parkwaycc.co.uk from comment #23) > (In reply to Serge Gautherie from comment #19) > > This file already uses Ci in some places > Only because I only just noticed them in your earlier attachment :-( Attachment 637020 [details] [diff] to be precise.
Assignee | ||
Comment 25•12 years ago
|
||
Comment on attachment 639032 [details] [diff] [review] (Ev1) Port |Bug 750454 Part 8: Reformat FUEL code| to SeaMonkey [Checked in: Comment 25] http://hg.mozilla.org/comm-central/rev/7b0299689777
Attachment #639032 -
Attachment description: (Ev1) Port |Bug 750454 Part 8: Reformat FUEL code| (whitespace and function name) to SeaMonkey → (Ev1) Port |Bug 750454 Part 8: Reformat FUEL code| to SeaMonkey
[Checked in: Comment 25]
Assignee | ||
Comment 26•12 years ago
|
||
Attachment #639617 -
Flags: review?(neil)
Assignee | ||
Updated•12 years ago
|
Attachment #637022 -
Attachment description: (Dv1) Port |Bug 750454 Part 8: Reformat FUEL code| to SeaMonkey → (Dv1) Port |Bug 750454 Part 8: Reformat FUEL code| to SeaMonkey
[Split into Ev1 and Fv1]
Attachment #637022 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Attachment #637024 -
Attachment description: (Ev1-FF) Resync/Fix fuelApplication.js from smileApplication.js → (Gv1-FF) Resync/Fix fuelApplication.js from smileApplication.js
Attachment #637024 -
Attachment filename: 767682-Ev1-FF_resync-fix.diff → 767682-Gv1-FF_resync-fix.diff
Comment 27•12 years ago
|
||
Comment on attachment 639617 [details] [diff] [review] (Fv1) smileApplication.js: Fix a few nits >+ return Components.classes["@mozilla.org/network/io-service;1"] >+ .getService(Components.interfaces.nsIIOService) >+ .newURI(aSpec, null, null); [I guess we'll switch to Services.io eventually] >+ for (var i = 0; i < tabNodes.length; ++i) [Not terribly keen on ++i]
Attachment #639617 -
Flags: review?(neil) → review+
Assignee | ||
Comment 28•12 years ago
|
||
Fv1, with comment 27 suggestion(s).
Attachment #639617 -
Attachment is obsolete: true
Attachment #640199 -
Flags: review?(neil)
Comment 29•12 years ago
|
||
Comment on attachment 640199 [details] [diff] [review] (Fv1a) smileApplication.js: Use Services.*, Fix a few nits Where does Services.jsm get imported?
Comment 30•12 years ago
|
||
Comment on attachment 637024 [details] [diff] [review] (Gv1-FF) Resync/Fix fuelApplication.js from smileApplication.js Review of attachment 637024 [details] [diff] [review]: ----------------------------------------------------------------- Looks good apart a couple things ::: browser/fuel/src/fuelApplication.js @@ +14,5 @@ > // Singleton that holds services and utilities > var Utilities = { > get bookmarks() { > let bookmarks = Cc["@mozilla.org/browser/nav-bookmarks-service;1"]. > + getService(Ci.nsINavBookmarksService); I honestly don't understand these indentation changes, we always indented as Cc[ getService or Cc[ .getService never seen it indented between the 2 "c" @@ +150,5 @@ > let fuelBrowserTab = fuelBrowserTabMap.get(aBrowser); > if (!fuelBrowserTab) { > fuelBrowserTab = new BrowserTab(aFUELWindow, aBrowser); > fuelBrowserTabMap.set(aBrowser, fuelBrowserTab); > + } else { all the file uses uncuddled else, so while cuddled is coherent with the coding guide, is not coherent with this specific file. @@ +553,3 @@ > Utilities.bookmarksObserver.addListener(self._id, aEvent, aListener); > } > + } else { ditto @@ +565,3 @@ > Utilities.bookmarksObserver.removeListener(self._id, aEvent, aListener); > } > + } else { ditto @@ +767,5 @@ > + QueryInterface: XPCOMUtils.generateQI( > + [Ci.fuelIApplication, > + Ci.extIApplication, > + Ci.nsIObserver, > + Ci.nsISupportsWeakReference]), nit: I'd rather suggest QueryInterface: XPCOMUtils.generateQI([ Ci.xxx ]),
Attachment #637024 -
Flags: review?(mak77) → feedback+
Comment 31•10 years ago
|
||
Comment on attachment 640199 [details] [diff] [review] (Fv1a) smileApplication.js: Use Services.*, Fix a few nits > (Fv1a) smileApplication.js: Use Services.*, Fix a few nits This should really go into a new bug. This one is long enough.
Attachment #640199 -
Flags: review?(neil)
Comment 32•10 years ago
|
||
> (Gv1-FF) Resync/Fix fuelApplication.js from smileApplication.js
Please file a new bug in Firefox. Thanks.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•