Support DataSaver for SharedWorker

Support DataSaver for SharedWorker

This CL pipes the DataSaver flag from the renderer process which creates a
Shared Worker to the shared worker process.
After this CL, both the worker script request and the fetch request from the
shared worker will have "Save-Data: on" HTTP header if DataSaver is enabled.

BUG=722774
Review-Url: https://codereview.chromium.org/2881073003
Cr-Commit-Position: refs/heads/master@{#473461}
Committed: https://chromium.googlesource.com/chromium/src/+/ed54f8394f536183a96aa9678483e94282d07044

[horo, 2017-05-16 07:08:52]

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

[commit-bot: I haz the power, 2017-05-16 07:09:08]

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-05-16 07:20:29]

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

[commit-bot: I haz the power, 2017-05-16 07:20:30]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[horo, 2017-05-16 08:36:22]

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

[commit-bot: I haz the power, 2017-05-16 08:36:36]

Dry run: CQ is trying da patch.

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

[horo, 2017-05-16 08:37:57]

Description was changed from

==========
Support DataSaver for SharedWorker

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

to

==========
Support DataSaver for SharedWorker

BUG=443374
==========

[horo, 2017-05-16 08:40:04]

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

[horo, 2017-05-16 08:40:05]

falken@
Please review this.

[commit-bot: I haz the power, 2017-05-16 08:40:07]

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

[commit-bot: I haz the power, 2017-05-16 08:40:09]

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

[falken1, 2017-05-16 08:47:03]

falken@google.com changed reviewers:
+ falken@google.com

[falken1, 2017-05-16 08:47:04]

Wow, fast fix :)

Can we link to a dedicated bug instead of as part of off-main-thread?

[falken, 2017-05-16 08:47:32]

falken@chromium.org changed reviewers:
- falken@google.com

[falken, 2017-05-16 08:47:33]

sent from wrong account

[horo, 2017-05-16 10:27:49]

Description was changed from

==========
Support DataSaver for SharedWorker

BUG=443374
==========

to

==========
Support DataSaver for SharedWorker

BUG=722774
==========

[horo, 2017-05-16 10:32:38]

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

[commit-bot: I haz the power, 2017-05-16 10:32:55]

Dry run: CQ is trying da patch.

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

[horo, 2017-05-16 10:36:14]

On 2017/05/16 08:47:04, falken1 wrote:
> Wow, fast fix :)
> 
> Can we link to a dedicated bug instead of as part of off-main-thread?

done

[horo, 2017-05-16 13:59:05]

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

[commit-bot: I haz the power, 2017-05-16 13:59:22]

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-05-16 19:23:54]

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

[commit-bot: I haz the power, 2017-05-16 19:23:55]

Dry run: This issue passed the CQ dry run.

[falken, 2017-05-17 00:54:53]

https://codereview.chromium.org/2881073003/diff/80001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_instance.h (right):

https://codereview.chromium.org/2881073003/diff/80001/content/browser/shared_...
content/browser/shared_worker/shared_worker_instance.h:33: bool
data_saver_enabled);
consider an enum like WebDataSaverFlag { Enabled, Disabled } or adding /*
data_saver_enabled */ comments to the callsites.

https://codereview.chromium.org/2881073003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/virtual/off-main-thread-fetch/http/tests/fetch/chromium/data-saver-expected.txt
(right):

https://codereview.chromium.org/2881073003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/off-main-thread-fetch/http/tests/fetch/chromium/data-saver-expected.txt:7:
FAIL fetch() from shared worker with Data-Saver enabled. assert_equals:
Save-Data header should be sent when enabled. method: GET expected "Save-Data:
on" but got "No Save-Data"
Can you update the CL description and the bug to explain that this change uses
the SaveData header when requesting the shared worker script, but it doesn't yet
use the SaveData header for fetch() calls by the worker?

[horo, 2017-05-17 02:08:45]

Description was changed from

==========
Support DataSaver for SharedWorker

BUG=722774
==========

to

==========
Support DataSaver for SharedWorker

This CL pipes the DataSaver flag from the renderer process which creates a
Shared Worker to the shared worker process.
After this CL, both the worker script request and the fetch request from the
shared worker will have "Save-Data: on" HTTP header if DataSaver is enabled.

