1466479 - Remove DAMP frontend

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement)

Description

[Alexandre Poirot [:ochameau]]

DAMP currently implements a frontend that noone use, at least within DevTools team.
We should get rid of it as DAMP is meant to be fully controlled from the command line.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1466479 - Cleanup damp from its unused frontend.

Review commit: https://reviewboard.mozilla.org/r/248856/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/248856/

Comment 2

[Julian Descottes [:jdescottes]]

Comment on attachment 8983005 [details]
Bug 1466479 - Cleanup damp from its unused frontend.

https://reviewboard.mozilla.org/r/248856/#review255380

Thanks for the cleanup Alex!

::: testing/talos/talos/tests/devtools/addon/content/addon-test-frontend.js:31
(Diff revision 1)
> -    }
> -  }
> -}
> -
>  function init() {
> -  updateOptionsFromUrl();
> +  runTest();

We are no longer passing "config, doneCallback" to runTest() anywhere so we should update the signature to reflect that. 

Maybe we could just have a single method here? init/runTest/chromeExec just call each other with no added value for each layer. And maybe inline it in damp.html?

Finally since we no longer need the #auto parameter to start the tests automatically, we should update https://searchfox.org/mozilla-central/source/testing/talos/talos/tests/devtools/damp.manifest#1

::: testing/talos/talos/tests/devtools/addon/content/initialize_browser.js:10
(Diff revision 1)
>    const PREFIX = "[e-mail]:";
>  
>    // "services" which the framescript can execute at the chrome process
>    var proxiedServices = {
>      runTest(config, callback) {
>        (new win.Damp()).startTest(callback, config);

startTest no longer takes a config parameter so we should update this call. We can simplify all the way to the `service(comand.data, sendResult);` call at L32.

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1466479 - Remove unused Profiler module.

Review commit: https://reviewboard.mozilla.org/r/250070/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250070/

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1466479 - Remove useless communication between damp.html and talos add-on.

Review commit: https://reviewboard.mozilla.org/r/250072/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250072/

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983005 [details]
Bug 1466479 - Cleanup damp from its unused frontend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248856/diff/1-2/

Comment 6

[Alexandre Poirot [:ochameau]]

I've removed even more things, like the 3 things you mentioned during the review, and a bit more.

Comment 7

[Julian Descottes [:jdescottes]]

Comment on attachment 8984264 [details]
Bug 1466479 - Remove unused Profiler module.

https://reviewboard.mozilla.org/r/250070/#review256400

Thanks! Some comments to update https://searchfox.org/mozilla-central/search?q=tests%2Fdevtools%2Faddon%2Fcontent%2FProfiler.js&case=true&regexp=false&path=

Comment 8

[Julian Descottes [:jdescottes]]

Comment on attachment 8984265 [details]
Bug 1466479 - Remove useless communication between damp.html and talos add-on.

https://reviewboard.mozilla.org/r/250072/#review256510

Looks good!

Not related to your patch, but since you are cleaning up: 
we can remove two XPCOMUtils.defineLazyGetter in damp.js, "gDevTools" and "TargetFactory" are not used in this file anymore.

::: testing/talos/talos/tests/devtools/addon/content/initialize_browser.js:11
(Diff revision 1)
> -  var groupMM = win.getGroupMessageManager("browsers");
>    groupMM.loadFrameScript("chrome://damp/content/framescript.js", true);
>  
> -  // listener/executor on the chrome process for damp.html
> -  groupMM.addMessageListener(PREFIX + "chrome-exec-message", function listener(m) {
> -    function sendResult(result) {
> +  // listen for damp.html load event before starting tests
> +  groupMM.addMessageListener(MESSAGE, function onMessage(m) {
> +    groupMM.addMessageListener(MESSAGE, onMessage);

is this supposed to be removeMessageListener?

Comment 9

[Alexandre Poirot [:ochameau]]

Comment on attachment 8983005 [details]
Bug 1466479 - Cleanup damp from its unused frontend.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/248856/diff/2-3/

Comment 10

[Alexandre Poirot [:ochameau]]

Comment on attachment 8984264 [details]
Bug 1466479 - Remove unused Profiler module.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250070/diff/1-2/

Comment 11

[Alexandre Poirot [:ochameau]]

Comment on attachment 8984265 [details]
Bug 1466479 - Remove useless communication between damp.html and talos add-on.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250072/diff/1-2/

Comment 12

[Alexandre Poirot [:ochameau]]

Comment on attachment 8984265 [details]
Bug 1466479 - Remove useless communication between damp.html and talos add-on.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250072/diff/2-3/

Comment 13

[Alexandre Poirot [:ochameau]]

(In reply to Julian Descottes [:jdescottes][:julian] from comment #8)
> Not related to your patch, but since you are cleaning up: 
> we can remove two XPCOMUtils.defineLazyGetter in damp.js, "gDevTools" and
> "TargetFactory" are not used in this file anymore.

I removed them.

> ::: testing/talos/talos/tests/devtools/addon/content/initialize_browser.js:11
> (Diff revision 1)
> > -  var groupMM = win.getGroupMessageManager("browsers");
> >    groupMM.loadFrameScript("chrome://damp/content/framescript.js", true);
> >  
> > -  // listener/executor on the chrome process for damp.html
> > -  groupMM.addMessageListener(PREFIX + "chrome-exec-message", function listener(m) {
> > -    function sendResult(result) {
> > +  // listen for damp.html load event before starting tests
> > +  groupMM.addMessageListener(MESSAGE, function onMessage(m) {
> > +    groupMM.addMessageListener(MESSAGE, onMessage);
> 
> is this supposed to be removeMessageListener?

Actually, I should keep the listener for supporting damp.html reloads (related to tppagecycles parameter).
I fixed that in the last revision.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2fff030b0ffc
Cleanup damp from its unused frontend. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/7b9f7eb7a0a9
Remove unused Profiler module. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/9a5cfa961ce3
Remove useless communication between damp.html and talos add-on. r=jdescottes

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/2fff030b0ffc
https://hg.mozilla.org/mozilla-central/rev/7b9f7eb7a0a9
https://hg.mozilla.org/mozilla-central/rev/9a5cfa961ce3