Closed Bug 1025044 Opened 6 years ago Closed 6 years ago

CSS coverage actor should have started and stopped events

Categories

(DevTools :: Style Editor, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: jwalker, Assigned: jwalker)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
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
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.