1267365 - remove dependencies on DevToolsUtils

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement, P1)

Description

[Tom Tromey :tromey]

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.

Comment 1

[James Long (:jlongster)]

Attachment: 1267365.patch

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).

Comment 2

[Tom Tromey :tromey]

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.)

Comment 3

[James Long (:jlongster)]

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.

Comment 4

[James Long (:jlongster)]

Attachment: 1267365.patch

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.

Comment 5

[James Long (:jlongster)]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=31351a61c855

Comment 6

[James Long (:jlongster)]

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

Comment 7

[James Long (:jlongster)]

Attachment: 1267365.patch

Comment 8

[Tom Tromey :tromey]

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.

Comment 9

[James Long (:jlongster)]

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

[Brian Grinstead [:bgrins]]

(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

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

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);

Comment 12

[James Long (:jlongster)]

New try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=68da4dd9b833

Comment 13

[James Long (:jlongster)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d0e315be6a1

Comment 14

[James Long (:jlongster)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3190935f857d

Comment 15

[James Long (:jlongster)]

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.

Comment 16

[James Long (:jlongster)]

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

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/1646172ac6e7