Closed Bug 1107686 Opened 5 years ago Closed 5 years ago

Remove redundant task check in DataReportingService


(Firefox Health Report Graveyard :: Client: Desktop, defect)

Not set


(Not tracked)

Firefox 37


(Reporter: gfritzsche, Assigned: gfritzsche)




(1 file, 1 obsolete file)

Attached patch Remove redundant check (obsolete) — Splinter Review
On spot-checking the client id code, i noticed that we have a redundant check there for the task check.
I'm not sure if this actually breaks anything, but its certainly unneeded.
Attachment #8532218 - Flags: review?(gps)
Comment on attachment 8532218 [details] [diff] [review]
Remove redundant check

Review of attachment 8532218 [details] [diff] [review]:

r+. But this removed check is defense in depth. The new code assumes there is exactly 1 caller of _loadClientID. Software always changes. That assumption may break some day. I think I'd feel better if _loadClientID were implemented as an inline function as part of getClientID.
Attachment #8532218 - Flags: review?(gps) → review+
If you're worried about robustness, i think we should rather make _loadClientID() use the pattern |this._task = Task.spawn(...); return this._task;|.
As is now, doing a task check there just seems broken (i don't think this can run properly).
Is this still fine with you then?
Attachment #8532218 - Attachment is obsolete: true
Attachment #8533125 - Flags: review?(gps)
Comment on attachment 8533125 [details] [diff] [review]
Remove redundant check

Review of attachment 8533125 [details] [diff] [review]:

Looks good. Thanks!
Attachment #8533125 - Flags: review?(gps) → review+
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
Product: Firefox Health Report → Firefox Health Report Graveyard
You need to log in before you can comment on or make changes to this bug.