[DocWriteBlocking] Add feature to block doc.written scripts if 2g-like network

[DocWriteBlocking] Add feature to block doc.written scripts if 2g-like network

Use the NetworkQualityEstimator to add the ability to block synchronous script
loading with document.write if the network conditions are 2G or 2G-like.

BUG=633569

[jkarlin, 2016-08-25 16:58:57]

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

[commit-bot: I haz the power, 2016-08-25 16:59:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jkarlin, 2016-08-25 17:42:12]

jkarlin@chromium.org changed reviewers:
+ japhet@chromium.org

[jkarlin, 2016-08-25 17:42:13]

japhet@ PTAL at the WebKit/ code, thanks!

[jkarlin, 2016-08-25 17:43:15]

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

[commit-bot: I haz the power, 2016-08-25 17:44:09]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jkarlin, 2016-08-25 18:34:16]

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

[commit-bot: I haz the power, 2016-08-25 18:34: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, 2016-08-25 20:30:30]

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

[commit-bot: I haz the power, 2016-08-25 20:30:31]

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_...)

[Yoav Weiss, 2016-09-04 05:11:42]

yoav@yoav.ws changed reviewers:
+ yoav@yoav.ws

[Yoav Weiss, 2016-09-04 05:11:43]

Drive-by comments/questions

https://codereview.chromium.org/2279543004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2279543004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:97: return false;
I'm new to this code, but won't that prevent scripts from running even if we
already have them in HTTP cache or SW cache? Are we betting on the fact that
document.written scripts are probably dynamic?

https://codereview.chromium.org/2279543004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:151: const bool
is2GOrLike2G = is2G || effectiveConnection == WebEffectiveConnectionType::Type2G
|| effectiveConnection == WebEffectiveConnectionType::TypeSlow2G;
Would it have been better to change the WebEffectiveConnectionType so that
Unknown is last, and we could define `<Type2G` here?

[jkarlin, 2016-09-06 11:58:45]

Hi Yoav, thanks for looking.

Nate PTAL!

https://codereview.chromium.org/2279543004/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right):

https://codereview.chromium.org/2279543004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:97: return false;
On 2016/09/04 05:11:43, Yoav Weiss wrote:
> I'm new to this code, but won't that prevent scripts from running even if we
> already have them in HTTP cache or SW cache? Are we betting on the fact that
> document.written scripts are probably dynamic?

If shouldDisallowFetchForMainFrameScript returns true, then we do check the
cache but don't go to the network.

https://codereview.chromium.org/2279543004/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:151: const bool
is2GOrLike2G = is2G || effectiveConnection == WebEffectiveConnectionType::Type2G
|| effectiveConnection == WebEffectiveConnectionType::TypeSlow2G;
On 2016/09/04 05:11:43, Yoav Weiss wrote:
> Would it have been better to change the WebEffectiveConnectionType so that
> Unknown is last, and we could define `<Type2G` here?

I don't think a strict ordering on the enum makes much sense. Connection types
will come and go over time and will be of various speeds. Once they're used by
histograms they can only be appended to.

That said, I do think a "isConnectionTypeSlow()" that uses a switch to make sure
we've addressed all of the connection types would be better.

[Nate Chapin, 2016-09-06 20:55:36]

Agreed that a helper to associated slow enum values would be nice. LGTM
otherwise.

[jkarlin, 2016-09-08 13:13:26]

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

[commit-bot: I haz the power, 2016-09-08 13:13:39]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jkarlin, 2016-09-08 13:13:41]

On 2016/09/06 20:55:36, Nate Chapin wrote:
> Agreed that a helper to associated slow enum values would be nice. LGTM
> otherwise.

Done, and thanks!

[jkarlin, 2016-09-08 13:14:57]

jkarlin@chromium.org changed reviewers:
+ jochen@chromium.org

[jkarlin, 2016-09-08 13:14:57]

jochen@chromium.org: Please review content/ changes and
RuntimeEnabledFeatures.in for owners. Thanks!

[jochen (gone - plz use gerrit), 2016-09-08 13:52:24]

can you please link the intent to implement thread from the CL description

Is it possible to add a test for this?

https://codereview.chromium.org/2279543004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2279543004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:38:
BlockDocWriteIfEffectively2G
I think this should be status=experimental if we also finch it already, no?

[commit-bot: I haz the power, 2016-09-08 14:47:48]

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

[commit-bot: I haz the power, 2016-09-08 14:47:49]

Dry run: This issue passed the CQ dry run.

[jkarlin, 2016-09-08 18:48:26]

I'll add tests, but it will depend on another CL that I need to write. So let's
put this one on hold for a few days.

https://codereview.chromium.org/2279543004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2279543004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:38:
BlockDocWriteIfEffectively2G
On 2016/09/08 13:52:24, jochen wrote:
> I think this should be status=experimental if we also finch it already, no?

There are three features related to doc write script injection. It's easier to
test and finch if we enable each one explicitly in tests/finch configs.

Once we figure out which we want to launch we can change it to
experimental/stable.

The three features:

1) Block on 2g only
2) Block on 2g and 2g-like
3) Preload doc-written scripts

WDYT?

[jochen (gone - plz use gerrit), 2016-09-09 13:43:09]

ok, lgtm

[jkarlin, 2016-09-13 18:26:55]

This CL has completely changed since LGTMs so I'm closing this one and have
created https://codereview.chromium.org/2338623004 to make reviewing easier.

[jkarlin, 2016-09-13 18:27:10]

Patchset #7 (id:120001) has been deleted

[jkarlin, 2016-09-13 18:27:18]

Description was changed from

==========
[DocWriteBlocking] Add feature to block doc.written scripts if 2g-like network

Use the NetworkQualityEstimator to add the ability to block synchronous script
loading with document.write if the network conditions are 2G or 2G-like.

BUG=633569
==========

to

==========
[DocWriteBlocking] Add feature to block doc.written scripts if 2g-like network

Use the NetworkQualityEstimator to add the ability to block synchronous script
loading with document.write if the network conditions are 2G or 2G-like.

BUG=633569
==========