Add UMA to count the usage of sendBeacon with non-simple Content-Type

Add UMA to count the usage of sendBeacon with non-simple Content-Type

Also add stuff to allow for Finch based control.

R=mkwst@chromium.org,isherman@chromium.org,jochen@chromium.org,yhirano@chromium.org
BUG=490015
Committed: https://crrev.com/035deef5d982dceebbc9fde4d9743a34ddc0bc72
Cr-Commit-Position: refs/heads/master@{#409713}

[tyoshino (SeeGerritForStatus), 2016-07-27 09:09:03]

tyoshino@chromium.org changed reviewers:
+ isherman@chromium.org, mkwst@chromium.org

[Ilya Sherman, 2016-07-27 16:28:00]

histograms.xml lgtm

[tyoshino (SeeGerritForStatus), 2016-07-28 05:12:53]

Changed not to count in ArrayBufferView. It has been sending "Content-Type:
application/octet-stream" but it's not specified in the spec.

[Mike West, 2016-07-28 09:14:22]

Skimming the associated bug, I hope we can quickly align with Mozilla's
behavior. Would you consider hardening this patch for users who have opted into
experimental behavior by throwing for non-safe MIME types? Throwing for folks in
"developer" mode is more likely to produce bug reports so we can evaluate the
extent of breakage more quickly.

https://codereview.chromium.org/2184973002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2184973002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:77282: +  <int value="1468"
label="SendBeaconWithNonSimpleContentType"/>
Do we have a count of the number of `sendBeacon` calls total? If we don't, I'd
suggest adding it, as it would be helpful in evaluating the number this counter
gives us.

[tyoshino (SeeGerritForStatus), 2016-07-28 11:47:39]

https://codereview.chromium.org/2184973002/diff/40001/tools/metrics/histogram...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2184973002/diff/40001/tools/metrics/histogram...
tools/metrics/histograms/histograms.xml:77282: +  <int value="1468"
label="SendBeaconWithNonSimpleContentType"/>
On 2016/07/28 09:14:22, Mike West wrote:
> Do we have a count of the number of `sendBeacon` calls total? If we don't, I'd
> suggest adding it, as it would be helpful in evaluating the number this
counter
> gives us.

Yes, by IDL.
https://codesearch.chromium.org/chromium/src/third_party/WebKit/Source/module...

https://www.chromestatus.com/metrics/feature/timeline/popularity/494

We can compare it with the total.

[tyoshino (SeeGerritForStatus), 2016-07-28 11:48:48]

On 2016/07/28 09:14:22, Mike West wrote:
> Skimming the associated bug, I hope we can quickly align with Mozilla's
> behavior. Would you consider hardening this patch for users who have opted
into
> experimental behavior by throwing for non-safe MIME types? Throwing for folks
in
> "developer" mode is more likely to produce bug reports so we can evaluate the
> extent of breakage more quickly.

Sounds good. Which are you suggesting, to enable it on Dev/Canary, or to enable
it for enable experimental platform features flag users.

[Mike West, 2016-07-28 12:36:50]

On 2016/07/28 at 11:48:48, tyoshino wrote:
> On 2016/07/28 09:14:22, Mike West wrote:
> > Skimming the associated bug, I hope we can quickly align with Mozilla's
> > behavior. Would you consider hardening this patch for users who have opted
into
> > experimental behavior by throwing for non-safe MIME types? Throwing for
folks in
> > "developer" mode is more likely to produce bug reports so we can evaluate
the
> > extent of breakage more quickly.
> 
> Sounds good. Which are you suggesting, to enable it on Dev/Canary, or to
enable it for enable experimental platform features flag users.

Ideally, you'd put the behavior behind a runtime flag which is tied to a feature
flag, which wires it up to Finch. Then based on the numbers we see, we can turn
it on dynamically.

[tyoshino (SeeGerritForStatus), 2016-07-29 07:07:06]

On 2016/07/28 12:36:50, Mike West wrote:
> Ideally, you'd put the behavior behind a runtime flag which is tied to a
feature
> flag, which wires it up to Finch. Then based on the numbers we see, we can
turn
> it on dynamically.

Added the RuntimeEnabledFeatures item and the content_features item to allow for
Finch based control.

[tyoshino (SeeGerritForStatus), 2016-07-29 07:11:07]

On 2016/07/29 07:07:06, tyoshino wrote:
> On 2016/07/28 12:36:50, Mike West wrote:
> > Ideally, you'd put the behavior behind a runtime flag which is tied to a
> feature
> > flag, which wires it up to Finch. Then based on the numbers we see, we can
> turn
> > it on dynamically.
> 
> Added the RuntimeEnabledFeatures item and the content_features item to allow
for
> Finch based control.

I skipped addition of an about_flags item because that it's basically
discouraged to provide an about://flags entry for this kind of usage and that
it's unlikely to bother users by this change as long as it's used properly (e.g.
in unload).

[tyoshino (SeeGerritForStatus), 2016-07-29 07:11:48]

Description was changed from

==========
Add UMA to count the usage of sendBeacon with non-simple Content-Type

BUG=490015
==========

to

==========
Add UMA to count the usage of sendBeacon with non-simple Content-Type

Also add stuff to allow for Finch based control.

BUG=490015
==========

[Mike West, 2016-08-01 11:44:19]

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

[Mike West, 2016-08-01 11:44:20]

LGTM, but you'll need a //content OWNER for the bits I can't stamp.

Jochen, perhaps?

[Mike West, 2016-08-01 11:44:47]

https://codereview.chromium.org/2184973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2184973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:248:
SendBeaconThrowForBlobWithNonSimpleType
I'd make this `experimental`.

[jochen (gone - plz use gerrit), 2016-08-01 12:06:45]

can we have a test for that behavior please?

[tyoshino (SeeGerritForStatus), 2016-08-02 11:30:02]

https://codereview.chromium.org/2184973002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in (right):

https://codereview.chromium.org/2184973002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/RuntimeEnabledFeatures.in:248:
SendBeaconThrowForBlobWithNonSimpleType
On 2016/08/01 11:44:47, Mike West wrote:
> I'd make this `experimental`.

Done.

[tyoshino (SeeGerritForStatus), 2016-08-02 11:30:52]

On 2016/08/01 12:06:45, jochen wrote:
> can we have a test for that behavior please?

Added a layout test after change runtime_features.cc not to copy the feature
flag value but turn the web platform feature flag on when the feature flag is
on.

[tyoshino (SeeGerritForStatus), 2016-08-02 11:31:02]

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

[tyoshino (SeeGerritForStatus), 2016-08-02 11:31:09]

The CQ bit was unchecked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-08-02 11:36:28]

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

