Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
314133002:
Initial ServiceWorker Cache API polyfill. (Closed)
|
Created:
6 years, 6 months ago by gavinp Modified:
6 years, 6 months ago CC:
blink-reviews, jsbell+serviceworker_chromium.org, tzik, serviceworker-reviews, nhiroki, falken, kinuko+serviceworker, horo+watch_chromium.org, alecflett+watch_chromium.org Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionInitial ServiceWorker Cache API polyfill.
A basic Cache API. No serialization of any kind, no testing, and no
add() method, as it depends on Fetch().
R=falken@chromium.org,jkarlin@chromium.org
TBR=jochen@chromium.org
BUG=374822
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175749
Patch Set 1 #Patch Set 2 : remove old Cache, fix copyright #Patch Set 3 : rebase #Patch Set 4 : really fix copyright #
Total comments: 25
Patch Set 5 : clean up, remediate to review. #
Total comments: 4
Patch Set 6 : add comment #
Total comments: 6
Patch Set 7 : jsbell + falken reviews #
Messages
Total messages: 14 (0 generated)
PTAL. Note this is downstream of https://codereview.chromium.org/311343002/ , which makes me confused about submitting blink try jobs.
On 2014/06/05 14:28:18, gavinp wrote: > PTAL. Note this is downstream of https://codereview.chromium.org/311343002/ , > which makes me confused about submitting blink try jobs. Now this is downstream of https://codereview.chromium.org/311233007/
Mostly style feedback, but the missing `return`s are important. (I wasn't looking at the Cache spec at the same time as the code, but can do another pass.) https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... File Source/modules/serviceworkers/polyfills/cachePolyfill.js (right): https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:1: /* Use new shorter license header: http://dev.chromium.org/blink/coding-style#TOC-License https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:34: var Cache = (function () { A good polyfill pattern is: (function(global) { var Cache = ....; // Use native implementation if available. global.Cache = global.Cache || Cache; }(self)); // window or worker global scope https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:39: method: 'GET', I notice that the Fetch polyfill doesn't explicitly set method; is it necessary? https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:53: var entriesByMethod = this.entriesByMethod; FYI, it's slightly more idiomatic JS to capture the 'this' object in the closure rather than particular members, e.g. var that = this; ... that.entriesByMethods - which defends against "whole brain swap" changes where this.entriesByMethods itself gets reassigned. But this is fine. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:55: return new Promise(function (resolve, reject) { Nit: be consistent between function() and function () https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:58: for (var method in entriesByMethod) { for... in is fragile and can break when interacting with user code E.g. if someone adds Object.prototype.foo, it'll show up in the enumeration. You can write: for (var p in o) { if (!o.hasOwnProperty(p)) continue; ... o[p] ... } Or alternately: Object.keys(o).forEach(function(p) { ... o[p] ... }); https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:73: if (entriesByMethod[request.method] == undefined) { Paranoia: use entriesByMethod.hasOwnProperty(request.method) instead (Only slight paranoia; users could override Object.property.hasOwnProperty, and while you can defend against that it's probably not worth it if we're not making security decisions based on this code.) https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:86: return new Promise(function (resolve, reject) { FYI, could just write: return Promise.reject("..."); https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:99: reject("not found"); Need to `return` after this. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:113: var entriesByUrl = entriesByMethod[request.method]; Again, may want to test with entriesByMethod.hasOwnProperty(request.method) for paranoia's sake. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:116: reject("not found"); Need to `return` after this. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:121: reject("not found"); Need to `return` after this.
Remediated to jsbell's review, and went through myself comparing to the spec and our style guide. Lots left to do but this simple thing is surprisingly useful. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... File Source/modules/serviceworkers/polyfills/cachePolyfill.js (right): https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:1: /* On 2014/06/05 18:06:01, jsbell wrote: > Use new shorter license header: > http://dev.chromium.org/blink/coding-style#TOC-License Done. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:34: var Cache = (function () { On 2014/06/05 18:06:00, jsbell wrote: > A good polyfill pattern is: > > (function(global) { > > var Cache = ....; > > // Use native implementation if available. > global.Cache = global.Cache || Cache; > > }(self)); // window or worker global scope Thanks. I did this, and removed the scope for "var Cache = ...", since that was really only around to keep things sanitary, and our anon function does that just as well. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:39: method: 'GET', On 2014/06/05 18:06:01, jsbell wrote: > I notice that the Fetch polyfill doesn't explicitly set method; is it necessary? It is not. Now fixed. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:53: var entriesByMethod = this.entriesByMethod; On 2014/06/05 18:06:00, jsbell wrote: > FYI, it's slightly more idiomatic JS to capture the 'this' object in the closure > rather than particular members, e.g. var that = this; ... that.entriesByMethods > - which defends against "whole brain swap" changes where this.entriesByMethods > itself gets reassigned. > > But this is fine. Fair point; I should not have optimized out the property lookup, that is something a compiler can do. Fixed. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:55: return new Promise(function (resolve, reject) { On 2014/06/05 18:06:00, jsbell wrote: > Nit: be consistent between function() and function () Done. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:58: for (var method in entriesByMethod) { On 2014/06/05 18:06:01, jsbell wrote: > for... in is fragile and can break when interacting with user code E.g. if > someone adds Object.prototype.foo, it'll show up in the enumeration. > > You can write: > > for (var p in o) { > if (!o.hasOwnProperty(p)) continue; > ... o[p] ... > } > > Or alternately: > > Object.keys(o).forEach(function(p) { > ... o[p] ... > }); Thanks. I took the Object.keys() approach, and went a bit more functional using Function.prototype.map(). https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:68: Cache.prototype.set = function(request, response) { Called "put" in the latest revision of the spec. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:73: if (entriesByMethod[request.method] == undefined) { On 2014/06/05 18:06:01, jsbell wrote: > Paranoia: use entriesByMethod.hasOwnProperty(request.method) instead > > (Only slight paranoia; users could override Object.property.hasOwnProperty, and > while you can defend against that it's probably not worth it if we're not making > security decisions based on this code.) Done. I added a TODO at the top about binding function references, since that probably should be done. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:86: return new Promise(function (resolve, reject) { On 2014/06/05 18:06:00, jsbell wrote: > FYI, could just write: return Promise.reject("..."); Done. Same thing in a few other places. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:99: reject("not found"); On 2014/06/05 18:06:00, jsbell wrote: > Need to `return` after this. Done. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:113: var entriesByUrl = entriesByMethod[request.method]; On 2014/06/05 18:06:01, jsbell wrote: > Again, may want to test with entriesByMethod.hasOwnProperty(request.method) for > paranoia's sake. Done. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:116: reject("not found"); On 2014/06/05 18:06:01, jsbell wrote: > Need to `return` after this. Done. https://codereview.chromium.org/314133002/diff/60001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:121: reject("not found"); On 2014/06/05 18:06:00, jsbell wrote: > Need to `return` after this. Done.
Also, this CL is now rooted on trunk. Yay!
I think this looks good to me, just one comment about the delete() function. https://codereview.chromium.org/314133002/diff/80001/Source/modules/servicewo... File Source/modules/serviceworkers/polyfills/cachePolyfill.js (right): https://codereview.chromium.org/314133002/diff/80001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:12: (function (global) { I don't see it in the style guide, but "function(...)" looks much more used than "function (...)" in the codebase. https://codereview.chromium.org/314133002/diff/80001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:76: delete entriesByUrl[request.url]; Should we reject if the URL isn't present, like we do for method?
lgtm with some nits https://codereview.chromium.org/314133002/diff/100001/Source/modules/servicew... File Source/modules/serviceworkers/polyfills/cachePolyfill.js (right): https://codereview.chromium.org/314133002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/polyfills/cachePolyfill.js:35: return Promise.resolve(Array.prototype.concat.apply([], Object.keys(this.entriesByMethod).map(function (method) { Maybe extract the Array.prototype.concat.apply([], x) magic into a flatten() helper, for readability? https://codereview.chromium.org/314133002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/polyfills/cachePolyfill.js:48: request = _castToRequest(request); Consider doing this inside the new Promise(function(r,r) { .. }) body, so that if this throws the method will convert the exception to a rejection rather than throwing. (Here and below) https://codereview.chromium.org/314133002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/polyfills/cachePolyfill.js:73: reject('not found'); This seems inconsistent - we reject if the *method* is not found, but not if the *url* is not found? It should probably be idempotent and just resolve() if the method is not found.
Thank you everybody for your help. Do we have a Blink coding standard for js? I searched and could not find one. https://codereview.chromium.org/314133002/diff/80001/Source/modules/servicewo... File Source/modules/serviceworkers/polyfills/cachePolyfill.js (right): https://codereview.chromium.org/314133002/diff/80001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:12: (function (global) { On 2014/06/06 17:02:45, falken wrote: > I don't see it in the style guide, but "function(...)" looks much more used than > "function (...)" in the codebase. Done. https://codereview.chromium.org/314133002/diff/80001/Source/modules/servicewo... Source/modules/serviceworkers/polyfills/cachePolyfill.js:76: delete entriesByUrl[request.url]; On 2014/06/06 17:02:45, falken wrote: > Should we reject if the URL isn't present, like we do for method? jsbell suggested favouring idempotence, and so I went with that; we now just resolve in all cases. https://codereview.chromium.org/314133002/diff/100001/Source/modules/servicew... File Source/modules/serviceworkers/polyfills/cachePolyfill.js (right): https://codereview.chromium.org/314133002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/polyfills/cachePolyfill.js:37: return new Request({method: method, url: url}); Have I followed the standard indentation practice for closing my lambda form here? I personally prefer this form: return new Request({method: method, url: url})})})))}; However, the Google JavaScript coding standard is at best vague on this subject, but I think it's not right. Did I make the right call? https://codereview.chromium.org/314133002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/polyfills/cachePolyfill.js:48: request = _castToRequest(request); On 2014/06/06 17:25:31, jsbell wrote: > Consider doing this inside the new Promise(function(r,r) { .. }) body, so that > if this throws the method will convert the exception to a rejection rather than > throwing. (Here and below) Done. https://codereview.chromium.org/314133002/diff/100001/Source/modules/servicew... Source/modules/serviceworkers/polyfills/cachePolyfill.js:73: reject('not found'); On 2014/06/06 17:25:31, jsbell wrote: > This seems inconsistent - we reject if the *method* is not found, but not if the > *url* is not found? > > It should probably be idempotent and just resolve() if the method is not found. Done.
Added jochen as TBR for changes to modules.gyp.
The CQ bit was checked by gavinp@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gavinp@chromium.org/314133002/120001
Message was sent while issue was closed.
Change committed as 175749
Message was sent while issue was closed.
A revert of this CL has been created in https://codereview.chromium.org/319383003/ by dcheng@chromium.org. The reason for reverting is: Triggering buffer overflow checks on Linux ASAN: http://build.chromium.org/p/chromium.memory/buildstatus?builder=Linux%20ASan%....
Message was sent while issue was closed.
See https://codereview.chromium.org/325683003/ for the reland of this. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
