Pass data saver pref value into embedded worker

Adds EmbeddedWorkerSettings to EmbeddedWorkerMsg_StartWorker_Params.
It contains v8 cache options and the data saver pref value. Whether
or not Data Saver is enabled is provided by the
ChromeContentBrowserClient. This value is read by
ServiceWorkerProcessManager::AllocateWorkerProcess and is propagated
to WebEmbeddedWorkerImpl::prepareShadowPageForLoader(). This is a
near term fix; changes to the data saver state do not update the
embedded worker once created.

BUG=587242
Committed: https://crrev.com/e8a146fb2900a92700479df4672189b7501b8918
Cr-Commit-Position: refs/heads/master@{#380283}

[falken, 2016-02-19 06:58:34]

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

[falken, 2016-02-19 06:58:35]

This is looking good.

In terms of testing, I think you want to test that when the pref is on, SW
fetches have the DataSaver request header, and the opposite when the pref is
off.

It depends how end-to-end you want to go, but I'd consider using
ServiceWorkerBrowserTest and:
- Use a MockContentBrowserClient to toggle the DataSaver flag
- Use ServiceWorkerBrowserTest to register a service worker  script like
"fetch_in_install.js" that does fetch() in its oninstall
- Listen for the request headers using
EmbeddedTestServer::RegisterRequestHandler

https://codereview.chromium.org/1708583003/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_process_manager.cc (right):

https://codereview.chromium.org/1708583003/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_process_manager.cc:155:
GetContentClient()->browser()->IsDataSaverEnabled(browser_context_);
We should add a comment that this the EWSettings from this function is only
populating data_saver_enabled (I guess, this function will populate things from
prefs, and the caller will populate things from command line switches).

[bengr, 2016-03-07 23:23:57]

Patchset #4 (id:60001) has been deleted

[bengr, 2016-03-07 23:24:08]

Patchset #3 (id:40001) has been deleted

[bengr, 2016-03-07 23:24:25]

Patchset #1 (id:1) has been deleted

[bengr, 2016-03-07 23:35:29]

bengr@chromium.org changed reviewers:
+ dcheng@chromium.org, kinuko@chromium.org

[bengr, 2016-03-07 23:35:30]

kinuko@chromium.org: Please review changes in
content/public/browser/content_browser_client.{cc,h}


dcheng@chromium.org: Please review changes in 
content/common/service_worker/embedder_worker_messages.{cc,h}
third_party/WebKit/*

falken@chronium.org: Please review everything else.

https://codereview.chromium.org/1708583003/diff/20001/content/browser/service...
File content/browser/service_worker/service_worker_process_manager.cc (right):

https://codereview.chromium.org/1708583003/diff/20001/content/browser/service...
content/browser/service_worker/service_worker_process_manager.cc:155:
GetContentClient()->browser()->IsDataSaverEnabled(browser_context_);
On 2016/02/19 06:58:35, falken wrote:
> We should add a comment that this the EWSettings from this function is only
> populating data_saver_enabled (I guess, this function will populate things
from
> prefs, and the caller will populate things from command line switches).

Done.

[falken, 2016-03-08 03:52:20]

This looks good. There is some additional test coverage that'd be useful:
- Test that the request for the worker script itself has the data saver header
- Test that main resource requests for a controlled page has the data saver
header, when the SW 1) does not respond to the fetch event, and 2) responds with
fetch() in the fetch event
- Same thing for subresources.
- Whether the SW can see the datasaver header inside onfetch (is it supposed to
be able to do this? Seems useful/important).

I expect that's all working now but it's conceivable that request interception
could  result in a dropped data saver header. IIRC there's some
whitelisting/processing of headers when we copy requests around, though I see
we've recently added save-data to the "simple header" definition
https://codereview.chromium.org/1698973002/

Also, BUG=587242

[bengr, 2016-03-08 19:00:06]

Description was changed from

==========
Pass data saver pref value into embedded worker

BUG=
==========

to

==========
Adds EmbeddedWorkerSettings to EmbeddedWorkerMsg_StartWorker_Params.
It contains v8 cache options and the data saver pref value. Whether
or not Data Saver is enabled is provided by the
ChromeContentBrowserClient. This value is read by
ServiceWorkerProcessManager::AllocateWorkerProcess and is propagated
to WebEmbeddedWorkerImpl::prepareShadowPageForLoader(). This is a
near term fix; changes to the data saver state do not update the
embedded worker once created.
    
BUG=587242
==========

[bengr, 2016-03-08 19:05:21]

On 2016/03/08 03:52:20, falken wrote:
> This looks good. There is some additional test coverage that'd be useful:
> - Test that the request for the worker script itself has the data saver header
> - Test that main resource requests for a controlled page has the data saver
> header, when the SW 1) does not respond to the fetch event, and 2) responds
with
> fetch() in the fetch event
> - Same thing for subresources.
> - Whether the SW can see the datasaver header inside onfetch (is it supposed
to
> be able to do this? Seems useful/important).
> 
> I expect that's all working now but it's conceivable that request interception
> could  result in a dropped data saver header. IIRC there's some
> whitelisting/processing of headers when we copy requests around, though I see
> we've recently added save-data to the "simple header" definition
> https://codereview.chromium.org/1698973002/
> 
> Also, BUG=587242

I updated the description and will look at the recommendations for additional
tests.
The whitelisting is needed for CORS. I don't know of other reasons it's needed.

[bengr, 2016-03-09 01:27:47]

Patchset #3 (id:100001) has been deleted

[bengr, 2016-03-09 01:31:56]

Patchset #3 (id:120001) has been deleted

[bengr, 2016-03-09 01:32:38]

On 2016/03/08 19:05:21, bengr wrote:
> On 2016/03/08 03:52:20, falken wrote:
> > This looks good. There is some additional test coverage that'd be useful:
> > - Test that the request for the worker script itself has the data saver
header
> > - Test that main resource requests for a controlled page has the data saver
> > header, when the SW 1) does not respond to the fetch event, and 2) responds
> with
> > fetch() in the fetch event
> > - Same thing for subresources.
> > - Whether the SW can see the datasaver header inside onfetch (is it supposed
> to
> > be able to do this? Seems useful/important).
> > 
> > I expect that's all working now but it's conceivable that request
interception
> > could  result in a dropped data saver header. IIRC there's some
> > whitelisting/processing of headers when we copy requests around, though I
see
> > we've recently added save-data to the "simple header" definition
> > https://codereview.chromium.org/1698973002/
> > 
> > Also, BUG=587242
> 
> I updated the description and will look at the recommendations for additional
> tests.
> The whitelisting is needed for CORS. I don't know of other reasons it's
needed.

I added all suggested tests except for the subresources, which I thought was
overkill.

[falken, 2016-03-09 02:05:36]

lgtm. tests look great, thank you.

https://codereview.chromium.org/1708583003/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/1708583003/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1065:
RequestWorkerScriptWithDataSaver) {
naming nit: XXXWithSaveData to be consistent with the others, or make them all
XXXWithDataSaver?

https://codereview.chromium.org/1708583003/diff/140001/content/test/data/serv...
File content/test/data/service_worker/add_save_data_to_title.js (right):

https://codereview.chromium.org/1708583003/diff/140001/content/test/data/serv...
content/test/data/service_worker/add_save_data_to_title.js:7:
headers.set('Content-Type', 'text/html; charset=UTF-8');
It looks like lines 6-7 are not doing anything since headers isn't passed to the
Response constructor. Let's remove them or pass them to the constructor.

[kinuko, 2016-03-09 11:35:11]

lgtm/2

[dcheng, 2016-03-09 22:23:47]

lgtm

[bengr, 2016-03-09 23:20:55]

The CQ bit was checked by bengr@chromium.org

[bengr, 2016-03-09 23:20:56]

The patchset sent to the CQ was uploaded after l-g-t-m from falken@chromium.org,
kinuko@chromium.org
Link to the patchset: https://codereview.chromium.org/1708583003/#ps160001
(title: "Addressed nits")

[commit-bot: I haz the power, 2016-03-09 23:21:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1708583003/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1708583003/160001

[commit-bot: I haz the power, 2016-03-10 01:20:30]

Description was changed from

==========
Adds EmbeddedWorkerSettings to EmbeddedWorkerMsg_StartWorker_Params.
It contains v8 cache options and the data saver pref value. Whether
or not Data Saver is enabled is provided by the
ChromeContentBrowserClient. This value is read by
ServiceWorkerProcessManager::AllocateWorkerProcess and is propagated
to WebEmbeddedWorkerImpl::prepareShadowPageForLoader(). This is a
near term fix; changes to the data saver state do not update the
embedded worker once created.
    
BUG=587242
==========

to

==========
Adds EmbeddedWorkerSettings to EmbeddedWorkerMsg_StartWorker_Params.
It contains v8 cache options and the data saver pref value. Whether
or not Data Saver is enabled is provided by the
ChromeContentBrowserClient. This value is read by
ServiceWorkerProcessManager::AllocateWorkerProcess and is propagated
to WebEmbeddedWorkerImpl::prepareShadowPageForLoader(). This is a
near term fix; changes to the data saver state do not update the
embedded worker once created.
    
BUG=587242
==========

[commit-bot: I haz the power, 2016-03-10 01:20:32]

Committed patchset #4 (id:160001)

[commit-bot: I haz the power, 2016-03-10 01:22:18]

Description was changed from

==========
Adds EmbeddedWorkerSettings to EmbeddedWorkerMsg_StartWorker_Params.
It contains v8 cache options and the data saver pref value. Whether
or not Data Saver is enabled is provided by the
ChromeContentBrowserClient. This value is read by
ServiceWorkerProcessManager::AllocateWorkerProcess and is propagated
to WebEmbeddedWorkerImpl::prepareShadowPageForLoader(). This is a
near term fix; changes to the data saver state do not update the
embedded worker once created.
    
BUG=587242
==========

to

==========
Adds EmbeddedWorkerSettings to EmbeddedWorkerMsg_StartWorker_Params.
It contains v8 cache options and the data saver pref value. Whether
or not Data Saver is enabled is provided by the
ChromeContentBrowserClient. This value is read by
ServiceWorkerProcessManager::AllocateWorkerProcess and is propagated
to WebEmbeddedWorkerImpl::prepareShadowPageForLoader(). This is a
near term fix; changes to the data saver state do not update the
embedded worker once created.

BUG=587242

Committed: https://crrev.com/e8a146fb2900a92700479df4672189b7501b8918
Cr-Commit-Position: refs/heads/master@{#380283}
==========

[commit-bot: I haz the power, 2016-03-10 01:22:20]

Patchset 4 (id:??) landed as
https://crrev.com/e8a146fb2900a92700479df4672189b7501b8918
Cr-Commit-Position: refs/heads/master@{#380283}

[bengr, 2016-03-17 22:24:38]

https://codereview.chromium.org/1708583003/diff/140001/content/browser/servic...
File content/browser/service_worker/service_worker_browsertest.cc (right):

https://codereview.chromium.org/1708583003/diff/140001/content/browser/servic...
content/browser/service_worker/service_worker_browsertest.cc:1065:
RequestWorkerScriptWithDataSaver) {
On 2016/03/09 02:05:36, falken wrote:
> naming nit: XXXWithSaveData to be consistent with the others, or make them all
> XXXWithDataSaver?

Done.

https://codereview.chromium.org/1708583003/diff/140001/content/test/data/serv...
File content/test/data/service_worker/add_save_data_to_title.js (right):

https://codereview.chromium.org/1708583003/diff/140001/content/test/data/serv...
content/test/data/service_worker/add_save_data_to_title.js:7:
headers.set('Content-Type', 'text/html; charset=UTF-8');
On 2016/03/09 02:05:36, falken wrote:
> It looks like lines 6-7 are not doing anything since headers isn't passed to
the
> Response constructor. Let's remove them or pass them to the constructor.

Good catch. Done.