[commit-bot: I haz the power, 2016-08-02 11:36:39]

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-02 12:57:13]

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

[commit-bot: I haz the power, 2016-08-02 12:57:16]

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

[jochen (gone - plz use gerrit), 2016-08-02 15:31:12]

lgtm

[tyoshino (SeeGerritForStatus), 2016-08-03 07:38:27]

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

[commit-bot: I haz the power, 2016-08-03 07:38:39]

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

[tyoshino (SeeGerritForStatus), 2016-08-03 07:38:39]

tyoshino@chromium.org changed reviewers:
+ yhirano@chromium.org

[tyoshino (SeeGerritForStatus), 2016-08-03 07:40:46]

+yhirano

yhirano, please review layout test files. After jochen's review on ps7, I
noticed that beacon-cross-origin-blob.html is not appropriate as it's not only
for cross-origin cases, and renamed it. And, I noticed that the existing
beacon-cross-origin-redirect-blob.html test is failing due to the change, and
moved it to virtual/stable to keep it tested.

[yhirano, 2016-08-03 07:46:27]

On 2016/08/03 07:40:46, tyoshino wrote:
> +yhirano
> 
> yhirano, please review layout test files. After jochen's review on ps7, I
> noticed that beacon-cross-origin-blob.html is not appropriate as it's not only
> for cross-origin cases, and renamed it. And, I noticed that the existing
> beacon-cross-origin-redirect-blob.html test is failing due to the change, and
> moved it to virtual/stable to keep it tested.