The worker script request will have the header even if off-main-thread-fetch is
enabled ("--enable-features=OffMainThreadFetch"). But the fetch request from the
shared worker will not have the header. So we can't remove the "-expected.txt"
file under virtual/off-main-thread-fetch/.
This will be fixed by https://codereview.chromium.org/2886663002/.

BUG=722774
==========

[kinuko, 2017-05-17 02:13:14]

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

[kinuko, 2017-05-17 02:13:15]

https://codereview.chromium.org/2881073003/diff/80001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_instance.h (right):

https://codereview.chromium.org/2881073003/diff/80001/content/browser/shared_...
content/browser/shared_worker/shared_worker_instance.h:33: bool
data_saver_enabled);
On 2017/05/17 00:54:53, falken wrote:
> consider an enum like WebDataSaverFlag { Enabled, Disabled } or adding /*
> data_saver_enabled */ comments to the callsites.

+1

[horo, 2017-05-17 02:15:42]

https://codereview.chromium.org/2881073003/diff/80001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_instance.h (right):

https://codereview.chromium.org/2881073003/diff/80001/content/browser/shared_...
content/browser/shared_worker/shared_worker_instance.h:33: bool
data_saver_enabled);
On 2017/05/17 00:54:53, falken wrote:
> consider an enum like WebDataSaverFlag { Enabled, Disabled } or adding /*
> data_saver_enabled */ comments to the callsites.

Done.

https://codereview.chromium.org/2881073003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/virtual/off-main-thread-fetch/http/tests/fetch/chromium/data-saver-expected.txt
(right):

https://codereview.chromium.org/2881073003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/off-main-thread-fetch/http/tests/fetch/chromium/data-saver-expected.txt:7:
FAIL fetch() from shared worker with Data-Saver enabled. assert_equals:
Save-Data header should be sent when enabled. method: GET expected "Save-Data:
on" but got "No Save-Data"
On 2017/05/17 00:54:53, falken wrote:
> Can you update the CL description and the bug to explain that this change uses
> the SaveData header when requesting the shared worker script, but it doesn't
yet
> use the SaveData header for fetch() calls by the worker?

Done.

[horo, 2017-05-17 02:15:48]

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

[commit-bot: I haz the power, 2017-05-17 02:16:46]

Dry run: CQ is trying da patch.

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

[horo, 2017-05-17 02:22:47]

Uploaded Patchset 7 which is a diff from
https://codereview.chromium.org/2886663002 which I'm submitting now.

https://codereview.chromium.org/2881073003/diff/80001/third_party/WebKit/Layo...
File
third_party/WebKit/LayoutTests/virtual/off-main-thread-fetch/http/tests/fetch/chromium/data-saver-expected.txt
(right):

https://codereview.chromium.org/2881073003/diff/80001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/virtual/off-main-thread-fetch/http/tests/fetch/chromium/data-saver-expected.txt:7:
FAIL fetch() from shared worker with Data-Saver enabled. assert_equals:
Save-Data header should be sent when enabled. method: GET expected "Save-Data:
on" but got "No Save-Data"
On 2017/05/17 02:15:42, horo wrote:
> On 2017/05/17 00:54:53, falken wrote:
> > Can you update the CL description and the bug to explain that this change
uses
> > the SaveData header when requesting the shared worker script, but it doesn't
> yet
> > use the SaveData header for fetch() calls by the worker?
> 
> Done.

Ah, I will submit https://codereview.chromium.org/2886663002 before this CL.
So I can remove this -expected.txt file.

[horo, 2017-05-17 02:22:58]

Description was changed from

==========
Support DataSaver for SharedWorker

This CL pipes the DataSaver flag from the renderer process which creates a
Shared Worker to the shared worker process.
After this CL, both the worker script request and the fetch request from the
shared worker will have "Save-Data: on" HTTP header if DataSaver is enabled.

The worker script request will have the header even if off-main-thread-fetch is
enabled ("--enable-features=OffMainThreadFetch"). But the fetch request from the
shared worker will not have the header. So we can't remove the "-expected.txt"
file under virtual/off-main-thread-fetch/.
This will be fixed by https://codereview.chromium.org/2886663002/.

BUG=722774
==========

to

==========
Support DataSaver for SharedWorker

This CL pipes the DataSaver flag from the renderer process which creates a
Shared Worker to the shared worker process.
After this CL, both the worker script request and the fetch request from the
shared worker will have "Save-Data: on" HTTP header if DataSaver is enabled.

