Closed
Bug 592469
Opened 14 years ago
Closed 13 years ago
Web Console cleanup: Kill getHeadsUpDisplay() and other methods
Categories
(DevTools :: General, defect, P3)
DevTools
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 8
People
(Reporter: pcwalton, Assigned: msucan)
References
Details
(Whiteboard: [has-patch][post-fx4] [console-1][qa-])
Attachments
(11 files, 14 obsolete files)
3.80 KB,
patch
|
dcamp
:
review+
Gavin
:
review+
|
Details | Diff | Splinter Review |
4.67 KB,
patch
|
dcamp
:
review+
Gavin
:
review+
|
Details | Diff | Splinter Review |
1.09 KB,
patch
|
dcamp
:
review+
Gavin
:
review+
|
Details | Diff | Splinter Review |
2.05 KB,
patch
|
dcamp
:
review+
Gavin
:
review+
|
Details | Diff | Splinter Review |
6.69 KB,
patch
|
dcamp
:
review+
Gavin
:
review+
|
Details | Diff | Splinter Review |
27.86 KB,
patch
|
dcamp
:
review+
|
Details | Diff | Splinter Review |
8.64 KB,
patch
|
dcamp
:
review+
|
Details | Diff | Splinter Review |
12.70 KB,
patch
|
dcamp
:
review+
|
Details | Diff | Splinter Review |
30.53 KB,
patch
|
Details | Diff | Splinter Review | |
6.82 KB,
patch
|
dcamp
:
review+
|
Details | Diff | Splinter Review |
7.97 KB,
patch
|
Details | Diff | Splinter Review |
getHeadsUpDisplay() (which actually returns the output node, so should be renamed) should be a member of the HeadsUpDisplay object, not the HUD Service object.
Reporter | ||
Comment 1•14 years ago
|
||
Correction: This function doesn't return the output node. Rather it returns the HUD box.
Reporter | ||
Updated•14 years ago
|
Summary: Rename getHeadsUpDisplay() and move it to the HeadsUpDisplay object in the Web Console → Web Console cleanup: Rename getHeadsUpDisplay() and move it to the HeadsUpDisplay object in the Web Console
Reporter | ||
Comment 2•14 years ago
|
||
The plan is now to kill getHeadsUpDisplay() entirely.
Summary: Web Console cleanup: Rename getHeadsUpDisplay() and move it to the HeadsUpDisplay object in the Web Console → Web Console cleanup: Kill getHeadsUpDisplay()
Comment 3•14 years ago
|
||
I know that there are other bugs that change how the console object is accessed. What work is represented by this bug? Is it just to remove the code or is there still other refactoring that will be required?
Blocks: devtools4
Reporter | ||
Comment 4•14 years ago
|
||
(In reply to comment #3) > I know that there are other bugs that change how the console object is > accessed. What work is represented by this bug? Is it just to remove the code > or is there still other refactoring that will be required? The changes tie in heavily with bug 580618. Essentially, we want to replace all instances of getHeadsUpDisplay() with accesses to the hudReferences array introduced by Mihai's patch to that one. The change should be basically mechanical.
Updated•14 years ago
|
Assignee: nobody → mihai.sucan
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #499067 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 7•14 years ago
|
||
Attachment #499068 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 8•14 years ago
|
||
Attachment #499069 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 9•14 years ago
|
||
Attachment #499071 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 10•14 years ago
|
||
Attachment #499072 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 11•14 years ago
|
||
This patch also removes the unused HUDService.getContentWindowFromHUDId() method. The new HUDService.getHudByWindow() is used by tests - it's the most common "operation" we perform in tests (get the HUD object from the window object).
Attachment #499074 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 12•14 years ago
|
||
Remove the use of displaysIndex() in the HUDService tests.
Attachment #499075 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 13•14 years ago
|
||
Remove the use of getHeadsUpDisplay() in the HUDService tests.
Attachment #499076 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 14•14 years ago
|
||
Remove the use of getOutputNodeById() in the HUDService tests.
Attachment #499077 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 15•14 years ago
|
||
Remove tests that are no longer relevant.
Attachment #499079 -
Flags: review?(gavin.sharp)
Updated•13 years ago
|
Whiteboard: [has-patch][needs-review][post-fx4]
Updated•13 years ago
|
Whiteboard: [has-patch][needs-review][post-fx4] → [has-patch][needs-review][post-fx4] [console-1]
Comment 17•13 years ago
|
||
Are these all up to date? We should probably try to get these landed soon.
Assignee | ||
Comment 18•13 years ago
|
||
There have been minimal changes since December, to the Web Console code. I think you can review them as-is. I'll update the patches with review comments and rebase. Some of the parts do not apply cleanly due to minor changes through out the Web Console code, but nothing serious. Alternatively, I can rebase the patches before review. How do you want us to proceed? Please also note that we have 3 patches in queue to land (marked as [checkin]). After those land we'll need to once again rebase these patches. Thanks!
Comment 19•13 years ago
|
||
Attachment #499067 -
Attachment is obsolete: true
Attachment #499068 -
Attachment is obsolete: true
Attachment #499069 -
Attachment is obsolete: true
Attachment #499071 -
Attachment is obsolete: true
Attachment #499072 -
Attachment is obsolete: true
Attachment #499074 -
Attachment is obsolete: true
Attachment #499075 -
Attachment is obsolete: true
Attachment #499076 -
Attachment is obsolete: true
Attachment #499077 -
Attachment is obsolete: true
Attachment #499079 -
Attachment is obsolete: true
Attachment #499067 -
Flags: review?(gavin.sharp)
Attachment #499068 -
Flags: review?(gavin.sharp)
Attachment #499069 -
Flags: review?(gavin.sharp)
Attachment #499071 -
Flags: review?(gavin.sharp)
Attachment #499072 -
Flags: review?(gavin.sharp)
Attachment #499074 -
Flags: review?(gavin.sharp)
Attachment #499075 -
Flags: review?(gavin.sharp)
Attachment #499076 -
Flags: review?(gavin.sharp)
Attachment #499077 -
Flags: review?(gavin.sharp)
Attachment #499079 -
Flags: review?(gavin.sharp)
Updated•13 years ago
|
Attachment #540784 -
Attachment is obsolete: true
Assignee | ||
Comment 20•13 years ago
|
||
I have completed a manual rebase for all the 10 patches. There has been breakage for each of them, but fixes were not too hard. All tests pass locally. Will push to the try server. Cleanup prioritized to help with incoming work on Web Console remoting support (e10s). Looking forward for the review comments. Thank you! (please note that the cleanup being done by these patches is pretty much agreed upon by Gavin Sharp and Shawn Wilsher, see the comments in bug 611789)
Attachment #543494 -
Flags: review?(dcamp)
Assignee | ||
Comment 21•13 years ago
|
||
Attachment #543495 -
Flags: review?(dcamp)
Assignee | ||
Comment 22•13 years ago
|
||
Attachment #543496 -
Flags: review?(dcamp)
Assignee | ||
Comment 23•13 years ago
|
||
Attachment #543497 -
Flags: review?(dcamp)
Assignee | ||
Comment 24•13 years ago
|
||
Attachment #543498 -
Flags: review?(dcamp)
Assignee | ||
Comment 25•13 years ago
|
||
Attachment #543499 -
Flags: review?(dcamp)
Assignee | ||
Comment 26•13 years ago
|
||
Attachment #543500 -
Flags: review?(dcamp)
Assignee | ||
Comment 27•13 years ago
|
||
Attachment #543501 -
Flags: review?(dcamp)
Assignee | ||
Comment 28•13 years ago
|
||
Attachment #543502 -
Flags: review?(dcamp)
Assignee | ||
Comment 29•13 years ago
|
||
Attachment #543504 -
Flags: review?(dcamp)
Assignee | ||
Updated•13 years ago
|
Summary: Web Console cleanup: Kill getHeadsUpDisplay() → Web Console cleanup: Kill getHeadsUpDisplay() and other methods
Version: unspecified → Trunk
Updated•13 years ago
|
Attachment #543494 -
Flags: review?(dcamp) → review+
Updated•13 years ago
|
Attachment #543495 -
Flags: review?(dcamp) → review+
Updated•13 years ago
|
Attachment #543496 -
Flags: review?(dcamp) → review+
Updated•13 years ago
|
Attachment #543497 -
Flags: review?(dcamp) → review+
Comment 30•13 years ago
|
||
Comment on attachment 543499 [details] [diff] [review] [in-fx-team] Part 6: HUDService - remove displaysIndex() and more review note, getHudByWindow isn't used in this patch, but is used in later (test) patches
Attachment #543499 -
Flags: review?(dcamp) → review+
Updated•13 years ago
|
Attachment #543500 -
Flags: review?(dcamp) → review+
Updated•13 years ago
|
Attachment #543501 -
Flags: review?(dcamp) → review+
Updated•13 years ago
|
Attachment #543502 -
Flags: review?(dcamp) → review+
Updated•13 years ago
|
Attachment #543504 -
Flags: review?(dcamp) → review+
Assignee | ||
Comment 31•13 years ago
|
||
Try server results are green: http://tbpl.mozilla.org/?tree=Try&rev=68267edfdb12 Builds and logs: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mihai.sucan@gmail.com-68267edfdb12
Updated•13 years ago
|
Attachment #543498 -
Flags: review?(dcamp) → review+
Assignee | ||
Comment 32•13 years ago
|
||
Comment on attachment 543494 [details] [diff] [review] [in-fx-team] Part 1: HUDService - remove getHeadsUpDisplay() Thank you Dave for the r+! Gavin: Updated the patches! ;) We have them reviewed within the team. We'd very much appreciate a quick review for this batch of patches. Thank you!
Attachment #543494 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•13 years ago
|
Attachment #543495 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•13 years ago
|
Attachment #543496 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•13 years ago
|
Attachment #543497 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•13 years ago
|
Attachment #543498 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•13 years ago
|
Attachment #543499 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•13 years ago
|
Attachment #543500 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•13 years ago
|
Attachment #543501 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•13 years ago
|
Attachment #543502 -
Flags: review?(gavin.sharp)
Assignee | ||
Updated•13 years ago
|
Attachment #543504 -
Flags: review?(gavin.sharp)
Updated•13 years ago
|
Attachment #543494 -
Flags: review?(gavin.sharp) → review+
Updated•13 years ago
|
Attachment #543495 -
Flags: review?(gavin.sharp) → review+
Updated•13 years ago
|
Attachment #543496 -
Flags: review?(gavin.sharp) → review+
Updated•13 years ago
|
Attachment #543497 -
Flags: review?(gavin.sharp) → review+
Updated•13 years ago
|
Attachment #543499 -
Flags: review?(gavin.sharp) → review+
Comment 33•13 years ago
|
||
Comment on attachment 543498 [details] [diff] [review] Part 5: HUDService - cleanup clear/unregisterDisplay() >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm > clearDisplay: function HS_clearDisplay(aHUD) >- if (hudRef) { >- hudRef.cssNodes = {}; >- } >- >- var outputNode = aHUD.querySelector(".hud-output-node"); >- >- while (outputNode.firstChild) { >- outputNode.removeChild(outputNode.firstChild); >- } >- >- aHUD.lastTimestamp = 0; I realize this was duplicating stuff that jsterm.clearOutput was doing, but it seems odd for this stuff to be done there rather than here. Can it move the other way instead? Seems like it's not particularly related to the JSTerm object. > unregisterDisplay: function HS_unregisterDisplay(aHUD) >- // remove the HeadsUpDisplay object from memory >- if ("cssNodes" in this.hudReferences[id]) { >- delete this.hudReferences[id].cssNodes; >- } This is unnecessary because cssNodes is reset in clearOutput/clearDisplay I guess? OK. >- ownerDoc = outputNode.ownerDocument; >- ownerDoc.getElementById(id).parentNode.removeChild(outputNode); >+ if (hud.consolePanel) { >+ hud.consolePanel.parentNode.removeChild(hud.consolePanel); >+ } >+ else { >+ hud.parentNode.removeChild(hud.HUDBox); >+ } I don't really understand how these are equivalent. In particular, I don't know what the consolePanel change is about. Why does it affect whether the HUDBox get removed?
Assignee | ||
Comment 34•13 years ago
|
||
(In reply to comment #33) > Comment on attachment 543498 [details] [diff] [review] [review] > Part 5: HUDService - cleanup clear/unregisterDisplay() > > >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm > > > clearDisplay: function HS_clearDisplay(aHUD) > > >- if (hudRef) { > >- hudRef.cssNodes = {}; > >- } > >- > >- var outputNode = aHUD.querySelector(".hud-output-node"); > >- > >- while (outputNode.firstChild) { > >- outputNode.removeChild(outputNode.firstChild); > >- } > >- > >- aHUD.lastTimestamp = 0; > > I realize this was duplicating stuff that jsterm.clearOutput was doing, but > it seems odd for this stuff to be done there rather than here. Can it move > the other way instead? Seems like it's not particularly related to the > JSTerm object. The Web Console output node is "worked on" by JSTerm mostly. We have a lot of stuff in JSTerm. If you want I can move the code in the other method. Do you want this? I can even remove jsterm.clearOutput() entirely, so we don't cause so much confusion. Shall I do so? > > unregisterDisplay: function HS_unregisterDisplay(aHUD) > > >- // remove the HeadsUpDisplay object from memory > >- if ("cssNodes" in this.hudReferences[id]) { > >- delete this.hudReferences[id].cssNodes; > >- } > > This is unnecessary because cssNodes is reset in clearOutput/clearDisplay I > guess? OK. Yep, and we delete this.hudReferences[id] entirely. No point in deleting some of the properties individually. > >- ownerDoc = outputNode.ownerDocument; > >- ownerDoc.getElementById(id).parentNode.removeChild(outputNode); > > >+ if (hud.consolePanel) { > >+ hud.consolePanel.parentNode.removeChild(hud.consolePanel); > >+ } > >+ else { > >+ hud.parentNode.removeChild(hud.HUDBox); > >+ } > > I don't really understand how these are equivalent. In particular, I don't > know what the consolePanel change is about. Why does it affect whether the > HUDBox get removed? The hud.HUDBox lives in the hud.consolePanel when the Web Console is positioned as a window (as a xul:panel). That's why the difference. Hmm, I can update this to: > >+ hud.parentNode.removeChild(hud.HUDBox); > >+ if (hud.consolePanel) { > >+ hud.consolePanel.parentNode.removeChild(hud.consolePanel); > >+ } ... to make it less confusing. Would this be fine?
Comment 35•13 years ago
|
||
Comment on attachment 543500 [details] [diff] [review] [in-fx-team] Part 7: tests - displaysIndex() I don't think I really need to review all of these test changes.
Attachment #543500 -
Flags: review?(gavin.sharp)
Updated•13 years ago
|
Attachment #543501 -
Flags: review?(gavin.sharp)
Updated•13 years ago
|
Attachment #543502 -
Flags: review?(gavin.sharp)
Updated•13 years ago
|
Attachment #543504 -
Flags: review?(gavin.sharp)
Comment 36•13 years ago
|
||
(In reply to comment #34) > The Web Console output node is "worked on" by JSTerm mostly. We have a lot > of stuff in JSTerm. Hmm, OK, I guess I had a different expectation, but that may not reflect reality. > If you want I can move the code in the other method. Do you want this? > > I can even remove jsterm.clearOutput() entirely, so we don't cause so much > confusion. Shall I do so? I'm tempted to say yes, but I think you probably know what makes most sense, so go with what you think is best. > Hmm, I can update this to: ... > ... to make it less confusing. Would this be fine? Yeah that sounds fine.
Assignee | ||
Comment 37•13 years ago
|
||
Updated the patch. Decided to keep jsterm.clearOutput() and removed HUDService.clearDisplay(). At the moment this what seems to make sense. Ultimately, HUDService should hold no methods specific to a HUD object. Please let me know if the patch is fine, because I'll have to rebase the console storage patches on top of this cleanup ASAP. Thank you!
Attachment #543498 -
Attachment is obsolete: true
Attachment #544437 -
Flags: review?(gavin.sharp)
Attachment #543498 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 38•13 years ago
|
||
Rebased the patch.
Attachment #543501 -
Attachment is obsolete: true
Assignee | ||
Comment 39•13 years ago
|
||
New patch to clean the tests - to update the calls for clearDisplay().
Attachment #544439 -
Flags: review?(dcamp)
Updated•13 years ago
|
Attachment #544439 -
Flags: review?(dcamp) → review+
Comment 40•13 years ago
|
||
Comment on attachment 544437 [details] [diff] [review] Part 5: HUDService - cleanup clear/unregisterDisplay() >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm >+ * @param string aHUD >+ * The ID of a HUD. > * @returns void > */ > unregisterDisplay: function HS_unregisterDisplay(aHUD) Can you make this aHUDId? Slightly confusing to have an "aHUD" that refers to an ID rather than a hud object. > clearOutput: function JST_clearOutput() >+ let hud = HUDService.getHudReferenceById(this.hudId); Seems like we should perhaps consider having jsterm objects should hold references to the hud directly at some point...
Attachment #544437 -
Flags: review?(gavin.sharp) → review+
Comment 41•13 years ago
|
||
Comment on attachment 544437 [details] [diff] [review] Part 5: HUDService - cleanup clear/unregisterDisplay() >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm > unregisterDisplay: function HS_unregisterDisplay(aHUD) >- ownerDoc = outputNode.ownerDocument; >- ownerDoc.getElementById(id).parentNode.removeChild(outputNode); >+ hud.HUDBox.parentNode.removeChild(hud.HUDBox); >+ if (hud.consolePanel) { >+ hud.consolePanel.parentNode.removeChild(hud.consolePanel); >+ } One more question about these differences - is it a bug fix that we now remove the entire HUDBox/consolePanel when we used to only remove the outputNode? Were we "leaking" elements when you toggled the web console on and off repeatedly?
Assignee | ||
Comment 42•13 years ago
|
||
(In reply to comment #41) > Comment on attachment 544437 [details] [diff] [review] [review] > Part 5: HUDService - cleanup clear/unregisterDisplay() > > >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm > > > unregisterDisplay: function HS_unregisterDisplay(aHUD) > > >- ownerDoc = outputNode.ownerDocument; > >- ownerDoc.getElementById(id).parentNode.removeChild(outputNode); > > >+ hud.HUDBox.parentNode.removeChild(hud.HUDBox); > >+ if (hud.consolePanel) { > >+ hud.consolePanel.parentNode.removeChild(hud.consolePanel); > >+ } > > One more question about these differences - is it a bug fix that we now > remove the entire HUDBox/consolePanel when we used to only remove the > outputNode? Were we "leaking" elements when you toggled the web console on > and off repeatedly? No, it's not a bug fix. It's only a cleanup to make the code less confusing than it was.
Assignee | ||
Comment 43•13 years ago
|
||
(In reply to comment #40) > Comment on attachment 544437 [details] [diff] [review] [review] > Part 5: HUDService - cleanup clear/unregisterDisplay() > > >diff --git a/toolkit/components/console/hudservice/HUDService.jsm b/toolkit/components/console/hudservice/HUDService.jsm > > >+ * @param string aHUD > >+ * The ID of a HUD. > > * @returns void > > */ > > unregisterDisplay: function HS_unregisterDisplay(aHUD) > > Can you make this aHUDId? Slightly confusing to have an "aHUD" that refers > to an ID rather than a hud object. Sure, I'll update the patch accordingly. And I'll look into making this method even less confusing. Thank you for the r+!
Assignee | ||
Comment 44•13 years ago
|
||
Comments addressed.
Attachment #544437 -
Attachment is obsolete: true
Assignee | ||
Updated•13 years ago
|
Whiteboard: [has-patch][needs-review][post-fx4] [console-1] → [has-patch][post-fx4] [console-1][land-in-fx-team]
Assignee | ||
Comment 45•13 years ago
|
||
Comment on attachment 543494 [details] [diff] [review] [in-fx-team] Part 1: HUDService - remove getHeadsUpDisplay() http://hg.mozilla.org/integration/fx-team/rev/978c21f5c68f
Attachment #543494 -
Attachment description: Part 1: HUDService - remove getHeadsUpDisplay() → [in-fx-team] Part 1: HUDService - remove getHeadsUpDisplay()
Assignee | ||
Comment 46•13 years ago
|
||
Comment on attachment 543495 [details] [diff] [review] [in-fx-team] Part 2: HUDService - remove getOutputNodeById() http://hg.mozilla.org/integration/fx-team/rev/2f5e953d2fbe
Attachment #543495 -
Attachment description: Part 2: HUDService - remove getOutputNodeById() → [in-fx-team] Part 2: HUDService - remove getOutputNodeById()
Assignee | ||
Comment 47•13 years ago
|
||
Comment on attachment 543496 [details] [diff] [review] [in-fx-team] Part 3: HUDService - remove getConsoleOutputNode() http://hg.mozilla.org/integration/fx-team/rev/e5bda2ebf1e4
Attachment #543496 -
Attachment description: Part 3: HUDService - remove getConsoleOutputNode() → [in-fx-team] Part 3: HUDService - remove getConsoleOutputNode()
Assignee | ||
Comment 48•13 years ago
|
||
Comment on attachment 543497 [details] [diff] [review] [in-fx-team] Part 4: HUDService - remove getChromeWindowForContentWindow() http://hg.mozilla.org/integration/fx-team/rev/152cb06533a6
Attachment #543497 -
Attachment description: Part 4: HUDService - remove getChromeWindowForContentWindow() → [in-fx-team] Part 4: HUDService - remove getChromeWindowForContentWindow()
Assignee | ||
Comment 49•13 years ago
|
||
Comment on attachment 544495 [details] [diff] [review] [in-fx-team] Part 5: HUDService - cleanup clear/unregisterDisplay() http://hg.mozilla.org/integration/fx-team/rev/11286078b11a
Attachment #544495 -
Attachment description: Part 5: HUDService - cleanup clear/unregisterDisplay() → [in-fx-team] Part 5: HUDService - cleanup clear/unregisterDisplay()
Assignee | ||
Comment 50•13 years ago
|
||
Comment on attachment 543499 [details] [diff] [review] [in-fx-team] Part 6: HUDService - remove displaysIndex() and more http://hg.mozilla.org/integration/fx-team/rev/a3657ba60787
Attachment #543499 -
Attachment description: Part 6: HUDService - remove displaysIndex() and more → [in-fx-team] Part 6: HUDService - remove displaysIndex() and more
Assignee | ||
Comment 51•13 years ago
|
||
Comment on attachment 543500 [details] [diff] [review] [in-fx-team] Part 7: tests - displaysIndex() http://hg.mozilla.org/integration/fx-team/rev/1f098ada09bb
Attachment #543500 -
Attachment description: Part 7: tests - displaysIndex() → [in-fx-team] Part 7: tests - displaysIndex()
Assignee | ||
Comment 52•13 years ago
|
||
Comment on attachment 544438 [details] [diff] [review] [in-fx-team] Part 8: tests - getHeadsUpDisplay() http://hg.mozilla.org/integration/fx-team/rev/94b065b8ec4e
Attachment #544438 -
Attachment description: Part 8: tests - getHeadsUpDisplay() → [in-fx-team] Part 8: tests - getHeadsUpDisplay()
Assignee | ||
Comment 53•13 years ago
|
||
Comment on attachment 543502 [details] [diff] [review] [in-fx-team] Part 9: tests - getOutputNodeById() http://hg.mozilla.org/integration/fx-team/rev/abcf5e88d5a1
Attachment #543502 -
Attachment description: Part 9: tests - getOutputNodeById() → [in-fx-team] Part 9: tests - getOutputNodeById()
Assignee | ||
Comment 54•13 years ago
|
||
Comment on attachment 543504 [details] [diff] [review] [in-fx-team] Part 10: tests - cleanup http://hg.mozilla.org/integration/fx-team/rev/1aaee943f66b
Attachment #543504 -
Attachment description: Part 10: tests - cleanup → [in-fx-team] Part 10: tests - cleanup
Assignee | ||
Comment 55•13 years ago
|
||
Comment on attachment 544439 [details] [diff] [review] [in-fx-team] Part 11: tests - clearDisplay() http://hg.mozilla.org/integration/fx-team/rev/59ae546b1da7
Attachment #544439 -
Attachment description: Part 11: tests - clearDisplay() → [in-fx-team] Part 11: tests - clearDisplay()
Assignee | ||
Updated•13 years ago
|
Whiteboard: [has-patch][post-fx4] [console-1][land-in-fx-team] → [has-patch][post-fx4] [console-1][fixed-in-fx-team]
Comment 56•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/978c21f5c68f http://hg.mozilla.org/mozilla-central/rev/2f5e953d2fbe http://hg.mozilla.org/mozilla-central/rev/e5bda2ebf1e4 http://hg.mozilla.org/mozilla-central/rev/152cb06533a6 http://hg.mozilla.org/mozilla-central/rev/11286078b11a http://hg.mozilla.org/mozilla-central/rev/a3657ba60787 http://hg.mozilla.org/mozilla-central/rev/1f098ada09bb http://hg.mozilla.org/mozilla-central/rev/94b065b8ec4e http://hg.mozilla.org/mozilla-central/rev/abcf5e88d5a1 http://hg.mozilla.org/mozilla-central/rev/1aaee943f66b http://hg.mozilla.org/mozilla-central/rev/59ae546b1da7
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Whiteboard: [has-patch][post-fx4] [console-1][fixed-in-fx-team] → [has-patch][post-fx4] [console-1]
Target Milestone: --- → Firefox 8
Comment 57•13 years ago
|
||
These changes made us leak data:text/html,Web%20Console%20test%20for%20bug%20626484 during mochitest-browser-chrome (bug 658738). Any idea what's going on and what a quick fix could be? I'm eager to back this out otherwise, as new leaks are creeping in faster than we're fixing lingering ones in bug 658738.
Comment 58•13 years ago
|
||
I appreciate the desire to make progress on bug 658738, but I don't think backing this out is the right tradeoff. We should aggressively attempt to track down the leak, though - Mihai, is that something you can look into (in a followup bug)?
Comment 59•13 years ago
|
||
A followup bug is fine as long as someone's working on it.
Assignee | ||
Comment 60•13 years ago
|
||
I will look into this. Does this patch cause constant memleaks? When I wrote these patches I sent them to try server, had no memleaks.
Comment 61•13 years ago
|
||
It's constant, and these kind of leaks currently go unnoticed. See bug 658738 for details.
Assignee | ||
Comment 62•13 years ago
|
||
(In reply to comment #61) > It's constant, and these kind of leaks currently go unnoticed. See bug > 658738 for details. Ah, that's unfortunate. I hope this kind of leaks will start to be tracked by the automated test systems as soon as possible. I'll look into what's causing the leaks. Shouldn't be too hard to fix this, once I have a reliable way to test it on my local system. (I hope to have a patch over the weekend.)
Assignee | ||
Comment 63•13 years ago
|
||
I just did a local "backout" of these patches, to see if I get memory leaks when I revert these changes. I still do get this kind of memleaks, minor differences. So, these patches do not have a direct impact.
Comment 64•13 years ago
|
||
Can you be more specific about what you're seeing? If data:text/html,Web%20Console%20test%20for%20bug%20626484 is part of the minor difference, then that's exactly what I'm talking about. I spent an hour bisecting this. It first showed up on this bug's fx-team landing and then on the fx-team -> mozilla-central merge.
Assignee | ||
Comment 65•13 years ago
|
||
(In reply to comment #64) > Can you be more specific about what you're seeing? If > data:text/html,Web%20Console%20test%20for%20bug%20626484 is part of the > minor difference, then that's exactly what I'm talking about. I spent an > hour bisecting this. It first showed up on this bug's fx-team landing and > then on the fx-team -> mozilla-central merge. Here's what I have: I ran all the HUDService tests on my system from fxteam r72737, no changes: http://pastebin.mozilla.org/1274945 I see a bunch DOM windows leaked. If I go back to fxteam r72418, before the Web Console cleanup patches landed, I get the following DOM window leaks: http://pastebin.mozilla.org/1274947 There were fewer, but we still had some of them. If I only back-out the patch from the current fxteam revision 72737: http://pastebin.mozilla.org/1274948 I still get a bad number of DOM windows leaked. It looks to me like the Web Console cleanup patches have only affected the manifestation of the leak. Additionally, something outside of the Web Console has affected the number of windows being leaked, because if I backout these patches we still get a lot more memleaks than we had in r72418, so ... something else since that revision until now ... exposes more of these leaks. (afaik we had no other Web Console patches landed in fxteam since then.) I've began work on hunting down the memory leak(s) we have in the code. Going to submit a patch when I managed to track them down.
Comment 66•13 years ago
|
||
(In reply to comment #65) > (In reply to comment #64) > > Can you be more specific about what you're seeing? If > > data:text/html,Web%20Console%20test%20for%20bug%20626484 is part of the > > minor difference, then that's exactly what I'm talking about. I spent an > > hour bisecting this. It first showed up on this bug's fx-team landing and > > then on the fx-team -> mozilla-central merge. > > Here's what I have: > > I ran all the HUDService tests on my system from fxteam r72737, no changes: > > http://pastebin.mozilla.org/1274945 > > I see a bunch DOM windows leaked. > > If I go back to fxteam r72418, before the Web Console cleanup patches > landed, I get the following DOM window leaks: > > http://pastebin.mozilla.org/1274947 > > There were fewer, but we still had some of them. > > If I only back-out the patch from the current fxteam revision 72737: > > http://pastebin.mozilla.org/1274948 > > I still get a bad number of DOM windows leaked. Again, that's not what I'm talking about. I know the list is long and growing (hence me nagging in bugs!). What I'm asking you here is to focus on "data:text/html,Web%20Console%20test%20for%20bug%20626484", which entered the list with this check-in. Your sample logs actually support this.
Assignee | ||
Comment 67•13 years ago
|
||
(In reply to comment #66) > Again, that's not what I'm talking about. I know the list is long and > growing (hence me nagging in bugs!). What I'm asking you here is to focus on > "data:text/html,Web%20Console%20test%20for%20bug%20626484", which entered > the list with this check-in. Your sample logs actually support this. Agreed, but that leak is not related to the test file which opens that data URI. Unfortunately, if I modify that test to *only* open a tab, open the Web Console, close the Web Console, then remove the tab, ... I still get the memory leak. That's very ugly. We need to find the root problem...
Comment 68•13 years ago
|
||
> Unfortunately, if I modify that test to *only* open a tab, open the Web
> Console, close the Web Console, then remove the tab, ... I still get the
> memory leak.
In an aggregated log or when running that test on its own?
With or without this bug's patches?
Assignee | ||
Comment 69•13 years ago
|
||
(In reply to comment #68) > > Unfortunately, if I modify that test to *only* open a tab, open the Web > > Console, close the Web Console, then remove the tab, ... I still get the > > memory leak. > > In an aggregated log or when running that test on its own? When running that test on its own. > With or without this bug's patches? With these patches. Looking through the code I did find some stuff we never removed, patching those... but still they don't fix *this* leak. Need to look into the code more.
Comment 70•13 years ago
|
||
Running the test on its own won't give you useful results without further modifications, as the window won't be destroyed synchronously. Try waiting ~15 seconds between removing the tab and finishing the test.
Assignee | ||
Comment 71•13 years ago
|
||
(In reply to comment #70) > Running the test on its own won't give you useful results without further > modifications, as the window won't be destroyed synchronously. Try waiting > ~15 seconds between removing the tab and finishing the test. Thanks for your suggestion! Currently I am tracking down the memleak(s). For the minimal test I was running I got it down to no memleak. I removed all the addEventListener() calls that have no paired removeEventListener(). It seems we need to properly remove all events, to all elements in the UI. Unfortunately, the code we have is not very nice about having balanced calls to add and remove event listeners. (This is something that could explain why the code cleanup patches here have changed the memleak behavior, without actually adding/removing memleaks.)
Assignee | ||
Comment 72•13 years ago
|
||
For reference: I opened bug 672470 and submitted a patch which fixes memory leaks in the Web Console code.
Updated•13 years ago
|
Whiteboard: [has-patch][post-fx4] [console-1] → [has-patch][post-fx4] [console-1][qa-]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•