Identify service worker version at main resource load time (BlinkSide)

Identify service worker version at main resource load time (BlinkSide)

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

[michaeln, 2014-03-11 02:40:53]

blink side

[michaeln, 2014-03-14 01:01:01]

https://codereview.chromium.org/194073002/diff/40001/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/194073002/diff/40001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextClient.h:46: // FIXME: put this in a seperate
.h
should be in another file, if we like this change i'll do that next and update
gyps and such

[kinuko, 2014-03-14 11:47:17]

https://codereview.chromium.org/194073002/diff/40001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/194073002/diff/40001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.h:47: class WebDataSource;
not necessary?

https://codereview.chromium.org/194073002/diff/40001/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/194073002/diff/40001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextClient.h:46: // FIXME: put this in a seperate
.h
On 2014/03/14 01:01:02, michaeln wrote:
> should be in another file, if we like this change i'll do that next and update
> gyps and such

Yes I think this change makes sense

https://codereview.chromium.org/194073002/diff/40001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextClient.h:54: // will be non-null and contain
the response that triggered the redirect.
Is this comment from WebFrameClient? May give impression that we do a lot in its
implementation.

We don't use redirectResponse at least, if we won't use it in a future too could
we drop redirectResponse param (and the comment 'If this request is the...')?

[michaeln, 2014-03-15 01:22:39]

https://codereview.chromium.org/194073002/diff/40001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.h (right):

https://codereview.chromium.org/194073002/diff/40001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.h:47: class WebDataSource;
On 2014/03/14 11:47:17, kinuko wrote:
> not necessary?

Done.

https://codereview.chromium.org/194073002/diff/40001/public/web/WebServiceWor...
File public/web/WebServiceWorkerContextClient.h (right):

https://codereview.chromium.org/194073002/diff/40001/public/web/WebServiceWor...
public/web/WebServiceWorkerContextClient.h:54: // will be non-null and contain
the response that triggered the redirect.
On 2014/03/14 11:47:17, kinuko wrote:
> Is this comment from WebFrameClient? May give impression that we do a lot in
its
> implementation.
> 
> We don't use redirectResponse at least, if we won't use it in a future too
could
> we drop redirectResponse param (and the comment 'If this request is the...')?

Done, removed the redirectResponse param and fixed the comment.

[kinuko, 2014-03-17 05:21:06]

lgtm (after you fix the compile error)

https://codereview.chromium.org/194073002/diff/60001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/194073002/diff/60001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:213:
m_networkProvider->willSendRequest(frame->dataSource(), request,
redirectResponse);
nit: you need to drop redirectResponse param here too

[michaeln, 2014-03-17 17:44:04]

@abarth, can you take a look as owner the cr-side is in
https://codereview.chromium.org/193723003

https://codereview.chromium.org/194073002/diff/60001/Source/web/WebEmbeddedWo...
File Source/web/WebEmbeddedWorkerImpl.cpp (right):

https://codereview.chromium.org/194073002/diff/60001/Source/web/WebEmbeddedWo...
Source/web/WebEmbeddedWorkerImpl.cpp:213:
m_networkProvider->willSendRequest(frame->dataSource(), request,
redirectResponse);
On 2014/03/17 05:21:06, kinuko wrote:
> nit: you need to drop redirectResponse param here too

Done. (ooops)

[michaeln, 2014-03-17 17:51:41]

@abarth, can you take a look as owner

[abarth-chromium, 2014-03-17 23:11:18]

LGTM

[michaeln, 2014-03-17 23:15:44]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-03-17 23:15:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/194073002/80001

[michaeln, 2014-03-17 23:28:07]

ty!

[commit-bot: I haz the power, 2014-03-18 01:09:43]

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

[commit-bot: I haz the power, 2014-03-18 01:09:43]

Try jobs failed on following builders:
  tryserver.blink on win_blink_rel

[michaeln, 2014-03-18 20:55:39]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-03-18 20:55:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/michaeln@chromium.org/194073002/80001

[commit-bot: I haz the power, 2014-03-18 21:40:47]

Change committed as 169479