BUG=722774
==========

[horo, 2017-05-17 02:30:47]

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

[commit-bot: I haz the power, 2017-05-17 02:31:53]

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-05-17 04:00:11]

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

[commit-bot: I haz the power, 2017-05-17 04:00:12]

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

[kinuko, 2017-05-17 06:13:22]

https://codereview.chromium.org/2881073003/diff/80001/content/browser/shared_...
File content/browser/shared_worker/shared_worker_instance.h (right):

https://codereview.chromium.org/2881073003/diff/80001/content/browser/shared_...
content/browser/shared_worker/shared_worker_instance.h:33: bool
data_saver_enabled);
On 2017/05/17 02:15:42, horo wrote:
> On 2017/05/17 00:54:53, falken wrote:
> > consider an enum like WebDataSaverFlag { Enabled, Disabled } or adding /*
> > data_saver_enabled */ comments to the callsites.
> 
> Done.

I know it's a bit of hassle, but could we use enum?

[horo, 2017-05-17 09:23:02]

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

[horo, 2017-05-17 09:23:05]

On 2017/05/17 06:13:22, kinuko wrote:
>
https://codereview.chromium.org/2881073003/diff/80001/content/browser/shared_...
> File content/browser/shared_worker/shared_worker_instance.h (right):
> 
>
https://codereview.chromium.org/2881073003/diff/80001/content/browser/shared_...
> content/browser/shared_worker/shared_worker_instance.h:33: bool
> data_saver_enabled);
> On 2017/05/17 02:15:42, horo wrote:
> > On 2017/05/17 00:54:53, falken wrote:
> > > consider an enum like WebDataSaverFlag { Enabled, Disabled } or adding /*
> > > data_saver_enabled */ comments to the callsites.
> > 
> > Done.
> 
> I know it's a bit of hassle, but could we use enum?

Done

[commit-bot: I haz the power, 2017-05-17 09:23:43]

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-05-17 09:29:51]

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

[commit-bot: I haz the power, 2017-05-17 09:29:52]

Dry run: Try jobs failed on following builders:
  android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
  cast_shell_android on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[horo, 2017-05-17 09:51:00]

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

[commit-bot: I haz the power, 2017-05-17 09:51:23]

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-05-17 11:34:07]

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

[commit-bot: I haz the power, 2017-05-17 11:34:08]

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

[falken, 2017-05-18 01:24:33]

lgtm, thanks for the enum

[kinuko, 2017-05-18 03:48:21]

https://codereview.chromium.org/2881073003/diff/180001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2881073003/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2713: :
blink::WebDataSaverFlag::kDisabled;
Ah sorry- I hadn't realized this change would propagate further.

We also have IsDataSaverEnabled code on this class which returns boolean, which
feels a little consistent now?

And totally orthogonal-- looking into further it looks what
GetDataSaverEnabledPref returns and IsDataSaverEnabled returns could be
different, is that ok?

...and then I found/recalled that we have EmbeddedWorkerSetting for service
worker case.

Ok, might it be cleaner if we just turn WebEmbeddedWorkerSettings in
WorkerSettings, put it under content/common and use it for both workers (with a
comment to note that v8 cache option is applicable only for SW)?

Either works, but what do you think? (Please comment back first if you're unsure
/ disagree)

Sorry for the back and forth!

https://codereview.chromium.org/2881073003/diff/180001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebDataSaverFlag.h (right):

https://codereview.chromium.org/2881073003/diff/180001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebDataSaverFlag.h:11: enum class WebDataSaverFlag
{ kDisabled = 0, kEnabled, kLast = kEnabled };
Does this need to be in public/web? (They're not that different today, but
public/platform seems totally fine too)

[horo, 2017-05-18 05:08:27]

https://codereview.chromium.org/2881073003/diff/180001/chrome/browser/chrome_...
File chrome/browser/chrome_content_browser_client.cc (right):

https://codereview.chromium.org/2881073003/diff/180001/chrome/browser/chrome_...
chrome/browser/chrome_content_browser_client.cc:2713: :
blink::WebDataSaverFlag::kDisabled;
On 2017/05/18 03:48:21, kinuko wrote:
> Ah sorry- I hadn't realized this change would propagate further.
> 
> We also have IsDataSaverEnabled code on this class which returns boolean,
which
> feels a little consistent now?

