Make WebServiceWorkerResponse support ResponseType

Make WebServiceWorkerResponse support ResponseType

BUG=418700
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183128

[jkarlin, 2014-09-30 19:24:23]

jkarlin@chromium.org changed reviewers:
+ falken@chromium.org, jochen@chromium.org, jsbell@chromium.org

[jkarlin, 2014-09-30 19:24:24]

jsbell@chromium.org: Please review changes in everything.

falken@chromium.org: Please review changes in everything.

jochen@chromium.org: Please review changes outside of serviceworker for owners. 

Thanks!

[jsbell, 2014-09-30 22:14:43]

jsbell@chromium.org changed reviewers:
+ horo@chromium.org

[jsbell, 2014-09-30 22:14:45]

lgtm but @falken or @horo should take a look

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

https://codereview.chromium.org/617003003/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchResponseData.cpp:110: if (m_internalResponse)
Splitting up the m_internalResponse logic (populate from it, then return early)
seems a bit fragile in case of further modifications to the code. How about
moving the response type conversion up to the top (and/or into a helper
function) and making this read:

if (m_internalResponse) {
  m_internalResponse->populateWebServiceWorkerResponse(response);
  // Override any internal response's type.
  response.setResponseType(type);
  return;
}

response.setResponseType(type);
...

That duplicates the setResponseType() call, so I dunno if this is an
improvement, bit it seems (to me) to simplify the code flow a bit.

https://codereview.chromium.org/617003003/diff/60001/public/platform/WebServi...
File public/platform/WebServiceWorkerResponse.h (right):

https://codereview.chromium.org/617003003/diff/60001/public/platform/WebServi...
public/platform/WebServiceWorkerResponse.h:39: };
Blank line after this?

[jkarlin, 2014-10-01 00:11:00]

Thanks jsbell!

horo: Please take a look at everything.

Thanks!

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

https://codereview.chromium.org/617003003/diff/60001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchResponseData.cpp:110: if (m_internalResponse)
On 2014/09/30 22:14:44, jsbell wrote:
> Splitting up the m_internalResponse logic (populate from it, then return
early)
> seems a bit fragile in case of further modifications to the code. How about
> moving the response type conversion up to the top (and/or into a helper
> function) and making this read:
> 
> if (m_internalResponse) {
>   m_internalResponse->populateWebServiceWorkerResponse(response);
>   // Override any internal response's type.
>   response.setResponseType(type);
>   return;
> }
> 
> response.setResponseType(type);
> ...
> 
> That duplicates the setResponseType() call, so I dunno if this is an
> improvement, bit it seems (to me) to simplify the code flow a bit.

Done.

https://codereview.chromium.org/617003003/diff/60001/public/platform/WebServi...
File public/platform/WebServiceWorkerResponse.h (right):

https://codereview.chromium.org/617003003/diff/60001/public/platform/WebServi...
public/platform/WebServiceWorkerResponse.h:39: };
On 2014/09/30 22:14:44, jsbell wrote:
> Blank line after this?

Done.

[jkarlin, 2014-10-01 00:11:02]

Thanks jsbell!

horo: Please take a look at everything.

Thanks!

[horo, 2014-10-01 01:38:11]

lgtm with nit

https://codereview.chromium.org/617003003/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchResponseData.cpp (right):

https://codereview.chromium.org/617003003/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchResponseData.cpp:15:
WebServiceWorkerResponse::ResponseType
fetchTypeToWebType(FetchResponseData::Type fetchType)
+ 1 line

https://codereview.chromium.org/617003003/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchResponseData.cpp:15:
WebServiceWorkerResponse::ResponseType
fetchTypeToWebType(FetchResponseData::Type fetchType)
No need to indent.

[jochen (gone - plz use gerrit), 2014-10-01 07:46:34]

lgtm with nit

https://codereview.chromium.org/617003003/diff/80001/public/platform/WebServi...
File public/platform/WebServiceWorkerResponse.h (right):

