ServiceWorker polyfill Fetch implementation.

ServiceWorker polyfill Fetch implementation.

This is by no means a good implementation of Fetch, however it is sufficient
to let us start proceeding with the cache API (and add).

R=falken@chromium.org,jkarlin@chromium.org
TBR=jochen@chromium.org
BUG=381179
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175846

[gavinp, 2014-06-05 15:00:30]

PTAL.

Boy is this incomplete and potentially controversial, but it's awesomely useful
for very lame testing and end-to-end work.

[gavinp, 2014-06-05 15:01:21]

Note: this CL is downstream of https://codereview.chromium.org/314133002/ ,
which hasn't landed yet, so it's tricky to know how to launch teh try jobs.

[jsbell, 2014-06-05 17:48:03]

Initial notes

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/polyfills/cachePolyfill.js (right):

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cachePolyfill.js:86: request =
_castToRequest(request);
_castToRequest isn't exposed, and you probably want the fetch() impl to do this
anyway?

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/polyfills/fetchPolyfill.js (right):

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:2: * Copyright (C) 2014
Google Inc. All rights reserved.
Use new (shorter) copyright header:
http://dev.chromium.org/blink/coding-style#TOC-License

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:31: // A simple,
incomplete implementation of Fetch, intended to facilitate end
Add link to spec in comment.

