Mute use counters for internal pages

Mute use counters for internal pages

Whenever a new top-level page is loaded, the UseCounter state is reset.  This CL extends that infrastructure to pass along the URL of the page that's being loaded.  We then use the scheme of the URL to decide if tracking usage metrics is useful or not, to avoid polluting our metrics with results from webui, DevTools, etc.

This CL makes no changes to the "legacy" counter histograms in order to preserve their semantics while we transition to the new histograms.

BUG=652336
Committed: https://crrev.com/146accf2581a4ac5184cc9c5f0f2e2a91bf4be23
Cr-Commit-Position: refs/heads/master@{#440660}

[Rick Byers, 2016-12-23 18:38:42]

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

[commit-bot: I haz the power, 2016-12-23 18:38:47]

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

[Rick Byers, 2016-12-23 18:41:07]

rbyers@chromium.org changed reviewers:
+ lunalu@chromium.org

[Rick Byers, 2016-12-23 18:41:07]

Hey Luna,
I think everyone with context on this is OOO today for the holidays.  Would you
mind reviewing this for me please?

Thanks,
   Rick

[Rick Byers, 2016-12-23 18:44:13]

Description was changed from

==========
Mute use counters for internal pages

BUG=652336
==========

to

==========
Mute use counters for internal pages

Whenever a new top-level page is loaded, the UseCounter state is reset.  This CL
extends that infrastructure to pass along the URL of the page that's being
loaded.  We then use the scheme of the URL to decide if tracking usage metrics
is useful or not, to avoid polluting our metrics with results from webui,
DevTools, etc.

This CL makes no changes to the "legacy" counter histograms in order to preserve
their semantics while we transition to the new histograms.

BUG=652336
==========

[lunalu1, 2016-12-23 19:26:26]

lgtm

https://codereview.chromium.org/2604633002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right):

https://codereview.chromium.org/2604633002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/UseCounterTest.cpp:38: KURL dummyUrl =
URLTestHelpers::toKURL("https://dummysite.com/");
Please correct me if I am wrong. I see "dummyUrl" used 3 times in this test
file, is it worth it to make it a constant?

https://codereview.chromium.org/2604633002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right):

https://codereview.chromium.org/2604633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounterTest.cpp:234:
useCounter.didCommitLoad(svgUrl);
Can we just do useCounter.didCommitLoad(URLTestHelpers::toKURL("about:blank"))
here?

[Rick Byers, 2016-12-23 19:34:59]

https://codereview.chromium.org/2604633002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right):

https://codereview.chromium.org/2604633002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/frame/UseCounterTest.cpp:38: KURL dummyUrl =
URLTestHelpers::toKURL("https://dummysite.com/");
On 2016/12/23 19:26:25, loonybear wrote:
> Please correct me if I am wrong. I see "dummyUrl" used 3 times in this test
> file, is it worth it to make it a constant? 

Since we have to run the KURL constructor at some point, and we discourage
global construction in chromium code, I'd have to refactor the tests to use a
test class with explicit setup.

Now that you mention it, I don't think this is even worth the indirection of a
local variable.  How's this instead?

https://codereview.chromium.org/2604633002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounterTest.cpp (right):

https://codereview.chromium.org/2604633002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounterTest.cpp:234:
useCounter.didCommitLoad(svgUrl);
On 2016/12/23 19:26:25, loonybear wrote:
> Can we just do useCounter.didCommitLoad(URLTestHelpers::toKURL("about:blank"))
> here?

Done.

[lunalu1, 2016-12-23 19:39:48]

Looks great! Thanks for letting me review this.

[Rick Byers, 2016-12-23 19:42:11]

/cc csharrison@ for the changes to platform/weborigin/SchemeRegistry but since
everyone is out for the holidays ATM I'm not going to block landing on further
review.

[Rick Byers, 2016-12-23 19:42:18]

The CQ bit was checked by rbyers@chromium.org

[Rick Byers, 2016-12-23 19:42:19]

The patchset sent to the CQ was uploaded after l-g-t-m from lunalu@chromium.org
Link to the patchset: https://codereview.chromium.org/2604633002/#ps40001
(title: "CR feedback")

[commit-bot: I haz the power, 2016-12-23 19:42:24]

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

[Charlie Harrison, 2016-12-23 20:53:38]

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

[Charlie Harrison, 2016-12-23 20:53:39]

+kinuko FYI

Since this is at the page load level it isn't a big deal, but I'm wondering if
there is a way to improve the APIs here to take advantage of the fact that
KURL::protocolIsInHttpFamily is cached in KURL.

[commit-bot: I haz the power, 2016-12-23 21:00:59]

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

[commit-bot: I haz the power, 2016-12-23 21:01:00]

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

[Rick Byers, 2016-12-23 21:20:06]

On 2016/12/23 20:53:39, Charlie Harrison OOO Dec 15-21 wrote:
> +kinuko FYI
> 
> Since this is at the page load level it isn't a big deal, but I'm wondering if
> there is a way to improve the APIs here to take advantage of the fact that
> KURL::protocolIsInHttpFamily is cached in KURL.

Thanks Chris - I'm happy to improve this.  Eg. I could invoke that and then
check explicitly for the file protocol rather than add a new method to
SchemeRegistry.  WDYT?

What is the http-so protocol that protocolIsInHttpFamily checks for?  I can't
find a description from a quick google search.

[Rick Byers, 2016-12-23 21:21:47]

On 2016/12/23 21:20:06, Rick Byers wrote:
> On 2016/12/23 20:53:39, Charlie Harrison OOO Dec 15-21 wrote:
> > +kinuko FYI
> > 
> > Since this is at the page load level it isn't a big deal, but I'm wondering
if
> > there is a way to improve the APIs here to take advantage of the fact that
> > KURL::protocolIsInHttpFamily is cached in KURL.
> 
> Thanks Chris

Damn, sorry - I mean Charlie of course :-).  I'm ready for holiday...

