Add Client Attribute to FetchEvent- Chromium Side.

Add Client Attribute to FetchEvent- Chromium Side.


..per https://slightlyoff.github.io/ServiceWorker/spec/service_worker/#fetch-event-interface, FetchEvent Interface should have a Client Attribute.

Intent to Implement and Ship Link: https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/fetchevent/blink-dev/naYY_V77Amo/zd4v_jwWlvsJ

[PATCH 1]: Blink Side: https://codereview.chromium.org/1102363002/
[Patch 2]: Chromium Side: This Patch
[Patch 3]: Layout Tests: https://codereview.chromium.org/1138223002/

BUG=470036

[Paritosh Kumar, 2015-05-06 11:53:44]

@jsbell: Please have a look.

https://codereview.chromium.org/1108273002/diff/1/content/browser/cache_stora...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/1108273002/diff/1/content/browser/cache_stora...
content/browser/cache_storage/cache_storage_cache.cc:1094:
ServiceWorkerHeaderMap(), Referrer(), false,
Here, I'm initializing ServiceWorkerFetchRequest with last
ServiceWorkerClientInfo. Is it right or we should initialize it with other
value. I'm not sure about this. Please share your input.

[Paritosh Kumar, 2015-05-06 11:54:35]

paritosh.in@samsung.com changed reviewers:
+ jsbell@chromium.org

[jsbell, 2015-05-06 16:53:42]

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

[jsbell, 2015-05-06 16:53:43]

this lgtm but you should get jkarlin and falken to review as well.

https://codereview.chromium.org/1108273002/diff/1/content/browser/cache_stora...
File content/browser/cache_storage/cache_storage_cache.cc (right):

https://codereview.chromium.org/1108273002/diff/1/content/browser/cache_stora...
content/browser/cache_storage/cache_storage_cache.cc:1094:
ServiceWorkerHeaderMap(), Referrer(), false,
On 2015/05/06 11:53:44, Paritosh Kumar wrote:
> Here, I'm initializing ServiceWorkerFetchRequest with last
> ServiceWorkerClientInfo. Is it right or we should initialize it with other
> value. I'm not sure about this. Please share your input.

This seems ok to me.

https://codereview.chromium.org/1108273002/diff/1/content/common/service_work...
File content/common/service_worker/service_worker_types.cc (right):

https://codereview.chromium.org/1108273002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.cc:33: const 
ServiceWorkerClientInfo& client)
nit: extra space

https://codereview.chromium.org/1108273002/diff/1/content/renderer/cache_stor...
File content/renderer/cache_storage/cache_storage_dispatcher.cc (right):

https://codereview.chromium.org/1108273002/diff/1/content/renderer/cache_stor...
content/renderer/cache_storage/cache_storage_dispatcher.cc:44:
ServiceWorkerClientInfo ToServiceWorkerClientInfo(
For consistency with other functions in this class, this should be named
'ClientInfoFromWebClientInfo'

(I personally prefer the 'ToXYZ' naming used in the service_worker_context
files, but we should match what's in use here)

https://codereview.chromium.org/1108273002/diff/1/content/renderer/cache_stor...
content/renderer/cache_storage/cache_storage_dispatcher.cc:52:
client_info.frame_type =
Note for other reviewers:

The static_cast<> is used elsewhere and we have matching-enums assertions in
chromium/src/content/child/web_url_loader_impl.cc

(Standard conversion functions - or just using the blink enums everywhere -
would be better, but not in this CL)

[jkarlin, 2015-05-06 19:03:10]

lgtm

[falken, 2015-05-07 04:13:42]

lgtm with jsbell's comments addressed

https://codereview.chromium.org/1108273002/diff/1/content/renderer/cache_stor...
File content/renderer/cache_storage/cache_storage_dispatcher.cc (right):

https://codereview.chromium.org/1108273002/diff/1/content/renderer/cache_stor...
content/renderer/cache_storage/cache_storage_dispatcher.cc:52:
client_info.frame_type =
On 2015/05/06 16:53:43, jsbell wrote:
> Note for other reviewers:

Thanks :)

[Paritosh Kumar, 2015-05-07 12:03:14]

Thanks jsbell, jkarlin & falken for reviewing this,

PTAL.

https://codereview.chromium.org/1108273002/diff/1/content/common/service_work...
File content/common/service_worker/service_worker_types.cc (right):

https://codereview.chromium.org/1108273002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.cc:33: const 
ServiceWorkerClientInfo& client)
On 2015/05/06 16:53:43, jsbell wrote:
> nit: extra space

