Closed Bug 1107686 Opened 5 years ago Closed 5 years ago

Remove redundant task check in DataReportingService

Categories

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

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 37

People

(Reporter: gfritzsche, Assigned: gfritzsche)

References

Details

Attachments

(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+
https://hg.mozilla.org/mozilla-central/rev/7365b7551ee3
Status: NEW → RESOLVED
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.