CSS coverage actor should have started and stopped events

RESOLVED FIXED in Firefox 33

Status

RESOLVED FIXED
5 years ago
6 months ago

People

(Reporter: jwalker, Assigned: jwalker)

Tracking

(Blocks: 2 bugs)

Trunk
Firefox 33
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Created attachment 8443547 [details] [diff] [review]
0004-Bug-1025044-CSS-coverage-actor-should-have-started-a.patch
Assignee: nobody → jwalker
Status: NEW → ASSIGNED
Attachment #8443547 - Flags: review?(fayearthur)
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?
(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.
(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.
Created attachment 8445029 [details] [diff] [review]
0004-Bug-1025044-CSS-coverage-actor-should-have-started-a.patch
Attachment #8443547 - Attachment is obsolete: true
Attachment #8443547 - Flags: review?(fayearthur)
Attachment #8445029 - Flags: review?(fayearthur)
Still wondering about the API changes here. So there's no more stop(), just a toggle()?
Flags: needinfo?(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.
Flags: needinfo?(jwalker)
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.
Attachment #8445029 - Flags: review?(fayearthur) → review+
https://hg.mozilla.org/mozilla-central/rev/70ff1b3ae551
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33

Updated

6 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.