I'm not sure this is extra space or just due to new line.

https://codereview.chromium.org/1108273002/diff/1/content/renderer/cache_stor...
File content/renderer/cache_storage/cache_storage_dispatcher.cc (right):

https://codereview.chromium.org/1108273002/diff/1/content/renderer/cache_stor...
content/renderer/cache_storage/cache_storage_dispatcher.cc:44:
ServiceWorkerClientInfo ToServiceWorkerClientInfo(
On 2015/05/06 16:53:43, jsbell wrote:
> For consistency with other functions in this class, this should be named
> 'ClientInfoFromWebClientInfo'
> 
> (I personally prefer the 'ToXYZ' naming used in the service_worker_context
> files, but we should match what's in use here)

Done. Thanks.

[jsbell, 2015-05-07 16:09:49]

Other than the extra space (unless I'm imagining it) lgtm!

https://codereview.chromium.org/1108273002/diff/1/content/common/service_work...
File content/common/service_worker/service_worker_types.cc (right):

https://codereview.chromium.org/1108273002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.cc:33: const 
ServiceWorkerClientInfo& client)
On 2015/05/07 12:03:13, Paritosh Kumar wrote:
> On 2015/05/06 16:53:43, jsbell wrote:
> > nit: extra space
> 
> I'm not sure this is extra space or just due to new line.

There are two spaces after 'const' - should just be one. It shows up in the raw
patch, too, so I don't think it's the code review tool.

[Paritosh Kumar, 2015-05-07 16:54:54]

Thanks Jsbell

https://codereview.chromium.org/1108273002/diff/1/content/common/service_work...
File content/common/service_worker/service_worker_types.cc (right):

https://codereview.chromium.org/1108273002/diff/1/content/common/service_work...
content/common/service_worker/service_worker_types.cc:33: const 
ServiceWorkerClientInfo& client)
On 2015/05/07 16:09:48, jsbell wrote:
> On 2015/05/07 12:03:13, Paritosh Kumar wrote:
> > On 2015/05/06 16:53:43, jsbell wrote:
> > > nit: extra space
> > 
> > I'm not sure this is extra space or just due to new line.
> 
> There are two spaces after 'const' - should just be one. It shows up in the
raw
> patch, too, so I don't think it's the code review tool.

Ohhk, extra space is in between, not expected. Thanks. Will remove it.

[Paritosh Kumar, 2015-05-12 12:08:25]

Thanks Josh
Found some issue in this cahnge, as I was not initializing 
ServiceWorkerRequest's client attribute. That was leading to
event.client.frameType value to top-level for nested frame also. Now made
changes to address them. Please have a look.

[jsbell, 2015-05-12 16:44:11]

lgtm

[Paritosh Kumar, 2015-05-13 10:52:38]

paritosh.in@samsung.com changed reviewers:
+ mkwst@chromium.org, nasko@chromium.org

[Paritosh Kumar, 2015-05-13 10:52:39]

@nasko, Mike: Please have a look.

[Mike West, 2015-05-13 11:09:34]

On 2015/05/13 at 10:52:39, paritosh.in wrote:
> @nasko, Mike: Please have a look.

I don't think I'm an OWNER of any of this code. What would you like me to take a
look at?

[Paritosh Kumar, 2015-05-13 11:35:00]

On 2015/05/13 11:09:34, Mike West wrote:
> On 2015/05/13 at 10:52:39, paritosh.in wrote:
> > @nasko, Mike: Please have a look.
> 
> I don't think I'm an OWNER of any of this code. What would you like me to take
a
> look at?

I think You are an OWNER for "*-messages.h" here,  
content/common/service_worker/service_worker_messages.h.

[Mike West, 2015-05-13 11:39:40]

Got it. Adding the ServiceWorkerClientInfo struct to the IPC LGTM, as we're
already passing it up in a number of other places.

[Paritosh Kumar, 2015-05-15 07:34:45]

The CQ bit was checked by paritosh.in@samsung.com to run a CQ dry run

[Paritosh Kumar, 2015-05-15 07:34:45]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/1108273002/#ps40001
(title: " ")

[commit-bot: I haz the power, 2015-05-15 07:35:13]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1108273002/40001

[commit-bot: I haz the power, 2015-05-15 07:38:28]

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

[commit-bot: I haz the power, 2015-05-15 07:38:30]

Dry run: Try jobs failed on following builders:
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Paritosh Kumar, 2015-05-15 13:38:29]

The CQ bit was checked by paritosh.in@samsung.com to run a CQ dry run

[Paritosh Kumar, 2015-05-15 13:38:30]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
jkarlin@chromium.org, mkwst@chromium.org, jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1108273002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-05-15 13:39:11]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1108273002/60001

[Paritosh Kumar, 2015-05-15 13:39:51]

There was some error in applying patch, so updated after rebase. Please have a
look.

[commit-bot: I haz the power, 2015-05-15 13:51:02]

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

[commit-bot: I haz the power, 2015-05-15 13:51:04]

Dry run: Try jobs failed on following builders:
  linux_chromium_gn_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Paritosh Kumar, 2015-05-15 16:16:53]

@jsbell: Please have a look. Updated after fixing build issues.
Now again running "CQ dry run".

[Paritosh Kumar, 2015-05-15 16:18:08]

The CQ bit was checked by paritosh.in@samsung.com to run a CQ dry run

[Paritosh Kumar, 2015-05-15 16:18:08]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
jkarlin@chromium.org, mkwst@chromium.org, jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1108273002/#ps80001
(title: " ")

[commit-bot: I haz the power, 2015-05-15 16:18:36]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1108273002/80001

[Paritosh Kumar, 2015-05-15 16:30:10]

The CQ bit was unchecked by paritosh.in@samsung.com

[falken, 2015-05-18 01:22:15]

On 2015/05/15 16:18:36, I haz the power (commit-bot) wrote:
> Dry run: CQ is trying da patch. Follow status at
>  https://chromium-cq-status.appspot.com/patch-status/1108273002/80001

Looks like there are still build errors. You might need to use CONTENT_EXPORT on
ServiceWorkerClientInfo so the unittests build.

[Paritosh Kumar, 2015-05-18 06:27:58]

On 2015/05/18 01:22:15, falken wrote:
> On 2015/05/15 16:18:36, I haz the power (commit-bot) wrote:
> > Dry run: CQ is trying da patch. Follow status at
> >  https://chromium-cq-status.appspot.com/patch-status/1108273002/80001
> 
> Looks like there are still build errors. You might need to use CONTENT_EXPORT
on
> ServiceWorkerClientInfo so the unittests build.

Thanks falken

Updated as per suggestion.

But, I'm not seeing any build error on building on Local Linux-64 Machine.

Please check it on trybot, as when I'm choosing any trybot some 'exception -1'
is coming always.

[falken, 2015-05-18 06:31:18]

The CQ bit was checked by falken@chromium.org to run a CQ dry run

[falken, 2015-05-18 06:31:19]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
jkarlin@chromium.org, mkwst@chromium.org, jsbell@chromium.org
Link to the patchset: https://codereview.chromium.org/1108273002/#ps100001
(title: " ")

[commit-bot: I haz the power, 2015-05-18 06:31:24]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1108273002/100001

[commit-bot: I haz the power, 2015-05-18 07:07:34]

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

[commit-bot: I haz the power, 2015-05-18 07:07:35]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[falken, 2015-05-19 03:16:39]

On 2015/05/18 07:07:35, I haz the power (commit-bot) wrote:
> Dry run: Try jobs failed on following builders:
>   cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

Hmm, it's building now but there are a lot of errors on the bots, which don't
look related to this patch. I'll kick the tryjobs again.

[falken, 2015-05-19 03:16:50]

The CQ bit was checked by falken@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-05-19 03:17:34]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1108273002/100001

