Small fixes for Servicified SW code path

Small fixes for Servicified SW code path

- Add ServiceWorkerUtils::IsServicificationEnabled() for easier check.
  Currently this simply internally check --enable-network-service &&
  PlzNavigate enabled.
- Disable request interception via Network provider if servicified code
  path is enabled (temporarily; otherwise various DCHECK could fail)
- Small fixes in ServiceWorkerURLLoaderJob

BUG=715640
Review-Url: https://codereview.chromium.org/2931033005
Cr-Commit-Position: refs/heads/master@{#478548}
Committed: https://chromium.googlesource.com/chromium/src/+/15a31111592e8676f488b42c39007ea6e2679ed4

[kinuko, 2017-06-09 16:04:30]

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

[commit-bot: I haz the power, 2017-06-09 16:04:48]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-09 20:51:42]

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

[commit-bot: I haz the power, 2017-06-09 20:51:45]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[kinuko, 2017-06-12 00:48:55]

Description was changed from

==========
Add small fixes for Servicified SW code path

- Add ServiceWorkerUtils::IsServicificationEnabled() for easier check.
  Currently this simply internally check --enable-network-service &&
  PlzNavigate enabled.
- Disable request interception via Network provider if servicified code
  path is enabled (temporarily; otherwise various DCHECK could fail)
- Small fixes in ServiceWorkerURLLoaderJob

BUG=715640
==========

to

==========
Small fixes for Servicified SW code path

- Add ServiceWorkerUtils::IsServicificationEnabled() for easier check.
  Currently this simply internally check --enable-network-service &&
  PlzNavigate enabled.
- Disable request interception via Network provider if servicified code
  path is enabled (temporarily; otherwise various DCHECK could fail)
- Small fixes in ServiceWorkerURLLoaderJob

BUG=715640
==========

[kinuko, 2017-06-12 00:48:55]

kinuko@chromium.org changed reviewers:
+ falken@chromium.org

[kinuko, 2017-06-12 00:49:22]

PTL

[kinuko, 2017-06-12 00:49:27]

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

[commit-bot: I haz the power, 2017-06-12 00:49:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[falken, 2017-06-12 01:10:37]

lgtm

https://codereview.chromium.org/2931033005/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_utils.h (right):

https://codereview.chromium.org/2931033005/diff/20001/content/common/service_...
content/common/service_worker/service_worker_utils.h:53: 
Maybe a lightweight comment like:
"Returns true if servicified service worker is enabled."

https://codereview.chromium.org/2931033005/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2931033005/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2974:
frame_->DataSource()->GetServiceWorkerNetworkProvider());
To make the two paths more clear, could this be better as:

if (!provider) {
  DCHECK(IsServicifcatinoEnabled());
  return nullptr;
}
if (!provider->context()) {
 // The context can be null when the frame is sandboxed.
  return nullptr;
}

[kinuko, 2017-06-12 02:15:35]

Thanks!

[kinuko, 2017-06-12 02:16:51]

https://codereview.chromium.org/2931033005/diff/20001/content/common/service_...
File content/common/service_worker/service_worker_utils.h (right):

https://codereview.chromium.org/2931033005/diff/20001/content/common/service_...
content/common/service_worker/service_worker_utils.h:53: 
On 2017/06/12 01:10:36, falken wrote:
> Maybe a lightweight comment like:
> "Returns true if servicified service worker is enabled."

Done.

https://codereview.chromium.org/2931033005/diff/20001/content/renderer/render...
File content/renderer/render_frame_impl.cc (right):

https://codereview.chromium.org/2931033005/diff/20001/content/renderer/render...
content/renderer/render_frame_impl.cc:2974:
frame_->DataSource()->GetServiceWorkerNetworkProvider());
On 2017/06/12 01:10:36, falken wrote:
> To make the two paths more clear, could this be better as:
> 
> if (!provider) {
>   DCHECK(IsServicifcatinoEnabled());
>   return nullptr;
> }
> if (!provider->context()) {
>  // The context can be null when the frame is sandboxed.
>   return nullptr;
> }

Done.

[kinuko, 2017-06-12 02:17:14]

The CQ bit was checked by kinuko@chromium.org

[kinuko, 2017-06-12 02:17:14]

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

[commit-bot: I haz the power, 2017-06-12 02:17:28]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-12 03:57:31]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1497233834052960,
"parent_rev": "370a037ae7d0cd617c45942610c5866aff44a35c", "commit_rev":
"15a31111592e8676f488b42c39007ea6e2679ed4"}