> check explicitly for the file protocol rather than add a new method to
> SchemeRegistry.  WDYT?
> 
> What is the http-so protocol that protocolIsInHttpFamily checks for?  I can't
> find a description from a quick google search.

[Charlie Harrison, 2016-12-23 21:28:40]

On 2016/12/23 21:20:06, Rick Byers wrote:
> On 2016/12/23 20:53:39, Charlie Harrison OOO Dec 15-21 wrote:
> > +kinuko FYI
> > 
> > Since this is at the page load level it isn't a big deal, but I'm wondering
if
> > there is a way to improve the APIs here to take advantage of the fact that
> > KURL::protocolIsInHttpFamily is cached in KURL.
> 
> Thanks Chris - I'm happy to improve this.  Eg. I could invoke that and then
> check explicitly for the file protocol rather than add a new method to
> SchemeRegistry.  WDYT?
I think it might be better to pass in the KURL explicitly here rather than at
the call site so the logic isn't separated. I think I'd prefer changing the APIs
for all these methods in a separate change(s) to do that, but I'll have to think
about it. No need (from me at least) to make any changes in this CL.

For context: I am a bit hesitant of the right solution because we are changing
it so that KURL::protocol is atomic [1], so I am considering removing the cached
bit in KURL in place of two pointer comparisons.
> 
> What is the http-so protocol that protocolIsInHttpFamily checks for?  I can't
> find a description from a quick google search.
These are "suborigin" schemes,
https://w3c.github.io/webappsec-suborigins/#representation I don't know much
about them though.

[Charlie Harrison, 2016-12-23 21:31:00]

