Closed Bug 1466479 Opened 6 years ago Closed 6 years ago

Remove DAMP frontend

Categories

(DevTools :: Framework, enhancement)

Product:
DevTools
Component:
Framework
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(firefox62 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 62
Tracking Flags:
Tracking Status
firefox62 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

Bug 1466479 - Cleanup damp from its unused frontend.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1466479 - Remove unused Profiler module.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details
Bug 1466479 - Remove useless communication between damp.html and talos add-on.
59 bytes, text/x-review-board-request
jdescottes
: review+
Details 
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.
Assignee: nobody → poirot.alex
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 = "damp@mozilla.org:";
>  
>    // "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.
Attachment #8983005 - Flags: review?(jdescottes) → review+
I've removed even more things, like the 3 things you mentioned during the review, and a bit more.
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?
Attachment #8984265 - Flags: review?(jdescottes) → review+
Product: Firefox → DevTools
(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.
Pushed by apoirot@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: