WebRequest API: add more resource types

WebRequest API: add more resource types

Added new resource types "font" and "ping" to identify fonts, <a ping>
and navigator.sendBeacon requests. Existing types are also extended:
Workers (web workers, shared workers, service workers) are mapped to
"script" and favicons are mapped to "image". Plugin requests are also
labeled as "object".
(all of these mentioned requests were previously labeled as "other").

Added two resource types:
- RESOURCE_TYPE_CSP_REPORT; Previously RequestContextCSPReport mapped to
  RESOURCE_TYPE_PING. ping and beacons are commonly used for tracking,
  whereas CSP reports aid the security of website owners. By not
  including CSP reports in the ping type, extensions that disable ping
  will not inadvertently block security features of a website.

- RESOURCE_TYPE_PLUGIN_RESOURCE; plugin requests should be tagged as
  "object" instead of "other". But we cannot use RESOURCE_TYPE_OBJECT
  because this is used by MimeTypeResourceHandler::SelectNextHandler to
  determine whether to intercept the resource and display the result in
  a plugin.

BUG=80230, 410382, 512406
TEST=ExtensionWebRequestApiTest.*

Committed: https://crrev.com/6a32a203bbae04ef29a42f3f364ec00a0adf9179
Cr-Commit-Position: refs/heads/master@{#366632}

[robwu, 2015-12-12 09:04:14]

Patchset #1 (id:1) has been deleted

[robwu, 2015-12-12 09:04:26]

Patchset #1 (id:20001) has been deleted

[robwu, 2015-12-12 09:04:33]

Patchset #1 (id:40001) has been deleted

[robwu, 2015-12-12 09:04:48]

Patchset #1 (id:60001) has been deleted

[robwu, 2015-12-12 09:04:55]

Patchset #1 (id:80001) has been deleted

[robwu, 2015-12-12 09:05:01]

Patchset #1 (id:100001) has been deleted

[robwu, 2015-12-12 13:32:01]

Patchset #1 (id:120001) has been deleted

[robwu, 2015-12-12 16:34:17]

rob@robwu.nl changed reviewers:
+ battre@chromium.org, creis@chromium.org, mattm@chromium.org, mek@chromium.org

[robwu, 2015-12-12 16:34:17]

Hi all, please review.

Dominic: *web_request* = most of the significant changes.

mattm: chrome/browser/safe_browsing/* stamp

Charlie: content/ stamp

Marijn: extensions/ stamp

[Charlie Reis, 2015-12-14 20:53:23]

content/ seems reasonable to me, with nits.  Happy to rubber stamp once battre@
has approved.

https://codereview.chromium.org/1515703005/diff/140001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler_unittest.cc (right):

https://codereview.chromium.org/1515703005/diff/140001/content/browser/loader...
content/browser/loader/mime_type_resource_handler_unittest.cc:321: // PLugin
resource request with download not allowed. Stream shouldn't be
nit: Plugin

https://codereview.chromium.org/1515703005/diff/140001/content/child/web_url_...
File content/child/web_url_request_util.cc (right):

https://codereview.chromium.org/1515703005/diff/140001/content/child/web_url_...
content/child/web_url_request_util.cc:142: // Subresource of plugins
nit: List below TYPE_PING to be alphabetical.  (I realize there's some
exceptions below, but we might as well stick with the general trend for new
code.)

https://codereview.chromium.org/1515703005/diff/140001/content/public/common/...
File content/public/common/resource_type.h (right):

https://codereview.chromium.org/1515703005/diff/140001/content/public/common/...
content/public/common/resource_type.h:31: RESOURCE_TYPE_CSP_REPORT = 16,      //
a CSP report
nit: Add period to be self-consistent.

[mattm, 2015-12-15 00:40:37]

+nparker fyi

safe_browsing lgtm

https://codereview.chromium.org/1515703005/diff/140001/content/public/common/...
File content/public/common/resource_type.h (right):

https://codereview.chromium.org/1515703005/diff/140001/content/public/common/...
content/public/common/resource_type.h:32: RESOURCE_TYPE_PLUGIN_RESOURCE = 17, //
a resource that a plugin requested.
Update ContentResourceType in histograms.xml

[battre, 2015-12-15 12:53:08]

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

[battre, 2015-12-15 12:53:09]

web_request stuff LGTM.

Jochen, are you ok with adding additional resource types? Asking you because you
requested to limit this in the past.

https://codereview.chromium.org/1515703005/diff/140001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

https://codereview.chromium.org/1515703005/diff/140001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/framework.js:175: function
isUnexpectedDetachedRequest(name, details) {
Could you add some more comments that explain what's going on here?

https://codereview.chromium.org/1515703005/diff/140001/content/public/common/...
File content/public/common/resource_type.h (right):

https://codereview.chromium.org/1515703005/diff/140001/content/public/common/...
content/public/common/resource_type.h:31: RESOURCE_TYPE_CSP_REPORT = 16,      //
a CSP report
How about explaining the abbreviation in the comment?

https://codereview.chromium.org/1515703005/diff/140001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api_helpers.cc (left):

https://codereview.chromium.org/1515703005/diff/140001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api_helpers.cc:76:
content::RESOURCE_TYPE_LAST_TYPE,
Jochen: FYI

[robwu, 2015-12-15 13:17:25]

https://codereview.chromium.org/1515703005/diff/140001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

https://codereview.chromium.org/1515703005/diff/140001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/framework.js:175: function
isUnexpectedDetachedRequest(name, details) {
On 2015/12/15 12:53:09, battre wrote:
> Could you add some more comments that explain what's going on here?

Done.

https://codereview.chromium.org/1515703005/diff/140001/content/browser/loader...
File content/browser/loader/mime_type_resource_handler_unittest.cc (right):

https://codereview.chromium.org/1515703005/diff/140001/content/browser/loader...
content/browser/loader/mime_type_resource_handler_unittest.cc:321: // PLugin
resource request with download not allowed. Stream shouldn't be
On 2015/12/14 20:53:23, Charlie Reis wrote:
> nit: Plugin

Done.

https://codereview.chromium.org/1515703005/diff/140001/content/child/web_url_...
File content/child/web_url_request_util.cc (right):

https://codereview.chromium.org/1515703005/diff/140001/content/child/web_url_...
content/child/web_url_request_util.cc:142: // Subresource of plugins
On 2015/12/14 20:53:23, Charlie Reis wrote:
> nit: List below TYPE_PING to be alphabetical.  (I realize there's some
> exceptions below, but we might as well stick with the general trend for new
> code.)

Done.

https://codereview.chromium.org/1515703005/diff/140001/content/public/common/...
File content/public/common/resource_type.h (right):

https://codereview.chromium.org/1515703005/diff/140001/content/public/common/...
content/public/common/resource_type.h:31: RESOURCE_TYPE_CSP_REPORT = 16,      //
a CSP report
On 2015/12/14 20:53:23, Charlie Reis wrote:
> nit: Add period to be self-consistent.

Done.

https://codereview.chromium.org/1515703005/diff/140001/content/public/common/...
content/public/common/resource_type.h:31: RESOURCE_TYPE_CSP_REPORT = 16,      //
a CSP report
On 2015/12/15 12:53:09, battre wrote:
> How about explaining the abbreviation in the comment?

Done.

https://codereview.chromium.org/1515703005/diff/140001/content/public/common/...
content/public/common/resource_type.h:32: RESOURCE_TYPE_PLUGIN_RESOURCE = 17, //
a resource that a plugin requested.
On 2015/12/15 00:40:37, mattm wrote:
> Update ContentResourceType in histograms.xml

Done.

https://codereview.chromium.org/1515703005/diff/140001/extensions/browser/api...
File extensions/browser/api/web_request/web_request_api_helpers.cc (left):

https://codereview.chromium.org/1515703005/diff/140001/extensions/browser/api...
extensions/browser/api/web_request/web_request_api_helpers.cc:76:
content::RESOURCE_TYPE_LAST_TYPE,
On 2015/12/15 12:53:09, battre wrote:
> Jochen: FYI

I removed this because the referenced bug is about a bug in a GCC version that
is no longer supported.

[battre, 2015-12-15 13:32:55]

Actually, I am still struggling with some of the logic.

https://codereview.chromium.org/1515703005/diff/160001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

https://codereview.chromium.org/1515703005/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/framework.js:176: if
(details.tabId !== -1 || details.frameId >= 0)
// The event is considered expected if it happens in a tab (tabId !== -1) or a
frame (frameId >= 0).

https://codereview.chromium.org/1515703005/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/framework.js:180: // details
match the given details.
I am struggling with the negations. Unexpected, a conjunction of disjunctions of
negations (every(or(not(equals(...)))).

Could this be done as follows? this may be wrong. I am still struggling with
this logic.
// A an event is considered expected if it is in contained in expectedEventData.
var found = expectedEventData.find(function(exp) {
  var matches = that.event === name &&
    exp.details.method === details.method &&
    exp.details.url === details.url &&
    exp.details.type === details.type;
  var disconnected = 
    exp.details.tabID === -1 &&
    exp.details.frameId === -1;
  return matches && disconnected;
  });
return found == undefined;

[robwu, 2015-12-15 14:09:28]

Patchset #3 (id:180001) has been deleted

[robwu, 2015-12-15 14:13:31]

https://codereview.chromium.org/1515703005/diff/160001/chrome/test/data/exten...
File chrome/test/data/extensions/api_test/webrequest/framework.js (right):

https://codereview.chromium.org/1515703005/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/framework.js:176: if
(details.tabId !== -1 || details.frameId >= 0)
On 2015/12/15 13:32:55, battre wrote:
> // The event is considered expected if it happens in a tab (tabId !== -1) or a
> frame (frameId >= 0).

Done, with a slightly different wording. It's not considered "expected", but
"not unexpected" (in boolean logical, there is no difference, but "expected"
carries a stronger meaning than "not unexpected").

https://codereview.chromium.org/1515703005/diff/160001/chrome/test/data/exten...
chrome/test/data/extensions/api_test/webrequest/framework.js:180: // details
match the given details.
On 2015/12/15 13:32:55, battre wrote:
> I am struggling with the negations. Unexpected, a conjunction of disjunctions
of
> negations (every(or(not(equals(...)))).
> 
> Could this be done as follows? this may be wrong. I am still struggling with
> this logic.
> // A an event is considered expected if it is in contained in
expectedEventData.
> var found = expectedEventData.find(function(exp) {
>   var matches = that.event === name &&
>     exp.details.method === details.method &&
>     exp.details.url === details.url &&
>     exp.details.type === details.type;
>   var disconnected = 
>     exp.details.tabID === -1 &&
>     exp.details.frameId === -1;
>   return matches && disconnected;
>   });
> return found == undefined;

I've uploaded a new patchset that moves the negation outside the body (and using
some instead of every to combine the results). Is it easier to understand now?

[battre, 2015-12-15 14:42:49]

great, thank you!

[Marijn Kruisselbrink, 2015-12-15 18:15:50]

extensions/ LGTM

[Charlie Reis, 2015-12-15 19:13:33]

Description was changed from

==========
WebRequest API: add more resource types

Added new resource types "font" and "ping" to identify fonts, <a ping>
and navigator.sendBeacon requests. Existing types are also extended:
Workers (web workers, shared workers, service workers) are mapped to
"script" and favicons are mapped to "image". Plugin requests are also
labeled as "object".
(all of these mentioned requests were previously labeled as "other").

Added two resource types:
- RESOURCE_TYPE_CSP_REPORT; Previously RequestContextCSPReport mapped to
  RESOURCE_TYPE_PING. ping and beacons are commonly used for tracking,
  whereas CSP reports aid the security of website owners. By not
  including CSP reports in the ping type, extensions that disable ping
  will not inadvertently block security features of a website.

- RESOURCE_TYPE_PLUGIN_RESOURCE; plugin requests should be tagged as
  "object" instead of "other". But we cannot use RESOURCE_TYPE_OBJECT
  because this is used by MimeTypeResourceHandler::SelectNextHandler to
  determine whether to intercept the resource and display the result in
  a plugin.

BUG=80230,410382,512406
TEST=ExtensionWebRequestApiTest.*
==========

to

==========
WebRequest API: add more resource types

Added new resource types "font" and "ping" to identify fonts, <a ping>
and navigator.sendBeacon requests. Existing types are also extended:
Workers (web workers, shared workers, service workers) are mapped to
"script" and favicons are mapped to "image". Plugin requests are also
labeled as "object".
(all of these mentioned requests were previously labeled as "other").

Added two resource types:
- RESOURCE_TYPE_CSP_REPORT; Previously RequestContextCSPReport mapped to
  RESOURCE_TYPE_PING. ping and beacons are commonly used for tracking,
  whereas CSP reports aid the security of website owners. By not
  including CSP reports in the ping type, extensions that disable ping
  will not inadvertently block security features of a website.

- RESOURCE_TYPE_PLUGIN_RESOURCE; plugin requests should be tagged as
  "object" instead of "other". But we cannot use RESOURCE_TYPE_OBJECT
  because this is used by MimeTypeResourceHandler::SelectNextHandler to
  determine whether to intercept the resource and display the result in
  a plugin.

BUG=80230,410382,512406
TEST=ExtensionWebRequestApiTest.*
==========

[Charlie Reis, 2015-12-15 19:13:33]

creis@chromium.org changed required reviewers:
+ jochen@chromium.org

[Charlie Reis, 2015-12-15 19:14:06]

On 2015/12/15 12:53:09, battre wrote:
> web_request stuff LGTM.
> 
> Jochen, are you ok with adding additional resource types? Asking you because
you
> requested to limit this in the past.

content/ LGTM once Jochen is happy.  (See Dominic's question above.)

[jochen (gone - plz use gerrit), 2015-12-16 09:03:01]

I'm fine with adding more types.

[robwu, 2015-12-16 09:03:27]

The CQ bit was checked by rob@robwu.nl

[robwu, 2015-12-16 09:03:28]

The patchset sent to the CQ was uploaded after l-g-t-m from battre@chromium.org,
mattm@chromium.org
Link to the patchset: https://codereview.chromium.org/1515703005/#ps200001
(title: "Update comment, move logical negation")

[commit-bot: I haz the power, 2015-12-16 09:03:42]

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

[commit-bot: I haz the power, 2015-12-16 09:03:44]

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

[commit-bot: I haz the power, 2015-12-16 09:03:45]

All required reviewers (with asterisk prefixes) have not yet approved this CL.

No L-G-T-M from a valid reviewer yet. Only full committers are accepted.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[robwu, 2015-12-16 09:08:36]

rob@robwu.nl changed required reviewers:
- jochen@chromium.org

[robwu, 2015-12-16 09:08:41]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2015-12-16 09:09:04]

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

[jochen (gone - plz use gerrit), 2015-12-16 09:10:51]

lgtm

[robwu, 2015-12-16 09:15:52]

rob@robwu.nl changed reviewers:
+ mpearson@chromium.org

[robwu, 2015-12-16 09:15:53]

+mpearson
Could you rubberstamp the changes to histograms.xml?

[commit-bot: I haz the power, 2015-12-16 09:17:54]

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

[commit-bot: I haz the power, 2015-12-16 09:17:56]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Mark P, 2015-12-16 21:00:36]

https://codereview.chromium.org/1515703005/diff/200001/content/public/common/...
File content/public/common/resource_type.h (right):

https://codereview.chromium.org/1515703005/diff/200001/content/public/common/...
content/public/common/resource_type.h:33: RESOURCE_TYPE_PLUGIN_RESOURCE = 17, //
a resource that a plugin requested.
Did these used to be emitted as RESOURCE_TYPE_OBJECT per the comment by that
entry that you deleted?

If so, then you're changing how old data is interpreted and you should also
change the name of the histogram so the two interpretations don't get mixed.

[robwu, 2015-12-16 21:17:03]

https://codereview.chromium.org/1515703005/diff/200001/content/public/common/...
File content/public/common/resource_type.h (right):

https://codereview.chromium.org/1515703005/diff/200001/content/public/common/...
content/public/common/resource_type.h:33: RESOURCE_TYPE_PLUGIN_RESOURCE = 17, //
a resource that a plugin requested.
On 2015/12/16 21:00:36, Mark P wrote:
> Did these used to be emitted as RESOURCE_TYPE_OBJECT per the comment by that
> entry that you deleted?
> 
> If so, then you're changing how old data is interpreted and you should also
> change the name of the histogram so the two interpretations don't get mixed.

RESOURCE_TYPE_PLUGIN_RESOURCE used to be combined with
RESOURCE_TYPE_SUB_RESOURCE. I removed the comment from RESOURCE_TYPE_OBJECT
because it seems that RESOURCE_TYPE_OBJECT is really only used for the first
plugin resource
(https://cs.chromium.org/blink::WebURLRequest::RequestContextObject).

RESOURCE_TYPE_CSP_REPORT used to be combined with RESOURCE_TYPE_PING.

In both cases, the relation between old and new is "old = old_minus_new + new".
Do you think that it is necessary to change the names for histograms to
something like:

RESOURCE_TYPE_SUB_RESOURCE2 + RESOURCE_TYPE_PLUGIN_RESOURCE
and
RESOURCE_TYPE_PING2 + RESOURCE_TYPE_CSP_REPORT.

?

[Mark P, 2015-12-16 22:43:36]

https://codereview.chromium.org/1515703005/diff/200001/content/public/common/...
File content/public/common/resource_type.h (right):

https://codereview.chromium.org/1515703005/diff/200001/content/public/common/...
content/public/common/resource_type.h:33: RESOURCE_TYPE_PLUGIN_RESOURCE = 17, //
a resource that a plugin requested.
On 2015/12/16 21:17:02, robwu wrote:
> On 2015/12/16 21:00:36, Mark P wrote:
> > Did these used to be emitted as RESOURCE_TYPE_OBJECT per the comment by that
> > entry that you deleted?
> > 
> > If so, then you're changing how old data is interpreted and you should also
> > change the name of the histogram so the two interpretations don't get mixed.
> 
> RESOURCE_TYPE_PLUGIN_RESOURCE used to be combined with
> RESOURCE_TYPE_SUB_RESOURCE. I removed the comment from RESOURCE_TYPE_OBJECT
> because it seems that RESOURCE_TYPE_OBJECT is really only used for the first
> plugin resource
> (https://cs.chromium.org/blink::WebURLRequest::RequestContextObject).
> 
> RESOURCE_TYPE_CSP_REPORT used to be combined with RESOURCE_TYPE_PING.
> 
> In both cases, the relation between old and new is "old = old_minus_new +
new".
> Do you think that it is necessary to change the names for histograms to
> something like:
> 
> RESOURCE_TYPE_SUB_RESOURCE2 + RESOURCE_TYPE_PLUGIN_RESOURCE
> and
> RESOURCE_TYPE_PING2 + RESOURCE_TYPE_CSP_REPORT.
> 
> ?

Yes.  People who use the dashboard (possibly not you) will see a shift in usage
over time (as browsers get updated) and think the web is changing.  This will be
misleading.  It's better to replace the histogram for this kind of change.

[robwu, 2015-12-18 09:35:54]

Mark - please take a look at my histogram changes. Is it OK?

[Mark P, 2015-12-18 18:28:35]

On Fri, Dec 18, 2015 at 1:35 AM, <rob@robwu.nl> wrote:

> Mark - please take a look at my histogram changes. Is it OK?
>

No, this is still not okay.  You're changing the meaning of particular
values of an enum in substantive ways.  Certain things will not be recorded
in the same buckets they were before.  You need to update the name of your
histogram so old data with one interpretation isn't mixed with data with a
new interpretation.

--mark



> https://codereview.chromium.org/1515703005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[robwu, 2015-12-18 18:33:57]

On 2015/12/18 18:28:35, Mark P wrote:
> On Fri, Dec 18, 2015 at 1:35 AM, <mailto:rob@robwu.nl> wrote:
> 
> > Mark - please take a look at my histogram changes. Is it OK?
> >
> 
> No, this is still not okay.  You're changing the meaning of particular
> values of an enum in substantive ways.  Certain things will not be recorded
> in the same buckets they were before.  You need to update the name of your
> histogram so old data with one interpretation isn't mixed with data with a
> new interpretation.

That's what I did, right?
The two old enums are marked as deprecated and not used any more.
I've introduced two enums with new enum values and unique names.

If this is not sufficient, could you point out what I did wrong, and suggest
documentation that I can read in order to understand how to properly use
histograms? Thanks!

[robwu, 2015-12-18 18:36:50]

On 2015/12/18 18:33:57, robwu wrote:
> On 2015/12/18 18:28:35, Mark P wrote:
> > On Fri, Dec 18, 2015 at 1:35 AM, <mailto:rob@robwu.nl> wrote:
> > 
> > > Mark - please take a look at my histogram changes. Is it OK?
> > >
> > 
> > No, this is still not okay.  You're changing the meaning of particular
> > values of an enum in substantive ways.  Certain things will not be recorded
> > in the same buckets they were before.  You need to update the name of your
> > histogram so old data with one interpretation isn't mixed with data with a
> > new interpretation.
> 
> That's what I did, right?
> The two old enums are marked as deprecated and not used any more.
> I've introduced two enums with new enum values and unique names.

Typo, I meant to say four enums (two for both old enums).

> If this is not sufficient, could you point out what I did wrong, and suggest
> documentation that I can read in order to understand how to properly use
> histograms? Thanks!

[Mark P, 2015-12-18 20:31:26]

On Fri, Dec 18, 2015 at 10:33 AM, <rob@robwu.nl> wrote:

> On 2015/12/18 18:28:35, Mark P wrote:
>
>> On Fri, Dec 18, 2015 at 1:35 AM, <mailto:rob@robwu.nl> wrote:
>>
>
> > Mark - please take a look at my histogram changes. Is it OK?
>> >
>>
>
> No, this is still not okay.  You're changing the meaning of particular
>> values of an enum in substantive ways.  Certain things will not be
>> recorded
>> in the same buckets they were before.  You need to update the name of your
>> histogram so old data with one interpretation isn't mixed with data with a
>> new interpretation.
>>
>
> That's what I did, right?
> The two old enums are marked as deprecated and not used any more.
> I've introduced two enums with new enum values and unique names.
>
> If this is not sufficient, could you point out what I did wrong, and
> suggest
> documentation that I can read in order to understand how to properly use
> histograms? Thanks!
>

The idea is that a bucket in a histogram should always mean the same thing,
period.

This is why all enums emitted to histograms have a warning "only add
elements at the end; do not delete / renumber existing elements."  If you
added or remove an element in the middle, suddenly bucket=6 (for example)
goes from meaning one thing to meaning something else.  People often look
at values over time.  If they look at one point in time and see a bucket,
say, bucket=6 drops from 15% to 1% of usage, they might make a lot of
interferences and start thinking about what about the web and chrome usage
must have changed in that period.

This is why we rename histograms when the meaning of existing buckets
change / the conditions under which existing buckets get emitted to
change.  By renaming the histograms, it explicitly breaks this continuity
over time, thus preventing all sorts of incorrect inferences.

I am not aware of a documentation that explains this explicitly.  I'll look
to create something.

--mark



>
> https://codereview.chromium.org/1515703005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[robwu, 2015-12-18 20:55:11]

On 2015/12/18 20:31:26, Mark P wrote:
> On Fri, Dec 18, 2015 at 10:33 AM, <mailto:rob@robwu.nl> wrote:
> 
> > On 2015/12/18 18:28:35, Mark P wrote:
> >
> >> On Fri, Dec 18, 2015 at 1:35 AM, <mailto:rob@robwu.nl> wrote:
> >>
> >
> > > Mark - please take a look at my histogram changes. Is it OK?
> >> >
> >>
> >
> > No, this is still not okay.  You're changing the meaning of particular
> >> values of an enum in substantive ways.  Certain things will not be
> >> recorded
> >> in the same buckets they were before.  You need to update the name of your
> >> histogram so old data with one interpretation isn't mixed with data with a
> >> new interpretation.
> >>
> >
> > That's what I did, right?
> > The two old enums are marked as deprecated and not used any more.
> > I've introduced two enums with new enum values and unique names.
> >
> > If this is not sufficient, could you point out what I did wrong, and
> > suggest
> > documentation that I can read in order to understand how to properly use
> > histograms? Thanks!
> >
> 
> The idea is that a bucket in a histogram should always mean the same thing,
> period.

Clear. After your first request I updated the CL to change the enum names and
values so that this requirement is satisfied. The two enums that changed
semantics are no longer used and superseded by 4 enums. All other buckets have
exactly the same input as before (including OBJECT). So, could you check again?

> This is why all enums emitted to histograms have a warning "only add
> elements at the end; do not delete / renumber existing elements."  If you
> added or remove an element in the middle, suddenly bucket=6 (for example)
> goes from meaning one thing to meaning something else.  People often look
> at values over time.  If they look at one point in time and see a bucket,
> say, bucket=6 drops from 15% to 1% of usage, they might make a lot of
> interferences and start thinking about what about the web and chrome usage
> must have changed in that period.
> 
> This is why we rename histograms when the meaning of existing buckets
> change / the conditions under which existing buckets get emitted to
> change.  By renaming the histograms, it explicitly breaks this continuity
> over time, thus preventing all sorts of incorrect inferences.
> 
> I am not aware of a documentation that explains this explicitly.  I'll look
> to create something.

[Marijn Kruisselbrink, 2015-12-18 21:20:38]

On 2015/12/18 at 20:55:11, rob wrote:
> On 2015/12/18 20:31:26, Mark P wrote:
> > On Fri, Dec 18, 2015 at 10:33 AM, <mailto:rob@robwu.nl> wrote:
> > 
> > > On 2015/12/18 18:28:35, Mark P wrote:
> > >
> > >> On Fri, Dec 18, 2015 at 1:35 AM, <mailto:rob@robwu.nl> wrote:
> > >>
> > >
> > > > Mark - please take a look at my histogram changes. Is it OK?
> > >> >
> > >>
> > >
> > > No, this is still not okay.  You're changing the meaning of particular
> > >> values of an enum in substantive ways.  Certain things will not be
> > >> recorded
> > >> in the same buckets they were before.  You need to update the name of
your
> > >> histogram so old data with one interpretation isn't mixed with data with
a
> > >> new interpretation.
> > >>
> > >
> > > That's what I did, right?
> > > The two old enums are marked as deprecated and not used any more.
> > > I've introduced two enums with new enum values and unique names.
> > >
> > > If this is not sufficient, could you point out what I did wrong, and
> > > suggest
> > > documentation that I can read in order to understand how to properly use
> > > histograms? Thanks!
> > >
> > 
> > The idea is that a bucket in a histogram should always mean the same thing,
> > period.
> 
> Clear. After your first request I updated the CL to change the enum names and
values so that this requirement is satisfied. The two enums that changed
semantics are no longer used and superseded by 4 enums. All other buckets have
exactly the same input as before (including OBJECT). So, could you check again?

I think the concern is that even no longer using an existing enum value is still
changing the meaning of that bucket in charts etc (going from some percentage to
0%). So instead of renaming enum values what you should be doing is renaming the
histogram. And if you do that there is no need to also rename the enum values
since it is obvious that data before/after the change is not comparable.

[Mark P, 2015-12-18 22:43:00]

On Fri, Dec 18, 2015 at 1:20 PM, <mek@chromium.org> wrote:

> On 2015/12/18 at 20:55:11, rob wrote:
>
>> On 2015/12/18 20:31:26, Mark P wrote:
>> > On Fri, Dec 18, 2015 at 10:33 AM, <mailto:rob@robwu.nl> wrote:
>> >
>> > > On 2015/12/18 18:28:35, Mark P wrote:
>> > >
>> > >> On Fri, Dec 18, 2015 at 1:35 AM, <mailto:rob@robwu.nl> wrote:
>> > >>
>> > >
>> > > > Mark - please take a look at my histogram changes. Is it OK?
>> > >> >
>> > >>
>> > >
>> > > No, this is still not okay.  You're changing the meaning of particular
>> > >> values of an enum in substantive ways.  Certain things will not be
>> > >> recorded
>> > >> in the same buckets they were before.  You need to update the name of
>>
> your
>
>> > >> histogram so old data with one interpretation isn't mixed with data
>> with
>>
> a
>
>> > >> new interpretation.
>> > >>
>> > >
>> > > That's what I did, right?
>> > > The two old enums are marked as deprecated and not used any more.
>> > > I've introduced two enums with new enum values and unique names.
>> > >
>> > > If this is not sufficient, could you point out what I did wrong, and
>> > > suggest
>> > > documentation that I can read in order to understand how to properly
>> use
>> > > histograms? Thanks!
>> > >
>> >
>> > The idea is that a bucket in a histogram should always mean the same
>> thing,
>> > period.
>>
>
> Clear. After your first request I updated the CL to change the enum names
>> and
>>
> values so that this requirement is satisfied. The two enums that changed
> semantics are no longer used and superseded by 4 enums. All other buckets
> have
> exactly the same input as before (including OBJECT). So, could you check
> again?
>
> I think the concern is that even no longer using an existing enum value is
> still
> changing the meaning of that bucket in charts etc (going from some
> percentage to
> 0%). So instead of renaming enum values what you should be doing is
> renaming the
> histogram. And if you do that there is no need to also rename the enum
> values
> since it is obvious that data before/after the change is not comparable.
>
+1 to what mek@ said.


>
> https://codereview.chromium.org/1515703005/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[robwu, 2015-12-18 22:52:19]

On 2015/12/18 22:43:00, Mark P wrote:
> On Fri, Dec 18, 2015 at 1:20 PM, <mailto:mek@chromium.org> wrote:
> > I think the concern is that even no longer using an existing enum value is
> > still
> > changing the meaning of that bucket in charts etc (going from some
> > percentage to
> > 0%). So instead of renaming enum values what you should be doing is
> > renaming the
> > histogram. And if you do that there is no need to also rename the enum
> > values
> > since it is obvious that data before/after the change is not comparable.
> >
> +1 to what mek@ said.

Thanks Martijn for your clarification.

Mark: I have renamed the histogram, PTAL.
(and while we are at it, could it be worthwhile to also rename the labels of the
enum for consistency? The last labels have the RESOURCE_TYPE_ prefix, the others
don't have it. Since this is a new histogram, I'm inclined to update the other
labels as well, so that they are all prefixed with RESOURCE_TYPE_).

[Mark P, 2015-12-18 22:55:18]

On 2015/12/18 22:52:19, robwu wrote:
> On 2015/12/18 22:43:00, Mark P wrote:
> > On Fri, Dec 18, 2015 at 1:20 PM, <mailto:mek@chromium.org> wrote:
> > > I think the concern is that even no longer using an existing enum value is
> > > still
> > > changing the meaning of that bucket in charts etc (going from some
> > > percentage to
> > > 0%). So instead of renaming enum values what you should be doing is
> > > renaming the
> > > histogram. And if you do that there is no need to also rename the enum
> > > values
> > > since it is obvious that data before/after the change is not comparable.
> > >
> > +1 to what mek@ said.
> 
> Thanks Martijn for your clarification.
> 
> Mark: I have renamed the histogram, PTAL.

You renamed the enum type in histograms.xml.

You did not rename the histogram in the place in chrome code where it is
emitted.

Also, you'll need to update histograms.xml to add the histogram under the new
name and mark the old histogram as obsolete.

--mark

> (and while we are at it, could it be worthwhile to also rename the labels of
the
> enum for consistency? The last labels have the RESOURCE_TYPE_ prefix, the
others
> don't have it. Since this is a new histogram, I'm inclined to update the other
> labels as well, so that they are all prefixed with RESOURCE_TYPE_).

[robwu, 2015-12-19 00:06:36]

On 2015/12/18 22:55:18, Mark P wrote:
> You did not rename the histogram in the place in chrome code where it is
> emitted.
> 
> Also, you'll need to update histograms.xml to add the histogram under the new
> name and mark the old histogram as obsolete.

Sorry for my ignorance. I've now replaced updated histograms as follows:
1. Locate non-obsolete uses of ContentResourceType
2. Copy-paste <histogram> and insert a "2" somewhere in the histogram name + set
enum type to ContentResourceType2.
3. Put <obsolete> in the original tag, referring to the new <histogram>.
4. In the *.RenderableStatusCode2, I also changed "0-14" to "0-17" for accuracy.
5. Updated callers in Chrome code.
6. Mark old enum type as obsolete, plus why.
7. Use new enum types (with consistent names).

[Mark P, 2015-12-22 06:18:57]

histograms lgtm
Nice work on fixing up the histograms as I asked.  It's clear you did a careful
job and didn't miss anything.

--mark

[robwu, 2015-12-22 10:42:32]

The CQ bit was checked by rob@robwu.nl

[robwu, 2015-12-22 10:42:32]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mattm@chromium.org, creis@chromium.org, mek@chromium.org, battre@chromium.org
Link to the patchset: https://codereview.chromium.org/1515703005/#ps260001
(title: "Rename histograms in the proper way")

[commit-bot: I haz the power, 2015-12-22 10:42:35]

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

[commit-bot: I haz the power, 2015-12-22 10:49:56]

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

[commit-bot: I haz the power, 2015-12-22 10:49:57]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[robwu, 2015-12-22 11:11:58]

The CQ bit was checked by rob@robwu.nl

[robwu, 2015-12-22 11:11:59]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mpearson@chromium.org, mattm@chromium.org, creis@chromium.org, mek@chromium.org,
battre@chromium.org
Link to the patchset: https://codereview.chromium.org/1515703005/#ps280001
(title: "run pretty_print.py")

[commit-bot: I haz the power, 2015-12-22 11:12:08]

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

[commit-bot: I haz the power, 2015-12-22 11:56:55]

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

[commit-bot: I haz the power, 2015-12-22 11:56:56]

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

[robwu, 2015-12-22 16:43:49]

Patchset #9 (id:320001) has been deleted

[robwu, 2015-12-22 16:43:57]

Patchset #8 (id:300001) has been deleted

[robwu, 2015-12-22 18:20:14]

The CQ bit was checked by rob@robwu.nl

[robwu, 2015-12-22 18:20:14]

The patchset sent to the CQ was uploaded after l-g-t-m from jochen@chromium.org,
mpearson@chromium.org, mattm@chromium.org, creis@chromium.org, mek@chromium.org,
battre@chromium.org
Link to the patchset: https://codereview.chromium.org/1515703005/#ps340001
(title: "Replace == with && 2x")

[commit-bot: I haz the power, 2015-12-22 18:20:29]

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

[commit-bot: I haz the power, 2015-12-22 19:46:59]

Description was changed from

==========
WebRequest API: add more resource types

Added new resource types "font" and "ping" to identify fonts, <a ping>
and navigator.sendBeacon requests. Existing types are also extended:
Workers (web workers, shared workers, service workers) are mapped to
"script" and favicons are mapped to "image". Plugin requests are also
labeled as "object".
(all of these mentioned requests were previously labeled as "other").

Added two resource types:
- RESOURCE_TYPE_CSP_REPORT; Previously RequestContextCSPReport mapped to
  RESOURCE_TYPE_PING. ping and beacons are commonly used for tracking,
  whereas CSP reports aid the security of website owners. By not
  including CSP reports in the ping type, extensions that disable ping
  will not inadvertently block security features of a website.

- RESOURCE_TYPE_PLUGIN_RESOURCE; plugin requests should be tagged as
  "object" instead of "other". But we cannot use RESOURCE_TYPE_OBJECT
  because this is used by MimeTypeResourceHandler::SelectNextHandler to
  determine whether to intercept the resource and display the result in
  a plugin.

BUG=80230,410382,512406
TEST=ExtensionWebRequestApiTest.*
==========

to

==========
WebRequest API: add more resource types

Added new resource types "font" and "ping" to identify fonts, <a ping>
and navigator.sendBeacon requests. Existing types are also extended:
Workers (web workers, shared workers, service workers) are mapped to
"script" and favicons are mapped to "image". Plugin requests are also
labeled as "object".
(all of these mentioned requests were previously labeled as "other").

Added two resource types:
- RESOURCE_TYPE_CSP_REPORT; Previously RequestContextCSPReport mapped to
  RESOURCE_TYPE_PING. ping and beacons are commonly used for tracking,
  whereas CSP reports aid the security of website owners. By not
  including CSP reports in the ping type, extensions that disable ping
  will not inadvertently block security features of a website.

- RESOURCE_TYPE_PLUGIN_RESOURCE; plugin requests should be tagged as
  "object" instead of "other". But we cannot use RESOURCE_TYPE_OBJECT
  because this is used by MimeTypeResourceHandler::SelectNextHandler to
  determine whether to intercept the resource and display the result in
  a plugin.

BUG=80230,410382,512406
TEST=ExtensionWebRequestApiTest.*
==========

[commit-bot: I haz the power, 2015-12-22 19:47:00]

Committed patchset #8 (id:340001)

[commit-bot: I haz the power, 2015-12-22 19:47:49]

Description was changed from

==========
WebRequest API: add more resource types

Added new resource types "font" and "ping" to identify fonts, <a ping>
and navigator.sendBeacon requests. Existing types are also extended:
Workers (web workers, shared workers, service workers) are mapped to
"script" and favicons are mapped to "image". Plugin requests are also
labeled as "object".
(all of these mentioned requests were previously labeled as "other").

Added two resource types:
- RESOURCE_TYPE_CSP_REPORT; Previously RequestContextCSPReport mapped to
  RESOURCE_TYPE_PING. ping and beacons are commonly used for tracking,
  whereas CSP reports aid the security of website owners. By not
  including CSP reports in the ping type, extensions that disable ping
  will not inadvertently block security features of a website.

- RESOURCE_TYPE_PLUGIN_RESOURCE; plugin requests should be tagged as
  "object" instead of "other". But we cannot use RESOURCE_TYPE_OBJECT
  because this is used by MimeTypeResourceHandler::SelectNextHandler to
  determine whether to intercept the resource and display the result in
  a plugin.

BUG=80230,410382,512406
TEST=ExtensionWebRequestApiTest.*
==========

to

==========
WebRequest API: add more resource types

Added new resource types "font" and "ping" to identify fonts, <a ping>
and navigator.sendBeacon requests. Existing types are also extended:
Workers (web workers, shared workers, service workers) are mapped to
"script" and favicons are mapped to "image". Plugin requests are also
labeled as "object".
(all of these mentioned requests were previously labeled as "other").

Added two resource types:
- RESOURCE_TYPE_CSP_REPORT; Previously RequestContextCSPReport mapped to
  RESOURCE_TYPE_PING. ping and beacons are commonly used for tracking,
  whereas CSP reports aid the security of website owners. By not
  including CSP reports in the ping type, extensions that disable ping
  will not inadvertently block security features of a website.

- RESOURCE_TYPE_PLUGIN_RESOURCE; plugin requests should be tagged as
  "object" instead of "other". But we cannot use RESOURCE_TYPE_OBJECT
  because this is used by MimeTypeResourceHandler::SelectNextHandler to
  determine whether to intercept the resource and display the result in
  a plugin.

BUG=80230,410382,512406
TEST=ExtensionWebRequestApiTest.*

Committed: https://crrev.com/6a32a203bbae04ef29a42f3f364ec00a0adf9179
Cr-Commit-Position: refs/heads/master@{#366632}
==========

[commit-bot: I haz the power, 2015-12-22 19:47:50]

Patchset 8 (id:??) landed as
https://crrev.com/6a32a203bbae04ef29a42f3f364ec00a0adf9179
Cr-Commit-Position: refs/heads/master@{#366632}