Are you saying "inconsistent"?
I can rename ChromeContentBrowserClient::IsDataSaverEnabled() to
GetDataSaverFlag() and make it to return the flag.
Shall I do so?

> And totally orthogonal-- looking into further it looks what
> GetDataSaverEnabledPref returns and IsDataSaverEnabled returns could be
> different, is that ok?

I think it is an existing bug that
ChromeContentBrowserClient::IsDataSaverEnabled() doesn't check FieldTrialList.
https://codereview.chromium.org/1698973002 introduced GetDataSaverEnabledPref()
which checks FieldTrialList.
https://codereview.chromium.org/1708583003 introduced IsDataSaverEnabled() which
doesn't check FieldTrialList.

+CC:rajendrant@, bengr@

> ...and then I found/recalled that we have EmbeddedWorkerSetting for service
> worker case.

I have already updated EmbeddedWorkerSetting.
https://codereview.chromium.org/2881073003/diff/180001/content/common/service...

> Ok, might it be cleaner if we just turn WebEmbeddedWorkerSettings in
> WorkerSettings, put it under content/common and use it for both workers (with
a
> comment to note that v8 cache option is applicable only for SW)?
>
> Either works, but what do you think? (Please comment back first if you're
unsure
> / disagree)

Currently we don't support V8CacheOptions for SharedWorker.
Yes, it is desirable to support it. But it is not in the scope of this CL.
And I think it is confusing to introduce a new structure with unused flag
(without tests).

If we need to check the DataSaverFlag out of FetchContext, we should have
WorkerSettings::GetDataSaverFlag(). But currently we don't need it.

[kinuko, 2017-05-18 05:18:19]

On 2017/05/18 05:08:27, horo wrote:
>
https://codereview.chromium.org/2881073003/diff/180001/chrome/browser/chrome_...
> File chrome/browser/chrome_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/2881073003/diff/180001/chrome/browser/chrome_...
> chrome/browser/chrome_content_browser_client.cc:2713: :
> blink::WebDataSaverFlag::kDisabled;
> On 2017/05/18 03:48:21, kinuko wrote:
> > Ah sorry- I hadn't realized this change would propagate further.
> > 
> > We also have IsDataSaverEnabled code on this class which returns boolean,
> which
> > feels a little consistent now?
> 
> Are you saying "inconsistent"?

Oops, yes that's what I wanted to say.

> I can rename ChromeContentBrowserClient::IsDataSaverEnabled() to
> GetDataSaverFlag() and make it to return the flag.
> Shall I do so?
> 
> > And totally orthogonal-- looking into further it looks what
> > GetDataSaverEnabledPref returns and IsDataSaverEnabled returns could be
> > different, is that ok?
> 
> I think it is an existing bug that
> ChromeContentBrowserClient::IsDataSaverEnabled() doesn't check FieldTrialList.
> https://codereview.chromium.org/1698973002 introduced
GetDataSaverEnabledPref()
> which checks FieldTrialList.
> https://codereview.chromium.org/1708583003 introduced IsDataSaverEnabled()
which
> doesn't check FieldTrialList.
> 
> +CC:rajendrant@, bengr@

Yep, looks like there's an existing bug.

> > ...and then I found/recalled that we have EmbeddedWorkerSetting for service
> > worker case.
> 
> I have already updated EmbeddedWorkerSetting.
>
https://codereview.chromium.org/2881073003/diff/180001/content/common/service...
> 
> > Ok, might it be cleaner if we just turn WebEmbeddedWorkerSettings in
> > WorkerSettings, put it under content/common and use it for both workers
(with
> a
> > comment to note that v8 cache option is applicable only for SW)?
> >
> > Either works, but what do you think? (Please comment back first if you're
> unsure
> > / disagree)
> 
> Currently we don't support V8CacheOptions for SharedWorker.
> Yes, it is desirable to support it. But it is not in the scope of this CL.
> And I think it is confusing to introduce a new structure with unused flag
> (without tests).

I wanted to say it feels more consistent if we do either:

1) use enum everywhere (including IsDataSaverEnabled), or
2) use settings bag type struct + boolean field everywhere (like we do in
WebPreferences / EmbeddedWorkerSettings)

I slightly prefer 2) over 1), but also felt creating a new struct only for a
single field might be a bit overkill, so I wondered if we can just reuse
EmbeddedWorkerSettings for both cases (with renaming).  And again either works
for me.  Is that clearer now?