http://fetch.spec.whatwg.org/#fetch-method

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:34: function
Fetch(request) {
Function name is lower case in spec: "fetch"

Is making it uppercase intentional?

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:36: if (typeof item ==
"string") {
Nit: Be consistent with == vs. === (latter is preferred)
Nit: Prefer ' to "

(And this is fine for a temporary polyfill, but to match ES/bindings more
precisely by detecting if item is a Request, and otherwise using  String(item).
Detecting if item is a Request is a pain due to different "realms" which can
make instanceof fail, so this is probably fine.)

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:48:
xhr.open(request.method, request.url, true);
Document the async flag, e.g. true /*async*/

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:48:
xhr.open(request.method, request.url, true);
Add extra request headers (or FIXME to do so) via xhr.setRequestHeader()

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:68: }
A good way to structure polyfills is to wrap everything in an IIFE and pass in
the global and conditionally assign the polyfills into it, e.g.:

(function(global) {

  // helper functions, constants, etc can go here

  function fetch() { ... }

  // use native implementation if available:
  global.fetch = global.fetch || fetch;

}(self)); // window or worker global scope

[jsbell, 2014-06-05 17:50:50]

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/polyfills/cachePolyfill.js (right):

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/cachePolyfill.js:86: request =
_castToRequest(request);
On 2014/06/05 17:48:02, jsbell wrote:
> _castToRequest isn't exposed, and you probably want the fetch() impl to do
this
> anyway?

Ah, sorry, I didn't see that there was a local copy of this in this polyfill.

[jsbell, 2014-06-05 23:06:44]

Just some minor notes from my reading of the Fetch spec (which was on my TODO
list anyway). Nothing important.

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/polyfills/fetchPolyfill.js (right):

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:34: function
Fetch(request) {
FYI, the fetch spec's API has changed slightly - it's now fetch(DOMString or
Request, Dictionary), which matches the spec's Request constructor.

At the Monday SW sync we decided to track the SW spec for now - it's older, but
not a moving target. So no change needed here.

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:56: method:
request.method,
Looks like method is NYI in our Response impl.

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:58: // FIXME: Implement
body once Response has it.
Add FIXME for url once Response has it.

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:64: reject(response);
So far as I can tell from http://fetch.spec.whatwg.org/#fetch-method and
http://fetch.spec.whatwg.org/#concept-network-error if xhr.status === 0 then we
should reject(TypeError("network error")) instead. 

(The SW spec suggests using a DOMException/"NetworkError" but that's harder to
polyfill.)

[gavinp, 2014-06-06 16:43:14]

I've cleaned up the code and remediated to many comments.

The network error issue is interesting. I'll resolve that a bit later.

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
File Source/modules/serviceworkers/polyfills/fetchPolyfill.js (right):

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:2: * Copyright (C) 2014
Google Inc. All rights reserved.
On 2014/06/05 17:48:02, jsbell wrote:
> Use new (shorter) copyright header:
> http://dev.chromium.org/blink/coding-style#TOC-License

Done.

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:31: // A simple,
incomplete implementation of Fetch, intended to facilitate end
On 2014/06/05 17:48:03, jsbell wrote:
> Add link to spec in comment.
> 
> http://fetch.spec.whatwg.org/#fetch-method

Done.

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:34: function
Fetch(request) {
On 2014/06/05 17:48:03, jsbell wrote:
> Function name is lower case in spec: "fetch"
> 
> Is making it uppercase intentional?

No. Fixed.

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:36: if (typeof item ==
"string") {
On 2014/06/05 17:48:03, jsbell wrote:
> Nit: Be consistent with == vs. === (latter is preferred)
> Nit: Prefer ' to "
> 
> (And this is fine for a temporary polyfill, but to match ES/bindings more
> precisely by detecting if item is a Request, and otherwise using 
String(item).
> Detecting if item is a Request is a pain due to different "realms" which can
> make instanceof fail, so this is probably fine.)

Done.

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:48:
xhr.open(request.method, request.url, true);
On 2014/06/05 17:48:02, jsbell wrote:
> Document the async flag, e.g. true /*async*/

Done.

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:48:
xhr.open(request.method, request.url, true);
On 2014/06/05 17:48:02, jsbell wrote:
> Document the async flag, e.g. true /*async*/

Done.

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:56: method:
request.method,
On 2014/06/05 23:06:44, jsbell wrote:
> Looks like method is NYI in our Response impl.

Whoops. The cachePolyfill needs updating on this, too.

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:58: // FIXME: Implement
body once Response has it.
On 2014/06/05 23:06:44, jsbell wrote:
> Add FIXME for url once Response has it.

Done.

https://codereview.chromium.org/311343003/diff/40001/Source/modules/servicewo...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:68: }
On 2014/06/05 17:48:03, jsbell wrote:
> A good way to structure polyfills is to wrap everything in an IIFE and pass in
> the global and conditionally assign the polyfills into it, e.g.:
> 
> (function(global) {
> 
>   // helper functions, constants, etc can go here
> 
>   function fetch() { ... }
> 
>   // use native implementation if available:
>   global.fetch = global.fetch || fetch;
> 
> }(self)); // window or worker global scope

Done.

[jsbell, 2014-06-06 17:06:31]

LGTM as a WIP

[falken, 2014-06-06 17:29:03]

lgtm2

https://codereview.chromium.org/311343003/diff/120001/Source/modules/servicew...
File Source/modules/serviceworkers/polyfills/fetchPolyfill.js (right):

https://codereview.chromium.org/311343003/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:11: var _castToRequest
= function (item) {
No space after "function"

https://codereview.chromium.org/311343003/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:32: var response = new
Response({
The Response ctor changed in https://codereview.chromium.org/304233017/

Now the first argument is a Blob for the body, and the second is the
ResponseInit dictionary. You can just pass null for the body to mean no body.

[horo, 2014-06-09 05:25:13]

https://codereview.chromium.org/311343003/diff/120001/Source/modules/servicew...
File Source/modules/serviceworkers/polyfills/fetchPolyfill.js (right):

https://codereview.chromium.org/311343003/diff/120001/Source/modules/servicew...
Source/modules/serviceworkers/polyfills/fetchPolyfill.js:32: var response = new
Response({
I noticed that passing a blob created by XMLHttpRequest to ServiceWorker's
respondWith() causes DCHECK failure.
 https://crbug.com/382311
And I created a patch to fix this.
 https://codereview.chromium.org/315863003/

I think after this patch is landed we can pass xhr.response to Response like
xhrBlobFetchPromise() in this.
https://code.google.com/p/chromium/issues/attachmentText?id=382311&aid=382311...

[gavinp, 2014-06-10 00:29:19]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-06-10 00:29:37]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gavinp@chromium.org/311343003/140001

[commit-bot: I haz the power, 2014-06-10 00:29:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-06-10 00:29:45]

Failed to apply patch for
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
  Hunk #2 FAILED at 50.
  1 out of 2 hunks FAILED -- saving rejects to file
Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp.rej

Patch:       Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
Index: Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
diff --git a/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
b/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
index
71269d241233fc180ca4b91cf59cc79c3fa423fa..24820652e1f3b423908a5bfcc3b38a02f5242a00
100644
--- a/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
+++ b/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
@@ -31,6 +31,7 @@
 #include "ServiceWorkerGlobalScope.h"
 
 #include "CachePolyfill.h"
+#include "FetchPolyfill.h"
 #include "core/workers/WorkerClients.h"
 #include "core/workers/WorkerThreadStartupData.h"
 #include "modules/EventTargetModules.h"
@@ -49,6 +50,7 @@ PassRefPtrWillBeRawPtr<ServiceWorkerGlobalScope>
ServiceWorkerGlobalScope::creat
 
    
context->applyContentSecurityPolicyFromString(startupData->m_contentSecurityPolicy,
startupData->m_contentSecurityPolicyType);
 
+    context->script()->evaluate(String(fetchPolyfillJs));
     context->script()->evaluate(String(cachePolyfillJs));
 
     return context.release();

[gavinp, 2014-06-10 00:31:12]

On 2014/06/10 00:29:45, I haz the power (commit-bot) wrote:
> Failed to apply patch for
> Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp:
> While running patch -p1 --forward --force --no-backup-if-mismatch;
>   patching file Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
>   Hunk #2 FAILED at 50.
>   1 out of 2 hunks FAILED -- saving rejects to file
> Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp.rej
> 
> Patch:       Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
> Index: Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
> diff --git a/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
> b/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
> index
>
71269d241233fc180ca4b91cf59cc79c3fa423fa..24820652e1f3b423908a5bfcc3b38a02f5242a00
> 100644
> --- a/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
> +++ b/Source/modules/serviceworkers/ServiceWorkerGlobalScope.cpp
> @@ -31,6 +31,7 @@
>  #include "ServiceWorkerGlobalScope.h"
>  
>  #include "CachePolyfill.h"
> +#include "FetchPolyfill.h"
>  #include "core/workers/WorkerClients.h"
>  #include "core/workers/WorkerThreadStartupData.h"
>  #include "modules/EventTargetModules.h"
> @@ -49,6 +50,7 @@ PassRefPtrWillBeRawPtr<ServiceWorkerGlobalScope>
> ServiceWorkerGlobalScope::creat
>  
>     
>
context->applyContentSecurityPolicyFromString(startupData->m_contentSecurityPolicy,
> startupData->m_contentSecurityPolicyType);
>  
> +    context->script()->evaluate(String(fetchPolyfillJs));
>      context->script()->evaluate(String(cachePolyfillJs));
>  
>      return context.release();

Just as well: this would have been a memory error. Fixing.

[gavinp, 2014-06-10 00:38:02]

Added jochen@chromium.org as a TBR for modules.gyp changes.

[gavinp, 2014-06-10 00:38:07]

The CQ bit was checked by gavinp@chromium.org

[gavinp, 2014-06-10 00:38:22]

The CQ bit was unchecked by gavinp@chromium.org

[gavinp, 2014-06-10 00:40:37]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-06-10 00:41:43]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/gavinp@chromium.org/311343003/180001

[commit-bot: I haz the power, 2014-06-10 01:42:03]

Change committed as 175846