Support embedded created Response objects in ServiceWorker.

Support embedded created Response objects in ServiceWorker.

Response objects were previously created in Blink and provided to the
embedder; however the Cache API requires that they can cross the other
way. This CL adds the accessors required to do this; both to fill in a
Response object from a WebServiceWorkerResponse and to allow Blink to
read all the fields in a WebServiceWorkerResponse.

R=jkarlin@chromium.org,jsbell@chromium.org,asanka@chromium.org
BUG=399178
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181327

[jsbell, 2014-08-29 17:53:23]

lgtm for what it does, with some minor nits.

I think we'll have to replace the use of HashMap for headers in
WebServiceWorkerResponse, but that's not this CL.

https://codereview.chromium.org/519803002/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/Response.cpp (right):

https://codereview.chromium.org/519803002/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/Response.cpp:37: for (HashMap<String,
String>::const_iterator i = webResponse.headers().begin(), end =
webResponse.headers().end(); i != end; ++i) {
Not new in this CL, but: does HashMap preserve insertion order? Header order is
supposed to be maintained, so we may need to swap out what container type
WebServiceWorkerResponse is using.

https://codereview.chromium.org/519803002/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/ResponseTest.cpp (right):

https://codereview.chromium.org/519803002/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/ResponseTest.cpp:61: 
Nit: can remove one of these blank lines

https://codereview.chromium.org/519803002/diff/60001/Source/platform/exported...
File Source/platform/exported/WebServiceWorkerResponse.cpp (right):

https://codereview.chromium.org/519803002/diff/60001/Source/platform/exported...
Source/platform/exported/WebServiceWorkerResponse.cpp:86: for (HashMap<String,
String>::const_iterator i = m_private->headers.begin(), end =
m_private->headers.end(); i != end; ++i)
nit: HashMap header should probably be included. (Not new with this CL though.)

[gavinp, 2014-08-29 18:28:38]

Thanks. I agree there's a good amount of downstream work to be done on Response.
The interface to headers is a bit redundant as well, I'll upload some CLs later
to remove the keys methods etc...

I've remediated to your suggestions, and added a FIXME pointing out the problem
with using a HashMap as the backing store.

https://codereview.chromium.org/519803002/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/Response.cpp (right):

https://codereview.chromium.org/519803002/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/Response.cpp:37: for (HashMap<String,
String>::const_iterator i = webResponse.headers().begin(), end =
webResponse.headers().end(); i != end; ++i) {
On 2014/08/29 17:53:23, jsbell wrote:
> Not new in this CL, but: does HashMap preserve insertion order? Header order
is
> supposed to be maintained, so we may need to swap out what container type
> WebServiceWorkerResponse is using.

HashMap does NOT preserve order.

I'm adding a FIXME in the WebServiceWorkerResponse object on this issue.

https://codereview.chromium.org/519803002/diff/60001/Source/modules/servicewo...
File Source/modules/serviceworkers/ResponseTest.cpp (right):

https://codereview.chromium.org/519803002/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/ResponseTest.cpp:61: 
On 2014/08/29 17:53:23, jsbell wrote:
> Nit: can remove one of these blank lines

You didn't say which one! However, I intuited and removed the correct one.

https://codereview.chromium.org/519803002/diff/60001/Source/platform/exported...
File Source/platform/exported/WebServiceWorkerResponse.cpp (right):

https://codereview.chromium.org/519803002/diff/60001/Source/platform/exported...
Source/platform/exported/WebServiceWorkerResponse.cpp:86: for (HashMap<String,
String>::const_iterator i = m_private->headers.begin(), end =
m_private->headers.end(); i != end; ++i)
On 2014/08/29 17:53:23, jsbell wrote:
> nit: HashMap header should probably be included. (Not new with this CL
though.)

Done.

[gavinp, 2014-09-02 13:14:41]

gavinp@chromium.org changed reviewers:
+ jochen@chromium.org

[gavinp, 2014-09-02 13:14:41]

jochen: PTAL at API changes in particular.

[jochen (gone - plz use gerrit), 2014-09-03 09:06:37]

lgtm

[gavinp, 2014-09-03 15:55:54]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2014-09-03 15:57:01]

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

[commit-bot: I haz the power, 2014-09-03 16:30:00]

Committed patchset #6 (id:100001) as 181327