Closed Bug 1446222 Opened 5 years ago Closed 5 years ago

Clean up DevTools server dir

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: jryans, Assigned: jryans)

References

(Blocks 1 open bug)

Details

Attachments

(8 files)

Bug 1446222 - Move WebGL primitives to Canvas actor dir.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1446222 - Move event parsers to Inspector actor dir.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1446222 - Move CssLogic to Inspector actor dir.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1446222 - Move websocket-server.js to socket dir.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1446222 - Move DevTools frame startup to new dir.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1446222 - Move DevTools content process startup to new dir.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1446222 - Move DevTools service worker control script to worker actor dir.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1446222 - Move DevTools worker startup to new dir.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details 
The DevTools server root dir is a bit of a jumble of unrelated modules.  Let's organize it better and move things where they are used.
Comment on attachment 8960423 [details]
Bug 1446222 - Move event parsers to Inspector actor dir.

https://reviewboard.mozilla.org/r/229196/#review235010
Attachment #8960423 - Flags: review?(jdescottes) → review+
Comment on attachment 8960424 [details]
Bug 1446222 - Move CssLogic to Inspector actor dir.

https://reviewboard.mozilla.org/r/229198/#review235012
Attachment #8960424 - Flags: review?(jdescottes) → review+
Comment on attachment 8960425 [details]
Bug 1446222 - Move websocket-server.js to socket dir.

https://reviewboard.mozilla.org/r/229200/#review235014

Should we also move devtools/server/tests/mochitest/test_websocket-server.html to devtools/server/socket/tests/test_websocket-server.html?
Attachment #8960425 - Flags: review?(jdescottes) → review+
Comment on attachment 8960427 [details]
Bug 1446222 - Move DevTools content process startup to new dir.

https://reviewboard.mozilla.org/r/229204/#review235018

::: devtools/server/main.js:750
(Diff revision 1)
>          // Load the process script that will receive the debug:init-content-server message
> -        Services.ppmm.loadProcessScript(CONTENT_PROCESS_DBG_SERVER_SCRIPT, true);
> -        this._contentProcessScriptLoaded = true;
> +        Services.ppmm.loadProcessScript(CONTENT_PROCESS_SERVER_STARTUP_SCRIPT, true);
> +        this._contentProcessServerStartupScriptLoaded = true;
>        }
>  
>        // Send a message to the content process debugger server script to forward it the

nit: content process debugger server script -> content process server startup script

::: devtools/server/startup/content-process.js:1
(Diff revision 1)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Could this be a move of content-process-debugger-server.js in order to preserve history?
Attachment #8960427 - Flags: review?(jdescottes) → review+
Comment on attachment 8960428 [details]
Bug 1446222 - Move DevTools service worker control script to worker actor dir.

https://reviewboard.mozilla.org/r/229206/#review235020

::: commit-message-4eed4:4
(Diff revision 1)
> +Bug 1446222 - Move DevTools service worker control script to worker actor dir. r=jdescottes
> +
> +The `service-worker-child.js` process script is used by actors to control
> +service workers as needed.  This moves it to helper directory new the actors

nit: new the actors -> next to the actors (?)
Attachment #8960428 - Flags: review?(jdescottes) → review+
Comment on attachment 8960429 [details]
Bug 1446222 - Move DevTools worker startup to new dir.

https://reviewboard.mozilla.org/r/229208/#review235022

::: commit-message-959ea:6
(Diff revision 1)
> +Bug 1446222 - Move DevTools worker startup to new dir. r=jdescottes
> +
> +Move and rename the server's worker debugger script that starts DevTools for a
> +worker from `worker.js` to `startup/worker.js`.
> +
> +These code paths will like change more as Site Isolation work continues, but for

nit: like -> likely
Attachment #8960429 - Flags: review?(jdescottes) → review+
Comment on attachment 8960422 [details]
Bug 1446222 - Move WebGL primitives to Canvas actor dir.

https://reviewboard.mozilla.org/r/229194/#review235024

::: devtools/server/actors/canvas.js:12
(Diff revision 1)
>  
>  const defer = require("devtools/shared/defer");
>  const protocol = require("devtools/shared/protocol");
>  const {CallWatcherActor} = require("devtools/server/actors/call-watcher");
>  const {CallWatcherFront} = require("devtools/shared/fronts/call-watcher");
> -const {WebGLPrimitiveCounter} = require("devtools/server/primitive");
> +const {WebGLPrimitiveCounter} = require("./canvas/primitive");

Could we use an absolute "/devtools/..." path here to be consistent?
Attachment #8960422 - Flags: review?(jdescottes) → review+
Comment on attachment 8960426 [details]
Bug 1446222 - Move DevTools frame startup to new dir.

https://reviewboard.mozilla.org/r/229202/#review235028

::: devtools/server/main.js:992
(Diff revision 1)
> +   * an active connection.
>     *
>     * @param object connection
>     *        The debugger server connection to use.
>     * @param nsIDOMElement frame
>     *        The browser element that holds the child process.

This JSDoc comment could be confusing now, maybe rephrase to something like "The frame element to connect to" ?
Attachment #8960426 - Flags: review?(jdescottes) → review+
Comment on attachment 8960422 [details]
Bug 1446222 - Move WebGL primitives to Canvas actor dir.

https://reviewboard.mozilla.org/r/229194/#review235024

> Could we use an absolute "/devtools/..." path here to be consistent?

Sure!  (I never can decide which I prefer...)
Comment on attachment 8960425 [details]
Bug 1446222 - Move websocket-server.js to socket dir.

https://reviewboard.mozilla.org/r/229200/#review235014

Makes sense to me, I'll move it.
Comment on attachment 8960428 [details]
Bug 1446222 - Move DevTools service worker control script to worker actor dir.

https://reviewboard.mozilla.org/r/229206/#review235020

> nit: new the actors -> next to the actors (?)

Thanks, fixed!
Comment on attachment 8960429 [details]
Bug 1446222 - Move DevTools worker startup to new dir.

https://reviewboard.mozilla.org/r/229208/#review235022

> nit: like -> likely

Thanks, fixed.  (Same error in previous messages too...)
Comment on attachment 8960426 [details]
Bug 1446222 - Move DevTools frame startup to new dir.

https://reviewboard.mozilla.org/r/229202/#review235028

> This JSDoc comment could be confusing now, maybe rephrase to something like "The frame element to connect to" ?

Good catch, I'll reword it...  (A lot of the terminology in this area needs some cleaning, should happen down the road.)
Comment on attachment 8960427 [details]
Bug 1446222 - Move DevTools content process startup to new dir.

https://reviewboard.mozilla.org/r/229204/#review235018

> nit: content process debugger server script -> content process server startup script

Thanks, fixed.

> Could this be a move of content-process-debugger-server.js in order to preserve history?

Ah sure, I'll fiddle with things to make it happen.
Pushed by jryans@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ad457a385c5b
Move WebGL primitives to Canvas actor dir. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/1ecd436ad73e
Move event parsers to Inspector actor dir. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/f5836adc9542
Move CssLogic to Inspector actor dir. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/d0ae3543e9e5
Move websocket-server.js to socket dir. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/8e69d0a1591a
Move DevTools frame startup to new dir. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/a6269e7bc700
Move DevTools content process startup to new dir. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/899306c2e0a3
Move DevTools service worker control script to worker actor dir. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/36b4eaecc56d
Move DevTools worker startup to new dir. r=jdescottes
Blocks: 1447641
No longer blocks: 1447641
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.