Tom Tromey :tromey Reporter
	Description • 8 years ago

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.

	James Long (:jlongster) 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).

	Tom Tromey :tromey 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.)

	James Long (:jlongster) 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.

	James Long (:jlongster) 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.

	James Long (:jlongster) Assignee
	Comment 5 • 8 years ago

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

	James Long (:jlongster) 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

	Tom Tromey :tromey 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.

	James Long (:jlongster) 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.

	Brian Grinstead [:bgrins]
	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).

	Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]
	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);

	James Long (:jlongster) Assignee
	Comment 12 • 8 years ago

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

	James Long (:jlongster) Assignee
	Comment 13 • 8 years ago

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

	James Long (:jlongster) Assignee
	Comment 14 • 8 years ago

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

	James Long (:jlongster) 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.

	James Long (:jlongster) 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

	Pulsebot
	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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 18 • 8 years ago
bugherder

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