I'm a bit confused. Don't you want to add
virtual/stable/http/tests/navigation/beacon-blob-with-non-simple-type-expected.txt
rather than
virtual/stable/http/tests/navigation/beacon-cross-origin-redirect-blob-expected.txt?

[yhirano, 2016-08-03 07:47:12]

https://codereview.chromium.org/2184973002/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2184973002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1396: crbug.com/490015
http/tests/navigation/beacon-cross-origin-redirect-blob.html [ Skip ]
Adding some comments may be helpful.

[tyoshino (SeeGerritForStatus), 2016-08-03 07:56:05]

On 2016/08/03 07:46:27, yhirano wrote:
> On 2016/08/03 07:40:46, tyoshino wrote:
> > +yhirano
> > 
> > yhirano, please review layout test files. After jochen's review on ps7, I
> > noticed that beacon-cross-origin-blob.html is not appropriate as it's not
only
> > for cross-origin cases, and renamed it. And, I noticed that the existing
> > beacon-cross-origin-redirect-blob.html test is failing due to the change,
and
> > moved it to virtual/stable to keep it tested.
> 
> I'm a bit confused. Don't you want to add
>
virtual/stable/http/tests/navigation/beacon-blob-with-non-simple-type-expected.txt

What this could test is tested by
virtual/stable/http/tests/navigation/beacon-cross-origin-redirect-blob-expected.txt
as it runs sendBeacon with a Blob with non-simple type
"application/octet-stream".

> rather than
>
virtual/stable/http/tests/navigation/beacon-cross-origin-redirect-blob-expected.txt?

This is needed to keep the redirect behavior tested with
experimental-web-platform-features not enabled.

[tyoshino (SeeGerritForStatus), 2016-08-03 08:00:41]

https://codereview.chromium.org/2184973002/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/TestExpectations (right):

https://codereview.chromium.org/2184973002/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/TestExpectations:1396: crbug.com/490015
http/tests/navigation/beacon-cross-origin-redirect-blob.html [ Skip ]
On 2016/08/03 07:47:12, yhirano wrote:
> Adding some comments may be helpful.

Done.

[yhirano, 2016-08-03 08:01:41]

On 2016/08/03 07:56:05, tyoshino wrote:
> On 2016/08/03 07:46:27, yhirano wrote:
> > On 2016/08/03 07:40:46, tyoshino wrote:
> > > +yhirano
> > > 
> > > yhirano, please review layout test files. After jochen's review on ps7, I
> > > noticed that beacon-cross-origin-blob.html is not appropriate as it's not
> only
> > > for cross-origin cases, and renamed it. And, I noticed that the existing
> > > beacon-cross-origin-redirect-blob.html test is failing due to the change,
> and
> > > moved it to virtual/stable to keep it tested.
> > 
> > I'm a bit confused. Don't you want to add
> >
>
virtual/stable/http/tests/navigation/beacon-blob-with-non-simple-type-expected.txt
> 
> What this could test is tested by
>
virtual/stable/http/tests/navigation/beacon-cross-origin-redirect-blob-expected.txt
> as it runs sendBeacon with a Blob with non-simple type
> "application/octet-stream".
> 
> > rather than
> >
>
virtual/stable/http/tests/navigation/beacon-cross-origin-redirect-blob-expected.txt?
> 
> This is needed to keep the redirect behavior tested with
> experimental-web-platform-features not enabled.

Ah, I see. LGTM.

[tyoshino (SeeGerritForStatus), 2016-08-03 10:02:01]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-08-03 10:02:01]

The patchset sent to the CQ was uploaded after l-g-t-m from
isherman@chromium.org, mkwst@chromium.org, jochen@chromium.org,
yhirano@chromium.org
Link to the patchset: https://codereview.chromium.org/2184973002/#ps200001
(title: "Mark
virtual/stable/http/tests/navigation/beacon-blob-with-non-simple-type.html
skipped")

[commit-bot: I haz the power, 2016-08-03 10:02:11]

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

[tyoshino (SeeGerritForStatus), 2016-08-03 10:03:26]

I've made last minute change to TestExpectations to skip
virtual/stable/http/tests/navigation/beacon-blob-with-non-simple-type.html.
Regardless of for which test case there expectation files are, all the tests
corresponding to the virtual/stable directory are run.
beacon-blob-with-non-simple-type.html shouldn't be run.

[commit-bot: I haz the power, 2016-08-03 10:03:42]

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

[commit-bot: I haz the power, 2016-08-03 10:03:44]

Try jobs failed on following builders:
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[tyoshino (SeeGerritForStatus), 2016-08-03 10:06:38]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-08-03 10:06:39]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
isherman@chromium.org, yhirano@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2184973002/#ps220001
(title: "Rebase")

