Count internal use of DOMImplementation::hasFeature() returning false

Count internal use of DOMImplementation::hasFeature() returning false

This is the only internal usage of DOMImplementation::hasFeature(), and
the only way that svg:requiredFeatures is observable, other than the
it->isEmpty() case.

If usage is low, DOMImplementation.hasFeature() and this internal usage
can be changed together, see
https://groups.google.com/a/chromium.org/d/msg/blink-dev/r2aARnIHG4k/_dN33g65mYgJ

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185133

[philipj_slow, 2014-11-08 23:10:41]

philipj@opera.com changed reviewers:
+ ed@opera.com, pdr@chromium.org

[philipj_slow, 2014-11-08 23:10:41]

PTAL

[kouhei (in TOK), 2014-11-08 23:51:41]

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

[kouhei (in TOK), 2014-11-08 23:51:42]

https://codereview.chromium.org/712813002/diff/1/Source/core/svg/SVGTests.cpp
File Source/core/svg/SVGTests.cpp (right):

https://codereview.chromium.org/712813002/diff/1/Source/core/svg/SVGTests.cpp...
Source/core/svg/SVGTests.cpp:59: UseCounter::count(document,
UseCounter::SVGRequiredFeatures);
This UseCounter is counted if requiredFeatures is used and if it is used to
disable displaying the nodes.

Would you rename the UseCounter name and update the CL description that this is
clear?

[Stephen Chennney, 2014-11-10 14:54:28]

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

[Stephen Chennney, 2014-11-10 14:54:28]

What are you hoping to discover? We already have, and have had for quite some
time:

DOMImplementationHasFeature: 3.9672%
DOMImplementationHasFeatureReturnFalse: 0.0053%\

So it is not uncommon for pages to call hasFeature but it is very uncommon for
it to return false. My understanding is that we were planning to remove it after
a deprecation period, and during deprecation we would just always return true.

[Stephen Chennney, 2014-11-10 14:59:32]

On 2014/11/10 14:54:28, Stephen Chenney wrote:
> What are you hoping to discover? We already have, and have had for quite some
> time:
> 
> DOMImplementationHasFeature: 3.9672%
> DOMImplementationHasFeatureReturnFalse: 0.0053%\
> 
> So it is not uncommon for pages to call hasFeature but it is very uncommon for
> it to return false. My understanding is that we were planning to remove it
after
> a deprecation period, and during deprecation we would just always return true.

Now I see the blink-dev thread. That's what I get for looking at review emails
before blink-dev emails.

I agree with Kouhei's request to make it the intention very clear in the CL
description.

[philipj_slow, 2014-11-10 15:21:03]

rename counter

[philipj_slow, 2014-11-10 15:58:30]

I've renamed the counter and updated the commit message, PTAL.

[Stephen Chennney, 2014-11-10 16:16:21]

LGTM. Wow, it took me a long time to find the one place where we actually invoke
this functionality.

I would also be interested in finding out how often the svg <switch> element is
used. It is obviously directly correlated with this case, and would inform us of
the likelihood that rendering will break.

Also, don't forget to run the script that updates the histograms:
src/tools/metrics/histograms/update_histogram_enum

Then you'll need to put up a chromium patch to get the changes committed. But
then, I think you've done this before.

[philipj_slow, 2014-11-10 20:39:58]

On 2014/11/10 16:16:21, Stephen Chenney wrote:
> LGTM. Wow, it took me a long time to find the one place where we actually
invoke
> this functionality.

Do you mean where SVGElement::isValid() ends up being used? I see three callers
in
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

> I would also be interested in finding out how often the svg <switch> element
is
> used. It is obviously directly correlated with this case, and would inform us
of
> the likelihood that rendering will break.

https://www.chromestatus.com/metrics/feature/timeline/popularity/80 is around
0.02%.

Since <switch> is not the only place where requiredFeatures has ann effect, that
use counter is not enough.

> Also, don't forget to run the script that updates the histograms:
> src/tools/metrics/histograms/update_histogram_enum
> 
> Then you'll need to put up a chromium patch to get the changes committed. But
> then, I think you've done this before.

Yep, I know the drill :)

[kouhei (in TOK), 2014-11-10 23:32:01]

lgtm

[philipj_slow, 2014-11-11 08:56:49]

rebase

[philipj_slow, 2014-11-11 08:57:14]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-11-11 08:57:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/712813002/40001

[commit-bot: I haz the power, 2014-11-11 11:15:38]

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

[commit-bot: I haz the power, 2014-11-11 11:15:39]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/35782)

[philipj_slow, 2014-11-11 16:11:22]

The CQ bit was checked by philipj@opera.com

[commit-bot: I haz the power, 2014-11-11 16:11:55]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/712813002/40001

[commit-bot: I haz the power, 2014-11-11 17:06:19]

Committed patchset #3 (id:40001) as 185133