1196047 - Reorganize /devtools/shared subdirectories

Status: RESOLVED FIXED

(DevTools :: General, defect)

Description

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

After the large move in bug 912121, there will be a few things in /devtools/shared that need more specific tweaks:

* /devtools/shared/shared: Most of these are server files, move them there
* Other bits may be server specific as well

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1196047 - Move system.js to devtools/shared. r=jsantell

Bug 1196047 - Move system.js to devtools/shared. r=jsantell

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1196047 - Move indentation.js to devtools/shared. r=bgrins

Bug 1196047 - Move indentation.js to devtools/shared. r=bgrins

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1196047 - Move async-storage.js to devtools/shared. r=bgrins

Bug 1196047 - Move async-storage.js to devtools/shared. r=bgrins

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1196047 - Move worker*.js to devtools/shared. r=jsantell

Bug 1196047 - Move worker*.js to devtools/shared. r=jsantell

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1196047 - Move most of shared/performance to client or server. r=jsantell

Bug 1196047 - Move most of shared/performance to server/performance. r=jsantell

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1196047 - Move shared/shared perf files to server/performance. r=jsantell

Bug 1196047 - Move shared/shared perf files to server/performance. r=jsantell

Comment 7

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: MozReview Request: Bug 1196047 - Move tern to client/sourceeditor. r=bgrins

Bug 1196047 - Move tern to client/sourceeditor. r=bgrins

Comment 8

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a342d0f8777d

Comment 9

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8671083 [details]
MozReview Request: Bug 1196047 - Move system.js to devtools/shared. r=jsantell

https://reviewboard.mozilla.org/r/21513/#review19391

Comment 10

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8671086 [details]
MozReview Request: Bug 1196047 - Move worker*.js to devtools/shared. r=jsantell

https://reviewboard.mozilla.org/r/21519/#review19393

I wonder if it'd be useful to nest all of these in devtools/shared/worker/*? Also, we should be able to do `devtools/shared/worker` and `devtools/shared/worker/other-worker-module` by using standard node resolution rules, if that helps at all

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8671084 [details]
MozReview Request: Bug 1196047 - Move indentation.js to devtools/shared. r=bgrins

https://reviewboard.mozilla.org/r/21515/#review19397

::: devtools/shared/tests/unit/xpcshell.ini:14
(Diff revision 1)
> +skip-if = toolkit == 'gonk' && debug # Bug 1206586

Don't think this is needed. I see skip-if = toolkit == 'android' || toolkit == 'gonk' at the top of the xpcshell.ini file here

Comment 12

[Brian Grinstead [:bgrins]]

Comment on attachment 8671085 [details]
MozReview Request: Bug 1196047 - Move async-storage.js to devtools/shared. r=bgrins

https://reviewboard.mozilla.org/r/21517/#review19399

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8671089 [details]
MozReview Request: Bug 1196047 - Move tern to client/sourceeditor. r=bgrins

https://reviewboard.mozilla.org/r/21525/#review19401

::: devtools/client/sourceeditor/tern/tests/unit/test_import_tern.js:9
(Diff revision 1)
> -  const tern = require("tern/tern");
> +  debugger;

Remove extra debugger statement

::: devtools/client/sourceeditor/tern/tests/unit/xpcshell.ini:5
(Diff revision 1)
> -skip-if = toolkit == 'android' || toolkit == 'gonk'
> +firefox-appdir = browser

Are the skip-if's meant to be removed here?

Comment 14

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8671087 [details]
MozReview Request: Bug 1196047 - Move most of shared/performance to client or server. r=jsantell

https://reviewboard.mozilla.org/r/21521/#review19395

Lots of notes -- I don't think this'll work when debugging Gecko < 43 for the reasons below. If you have any questions or want me to do any moving, let me know! These modules are a mess.

::: devtools/server/performance/moz.build:8
(Diff revision 1)
> +    'legacy',

Legacy is exclusively on the client. We should move this entire directory to devtools/client/performance/legacy I think.

::: devtools/server/performance/moz.build:14
(Diff revision 1)
> +    'recording-common.js',

recording-common is used to decorate both Actor and Front for PerformanceRecording, as well as legacy Recordings -- so this should probably be shared then, yeah?

::: devtools/shared/performance/moz.build
(Diff revision 1)
> -  'io.js',

io.js manages the importing/exporting of profile recordings -- meaning this should never be on the server as it is accessing the file system. Both legacy and performance actor style use this on the client. I don't believe there are any uses that are not on the client.

::: devtools/shared/performance/moz.build:11
(Diff revision 1)
> -  'utils.js',
> +    'utils.js',

I *think* utils is only on the client, it mostly (entirely?) massages recording data (inflates profile, normalizes timing, etc). This could also live on the client (and renamed to recording-data.js)

Comment 15

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8671088 [details]
MozReview Request: Bug 1196047 - Move shared/shared perf files to server/performance. r=jsantell

https://reviewboard.mozilla.org/r/21523/#review19407

Comment 16

[Jordan Santell [:jsantell] [@jsantell]]

https://reviewboard.mozilla.org/r/21521/#review19409

Comment 17

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/21515/#review19397

> Don't think this is needed. I see skip-if = toolkit == 'android' || toolkit == 'gonk' at the top of the xpcshell.ini file here

Okay, removed.

Comment 18

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/21519/#review19393