https://codereview.chromium.org/617003003/diff/80001/public/platform/WebServi...
public/platform/WebServiceWorkerResponse.h:33: BasicType,
should be ResponseTypeBasic, etc..

[jkarlin, 2014-10-01 11:26:57]

Thanks!

https://codereview.chromium.org/617003003/diff/80001/Source/modules/servicewo...
File Source/modules/serviceworkers/FetchResponseData.cpp (right):

https://codereview.chromium.org/617003003/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchResponseData.cpp:15:
WebServiceWorkerResponse::ResponseType
fetchTypeToWebType(FetchResponseData::Type fetchType)
On 2014/10/01 01:38:11, horo wrote:
> No need to indent.

Done.

https://codereview.chromium.org/617003003/diff/80001/Source/modules/servicewo...
Source/modules/serviceworkers/FetchResponseData.cpp:15:
WebServiceWorkerResponse::ResponseType
fetchTypeToWebType(FetchResponseData::Type fetchType)
On 2014/10/01 01:38:11, horo wrote:
> + 1 line

Done.

https://codereview.chromium.org/617003003/diff/80001/public/platform/WebServi...
File public/platform/WebServiceWorkerResponse.h (right):

https://codereview.chromium.org/617003003/diff/80001/public/platform/WebServi...
public/platform/WebServiceWorkerResponse.h:33: BasicType,
On 2014/10/01 07:46:34, jochen wrote:
> should be ResponseTypeBasic, etc..

Done.

[jkarlin, 2014-10-01 12:17:17]

jochen: PTAL one more time as I moved the WebServiceWorkerResponseType enum into
its own file in public/platform so that it can be used in content/browser later.

Thanks!

[jkarlin, 2014-10-01 17:51:58]

On 2014/10/01 12:17:17, jkarlin wrote:
> jochen: PTAL one more time as I moved the WebServiceWorkerResponseType enum
into
> its own file in public/platform so that it can be used in content/browser
later.
> 
> Thanks!

Since it's a minor change, will land now with a TBR.

[jkarlin, 2014-10-01 17:52:33]

The CQ bit was checked by jkarlin@chromium.org

[jamesr, 2014-10-01 18:25:59]

On 2014/10/01 17:51:58, jkarlin wrote:
> On 2014/10/01 12:17:17, jkarlin wrote:
> > jochen: PTAL one more time as I moved the WebServiceWorkerResponseType enum
> into
> > its own file in public/platform so that it can be used in content/browser
> later.
> > 
> > Thanks!
> 
> Since it's a minor change, will land now with a TBR.

http://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Reviewe...
lists the appropriate uses of TBR.  This is not one.

[jamesr, 2014-10-01 18:26:02]

The CQ bit was unchecked by jamesr@chromium.org

[jkarlin, 2014-10-01 18:34:41]

On 2014/10/01 18:25:59, jamesr wrote:
> On 2014/10/01 17:51:58, jkarlin wrote:
> > On 2014/10/01 12:17:17, jkarlin wrote:
> > > jochen: PTAL one more time as I moved the WebServiceWorkerResponseType
enum
> > into
> > > its own file in public/platform so that it can be used in content/browser
> > later.
> > > 
> > > Thanks!
> > 
> > Since it's a minor change, will land now with a TBR.
> 
>
http://www.chromium.org/developers/owners-files#TOC-When-to-use-To-Be-Reviewe...
> lists the appropriate uses of TBR.  This is not one.

My bad, thanks jamesr. Removed the TBR.

jochen: PTAL when you're in tomorrow, many thanks! :)

[jochen (gone - plz use gerrit), 2014-10-02 07:41:06]

lgtm

[jkarlin, 2014-10-02 11:03:26]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2014-10-02 11:04:00]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/617003003/120001

[commit-bot: I haz the power, 2014-10-02 11:08:21]

Committed patchset #7 (id:120001) as 183128