Support DataSaver for SharedWorker

chrome/browser/chrome_content_browser_client.cc

Index: chrome/browser/chrome_content_browser_client.cc
diff --git a/chrome/browser/chrome_content_browser_client.cc b/chrome/browser/chrome_content_browser_client.cc
index 0738496bfc2a5ce97414bf885c594f7055105be4..b67b6656da1b443e872d220f866f8f4ab197ad1a 100644
--- a/chrome/browser/chrome_content_browser_client.cc
+++ b/chrome/browser/chrome_content_browser_client.cc
@@ -208,6 +208,7 @@
 #include "storage/browser/fileapi/external_mount_points.h"
 #include "third_party/WebKit/public/platform/modules/installedapp/installed_app_provider.mojom.h"
 #include "third_party/WebKit/public/platform/modules/webshare/webshare.mojom.h"
+#include "third_party/WebKit/public/web/WebDataSaverFlag.h"
 #include "ui/base/l10n/l10n_util.h"
 #include "ui/base/resource/resource_bundle.h"
 #include "ui/resources/grit/ui_resources.h"
@@ -2707,7 +2708,9 @@ void ChromeContentBrowserClient::OverrideWebkitPrefs(
     web_prefs->strict_powerful_feature_restrictions = true;
   }
 
-  web_prefs->data_saver_enabled = GetDataSaverEnabledPref(prefs);
+  web_prefs->data_saver_flag = GetDataSaverEnabledPref(prefs)
+                                   ? blink::WebDataSaverFlag::kEnabled
+                                   : blink::WebDataSaverFlag::kDisabled;

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

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!

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

Are you saying "inconsistent"?
I can rename ChromeContentBrowserClient::IsDataSaverEnabled() to
GetDataSaverFlag() and make it to return the flag.
Shall I do so?
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@
I have already updated EmbeddedWorkerSetting.
https://codereview.chromium.org/2881073003/diff/180001/content/common/service...
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.

 
 #if defined(OS_ANDROID)
   content::WebContents* contents =