On 2016/12/23 21:21:47, Rick Byers wrote:
> On 2016/12/23 21:20:06, Rick Byers wrote:
> > On 2016/12/23 20:53:39, Charlie Harrison OOO Dec 15-21 wrote:
> > > +kinuko FYI
> > > 
> > > Since this is at the page load level it isn't a big deal, but I'm
wondering
> if
> > > there is a way to improve the APIs here to take advantage of the fact that
> > > KURL::protocolIsInHttpFamily is cached in KURL.
> > 
> > Thanks Chris
> 
> Damn, sorry - I mean Charlie of course :-).  I'm ready for holiday...
No worries, our names are pretty similar :D
> 
> > check explicitly for the file protocol rather than add a new method to
> > SchemeRegistry.  WDYT?
> > 
> > What is the http-so protocol that protocolIsInHttpFamily checks for?  I
can't
> > find a description from a quick google search.

[Rick Byers, 2016-12-23 21:36:42]

On 2016/12/23 21:28:40, Charlie Harrison (back Jan 3) wrote:
> On 2016/12/23 21:20:06, Rick Byers wrote:
> > On 2016/12/23 20:53:39, Charlie Harrison OOO Dec 15-21 wrote:
> > > +kinuko FYI
> > > 
> > > Since this is at the page load level it isn't a big deal, but I'm
wondering
> if
> > > there is a way to improve the APIs here to take advantage of the fact that
> > > KURL::protocolIsInHttpFamily is cached in KURL.
> > 
> > Thanks Chris - I'm happy to improve this.  Eg. I could invoke that and then
> > check explicitly for the file protocol rather than add a new method to
> > SchemeRegistry.  WDYT?
> I think it might be better to pass in the KURL explicitly here rather than at
> the call site so the logic isn't separated. I think I'd prefer changing the
APIs
> for all these methods in a separate change(s) to do that, but I'll have to
think
> about it. No need (from me at least) to make any changes in this CL.
> 
> For context: I am a bit hesitant of the right solution because we are changing
> it so that KURL::protocol is atomic [1], so I am considering removing the
cached
> bit in KURL in place of two pointer comparisons.

Ok, cool - thanks.  Let's follow-up in January.  Would you like me to file a bug
or add a TODO or anything to track the open question here?

> > 
> > What is the http-so protocol that protocolIsInHttpFamily checks for?  I
can't
> > find a description from a quick google search.
> These are "suborigin" schemes,
> https://w3c.github.io/webappsec-suborigins/#representation I don't know much
> about them though.

[Charlie Harrison, 2016-12-23 22:04:40]

On 2016/12/23 21:36:42, Rick Byers wrote:
> On 2016/12/23 21:28:40, Charlie Harrison (back Jan 3) wrote:
> > On 2016/12/23 21:20:06, Rick Byers wrote:
> > > On 2016/12/23 20:53:39, Charlie Harrison OOO Dec 15-21 wrote:
> > > > +kinuko FYI
> > > > 
> > > > Since this is at the page load level it isn't a big deal, but I'm
> wondering
> > if
> > > > there is a way to improve the APIs here to take advantage of the fact
that
> > > > KURL::protocolIsInHttpFamily is cached in KURL.
> > > 
> > > Thanks Chris - I'm happy to improve this.  Eg. I could invoke that and
then
> > > check explicitly for the file protocol rather than add a new method to
> > > SchemeRegistry.  WDYT?
> > I think it might be better to pass in the KURL explicitly here rather than
at
> > the call site so the logic isn't separated. I think I'd prefer changing the
> APIs
> > for all these methods in a separate change(s) to do that, but I'll have to
> think
> > about it. No need (from me at least) to make any changes in this CL.
> > 
> > For context: I am a bit hesitant of the right solution because we are
changing
> > it so that KURL::protocol is atomic [1], so I am considering removing the
> cached
> > bit in KURL in place of two pointer comparisons.
> 
> Ok, cool - thanks.  Let's follow-up in January.  Would you like me to file a
bug
> or add a TODO or anything to track the open question here?
Oops I forget to link the CL [1]. I don't think a bug is necessary right now,
the
changes will be a direct result of adding http/https static string atoms I
think. 