[commit-bot: I haz the power, 2015-05-19 03:45:14]

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

[commit-bot: I haz the power, 2015-05-19 03:45:17]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[Paritosh Kumar, 2015-05-19 04:55:36]

On 2015/05/19 03:16:39, falken wrote:
> On 2015/05/18 07:07:35, I haz the power (commit-bot) wrote:
> > Dry run: Try jobs failed on following builders:
> >   cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
> 
> Hmm, it's building now but there are a lot of errors on the bots, which don't
> look related to this patch. I'll kick the tryjobs again.

Thanks falken for looking this.


It's building but, I think, error may be due to
call of "GetWindowClientInfoOnUI" in 
https://codereview.chromium.org/1108273002/diff/100001/content/browser/servic...
As per my understanding this call is on the IO thread, that causes FATAL error
in
render_frame_host_impl.cc(132).

Our main intention to call "GetWindowClientInfoOnUI" is to initialize
request handler, which is happening on the IO thread. So, I think, we have to
call
"GetWindowClientInfoOnUI" on UI thread.

Please suggest for further proceeding.

[falken, 2015-05-20 05:44:18]

On 2015/05/19 04:55:36, Paritosh Kumar wrote:
> On 2015/05/19 03:16:39, falken wrote:
> > On 2015/05/18 07:07:35, I haz the power (commit-bot) wrote:
> > > Dry run: Try jobs failed on following builders:
> > >   cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
> > 
> > Hmm, it's building now but there are a lot of errors on the bots, which
don't
> > look related to this patch. I'll kick the tryjobs again.
> 
> Thanks falken for looking this.
> 
> 
> It's building but, I think, error may be due to
> call of "GetWindowClientInfoOnUI" in 
>
https://codereview.chromium.org/1108273002/diff/100001/content/browser/servic...
> As per my understanding this call is on the IO thread, that causes FATAL error
> in
> render_frame_host_impl.cc(132).
> 
> Our main intention to call "GetWindowClientInfoOnUI" is to initialize
> request handler, which is happening on the IO thread. So, I think, we have to
> call
> "GetWindowClientInfoOnUI" on UI thread.
> 
> Please suggest for further proceeding.

Yes you must call GetWindowClientInfoOnUI on the UI thread. See how
ServiceWorkerProviderHost::GetWindowClientInfo is used to call it on the correct
thread.

[falken, 2015-05-28 09:20:40]

On 2015/05/20 05:44:18, falken wrote:
> On 2015/05/19 04:55:36, Paritosh Kumar wrote:
> > On 2015/05/19 03:16:39, falken wrote:
> > > On 2015/05/18 07:07:35, I haz the power (commit-bot) wrote:
> > > > Dry run: Try jobs failed on following builders:
> > > >   cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
> > > 
> > > Hmm, it's building now but there are a lot of errors on the bots, which
> don't
> > > look related to this patch. I'll kick the tryjobs again.
> > 
> > Thanks falken for looking this.
> > 
> > 
> > It's building but, I think, error may be due to
> > call of "GetWindowClientInfoOnUI" in 
> >
>
https://codereview.chromium.org/1108273002/diff/100001/content/browser/servic...
> > As per my understanding this call is on the IO thread, that causes FATAL
error
> > in
> > render_frame_host_impl.cc(132).
> > 
> > Our main intention to call "GetWindowClientInfoOnUI" is to initialize
> > request handler, which is happening on the IO thread. So, I think, we have
to
> > call
> > "GetWindowClientInfoOnUI" on UI thread.
> > 
> > Please suggest for further proceeding.
> 
> Yes you must call GetWindowClientInfoOnUI on the UI thread. See how
> ServiceWorkerProviderHost::GetWindowClientInfo is used to call it on the
correct
> thread.

Hi Paritosh are you still working on this? Since we already landed the IDL
change we should be sure to finish this up in the current milestone.

[falken, 2015-07-22 02:11:51]

Paritosh can you close this review? The spec has changed to remove this
attribute: https://code.google.com/p/chromium/issues/detail?id=470036

Thanks for this initial implementation. It exposed the problem in the spec that
will be fixed.

not lgtm to clear my previous lg-tm