If you mean `devtools/shared/worker` would resolve to `devtools/shared/worker/index.js`, it looks like that requires setting `isNative: true` on the loader, which DevTools Loader.jsm does not do currently.  Filed bug 1213028 to explore it.

Comment 19

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/21525/#review19401

> Remove extra debugger statement

Thanks for catching that!

> Are the skip-if's meant to be removed here?

Yes, I removed them because they should be redundant: files in /devtools/client aren't shipped to those platforms, so these tests should never be run there either.  I'll put it back if try gets mad.

Comment 20

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/21519/#review19393

Okay, I've moved them all under `devtools/shared/worker/*`.

Comment 21

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/21521/#review19395

As discussed on IRC, it should be fine for <43 since the client app (desktop Firefox) has to ship all 3 DevTools directories, mainly because fronts are usually in `server` with their actors.

> Legacy is exclusively on the client. We should move this entire directory to devtools/client/performance/legacy I think.

Okay, I see what you mean.  It's lazily referenced from the actor file in `server`, but only ever actually loaded by the client app, so I'll move it to `client`.

> recording-common is used to decorate both Actor and Front for PerformanceRecording, as well as legacy Recordings -- so this should probably be shared then, yeah?

Now that legacy is in `client`, yes, it's split across.  I'll move it back to shared.

> io.js manages the importing/exporting of profile recordings -- meaning this should never be on the server as it is accessing the file system. Both legacy and performance actor style use this on the client. I don't believe there are any uses that are not on the client.

You're right, it's lazily referenced from actor files, but only used in fronts.  I'll move to `client`.

> I *think* utils is only on the client, it mostly (entirely?) massages recording data (inflates profile, normalizes timing, etc). This could also live on the client (and renamed to recording-data.js)

It's actually used on both client and server, as in[1].  I'll leave it in `shared`, but I'll rename to `recording-utils.js`, since you usually import it as `RecordingUtils`.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/performance-recording.js#141

Comment 22

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8671083 [details]
MozReview Request: Bug 1196047 - Move system.js to devtools/shared. r=jsantell

Bug 1196047 - Move system.js to devtools/shared. r=jsantell

Comment 23

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8671084 [details]
MozReview Request: Bug 1196047 - Move indentation.js to devtools/shared. r=bgrins

Bug 1196047 - Move indentation.js to devtools/shared. r=bgrins

Comment 24

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8671085 [details]
MozReview Request: Bug 1196047 - Move async-storage.js to devtools/shared. r=bgrins

Bug 1196047 - Move async-storage.js to devtools/shared. r=bgrins

Comment 25

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8671086 [details]
MozReview Request: Bug 1196047 - Move worker*.js to devtools/shared. r=jsantell

Bug 1196047 - Move worker*.js to devtools/shared. r=jsantell

Comment 26

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8671087 [details]
MozReview Request: Bug 1196047 - Move most of shared/performance to client or server. r=jsantell

Bug 1196047 - Move most of shared/performance to client or server. r=jsantell

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8671088 [details]
MozReview Request: Bug 1196047 - Move shared/shared perf files to server/performance. r=jsantell

Bug 1196047 - Move shared/shared perf files to server/performance. r=jsantell

Comment 28

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8671089 [details]
MozReview Request: Bug 1196047 - Move tern to client/sourceeditor. r=bgrins

Bug 1196047 - Move tern to client/sourceeditor. r=bgrins

Comment 29

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2701a871532

Comment 30

[Jordan Santell [:jsantell] [@jsantell]]

Comment on attachment 8671087 [details]
MozReview Request: Bug 1196047 - Move most of shared/performance to client or server. r=jsantell

https://reviewboard.mozilla.org/r/21521/#review19449

I think there's something up with the legacy actor tests (browser_perf-legacy-front-0*.js, 01, 02, 03), they are renamed to 07, 08, 09, removed from browser.ini, and looks like some other changes in the tests itself that will cause them to fail. R+ with those tests moved back to 01, 02, 03, and readded in browser.ini (and ensure the removal of other requires in those tests are intentional)

Comment 31

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/d3f501cfed0a

Comment 32

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Pulsebot from comment #31)
> https://hg.mozilla.org/integration/fx-team/rev/d3f501cfed0a

Ugh, wrong bug number.  Meant for bug 1212653.

Comment 33

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/e155afb82d7c

Comment 34

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://reviewboard.mozilla.org/r/21521/#review19449

We discussed on IRC that I had moved the tests to client, and there were existing tests with the same name, so I re-numbered them.

The test changes are because of different head files between the test directories.

Comment 35

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/51d1e143211d
https://hg.mozilla.org/integration/fx-team/rev/9b428761f642
https://hg.mozilla.org/integration/fx-team/rev/ecb8cb5ce0eb
https://hg.mozilla.org/integration/fx-team/rev/9003dfe56149
https://hg.mozilla.org/integration/fx-team/rev/4d1f1902c410
https://hg.mozilla.org/integration/fx-team/rev/c53c4223a7a0
https://hg.mozilla.org/integration/fx-team/rev/436ccd961337

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51d1e143211d
https://hg.mozilla.org/mozilla-central/rev/9b428761f642
https://hg.mozilla.org/mozilla-central/rev/ecb8cb5ce0eb
https://hg.mozilla.org/mozilla-central/rev/9003dfe56149
https://hg.mozilla.org/mozilla-central/rev/4d1f1902c410
https://hg.mozilla.org/mozilla-central/rev/c53c4223a7a0
https://hg.mozilla.org/mozilla-central/rev/436ccd961337