[ServiceWorker] Make Request class better conformance with the spec.

LayoutTests/http/tests/serviceworker/resources/request-worker.js

Index: LayoutTests/http/tests/serviceworker/resources/request-worker.js
diff --git a/LayoutTests/http/tests/serviceworker/resources/request-worker.js b/LayoutTests/http/tests/serviceworker/resources/request-worker.js
index 34629f9ecfebf2d884901bc20cad299bf7dfdc1b..d4d4b1b95cbb83172d0985ac433805cc7e9aa9cf 100644
--- a/LayoutTests/http/tests/serviceworker/resources/request-worker.js
+++ b/LayoutTests/http/tests/serviceworker/resources/request-worker.js
@@ -1,34 +1,249 @@
 importScripts('worker-test-helpers.js');
 
 test(function() {
-    var headers = new HeaderMap;
-    headers.set('User-Agent', 'Mozilla/5.0');
-    headers.set('Accept', 'text/html');
-    headers.set('X-ServiceWorker-Test', 'request test field');
-
-    var request = new Request({
-        url: 'https://www.example.com/test.html',
-        method: 'GET',
-        headers: headers
-    });
-
-    assert_equals(request.url, 'https://www.example.com/test.html', 'Request.url should match');
-    assert_equals(request.method, 'GET', 'Request.method should match');
-    assert_equals(request.origin, 'https://www.example.com', 'Request.origin should match');
-    assert_true(request.headers instanceof HeaderMap, 'Request.headers should be HeaderMap');
-    assert_equals(request.headers.size, 3, 'Request.headers.size should match');
-    // Note: detailed behavioral tests for HeaderMap are in another test,
-    // http/tests/serviceworker/headermap.html.
-
-    request.url = 'http://localhost/';
-    assert_equals(request.url, 'http://localhost/', 'Request.url should be writable');
-    request.url = 'http://localhost/\uD800'; // Unmatched lead surrogate.
-    assert_equals(request.url,
-                  'http://localhost/' + encodeURIComponent('\uFFFD'),
-                  'Request.url should have unmatched surrogates replaced.');
-    request.method = 'POST';
-    assert_equals(request.method, 'POST', 'Request.method should be writable');
-    assert_throws({name: 'TypeError'}, function() { request.method = 'invalid \u0100'; },
-                  'Request.method should throw on invalid ByteString');
+  var url = 'https://www.example.com/test.html';

[falken, 2014/07/03 10:05:32]

unless I'm mistaken, SW layout tests (and I think modern Blink layout tests) use
4 space indent

[horo, 2014/07/03 12:40:29]

Done.

+  var headers = new Headers;
+  headers.set('User-Agent', 'Mozilla/5.0');
+  headers.set('Accept', 'text/html');
+  headers.set('X-ServiceWorker-Test', 'request test field');
+
+  var request = new Request(url, {method: 'GET', headers: headers});
+
+  assert_equals(request.url, url, 'Request.url should match');
+  assert_equals(request.method, 'GET', 'Request.method should match');
+  assert_equals(request.referrer, location.href, 'Request.referrer should match');
+  assert_true(request.headers instanceof Headers, 'Request.headers should be Headers');
+
+  // 'User-Agent' is a forbidden header.
+  assert_equals(request.headers.size, 2, 'Request.headers.size should match');
+  // Note: detailed behavioral tests for Headers are in another test,
+  // http/tests/serviceworker/headers.html.
+
+  request.url = 'http://localhost/';
+  assert_equals(request.url, 'https://www.example.com/test.html', 'Request.url should be readonly');
+  request = new Request('http://localhost/\uD800'); // Unmatched lead surrogate.
+  assert_equals(request.url,
+                'http://localhost/' + encodeURIComponent('\uFFFD'),
+                'Request.url should have unmatched surrogates replaced.');
+  request.method = 'POST';
+  assert_equals(request.method, 'GET', 'Request.method should be readonly');
+
+  [new Request(url),
+   new Request(url, {method: ''}),

[falken, 2014/07/03 10:05:32]

is 'invalid method' also expected to resolve to the default?

[horo, 2014/07/03 12:40:29]

new Request(url, {method: 'invalid method'})
throws a TypeError because method can't have space.
Added test below.

+   new Request(url, {mode: ''}),
+   new Request(url, {mode: 'invalid mode'}),
+   new Request(url, {credentials: ''}),
+   new Request(url, {credentials: 'invalid credentials'})].forEach(function(request) {
+    assert_equals(request.url, url, 'Request.url should match');

[falken, 2014/07/03 10:05:32]

indent

[horo, 2014/07/03 12:40:29]

Done.

+    assert_equals(request.method, 'GET', 'Default Request.method should be GET');
+    assert_equals(request.mode, 'cors', 'Default Request.mode should be cors');
+    assert_equals(request.credentials, 'omit', 'Default Request.credentials should be omit');
+  });
+
+  request = new Request(url);

[falken, 2014/07/03 10:05:32]

this function is getting big and reuses variables throughout. would it be more
readable to split it up into helper functions like
TestPassingRequestToConstructor?

[horo, 2014/07/03 12:40:29]

Done.

+  request.headers.append('X-ServiceWorker-Foo', 'foo1');
+  request.headers.append('X-ServiceWorker-Foo', 'foo2');
+  request.headers.append('X-ServiceWorker-Bar', 'bar');
+  var request2 = new Request(request);
+  assert_equals(request2.url, url, 'Request.url should match');
+  assert_equals(request2.method, 'GET', 'Request.method should match');
+  assert_equals(request2.mode, 'cors', 'Request.mode should match');
+  assert_equals(request2.credentials, 'omit', 'Request.credentials should match');
+  assert_equals(request2.headers.getAll('X-ServiceWorker-Foo')[0], 'foo1',
+                'Request.headers should match');
+  assert_equals(request2.headers.getAll('X-ServiceWorker-Foo')[1], 'foo2',
+                'Request.headers should match');
+  assert_equals(request2.headers.getAll('X-ServiceWorker-Bar')[0], 'bar',
+                'Request.headers should match');
+
+  var kMethods = ['GET', 'HEAD', 'POST', 'PUT', 'DELETE', 'OPTIONS', '', undefined];
+  var kSimpleMethod = ['GET', 'HEAD', 'POST', '', undefined];
+  var kModes = ['same-origin', 'no-cors', 'cors', '', undefined];
+  var kCredentials = ['omit', 'same-origin', 'include', '', undefined];
+
+  kMethods.forEach(function(method) {
+    var isSimpleMethod = (kSimpleMethod.indexOf(method) != -1);
+    kModes.forEach(function(mode) {
+      var init = {};
+      if (method != undefined) { init['method'] = method; }
+      if (mode != undefined) { init['mode'] = mode; }
+      if (!isSimpleMethod && mode == 'no-cors') {
+        assert_throws(
+            {name:'TypeError'},
+            function() { var request = new Request(url, init); },
+            'new no-cors Request with non simple method (' + method +') should throw');
+        return;
+      }
+      request = new Request(url, init);
+      assert_equals(request.method, method ? method : 'GET', 'Request.method should match');
+      assert_equals(request.mode, mode ? mode : 'cors', 'Request.mode should match');
+      request = new Request(request);
+      assert_equals(request.method, method ? method : 'GET', 'Request.method should match');
+      assert_equals(request.mode, mode ? mode : 'cors', 'Request.mode should match');
+      kMethods.forEach(function(method2) {
+        var isSimpleMethod2 = (kSimpleMethod.indexOf(method2) != -1);
+        kModes.forEach(function(mode2) {
+          var init2 = {};
+          if (method2 != undefined) { init2['method'] = method2; }
+          if (mode2 != undefined) { init2['mode'] = mode2; }
+          if ((!isSimpleMethod2 && mode2 == 'no-cors') ||
+              (!isSimpleMethod && !method2 && mode2 == 'no-cors') ||
+              (!isSimpleMethod2 && mode == 'no-cors' && !mode2)) {

[falken, 2014/07/03 10:05:32]

i found this hard to read. could it be more readable with helper functions like
effectiveMethod(method1, method2) and effectiveMode(mode1, mode2)? Then it's
something like:

if (!isSimpleMethod(effectiveMethod(method1, method2)) && effectiveMode(mode1,
mode2) == 'no-cors') {
   ...
}

[horo, 2014/07/03 12:40:29]

Done.

+            assert_throws(
+                {name:'TypeError'},
+                function() { var request2 = new Request(request, init2); },
+                'new no-cors Request with non simple method should throw');
+            return;
+          }
+          request2 = new Request(request, init2);
+          assert_equals(request2.method,
+                        method2 ? method2 : request.method,
+                        'Request.method should be overridden');
+          assert_equals(request2.mode,
+                        mode2 ? mode2 : request.mode,
+                        'Request.mode should be overridden');
+        });
+      });
+    });
+  });
+
+  kCredentials.forEach(function(credentials) {

[falken, 2014/07/03 10:05:32]

yea I think it'd be nicer to split these in functions. kCredentials is defined
50 lines above and only used in this stanza

[horo, 2014/07/03 12:40:29]

Done.

+    var init = {};
+    if (credentials != undefined) { init['credentials'] = credentials; }
+    request = new Request(url, init);
+    assert_equals(request.credentials, credentials ? credentials : 'omit', 'Request.credentials should match');
+    request = new Request(request);
+    assert_equals(request.credentials, credentials ? credentials : 'omit', 'Request.credentials should match');
+    request = new Request(request);

[falken, 2014/07/03 10:05:32]

This line looks not needed? It's just the same as line 115

[horo, 2014/07/03 12:40:29]

deleted

+    kCredentials.forEach(function(credentials2) {
+      var init2 = {};
+      if (credentials2 != undefined) { init2['credentials'] = credentials2; }
+      request2 = new Request(request, init2);
+      assert_equals(request2.credentials,
+                    credentials2 ? credentials2 : request.credentials,
+                    'Request.credentials should be overridden');
+    });
+  });
+
+  var forbiddenMethods = ['TRACE', 'TRACK', 'CONNECT'];
+  var invalidNames = ['(', ')', '<', '>', '@', ',', ';', ':', '\\', '"',
+                      '/', '[', ']', '?', '=', '{', '}', '\u3042', 'a(b'];
+  forbiddenMethods.forEach(function(method) {
+    assert_throws({name:'TypeError'},
+                  function() { var request = new Request(url, {method: method}); },
+                  'new Request with a forbidden method (' + method +') should throw');
+  });
+  invalidNames.forEach(function(name) {
+    assert_throws({name:'TypeError'},
+                  function() { var request = new Request(url, {method: name}); },
+                  'new Request with an invalid method (' + name +') should throw');
+  });
+
+  ['GET', 'HEAD', 'POST', 'PUT', 'DELETE', 'OPTIONS'].forEach(function(method) {
+    request = new Request(url, {method: method.toLowerCase()});
+    assert_equals(request.method, method, 'Request.method should be case corrected');
+  });
+
+  var kForbiddenHeaders =
+      ['Accept-Charset', 'Accept-Encoding', 'Access-Control-Request-Headers',
+       'Access-Control-Request-Method', 'Connection', 'Content-Length', 'Cookie',
+       'Cookie2', 'Date', 'DNT', 'Expect', 'Host', 'Keep-Alive', 'Origin',
+       'Referer', 'TE', 'Trailer', 'Transfer-Encoding', 'Upgrade', 'User-Agent',
+       'Via', 'Proxy-', 'Sec-', 'Proxy-FooBar', 'Sec-FooBar'];
+  var kSimpleHeaders =
+      [['Accept', '*'], ['Accept-Language', 'ru'], ['Content-Language', 'ru'],
+       ['Content-Type', 'application/x-www-form-urlencoded'],
+       ['Content-Type', 'multipart/form-data'],
+       ['Content-Type', 'text/plain']];
+  var kNonSimpleHeaders =
+      [['X-ServiceWorker-Test', 'test'],
+       ['X-ServiceWorker-Test2', 'test2'],
+       ['Content-Type', 'foo/bar']];
+
+  ['same-origin', 'cors'].forEach(function(mode) {
+    request = new Request(url, {mode: mode});
+    kForbiddenHeaders.forEach(function(header) {
+      request.headers.append(header, 'test');
+      assert_equals(request.headers.size, 0,
+                    'Request.headers.append should ignore the forbidden headers');
+      request.headers.set(header, 'test');
+      assert_equals(request.headers.size, 0,
+                    'Request.headers.append should ignore the forbidden headers');

[falken, 2014/07/03 10:05:32]

"Request.headers.set"

[horo, 2014/07/03 12:40:29]

Done.

+    });
+    var request = new Request(url, {mode: mode});

[falken, 2014/07/03 10:05:32]

var doesn't seem to make sense here, we already used 'request' in this function

[horo, 2014/07/03 12:40:29]

Done.

+    assert_equals(request.headers.size, 0);
+    kNonSimpleHeaders.forEach(function(header) {
+      request.headers.append(header[0], header[1]);
+    });
+    assert_equals(request.headers.size, kNonSimpleHeaders.length);
+    kNonSimpleHeaders.forEach(function(header) {
+      assert_equals(request.headers.get(header[0]), header[1]);
+    });
+    request = new Request(url, {mode: mode});
+    assert_equals(request.headers.size, 0);
+    kNonSimpleHeaders.forEach(function(header) {
+      request.headers.set(header[0], header[1]);

[falken, 2014/07/03 10:05:32]

the asserts will pass even if set is just a no-op, maybe set to something like
header[1] + 'modified'

[horo, 2014/07/03 12:40:29]

|request| is recreated in line 182.
So we don't need to add 'modified'.

+    });
+    assert_equals(request.headers.size, kNonSimpleHeaders.length);
+    kNonSimpleHeaders.forEach(function(header) {
+      assert_equals(request.headers.get(header[0]), header[1]);
+    });
+  });
+
+  request = new Request(url, {mode: 'no-cors'});
+  kForbiddenHeaders.forEach(function(header) {
+    request.headers.set(header, 'test');
+    request.headers.append(header, 'test');
+  });
+  kNonSimpleHeaders.forEach(function(header) {
+    request.headers.set(header[0], header[1]);
+    request.headers.append(header[0], header[1]);
+  });
+  assert_equals(request.headers.size, 0,
+                'no-cors request should only accept simple headers');
+
+  kSimpleHeaders.forEach(function(header) {
+    request = new Request(url, {mode: 'no-cors'});
+    request.headers.append(header[0], header[1]);
+    assert_equals(request.headers.size, 1,
+                  'no-cors request should accept simple headers');
+    request = new Request(url, {mode: 'no-cors'});
+    request.headers.set(header[0], header[1]);

[falken, 2014/07/03 10:05:32]

same here

[horo, 2014/07/03 12:40:29]

|request| is recreated in line 210.
So we don't need to add 'modified'.

+    assert_equals(request.headers.size, 1,
+                  'no-cors request should accept simple headers');
+    request.headers.delete(header[0]);
+    if (header[0] == 'Content-Type') {
+      assert_equals(
+          request.headers.size, 1,
+          'Content-Type header of no-cors request shouldn\'t be deleted');
+    } else {
+      assert_equals(request.headers.size, 0);
+    }
+  });
+
+  kSimpleHeaders.forEach(function(header) {
+    var headers = {};
+    kNonSimpleHeaders.forEach(function(header2) {
+      headers[header2[0]] = header2[1];
+    });
+    kForbiddenHeaders.forEach(function(header) { headers[header] = 'foo';});
+    headers[header[0]] = header[1];
+    var expectedSize = kNonSimpleHeaders.length;
+    if (header[0] != 'Content-Type') {
+      ++expectedSize;
+    }
+    request = new Request(url, {headers: headers});
+    assert_equals(request.headers.size, expectedSize,
+                  'Request.headers should not support the forbidden headers');
+    request = new Request(url, {mode: 'same-origin', headers: headers});
+    assert_equals(request.headers.size, expectedSize,
+                  'Request.headers should not support the forbidden headers');
+    request = new Request(url, {mode: 'cors', headers: headers});
+    assert_equals(request.headers.size, expectedSize,
+                  'Request.headers should not support the forbidden headers');

[falken, 2014/07/03 10:05:32]

can you make the previous three assert descriptions distinct?

[horo, 2014/07/03 12:40:29]

Changed to use forEach.

+    request = new Request(url, {mode: 'no-cors', headers: headers});
+    assert_equals(request.headers.size, 1,
+                  'No-CORS Request.headers should only support simple headers');
+  });
 

[falken, 2014/07/03 10:05:32]

newline?

[horo, 2014/07/03 12:40:29]

Done.

 }, 'Request in ServiceWorkerGlobalScope');