[ServiceWorker] Don't pass DevTools related HTTP headers to the ServiceWorker

[ServiceWorker] Don't pass DevTools related HTTP headers to the ServiceWorker

"X-DevTools-Request-Initiator" and "X-DevTools-Emulate-Network-Conditions-Client-Id" HTTP headers may be set by InspectorResourceAgent when the inspector window is open.
This headers are removed by DevToolsNetworkTransaction before sending the request to the network.
But when the request passed to the ServiceWorker these headers are not removed.
This may cause accidental problems.

BUG=419318
Committed: https://crrev.com/9accc09a59b1b5e6a2b40d8dace0a47e16ba4a12
Cr-Commit-Position: refs/heads/master@{#297807}

[horo, 2014-10-01 11:15:12]

Patchset #1 (id:1) has been deleted

[horo, 2014-10-01 11:20:09]

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

[horo, 2014-10-01 11:20:41]

falken@
Could you please review this?

[falken, 2014-10-02 03:54:46]

I'm not sure about this. I feel devtools should remove the header, not SW. Why
can't devtools do it?

https://codereview.chromium.org/615413002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/615413002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:37: //
devtools_network_controller.cc
It looks like they are defined in devtools_network_transaction.cc not
_controller.

Can you add a comment on the devtools too, so we actually can keep in sync?

[horo, 2014-10-02 04:52:15]

> I'm not sure about this. I feel devtools should remove the header, not SW. Why
> can't devtools do it?

DevToolsNetworkTransaction is created only when the HTTP request will go to the
network.

URLRequestHttpJob
 -> URLRequestHttpJob::StartTransactionInternal
  -> HttpCache::Transaction::DoSendRequest
   -> DevToolsNetworkTransaction

So when ServiceWorkerControlleeRequestHandler handles the HTTP request, this
request couldn't be modified by DevToolsNetworkTransaction.

https://codereview.chromium.org/615413002/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/615413002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:37: //
devtools_network_controller.cc
On 2014/10/02 03:54:46, falken wrote:
> It looks like they are defined in devtools_network_transaction.cc not
> _controller.
> 
> Can you add a comment on the devtools too, so we actually can keep in sync?

Done.

[falken, 2014-10-02 05:22:52]

Ah, that makes more sense now that I see the header is added by
InspectorResourceAgent.cpp.

lgtm... but I'd prefer the string not be hardcoded in different places. Maybe
devtool folks have an idea.

I also thought the bug wasn't so important until seeing your comment on
crbug.com/417896

https://codereview.chromium.org/615413002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/615413002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:36: //
kDevToolsEmulateNetworkConditionsClient Id defined in
No space before Id

[horo, 2014-10-02 06:32:48]

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

[horo, 2014-10-02 06:32:48]

vsevik@
Could you please review this?

[horo, 2014-10-02 06:33:05]

https://codereview.chromium.org/615413002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_url_request_job.cc (right):

https://codereview.chromium.org/615413002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_url_request_job.cc:36: //
kDevToolsEmulateNetworkConditionsClient Id defined in
On 2014/10/02 05:22:52, falken wrote:
> No space before Id

Done.

[vsevik, 2014-10-02 08:07:39]

vsevik@chromium.org changed reviewers:
+ eustas@chromium.org

[vsevik, 2014-10-02 08:07:40]

eustas@, could you please take a look?

[eustas, 2014-10-02 09:11:12]

On 2014/10/02 08:07:40, vsevik wrote:
> eustas@, could you please take a look?

This patch fixes consequences of our design error, not the problem itself.
Perfect solution is to omit injection of DevTools headers until it is required.

[eustas, 2014-10-02 09:11:18]

lgtm

[vsevik, 2014-10-02 09:25:07]

> This patch fixes consequences of our design error, not the problem itself.
> Perfect solution is to omit injection of DevTools headers until it is
required.
To clarify: let's land it as is for now and later we (DevTools team) will update
our implementation so that the use of this hack could be avoided.

lgtm

[horo, 2014-10-02 09:58:18]

On 2014/10/02 09:25:07, vsevik wrote:
> > This patch fixes consequences of our design error, not the problem itself.
> > Perfect solution is to omit injection of DevTools headers until it is
> required.
> To clarify: let's land it as is for now and later we (DevTools team) will
update
> our implementation so that the use of this hack could be avoided.
> 
> lgtm


Thank you!

[horo, 2014-10-02 09:58:22]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-10-02 09:59:10]

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

[commit-bot: I haz the power, 2014-10-02 10:07:45]

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

[commit-bot: I haz the power, 2014-10-02 10:07:46]

Try jobs failed on following builders:
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2014-10-02 10:34:22]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2014-10-02 10:34:41]

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

[commit-bot: I haz the power, 2014-10-02 11:45:23]

Committed patchset #3 (id:60001) as 3898f71d33270e916a9e9ccb3c6f2087b63ba58e

[commit-bot: I haz the power, 2014-10-02 11:46:18]

Patchset 3 (id:??) landed as
https://crrev.com/9accc09a59b1b5e6a2b40d8dace0a47e16ba4a12
Cr-Commit-Position: refs/heads/master@{#297807}