1604539 - Move all devtools/shared/fronts files to devtools/client

Status: RESOLVED FIXED

(DevTools :: Framework, task, P3)

Description

[Julian Descottes [:jdescottes]]

DevTools Fronts are the client side representation of our actors. They are only loaded on the client. I think moving them to devtools/client would make the architecture easier to understand:

• devtools/server: actors
• devtools/shared: specs
• devtools/client: fronts

Currently, outside of tests, fronts are only required by client code, so this should be pretty easy to do if we decide to go for it.

This came up while adding an eslint rule against loading client files from devtools/shared files in Bug 1604485

Comment 1

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes] from comment #0)

DevTools Fronts are the client side representation of our actors. They are only loaded on the client. I think moving them to devtools/client would make the architecture easier to understand:

• devtools/server: actors
• devtools/shared: specs
• devtools/client: fronts

This layout matches quite well the definition of these three folders:

• server contains the necesary modules to spawn a RDP server on any runtime (firefox desktop, fenix, geckoview, thunderbird, ...). Only backend, no UI.
• client contains the DevTools frontend, so the modules which allow to implement to UI of DevTools, but doesn't include any server part. Only UI, no backend.
• shared contains the random modules which are meaninful to both. I think the word "random" is relevant here, as it is hard to better define what we would find. But I think a good alternative description would be: any module we could share between backend and frontend which would prevent duplicating code.

I imagine it is important to also callout why we have distinct folders and why we have issues when we load client things from shared or server.
We only ship server and client on mobile runtimes, which don't expose any frontend. It is the case for fenix and geckoview.

Given this description, specs are clearly the only thing that is required by both frontend (fronts) and backend (actors). And fronts are only relevant for the frontend, while actors are only relevant for the backend.

So. +1!

Do you think the existing doc translate this well enough?
https://searchfox.org/mozilla-central/source/devtools/docs/files/README.md
And, would it be relevant to have this description in duplicated in a /devtools/README.md file?

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 3

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #1)

Do you think the existing doc translate this well enough?
https://searchfox.org/mozilla-central/source/devtools/docs/files/README.md
And, would it be relevant to have this description in duplicated in a /devtools/README.md file?

I think the current documentation still works after moving the files!
But maybe we could add a mention of devtools/shared/specs, devtools/client/fronts and devtools/server/actors in each section. I'll try to update the file!

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1604539 - Move devtools/shared/fronts to devtools/client

This changeset is the result of running hg mv devtools/shared/fronts/ devtools/client
The necessary adaptations (renaming and moz.build changes) have been split in other patches for the review.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1604539 - Rename shared/fronts to client/fronts in the devtools codebase

Depends on D67603
This patch is an automated string replace of shared/fronts/ to client/fronts/ in devtools.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1604539 - Update moz.build in devtools/shared and devtools/client

Depends on D67604

Moving the fronts folder effectively in the corresponding moz.build files

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1604539 - Update eslint reject-some-requires "devtools/client" configuration

Depends on D67605

The eslint rule prevent devtools/client requires can now target all shared/** files
We also exclude devtools-client specifically, since it imports the object-front despite being in devtools/shared.
This file is always used from devtools/client at runtime but is used in many shared & server test suites

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1604539 - Remove unused import of ObjectFront in devtools/shared/webconsole

Depends on D67606
Didn't find any call site for this import, hopefully we can just remove it.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/866d6794bbba
Move devtools/shared/fronts to devtools/client r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/b4b54335ee2d
Rename shared/fronts to client/fronts in the devtools codebase r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/da94f7d503a1
Update moz.build in devtools/shared and devtools/client r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/fb410a1bf9aa
Update eslint reject-some-requires "devtools/client" configuration r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/8455e50002a9
Remove unused import of ObjectFront in devtools/shared/webconsole r=nchevobbe

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 5 changesets (bug 1604539) for xpc failures complanining about test_encryption.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&collapsedPushes=664605%2C664683&searchStr=xpcshell&fromchange=1c9eb89172abbf4668dd27026053bc629fa90c15&tochange=0c1d2bd9ef68edd1308bd1d235b97f36f2f85581&selectedJob=294051483

Backout link: https://hg.mozilla.org/integration/autoland/rev/0c1d2bd9ef68edd1308bd1d235b97f36f2f85581

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=294051483&repo=autoland&lineNumber=1389

[task 2020-03-20T13:20:47.336Z] 13:20:47     INFO -  TEST-START | devtools/shared/security/tests/xpcshell/test_encryption.js
[task 2020-03-20T13:25:47.336Z] 13:25:47  WARNING -  TEST-UNEXPECTED-TIMEOUT | devtools/shared/security/tests/xpcshell/test_encryption.js | Test timed out
[task 2020-03-20T13:25:47.336Z] 13:25:47     INFO -  TEST-INFO took 300001ms
[task 2020-03-20T13:25:47.742Z] 13:25:47     INFO -  xpcshell return code: 143
[task 2020-03-20T13:25:47.841Z] 13:25:47  WARNING -  TEST-UNEXPECTED-FAIL | Received SIGINT (control-C), so stopped run. (Use --keep-going to keep running tests after killing one with SIGINT)
[task 2020-03-20T13:25:47.841Z] 13:25:47     INFO -  INFO | Result summary:
[task 2020-03-20T13:25:47.841Z] 13:25:47     INFO -  INFO | Passed: 15
[task 2020-03-20T13:25:47.841Z] 13:25:47  WARNING -  INFO | Failed: 1
[task 2020-03-20T13:25:47.841Z] 13:25:47  WARNING -  One or more unittests failed.
[task 2020-03-20T13:25:47.841Z] 13:25:47     INFO -  INFO | Todo: 0
[task 2020-03-20T13:25:47.841Z] 13:25:47     INFO -  INFO | Retried: 0
[task 2020-03-20T13:25:47.841Z] 13:25:47     INFO -  SUITE-END | took 356s
[task 2020-03-20T13:25:47.841Z] 13:25:47     INFO -  Node moz-http2 server shutting down ...
[task 2020-03-20T13:25:47.866Z] 13:25:47    ERROR - Return code: 1
[task 2020-03-20T13:25:47.866Z] 13:25:47     INFO - TinderboxPrint: xpcshell<br/>15/<em class="testfail">1</em>/0
[task 2020-03-20T13:25:47.866Z] 13:25:47     INFO - ##### xpcshell log ends

Comment 11

[Julian Descottes [:jdescottes]]

Ah, the xpcshell tests also run on Android, so I guess the import from devtools-client is an issue here. Looking!

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: Bug 1604539 - Skip xpcshell test using devtools client on android

Depends on D67607

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d93a6d34969
Move devtools/shared/fronts to devtools/client r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/58ee4a2de91a
Rename shared/fronts to client/fronts in the devtools codebase r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/9da268514609
Update moz.build in devtools/shared and devtools/client r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/e065206985aa
Update eslint reject-some-requires "devtools/client" configuration r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/d2a7418ee5e3
Remove unused import of ObjectFront in devtools/shared/webconsole r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/05a5f1af4e40
Skip xpcshell test using devtools client on android r=nchevobbe

Comment 14

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/0d93a6d34969
https://hg.mozilla.org/mozilla-central/rev/58ee4a2de91a
https://hg.mozilla.org/mozilla-central/rev/9da268514609
https://hg.mozilla.org/mozilla-central/rev/e065206985aa
https://hg.mozilla.org/mozilla-central/rev/d2a7418ee5e3
https://hg.mozilla.org/mozilla-central/rev/05a5f1af4e40