[tyoshino (SeeGerritForStatus), 2016-08-03 10:06:41]

The CQ bit was unchecked by tyoshino@chromium.org

[commit-bot: I haz the power, 2016-08-03 10:06:45]

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

[tyoshino (SeeGerritForStatus), 2016-08-03 10:07:26]

Description was changed from

==========
Add UMA to count the usage of sendBeacon with non-simple Content-Type

Also add stuff to allow for Finch based control.

BUG=490015
==========

to

==========
Add UMA to count the usage of sendBeacon with non-simple Content-Type

Also add stuff to allow for Finch based control.

R=mkwst@chromium.org,isherman@chromium.org,jochen@chromium.org,yhirano@chromi...
BUG=490015
==========

[tyoshino (SeeGerritForStatus), 2016-08-03 10:07:35]

The CQ bit was checked by tyoshino@chromium.org

[commit-bot: I haz the power, 2016-08-03 10:07:45]

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-03 11:25:23]

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

[commit-bot: I haz the power, 2016-08-03 11:25:25]

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

[tyoshino (SeeGerritForStatus), 2016-08-03 12:03:21]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-08-03 12:03:23]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
isherman@chromium.org, yhirano@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2184973002/#ps240001
(title: "Disable same-and-different-back.html")

[commit-bot: I haz the power, 2016-08-03 12:03:38]

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-03 16:02:49]

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

[commit-bot: I haz the power, 2016-08-03 16:02:51]

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

[tyoshino (SeeGerritForStatus), 2016-08-04 01:23:25]

The CQ bit was checked by tyoshino@chromium.org

[tyoshino (SeeGerritForStatus), 2016-08-04 01:23:26]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
isherman@chromium.org, yhirano@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/2184973002/#ps260001
(title: "Rebase")

[commit-bot: I haz the power, 2016-08-04 01:23:44]

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-04 03:25:46]

Description was changed from

==========
Add UMA to count the usage of sendBeacon with non-simple Content-Type

Also add stuff to allow for Finch based control.

R=mkwst@chromium.org,isherman@chromium.org,jochen@chromium.org,yhirano@chromi...
BUG=490015
==========

to

==========
Add UMA to count the usage of sendBeacon with non-simple Content-Type

Also add stuff to allow for Finch based control.

R=mkwst@chromium.org,isherman@chromium.org,jochen@chromium.org,yhirano@chromi...
BUG=490015
==========

[commit-bot: I haz the power, 2016-08-04 03:25:48]

Committed patchset #14 (id:260001)

[commit-bot: I haz the power, 2016-08-04 03:27:26]

Description was changed from

==========
Add UMA to count the usage of sendBeacon with non-simple Content-Type

Also add stuff to allow for Finch based control.

R=mkwst@chromium.org,isherman@chromium.org,jochen@chromium.org,yhirano@chromi...
BUG=490015
==========

to

==========
Add UMA to count the usage of sendBeacon with non-simple Content-Type

Also add stuff to allow for Finch based control.

R=mkwst@chromium.org,isherman@chromium.org,jochen@chromium.org,yhirano@chromi...
BUG=490015

Committed: https://crrev.com/035deef5d982dceebbc9fde4d9743a34ddc0bc72
Cr-Commit-Position: refs/heads/master@{#409713}
==========

[commit-bot: I haz the power, 2016-08-04 03:27:28]

Patchset 14 (id:??) landed as
https://crrev.com/035deef5d982dceebbc9fde4d9743a34ddc0bc72
Cr-Commit-Position: refs/heads/master@{#409713}