Push API: Replace buggy FutureData test helper with ResultQueue

chrome/test/data/push_messaging/push_test.js

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 'use strict';
 
-var pushData = new FutureData();
+var resultQueue = new ResultQueue();

[Peter Beverloo, 2015/01/13 16:53:38]

nit: since this is Chromium, variables should be named "result_queue". While
you're just updating this one, you're also adding pendingGets and it'd be nice
to get this fixed.

[johnme, 2015/01/13 18:03:04]

Actually,
http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml?showone...
says variableNamesLikeThis and it is indeed 5 times more common in
chrome/test/data (excluding third_party)

 var pushRegistration = null;
 
 // Sends data back to the test. This must be in response to an earlier
 // request, but it's ok to respond asynchronously. The request blocks until
 // the response is sent.
 function sendResultToTest(result) {
   console.log('sendResultToTest: ' + result);
   if (window.domAutomationController) {
     domAutomationController.send('' + result);
   }
 }
 
 function sendErrorToTest(error) {
   sendResultToTest(error.name + ' - ' + error.message);
 }
 
-// A container for a single piece of data. The data does not have to be
-// available yet when the getter is called, as all responses to the test are
-// asynchronous.
-function FutureData() {
-  this.data = null;
-  this.waiting = false;
+// Queue storing asynchronous results received from the Service Worker. Results
+// are sent to the test when requested.
+function ResultQueue() {

[Michael van Ouwerkerk, 2015/01/13 11:14:51]

I don't mind a more powerful class here, but in the past Peter did [1], so
please get him to review this patch.

[1]
https://codereview.chromium.org/673783003/diff/40001/chrome/test/data/push_me...

[Peter Beverloo, 2015/01/13 16:53:38]

Thanks for checking!

I think that the additional complexity introduced by this patch is OK given that
we gain queue functionality. It takes an approach very similar to the FutureData
class you wrote, albeit with a queue, whereas AsyncData supported named data. I
do hope situations in which we'll be waiting for more than one message will be
rare -- otherwise we may need named data after all, in the event message B
arrives before message A, causing the items to be queued in the wrong order.

+  // Invariant: this.queue.length == 0 || this.pendingGets == 0
+  this.queue = [];
+  this.pendingGets = 0;
 }
 
-// Sends the data to the test if it is available. Otherwise sets the
-// |waiting| flag.
-FutureData.prototype.get = function() {
-  if (this.data) {
-    sendResultToTest(this.data);
+// Adds a data item to the queue. Will be sent to the test if there are
+// pendingGets.
+ResultQueue.prototype.push = function(data) {
+  if (this.pendingGets > 0) {
+    this.pendingGets--;
+    sendResultToTest(data);
   } else {
-    this.waiting = true;
+    this.queue.unshift(data);
   }
 };
 
-// Sets a new data value. If the |waiting| flag is on, it is turned off and
-// the data is sent to the test.
-FutureData.prototype.set = function(data) {
-  this.data = data;
-  if (this.waiting) {
-    sendResultToTest(data);
-    this.waiting = false;
+// Called by native. Sends the next data item to the test if it is available.
+// Otherwise increments pendingGets so it will be delivered when received.
+ResultQueue.prototype.pop = function() {
+  if (this.queue.length) {
+    sendResultToTest(this.queue.pop());
+  } else {
+    this.pendingGets++;
   }
 };
 
-FutureData.prototype.getImmediately = function() {
-  sendResultToTest(this.data);
+// Called by native. Immediately sends the next data item to the test if it is
+// available, otherwise sends null.
+ResultQueue.prototype.popImmediately = function() {
+  sendResultToTest(this.queue.length ? this.queue.pop() : null);
 };
 
 // Notification permission has been coalesced with Push permission. After
 // this is granted, Push API registration can succeed.
 function requestNotificationPermission() {
   Notification.requestPermission(function(permission) {
     sendResultToTest('permission status - ' + permission);
   });
 }
 
@@ skipping 61 matching lines @@
 
   pushRegistration.unregister().then(function(result) {
     sendResultToTest('unregister result: ' + result);
   }, function(error) {
     sendResultToTest('unregister error: ' + error.name + ': ' + error.message);
   });
 }
 
 addEventListener('message', function(event) {
   var message = JSON.parse(event.data);
-  if (message.type == 'push') {
-    pushData.set(message.data);
-  }
+  if (message.type == 'push')
+    resultQueue.push(message.data);
 }, false);