[1]: https://codereview.chromium.org/2599853002/
> 
> > > 
> > > What is the http-so protocol that protocolIsInHttpFamily checks for?  I
> can't
> > > find a description from a quick google search.
> > These are "suborigin" schemes,
> > https://w3c.github.io/webappsec-suborigins/#representation I don't know much
> > about them though.

[Rick Byers, 2016-12-23 22:19:03]

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

[commit-bot: I haz the power, 2016-12-23 22:19:11]

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

[Rick Byers, 2016-12-23 22:19:44]

On 2016/12/23 22:04:40, Charlie Harrison (back Jan 3) wrote:
> On 2016/12/23 21:36:42, Rick Byers wrote:
> > On 2016/12/23 21:28:40, Charlie Harrison (back Jan 3) wrote:
> > > On 2016/12/23 21:20:06, Rick Byers wrote:
> > > > On 2016/12/23 20:53:39, Charlie Harrison OOO Dec 15-21 wrote:
> > > > > +kinuko FYI
> > > > > 
> > > > > Since this is at the page load level it isn't a big deal, but I'm
> > wondering
> > > if
> > > > > there is a way to improve the APIs here to take advantage of the fact
> that
> > > > > KURL::protocolIsInHttpFamily is cached in KURL.
> > > > 
> > > > Thanks Chris - I'm happy to improve this.  Eg. I could invoke that and
> then
> > > > check explicitly for the file protocol rather than add a new method to
> > > > SchemeRegistry.  WDYT?
> > > I think it might be better to pass in the KURL explicitly here rather than
> at
> > > the call site so the logic isn't separated. I think I'd prefer changing
the
> > APIs
> > > for all these methods in a separate change(s) to do that, but I'll have to
> > think
> > > about it. No need (from me at least) to make any changes in this CL.
> > > 
> > > For context: I am a bit hesitant of the right solution because we are
> changing
> > > it so that KURL::protocol is atomic [1], so I am considering removing the
> > cached
> > > bit in KURL in place of two pointer comparisons.
> > 
> > Ok, cool - thanks.  Let's follow-up in January.  Would you like me to file a
> bug
> > or add a TODO or anything to track the open question here?
> Oops I forget to link the CL [1]. I don't think a bug is necessary right now,
> the
> changes will be a direct result of adding http/https static string atoms I
> think. 
> 
> [1]: https://codereview.chromium.org/2599853002/

Sounds good.  Thanks Charlie!

> > 
> > > > 
> > > > What is the http-so protocol that protocolIsInHttpFamily checks for?  I
> > can't
> > > > find a description from a quick google search.
> > > These are "suborigin" schemes,
> > > https://w3c.github.io/webappsec-suborigins/#representation I don't know
much
> > > about them though.

[Rick Byers, 2016-12-23 22:21:25]

Luna,
The CQ identified a subtle bug in my patch.  Internal pages (like
chrome://settings) which generate deprecation warnings stopped throttling those
warnings to one a page and would instead spew one warning for each usage.

This was because with those pages being muted, the call to
UseCounter::hasRecordedFeature from Deprecation::countDeprecation would always
return false.

To fix this I've split the "mute" concept up into two different concepts: muting
and disabled reporting, which I've tried to document in the header.  WDYT?

[lunalu1, 2016-12-23 22:26:11]

On 2016/12/23 22:21:25, Rick Byers OOO until 1-2 wrote:
> Luna,
> The CQ identified a subtle bug in my patch.  Internal pages (like
> chrome://settings) which generate deprecation warnings stopped throttling
those
> warnings to one a page and would instead spew one warning for each usage.
> 
> This was because with those pages being muted, the call to
> UseCounter::hasRecordedFeature from Deprecation::countDeprecation would always
> return false.
> 
> To fix this I've split the "mute" concept up into two different concepts:
muting
> and disabled reporting, which I've tried to document in the header.  WDYT?

That makes sense to me.Still lgtm

[Rick Byers, 2016-12-23 22:27:18]

The CQ bit was unchecked by rbyers@chromium.org

[Rick Byers, 2016-12-23 22:31:38]

The CQ bit was checked by rbyers@chromium.org

[Rick Byers, 2016-12-23 22:31:38]

The patchset sent to the CQ was uploaded after l-g-t-m from lunalu@chromium.org
Link to the patchset: https://codereview.chromium.org/2604633002/#ps80001
(title: "Update a related comment")

[commit-bot: I haz the power, 2016-12-23 22:31:46]

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-12-24 00:22:09]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482532298590700,
"parent_rev": "00bf8ad9841061798e7dffc983d13ab8c223b031", "commit_rev":
"6f35e1e254f6102354e9ea4a51d6201d44ee87c8"}