[commit-bot: I haz the power, 2017-06-12 03:57:39]

Description was changed from

==========
Small fixes for Servicified SW code path

- Add ServiceWorkerUtils::IsServicificationEnabled() for easier check.
  Currently this simply internally check --enable-network-service &&
  PlzNavigate enabled.
- Disable request interception via Network provider if servicified code
  path is enabled (temporarily; otherwise various DCHECK could fail)
- Small fixes in ServiceWorkerURLLoaderJob

BUG=715640
==========

to

==========
Small fixes for Servicified SW code path

- Add ServiceWorkerUtils::IsServicificationEnabled() for easier check.
  Currently this simply internally check --enable-network-service &&
  PlzNavigate enabled.
- Disable request interception via Network provider if servicified code
  path is enabled (temporarily; otherwise various DCHECK could fail)
- Small fixes in ServiceWorkerURLLoaderJob

BUG=715640

Review-Url: https://codereview.chromium.org/2931033005
Cr-Commit-Position: refs/heads/master@{#478548}
Committed:
https://chromium.googlesource.com/chromium/src/+/15a31111592e8676f488b42c3900...
==========

[commit-bot: I haz the power, 2017-06-12 03:57:41]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/15a31111592e8676f488b42c3900...

[jam, 2017-06-12 15:26:54]

FYI this started to make
ServiceWorkerDisableWebSecurityTest.RegisterNoCrash
ServiceWorkerDisableWebSecurityTest.GetRegistrationNoCrash
fail with --enable-network-service, see
https://build.chromium.org/p/chromium.fyi/builders/Mojo%20Linux/builds/2012

[kinuko, 2017-06-13 01:10:21]

On 2017/06/12 15:26:54, jam wrote:
> FYI this started to make
> ServiceWorkerDisableWebSecurityTest.RegisterNoCrash
> ServiceWorkerDisableWebSecurityTest.GetRegistrationNoCrash
> fail with --enable-network-service, see
> https://build.chromium.org/p/chromium.fyi/builders/Mojo%20Linux/builds/2012

Will take a look...

[kinuko, 2017-06-13 05:28:45]

On 2017/06/13 01:10:21, kinuko wrote:
> On 2017/06/12 15:26:54, jam wrote:
> > FYI this started to make
> > ServiceWorkerDisableWebSecurityTest.RegisterNoCrash
> > ServiceWorkerDisableWebSecurityTest.GetRegistrationNoCrash
> > fail with --enable-network-service, see
> > https://build.chromium.org/p/chromium.fyi/builders/Mojo%20Linux/builds/2012
> 
> Will take a look...

Ok, mojo.fyi.network_content_browsertests.filter is the one I should look?

[jam, 2017-06-13 17:15:51]

On 2017/06/13 05:28:45, kinuko wrote:
> On 2017/06/13 01:10:21, kinuko wrote:
> > On 2017/06/12 15:26:54, jam wrote:
> > > FYI this started to make
> > > ServiceWorkerDisableWebSecurityTest.RegisterNoCrash
> > > ServiceWorkerDisableWebSecurityTest.GetRegistrationNoCrash
> > > fail with --enable-network-service, see
> > >
https://build.chromium.org/p/chromium.fyi/builders/Mojo%20Linux/builds/2012
> > 
> > Will take a look...
> 
> Ok, mojo.fyi.network_content_browsertests.filter is the one I should look?

Yep I updated it to green the bot in the meantime.

[kinuko, 2017-06-14 07:51:02]

On 2017/06/13 17:15:51, jam (ooo 6-14 - 6-20) wrote:
> On 2017/06/13 05:28:45, kinuko wrote:
> > On 2017/06/13 01:10:21, kinuko wrote:
> > > On 2017/06/12 15:26:54, jam wrote:
> > > > FYI this started to make
> > > > ServiceWorkerDisableWebSecurityTest.RegisterNoCrash
> > > > ServiceWorkerDisableWebSecurityTest.GetRegistrationNoCrash
> > > > fail with --enable-network-service, see
> > > >
> https://build.chromium.org/p/chromium.fyi/builders/Mojo%20Linux/builds/2012
> > > 
> > > Will take a look...
> > 
> > Ok, mojo.fyi.network_content_browsertests.filter is the one I should look?
> 
> Yep I updated it to green the bot in the meantime.

Fyi, updating it again for the fix: https://codereview.chromium.org/2937903002/