Closed
Bug 1456274
Opened 6 years ago
Closed 6 years ago
Try to remove client-side progress listeners from target
Categories
(DevTools :: Framework, enhancement, P3)
DevTools
Framework
Tracking
(firefox61 fixed)
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: jryans, Assigned: jryans)
References
(Blocks 1 open bug)
Details
(Whiteboard: dt-fission)
Attachments
(3 files)
The target currently includes client-side progress listeners that would appear to replicate similar tracking done server-side. We should attempt to remove this from the target to reduce complexity and force local and remote connections to use the same code.
Assignee | ||
Updated•6 years ago
|
Assignee: nobody → jryans
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=185078ced0f1d82b25904e8f47ef7f4732493a0c
Assignee | ||
Comment 2•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b90ffab0654b69b0999f8c5c655ae0569197d040
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•6 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3178b33f35b60c9037cc71e47a980abd38ffaba9
Assignee | ||
Comment 7•6 years ago
|
||
Added Talos runs as well: https://treeherder.mozilla.org/#/jobs?repo=try&revision=532752fb414268dbbb31289d73441a2683e77570
Assignee | ||
Comment 8•6 years ago
|
||
There are a few changes on DAMP, but it's hard for me to tell if there are meaningful... https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&newProject=try&newRevision=532752fb414268dbbb31289d73441a2683e77570&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1&selectedTimeRange=172800 damp console.objectexpand 2.92% 14.06 (high) damp console.openwithcache.open.settle.DAMP -11.93% 22.45 (high) damp custom.webconsole.close.DAMP 10.96% 4.09 (med) damp simple.saveHeapSnapshot 11.46% 5.71 (high)
Comment 9•6 years ago
|
||
(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8) > damp console.objectexpand 2.92% 14.06 (high) > damp console.openwithcache.open.settle.DAMP -11.93% 22.45 (high) > damp custom.webconsole.close.DAMP 10.96% 4.09 (med) > damp simple.saveHeapSnapshot 11.46% 5.71 (high) It looks like a very random set of changes which typically comes from comparing against last x days of m-c. These tests baseline must have changed recently. You may push your base revision to get better results.
Comment 10•6 years ago
|
||
mozreview-review |
Comment on attachment 8970732 [details] Bug 1456274 - Remove local progress listener in DevTools target. https://reviewboard.mozilla.org/r/239470/#review245250 Thanks for cleaning these things up! ::: devtools/client/framework/target.js:499 (Diff revision 1) > _setupListeners: function() { > - this._webProgressListener = new TabWebProgressListener(this); > - this.tab.linkedBrowser.addProgressListener(this._webProgressListener); > this.tab.addEventListener("TabClose", this); > this.tab.parentNode.addEventListener("TabSelect", this); > this.tab.ownerDocument.defaultView.addEventListener("unload", this); Does that still work !? Aren't we throwing when using CPOW now? Are you planning to follow-up to remove even more listeners? I'm not sure unload, nor TabClose are necessary here. Or getting rid of non-remote codepath entirely as I think it is only used in tests, right?
Attachment #8970732 -
Flags: review?(poirot.alex) → review+
Comment 11•6 years ago
|
||
mozreview-review |
Comment on attachment 8970733 [details] Bug 1456274 - Rewrite browser_target_events in async style. https://reviewboard.mozilla.org/r/239472/#review245252
Attachment #8970733 -
Flags: review?(poirot.alex) → review+
Comment 12•6 years ago
|
||
mozreview-review |
Comment on attachment 8970734 [details] Bug 1456274 - Add target.makeRemote in several tests. https://reviewboard.mozilla.org/r/239474/#review245254 Looks good but that brings some questions: - was that the few tests that was still using non remote target codepath? or just the one depending on navigation events? - could we somehow force makeRemote call at (test) framework level? I imagine that's bug 1072764 (also see bug 1397020)
Attachment #8970734 -
Flags: review?(poirot.alex) → review+
Assignee | ||
Comment 13•6 years ago
|
||
(In reply to Alexandre Poirot [:ochameau] from comment #9) > (In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #8) > > > damp console.objectexpand 2.92% 14.06 (high) > > damp console.openwithcache.open.settle.DAMP -11.93% 22.45 (high) > > damp custom.webconsole.close.DAMP 10.96% 4.09 (med) > > damp simple.saveHeapSnapshot 11.46% 5.71 (high) > > It looks like a very random set of changes which typically comes from > comparing against last x days of m-c. These tests baseline must have changed > recently. > You may push your base revision to get better results. You are right. Here's a comparison with the specific base rev I used: https://treeherder.mozilla.org/perf.html#/comparesubtest?originalProject=mozilla-central&originalRevision=39ccabfd7d0712a45335325cb24b0e0b2ba498c7&newProject=try&newRevision=532752fb414268dbbb31289d73441a2683e77570&originalSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&newSignature=f79b6a4f1f8f53326d3056f6c8008c0ff4de0a94&showOnlyImportant=1&framework=1 No important changes found.
Assignee | ||
Comment 14•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8970732 [details] Bug 1456274 - Remove local progress listener in DevTools target. https://reviewboard.mozilla.org/r/239470/#review245250 > Does that still work !? > Aren't we throwing when using CPOW now? > Are you planning to follow-up to remove even more listeners? I'm not sure unload, nor TabClose are necessary here. > Or getting rid of non-remote codepath entirely as I think it is only used in tests, right? This one is `tab.ownerDocument`, so it means the browser window. Maybe you thought it said `contentDocument`? In any case, removing these remaining ones feels less straightforward to me... Perhaps some are important during tab / window shutdown where we may lose the server side abruptly? Where possible though, I agree we should remove this local stuff if we can do it on there server side without losing features.
Assignee | ||
Comment 15•6 years ago
|
||
mozreview-review-reply |
Comment on attachment 8970734 [details] Bug 1456274 - Add target.makeRemote in several tests. https://reviewboard.mozilla.org/r/239474/#review245254 There are many tests that don't call `makeRemote` themselves. Typically they open a toolbox and show some tool whose panel module will eventually call `makeRemote`. The only things that's change here is for tests that explicitly wanted the navigation events. If they are needed, someone in the pipeline now needs to call `makeRemote`. I think it's okay to just tune these few tests for now. We should solve the "real" problem via the general bugs you linked about `makeRemote`.
Comment 16•6 years ago
|
||
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7cf34fe2036e Remove local progress listener in DevTools target. r=ochameau https://hg.mozilla.org/integration/autoland/rev/97e8882d0343 Rewrite browser_target_events in async style. r=ochameau https://hg.mozilla.org/integration/autoland/rev/707d5e397407 Add target.makeRemote in several tests. r=ochameau
Comment 17•6 years ago
|
||
Backed out 3 changesets (bug 1456274) for Linting failure. CLOSED TREE Log: https://treeherder.mozilla.org/logviewer.html#?job_id=175610877&repo=autoland&lineNumber=252 Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt) [task 2018-04-25T22:16:37.449Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/devtools/client/framework/target.js:11:9 | 'XPCOMUtils' is assigned a value but never used. (no-unused-vars) [taskcluster 2018-04-25 22:16:37.741Z] === Task Finished === [taskcluster 2018-04-25 22:16:37.741Z] Unsuccessful task run with exit code: 1 completed in 546.262 seconds Push with fails: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=707d5e397407bdee7cdfbb5ee6ff644126b778a9&selectedJob=175610877 Backout: https://hg.mozilla.org/integration/autoland/rev/e43c1c2ce4131918fffab239dc7b61453ce2c736
Flags: needinfo?(jryans)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(jryans)
Comment 21•6 years ago
|
||
Pushed by jryans@gmail.com: https://hg.mozilla.org/integration/autoland/rev/2c3e334c75b6 Remove local progress listener in DevTools target. r=ochameau https://hg.mozilla.org/integration/autoland/rev/08f354110a57 Rewrite browser_target_events in async style. r=ochameau https://hg.mozilla.org/integration/autoland/rev/6c411b0781cc Add target.makeRemote in several tests. r=ochameau
Comment 22•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2c3e334c75b6 https://hg.mozilla.org/mozilla-central/rev/08f354110a57 https://hg.mozilla.org/mozilla-central/rev/6c411b0781cc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Updated•6 years ago
|
Product: Firefox → DevTools
Updated•6 years ago
|
Whiteboard: dt-fission
You need to log in
before you can comment on or make changes to this bug.
Description
•