1025044 - CSS coverage actor should have started and stopped events

Status: RESOLVED FIXED

(DevTools :: Style Editor, defect)

Description

[Joe Walker [:jwalker]]

Useful to keep the UI in sync with what the server is doing, and it will enable us to get rid of the ugly _testOnly_isRunning function.

Comment 1

[Joe Walker [:jwalker]]

Attachment: 0004-Bug-1025044-CSS-coverage-actor-should-have-started-a.patch

Comment 2

[Heather Arthur [:harth]]

Comment on attachment 8443547 [details] [diff] [review]
0004-Bug-1025044-CSS-coverage-actor-should-have-started-a.patch

Review of attachment 8443547 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, but I'm confused about the API changes here. If I'm reading right there's no more stop()? Is this backwards compatible?

::: toolkit/devtools/server/actors/csscoverage.js
@@ -134,5 @@
> -    return this._running ?
> -        this.stop().then(() => false) :
> -        this.start().then(() => true);
> -  }, {
> -    response: RetVal("boolean"),

You took this out, do we not need to specify the return type anymore?

Comment 3

[Joe Walker [:jwalker]]

(In reply to Heather Arthur [:harth] from comment #2)
> Comment on attachment 8443547 [details] [diff] [review]
> 0004-Bug-1025044-CSS-coverage-actor-should-have-started-a.patch
> 
> Review of attachment 8443547 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, but I'm confused about the API changes here. If I'm reading
> right there's no more stop()? Is this backwards compatible?
> 
> ::: toolkit/devtools/server/actors/csscoverage.js
> @@ -134,5 @@
> > -    return this._running ?
> > -        this.stop().then(() => false) :
> > -        this.start().then(() => true);
> > -  }, {
> > -    response: RetVal("boolean"),
> 
> You took this out, do we not need to specify the return type anymore?

No, not now that we track the running state in the client.

Comment 4

[Joe Walker [:jwalker]]

(In reply to Joe Walker [:jwalker] from comment #3)
> (In reply to Heather Arthur [:harth] from comment #2)
> > You took this out, do we not need to specify the return type anymore?
>
> No, not now that we track the running state in the client.

Which reminds me, I need to fix that test at the same time. I'll remove usage._testOnly_isRunning and replace it with usage.isRunning instead.

Comment 5

[Joe Walker [:jwalker]]

Attachment: 0004-Bug-1025044-CSS-coverage-actor-should-have-started-a.patch

Comment 6

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=d8d6fce40dbd

Comment 7

[Heather Arthur [:harth]]

Still wondering about the API changes here. So there's no more stop(), just a toggle()?

Comment 8

[Joe Walker [:jwalker]]

(In reply to Heather Arthur [:harth] from comment #7)
> Still wondering about the API changes here. So there's no more stop(), just
> a toggle()?

stop/start/toggle all still exist and are used by the commands "csscoverage [start|stop|toggle]".

From a UI POV, the proposed start and stop buttons in bug 1016288 uses start/stop, and "csscoverage toggle" means that someone could add the button to the toolbox toolbar if they wanted, which has been useful in sorting out bug 1028252.

Comment 9

[Heather Arthur [:harth]]

Comment on attachment 8445029 [details] [diff] [review]
0004-Bug-1025044-CSS-coverage-actor-should-have-started-a.patch

Review of attachment 8445029 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/devtools/server/actors/csscoverage.js
@@ +698,5 @@
>  /**
> + * Running more than one usage report at a time is probably bad for performance
> + * and it isn't particularly useful, and it's confusing from a notification POV
> + * so we only allow one.
> + */

Yeah, this is too bad. In the future we might want to make it per-tab once we have a UI in the style editor.

Comment 10

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=f6cee8c74faf
https://hg.mozilla.org/integration/fx-team/rev/70ff1b3ae551

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/70ff1b3ae551