Closed Bug 1194978 Opened 5 years ago Closed 5 years ago

Renable RequestSync tests in b2g

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: baku, Assigned: baku)

References

Details

Attachments

(1 file, 2 obsolete files)

No description provided.
Attached patch rs.patch (obsolete) — Splinter Review
Attachment #8648416 - Flags: review?(nsm.nikhil)
Comment on attachment 8648416 [details] [diff] [review]
rs.patch

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

::: dom/requestsync/RequestSyncService.jsm
@@ +75,5 @@
>  
>    _timers: {},
>    _pendingRequests: {},
>  
> +  _afterSchedulingTasks: [],

Can you add a comment explaining the use of this? Particularly, callbacks passed to executeAfterScheduling get immediately called if there is no scheduling happening currently. Which functions should be wrapped into executeAfterScheduling() if new code is added?

@@ +104,3 @@
>          }
>        }
> +    }.bind(this),

Nit: Fat arrow function?

@@ +130,4 @@
>      this.forEachRegistration(function(aObj) {
> +      let key = this.principalToKey(aObj.principal);
> +      this.removeRegistrationInternal(aObj.data.task, key);
> +    }.bind(this));

Nit: same here.

@@ +655,5 @@
> +
> +    this.scheduling = true;
> +
> +    this.createTimer(aObj, function() {
> +      this.scheduling = false;

Important bug: createTimer() calls this callback only if the alarm service successfully adds the timer. If that fails, createTimer() isn't passing on a error callback which must reset this.scheduling (and maybe call the _afterSchedulingTasks?)

@@ +785,2 @@
>      this.updateObjectInDB(this._activeTask, function() {
>        // SchedulerTimer creates a timer and a nsITimer cannot be cloned. This

Considering this uses AlarmService, could you update the comment if it is still relevant.
Attachment #8648416 - Flags: review?(nsm.nikhil)
Attached patch rs.patch (obsolete) — Splinter Review
Attachment #8648416 - Attachment is obsolete: true
Attachment #8649161 - Flags: review?(nsm.nikhil)
Comment on attachment 8649161 [details] [diff] [review]
rs.patch

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

::: dom/requestsync/RequestSyncService.jsm
@@ +76,5 @@
>    _timers: {},
>    _pendingRequests: {},
>  
> +  // This array contains functions to be executed after the scheduling of the
> +  // curren task or immediately if there are not scheduling in progress.

Nit: 'current' and s/are not/is no

@@ +651,5 @@
>        return;
>      }
>  
> +    if (this.scheduling) {
> +      dump("ERROR!! RequestSyncService - ScheduleTimer called into ScheduleTimer.\n");

Do you want to abort here?
Attachment #8649161 - Flags: review?(nsm.nikhil) → review+
Attached patch rs.patchSplinter Review
Attachment #8649161 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/7936d101d4f5
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Depends on: 1216002
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.