> If we need to check the DataSaverFlag out of FetchContext, we should have
> WorkerSettings::GetDataSaverFlag(). But currently we don't need it.

[horo, 2017-05-18 06:01:06]

> 2) use settings bag type struct + boolean field everywhere (like we do in
> WebPreferences / EmbeddedWorkerSettings)WebPreferences /
EmbeddedWorkerSettings)

Why boolean??

[kinuko, 2017-05-18 06:47:16]

On 2017/05/18 06:01:06, horo wrote:
> > 2) use settings bag type struct + boolean field everywhere (like we do in
> > WebPreferences / EmbeddedWorkerSettings)WebPreferences /
> EmbeddedWorkerSettings)
> 
> Why boolean??

Enum is fine too.  The source of this value (preference value in profile) is
boolean, and using boolean field in struct for enable/disable flag is one of the
most common case, so I didn't strongly feel we need to use enum if we use
struct.

One more thing I probably should be clearer: we generally prefer enums over
boolean for function parameters, while having a boolean field in a struct or
using a boolean parameter for a setter method is different, because it's a named
field and it's much clearer what the boolean value is for.  (It's also written
in our style guide btw)  So if we bring a struct here just keeping to use a
boolean seems fine too, while struct + enum (or enum only) also works for me.

(Communication is hard!)

[horo, 2017-05-18 07:28:41]

On 2017/05/18 06:47:16, kinuko wrote:
> On 2017/05/18 06:01:06, horo wrote:
> > > 2) use settings bag type struct + boolean field everywhere (like we do in
> > > WebPreferences / EmbeddedWorkerSettings)WebPreferences /
> > EmbeddedWorkerSettings)
> > 
> > Why boolean??
> 
> Enum is fine too.  The source of this value (preference value in profile) is
> boolean, and using boolean field in struct for enable/disable flag is one of
the
> most common case, so I didn't strongly feel we need to use enum if we use
> struct.
> 
> One more thing I probably should be clearer: we generally prefer enums over
> boolean for function parameters, while having a boolean field in a struct or
> using a boolean parameter for a setter method is different, because it's a
named
> field and it's much clearer what the boolean value is for.  (It's also written
> in our style guide btw)  So if we bring a struct here just keeping to use a
> boolean seems fine too, while struct + enum (or enum only) also works for me.
> 
> (Communication is hard!)

The flag is sent from the page in SharedWorkerRepositoryClientImpl::Connect().
Do you prefer creating the struct here?

[kinuko, 2017-05-18 07:54:42]

On 2017/05/18 07:28:41, horo wrote:
> On 2017/05/18 06:47:16, kinuko wrote:
> > On 2017/05/18 06:01:06, horo wrote:
> > > > 2) use settings bag type struct + boolean field everywhere (like we do
in
> > > > WebPreferences / EmbeddedWorkerSettings)WebPreferences /
> > > EmbeddedWorkerSettings)
> > > 
> > > Why boolean??
> > 
> > Enum is fine too.  The source of this value (preference value in profile) is
> > boolean, and using boolean field in struct for enable/disable flag is one of
> the
> > most common case, so I didn't strongly feel we need to use enum if we use
> > struct.
> > 
> > One more thing I probably should be clearer: we generally prefer enums over
> > boolean for function parameters, while having a boolean field in a struct or
> > using a boolean parameter for a setter method is different, because it's a
> named
> > field and it's much clearer what the boolean value is for.  (It's also
written
> > in our style guide btw)  So if we bring a struct here just keeping to use a
> > boolean seems fine too, while struct + enum (or enum only) also works for
me.
> > 
> > (Communication is hard!)
> 
> The flag is sent from the page in SharedWorkerRepositoryClientImpl::Connect().
> Do you prefer creating the struct here?

How about creating a struct for the things that are passed down from parent
document to the worker, e.g. WebContentSecurityPolicyType, WebAddressSpace,
WebSharedWorkerCreationContextType and data saver flag?

[kinuko, 2017-05-18 08:00:42]

