Closed Bug 1267365 Opened 4 years ago Closed 3 years ago

remove dependencies on DevToolsUtils

Categories

(DevTools :: Framework, enhancement, P1)

enhancement

Tracking

(firefox51 fixed)

RESOLVED FIXED
Firefox 51
Iteration:
51.1 - Aug 15
Tracking Status
firefox51 --- fixed

People

(Reporter: tromey, Assigned: jlong)

References

Details

(Whiteboard: [devtools-html])

Attachments

(1 file, 2 obsolete files)

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.
Assignee: nobody → jlong
Status: NEW → ASSIGNED
Iteration: --- → 49.1 - May 9
Flags: qe-verify-
Priority: -- → P1
Iteration: 49.1 - May 9 → 49.2 - May 23
Iteration: 49.2 - May 23 → 49.3 - Jun 6
Iteration: 49.3 - Jun 6 → 50.1
Iteration: 50.1 → 50.2
Attached patch 1267365.patch (obsolete) — Splinter Review
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)
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+
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.
Iteration: 50.2 - Jul 4 → 50.3 - Jul 18
Attached patch 1267365.patch (obsolete) — Splinter Review
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)
Attachment #8766804 - Attachment is obsolete: true
Summary: investigate DevToolsUtils.js → remove dependencies on DevToolsUtils
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
Attached patch 1267365.patchSplinter Review
Attachment #8769793 - Attachment is obsolete: true
Attachment #8769793 - Flags: review?(ttromey)
Attachment #8769821 - Flags: review?(ttromey)
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+
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.
(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).
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);
Iteration: 50.3 - Jul 18 → 50.4 - Aug 1
Iteration: 50.4 - Aug 1 → 51.1 - Aug 15
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.
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
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
https://hg.mozilla.org/mozilla-central/rev/1646172ac6e7
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 51
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.