[commit-bot: I haz the power, 2016-12-24 00:22:37]

Description was changed from

==========
Mute use counters for internal pages

Whenever a new top-level page is loaded, the UseCounter state is reset.  This CL
extends that infrastructure to pass along the URL of the page that's being
loaded.  We then use the scheme of the URL to decide if tracking usage metrics
is useful or not, to avoid polluting our metrics with results from webui,
DevTools, etc.

This CL makes no changes to the "legacy" counter histograms in order to preserve
their semantics while we transition to the new histograms.

BUG=652336
==========

to

==========
Mute use counters for internal pages

Whenever a new top-level page is loaded, the UseCounter state is reset.  This CL
extends that infrastructure to pass along the URL of the page that's being
loaded.  We then use the scheme of the URL to decide if tracking usage metrics
is useful or not, to avoid polluting our metrics with results from webui,
DevTools, etc.

This CL makes no changes to the "legacy" counter histograms in order to preserve
their semantics while we transition to the new histograms.

BUG=652336

Review-Url: https://codereview.chromium.org/2604633002
==========

[commit-bot: I haz the power, 2016-12-24 00:22:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-12-24 00:24:08]

Description was changed from

==========
Mute use counters for internal pages

Whenever a new top-level page is loaded, the UseCounter state is reset.  This CL
extends that infrastructure to pass along the URL of the page that's being
loaded.  We then use the scheme of the URL to decide if tracking usage metrics
is useful or not, to avoid polluting our metrics with results from webui,
DevTools, etc.

This CL makes no changes to the "legacy" counter histograms in order to preserve
their semantics while we transition to the new histograms.

BUG=652336

Review-Url: https://codereview.chromium.org/2604633002
==========

to

==========
Mute use counters for internal pages

Whenever a new top-level page is loaded, the UseCounter state is reset.  This CL
extends that infrastructure to pass along the URL of the page that's being
loaded.  We then use the scheme of the URL to decide if tracking usage metrics
is useful or not, to avoid polluting our metrics with results from webui,
DevTools, etc.

This CL makes no changes to the "legacy" counter histograms in order to preserve
their semantics while we transition to the new histograms.

BUG=652336

Committed: https://crrev.com/146accf2581a4ac5184cc9c5f0f2e2a91bf4be23
Cr-Commit-Position: refs/heads/master@{#440660}
==========

[commit-bot: I haz the power, 2016-12-24 00:24:09]

Patchset 5 (id:??) landed as
https://crrev.com/146accf2581a4ac5184cc9c5f0f2e2a91bf4be23
Cr-Commit-Position: refs/heads/master@{#440660}

[PhistucK, 2016-12-24 08:19:25]

phistuck@gmail.com changed reviewers:
+ phistuck@gmail.com

[PhistucK, 2016-12-24 08:19:26]

https://codereview.chromium.org/2604633002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp (right):

https://codereview.chromium.org/2604633002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/platform/weborigin/SchemeRegistry.cpp:247: // "data"
is not included because real sites shouldn't be using it for
I have seen data: used as a top level browsing context. It was mostly used for
anonymous redirection (or anonymous redirection with a delay in order to show
advertisements).

"shouldn't" died when the web was created. "We do not care about those" is more
appropriate, but politically incorrect. :)