On 2017/05/18 07:54:42, kinuko wrote:
> On 2017/05/18 07:28:41, horo wrote:
> > On 2017/05/18 06:47:16, kinuko wrote:
> > > On 2017/05/18 06:01:06, horo wrote:
> > > > > 2) use settings bag type struct + boolean field everywhere (like we do
> in
> > > > > WebPreferences / EmbeddedWorkerSettings)WebPreferences /
> > > > EmbeddedWorkerSettings)
> > > > 
> > > > Why boolean??
> > > 
> > > Enum is fine too.  The source of this value (preference value in profile)
is
> > > boolean, and using boolean field in struct for enable/disable flag is one
of
> > the
> > > most common case, so I didn't strongly feel we need to use enum if we use
> > > struct.
> > > 
> > > One more thing I probably should be clearer: we generally prefer enums
over
> > > boolean for function parameters, while having a boolean field in a struct
or
> > > using a boolean parameter for a setter method is different, because it's a
> > named
> > > field and it's much clearer what the boolean value is for.  (It's also
> written
> > > in our style guide btw)  So if we bring a struct here just keeping to use
a
> > > boolean seems fine too, while struct + enum (or enum only) also works for
> me.
> > > 
> > > (Communication is hard!)
> > 
> > The flag is sent from the page in
SharedWorkerRepositoryClientImpl::Connect().
> > Do you prefer creating the struct here?
> 
> How about creating a struct for the things that are passed down from parent
> document to the worker, e.g. WebContentSecurityPolicyType, WebAddressSpace,
> WebSharedWorkerCreationContextType and data saver flag?

Also- feel free to land enum or bool version (like PS6) for now and work on
cleanup in a follow-up.  Proactively sending lgtm as I'll be afk soon and I feel
I'm blocking you.  Thanks for your patience!

[horo, 2017-05-18 08:12:52]

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

[horo, 2017-05-18 08:12:57]

Binding WebContentSecurityPolicyType, WebAddressSpace,
WebSharedWorkerCreationContextType and data saver flag sounds good to me.
(We should send the WebSharedWorkerCreationContextType for crbug.com/723575.)

But I want to use boolean for this CL.
Uploaded Patch Set 11.

[commit-bot: I haz the power, 2017-05-18 08:13:14]

Dry run: CQ is trying da patch.

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

[horo, 2017-05-18 08:17:07]

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

[horo, 2017-05-18 08:17:08]

mkwst@
Could you please review  content/common/view_messages.h and 
content/common/worker_messages.h?

[commit-bot: I haz the power, 2017-05-18 09:59:43]

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

[commit-bot: I haz the power, 2017-05-18 09:59:44]

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

[horo, 2017-05-19 23:13:50]

On 2017/05/18 08:17:08, horo wrote:
> mkwst@
> Could you please review  content/common/view_messages.h and 
> content/common/worker_messages.h?

ping?

[Mike West, 2017-05-20 20:49:18]

IPC LGTM.

[horo, 2017-05-21 03:29:15]

The CQ bit was checked by horo@chromium.org

[horo, 2017-05-21 03:29:16]

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/2881073003/#ps200001
(title: "same as PS6")

[commit-bot: I haz the power, 2017-05-21 03:29:25]

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-05-21 05:25:50]

CQ is committing da patch.

Bot data: {"patchset_id": 200001, "attempt_start_ts": 1495337355890150,
"parent_rev": "cb23bc6b0dae9c82bb9845b4ea367d72e8c2edaf", "commit_rev":
"ed54f8394f536183a96aa9678483e94282d07044"}

[commit-bot: I haz the power, 2017-05-21 05:26:01]

Description was changed from

==========
Support DataSaver for SharedWorker

This CL pipes the DataSaver flag from the renderer process which creates a
Shared Worker to the shared worker process.
After this CL, both the worker script request and the fetch request from the
shared worker will have "Save-Data: on" HTTP header if DataSaver is enabled.

BUG=722774
==========

to

==========
Support DataSaver for SharedWorker

This CL pipes the DataSaver flag from the renderer process which creates a
Shared Worker to the shared worker process.
After this CL, both the worker script request and the fetch request from the
shared worker will have "Save-Data: on" HTTP header if DataSaver is enabled.

BUG=722774

Review-Url: https://codereview.chromium.org/2881073003
Cr-Commit-Position: refs/heads/master@{#473461}
Committed:
https://chromium.googlesource.com/chromium/src/+/ed54f8394f536183a96aa9678483...
==========

[commit-bot: I haz the power, 2017-05-21 05:26:02]

Committed patchset #11 (id:200001) as
https://chromium.googlesource.com/chromium/src/+/ed54f8394f536183a96aa9678483...