Closed
Bug 1267365
Opened 8 years ago
Closed 8 years ago
remove dependencies on DevToolsUtils
Categories
(DevTools :: Framework, enhancement, P1)
DevTools
Framework
Tracking
(firefox51 fixed)
Tracking | Status | |
---|---|---|
firefox51 | --- | fixed |
People
(Reporter: tromey, Assigned: jlong)
References
Details
(Whiteboard: [devtools-html])
Attachments
(1 file, 2 obsolete files)
109.78 KB,
patch
|
tromey
:
review+
|
Details | Diff | Splinter Review |
DevToolsUtils.js has some thing that are definitely used by the clients and must be replaced for the de-chrome-ification project. It also has some things that are probably not used client-side. This needs to be investigated; some things need replacing (like Services.tm use, see bug 1265840) and some perhaps split into a separate server-only file.
Updated•8 years ago
|
Assignee: nobody → jlong
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Flags: qe-verify-
Priority: -- → P1
Updated•8 years ago
|
Iteration: 49.1 - May 9 → 49.2 - May 23
Updated•8 years ago
|
Iteration: 49.2 - May 23 → 49.3 - Jun 6
Updated•8 years ago
|
Iteration: 49.3 - Jun 6 → 50.1
Updated•8 years ago
|
Iteration: 50.1 → 50.2
Assignee | ||
Comment 1•8 years ago
|
||
DevToolsUtils specifically wraps chrome functionality, so it doesn't really make sense to dechromify it. What we should do it just avoid using it in client code. The inspector doesn't use it very much; in fact there's just one `waitForTick` that needs to be changed to `setTimeout` (which is now available through a loader global).
Attachment #8766804 -
Flags: review?(ttromey)
Reporter | ||
Comment 2•8 years ago
|
||
Comment on attachment 8766804 [details] [diff] [review] 1267365.patch Review of attachment 8766804 [details] [diff] [review]: ----------------------------------------------------------------- Thank you for the patch. This is definitely ok to go in. I see a bunch of uses of DevToolsUtils in devtools/client/shared and in devtools/shared; and I wonder if some of those are loaded by the inspector and therefore need an update. (Though at least one, css-logic.js, is already addressed elsewhere.)
Attachment #8766804 -
Flags: review?(ttromey) → review+
Assignee | ||
Comment 3•8 years ago
|
||
Oh, I don't know how I forgot about the shared files. Many of those seem to be things like lazy requires, which are covered by other bugs... I'll try to audit the usages.
Updated•8 years ago
|
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
Assignee | ||
Comment 4•8 years ago
|
||
As mentioned on IRC, this patch became quite big due to moving `DevToolsUtils.testing` flag to `flags.testing` (all of the flags are in their own simple module now). This removes the dependency on DevToolsUtils in lots of places (and I like separating out the flags better). Marking for review, but don't look at it too closely let. I'll run it on try and see if there are any major errors first, but thought you might want to have a quick look.
Attachment #8769793 -
Flags: review?(ttromey)
Assignee | ||
Updated•8 years ago
|
Attachment #8766804 -
Attachment is obsolete: true
Assignee | ||
Comment 5•8 years ago
|
||
try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31351a61c855
Assignee | ||
Updated•8 years ago
|
Summary: investigate DevToolsUtils.js → remove dependencies on DevToolsUtils
Assignee | ||
Comment 6•8 years ago
|
||
I just realized I left a line of code in DebuggerController.js that will fail on purpose, was making sure that some checks I added worked. New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=69bc78560dc9
Assignee | ||
Comment 7•8 years ago
|
||
Attachment #8769793 -
Attachment is obsolete: true
Attachment #8769793 -
Flags: review?(ttromey)
Attachment #8769821 -
Flags: review?(ttromey)
Reporter | ||
Comment 8•8 years ago
|
||
Comment on attachment 8769821 [details] [diff] [review] 1267365.patch Review of attachment 8769821 [details] [diff] [review]: ----------------------------------------------------------------- Thank you. This looks good overall. ::: devtools/client/shared/widgets/view-helpers.js @@ +455,1 @@ > /** Given the comment, I think this change requires extra justification. This was introduced in bug 951633 but without any information about what kind of testing was done :( ::: devtools/shared/flags.js @@ +1,1 @@ > + There's a standard header comment that should go here, plus "use strict". Also I think this should be born eslint-clean and have the top-level .eslintignore updated to check this file.
Attachment #8769821 -
Flags: review?(ttromey) → review+
Assignee | ||
Comment 9•8 years ago
|
||
Fixed some errors: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac8f13449a32 > Given the comment, I think this change requires extra justification. I personally think it's an unnecessary optimization. If there were numbers to back it up, I'd try to figure out a different way, but we have to get rid of lazy magic. We could define a getter that lazily instantiates the map, but I really don't see the point. The map is immediately used whenever anyone does something with the item. Don't we track memory usage in something like damp? An easy way to see this is to see if any of those graphs change.
Comment 10•8 years ago
|
||
(In reply to James Long (:jlongster) from comment #9) > Don't we track memory usage in something like damp? An easy way to see this > is to see if any of those graphs change. Unfortunately, no. I believe we could use some of the utils added for the memory tool to get memory usage of the top level devtools window while running and track that as any other metric in DAMP. There's also a way now to add an individual mochitest that exports perfherder data for reporting instead of needing to make changes to DAMP. See https://groups.google.com/d/msg/mozilla.dev.platform/9sHSQOXIkcM/vFa6eGynBQAJ / https://gbrownmozilla.wordpress.com/2016/01/27/test_awsy_lite / Bug 1233220. We could consider doing that for memory usage since it should be a little easier to develop (plus no addon signing).
Comment 11•8 years ago
|
||
You could do something like this: // Force GC+CC to get rid of jit code and to calm down the variation across runs. Cu.forceGC(); Cu.forceCC(); Cu.forceGC(); // Save a snapshot of the full runtime. const path = ThreadSafeChromeUtils.saveHeapSnapshot({ runtime: true }); // Read the snapshot file back into memory. const snapshot = ThreadSafeChromeUtils.readHeapSnapshot(path); // Count and report the total number of bytes in the snapshot. const totalBytes = snapshot.takeCensus({ breakdown: { by: "count", bytes: true, count: false } }).bytes; reportToDAMP(totalBytes);
Assignee | ||
Comment 12•8 years ago
|
||
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68da4dd9b833
Updated•8 years ago
|
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
Assignee | ||
Comment 13•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d0e315be6a1
Assignee | ||
Comment 14•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3190935f857d
Updated•8 years ago
|
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15
Assignee | ||
Comment 15•8 years ago
|
||
New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9da6031f9f8c I'm hoping this one is clear. If it reproduces the previous failure I saw it's definitely my fault and I'll try to fix it.
Assignee | ||
Comment 16•8 years ago
|
||
Ended up tweaking a few things (eslint errors mainly) so I might as well do another try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ae0c3594705 Will watch this today and hopefully mostly finished by end of day I'll commit
Comment 17•8 years ago
|
||
Pushed by jlong@mozilla.com: https://hg.mozilla.org/integration/fx-team/rev/1646172ac6e7 move various flags out of DevToolsUtils and don't depend on that module so much r=tromey
Comment 18•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/1646172ac6e7
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox51:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•