Add UseCounter to see how often the "allowfullscreen" attribute is modified.

Add UseCounter to see how often the "allowfullscreen" attribute is modified.

Before implementing snapshotting of the iframe allowfullscreen attribute, we
should have some idea of how often the existing live attribute is being modified
after the frame is attached to a document. This adds a new UseCounter named
HTMLIFrameElementAllowfullscreenAttribute to track this.

BUG=682282
Review-Url: https://codereview.chromium.org/2672823006
Cr-Commit-Position: refs/heads/master@{#448923}
Committed: https://chromium.googlesource.com/chromium/src/+/2906e0c1298431ea3cdb9fb3a8da8046d7c9e6bb

[iclelland, 2017-02-03 20:14:37]

iclelland@chromium.org changed reviewers:
+ foolip@chromium.org

[iclelland, 2017-02-03 20:14:37]

+r foolip, can you sanity-check this?

Also, I'm unsure whether this counter is sufficient for interop risk estimation,
or if we should also be counting the times when webkitRequestFullscreen is
called after this attribute is modified. WDYT?

[iclelland, 2017-02-03 20:14:45]

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

[commit-bot: I haz the power, 2017-02-03 20:15:12]

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, 2017-02-03 22:08:36]

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

[commit-bot: I haz the power, 2017-02-03 22:08:37]

Dry run: This issue passed the CQ dry run.

[foolip, 2017-02-05 23:22:08]

Tentatively LGTM, but I'm not sure if this is measuring exactly the risk
involved with changing the behavior, or "just" a fair proxy of it.

To be sure that we'll have the data we need, I'd say ideally try to measure
these two things:

1. When the https://fullscreen.spec.whatwg.org/#fullscreen-element-ready-check
would return false instead of true for some element which is connected and in an
iframe. (Exactly or almost the counter you added, not sure.)

2. When that difference is actually decisive in whether a fullscreen request
succeeds or fails. In other words, something in
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Fulls...
that checks using both methods and counts if the the new method fails but the
old does not.

https://codereview.chromium.org/2672823006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/2672823006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:129: if
(contentFrame()) {
I believe this means roughly "has this iframe loaded", but does the null-ness of
contentFrame() change at exactly the time that feature policy and sanbox
attributes are snapshotted?

I have no reason to suspect that it'd inflate the numbers, but this counter also
includes a case where the allowfullscreen attribute is removed after load. Don't
know why you'd do that, but interwebs are full of surprises.

https://codereview.chromium.org/2672823006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:131: document(),
UseCounter::HTMLIFrameElementAllowfullscreenAttribute);
Can you append "ChangedAfterContentLoad" or something like that? People
sometimes go hunting in chromestatus.com for counters to answer a particular
question, and counters cover much more or less than the name suggest can
confuse. (Obviously 100% precision is not reasonable though.)

[iclelland, 2017-02-06 20:15:26]

Holding off on committing this for now.

I'm going to see what I can do to try to collect the more precise data of when
the attribute is changed between content load and requestFullscreen still, but
if that looks like a ton of accounting and plumbing, then I'd still like to land
this to get some kind of estimate.

https://codereview.chromium.org/2672823006/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp (right):

https://codereview.chromium.org/2672823006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:129: if
(contentFrame()) {
On 2017/02/05 23:22:08, foolip wrote:
> I believe this means roughly "has this iframe loaded", but does the null-ness
of
> contentFrame() change at exactly the time that feature policy and sanbox
> attributes are snapshotted?

Sandbox attributes are snapshotted in |WebLocalFrameImpl::createChildFrame,
immediately before |WebLocalFrameImpl::initializeCoreFrame| is called. During
that method, the |m_contentFrame| member is set (through LocalFrame::create.)

|m_contentFrame| is nulled when the frame is detached from the document. If
reattached, the new frame contents are initialized the same way.

This is not reset on subframe navigation, though, so it won't catch the pattern
of Add Iframe -> Change Attribute -> Navigate -> Request Fullscreen; that will
look like a blocked fullscreen request.

> 
> I have no reason to suspect that it'd inflate the numbers, but this counter
also
> includes a case where the allowfullscreen attribute is removed after load.
Don't
> know why you'd do that, but interwebs are full of surprises.

That's deliberate, as it would also be a situation in which snapshotting would
change existing behaviour. (If a site tried to block an otherwise allowed
fullscreen request by removing the attribute)

Both of these issues, I believe, will cause us to overcount the impact of this
change. We could tighten this up, to try to only catch the exact situation where
fullscreen would break, but that should be a strictly smaller number than this
one. I'd like to land this counter, to try to merge to M57, to get as much data
as we can. I'll add the other, tighter use counter in a separate CL if you're
okay with that.

https://codereview.chromium.org/2672823006/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/html/HTMLIFrameElement.cpp:131: document(),
UseCounter::HTMLIFrameElementAllowfullscreenAttribute);
On 2017/02/05 23:22:08, foolip wrote:
> Can you append "ChangedAfterContentLoad" or something like that? People
> sometimes go hunting in http://chromestatus.com for counters to answer a
particular
> question, and counters cover much more or less than the name suggest can
> confuse. (Obviously 100% precision is not reasonable though.)

Done.

[iclelland, 2017-02-06 21:36:47]

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

[commit-bot: I haz the power, 2017-02-06 21:37:36]

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

[iclelland, 2017-02-06 21:50:23]

foolip, is this what you had in mind for only testing when the attribute is set
(not cleared) after load?

[foolip, 2017-02-06 22:01:56]

On 2017/02/06 21:50:23, iclelland wrote:
> foolip, is this what you had in mind for only testing when the attribute is
set
> (not cleared) after load?

yep, lgtm

[foolip, 2017-02-06 22:03:51]

On 2017/02/06 22:01:56, foolip wrote:
> On 2017/02/06 21:50:23, iclelland wrote:
> > foolip, is this what you had in mind for only testing when the attribute is
> set
> > (not cleared) after load?
> 
> yep, lgtm

Also, I've realized that in order to really measure when the difference matters,
one would not only have to store a m_allowFullscreenSnapshot, but also replicate
that for OOPIF. Fingers crossed that the numbers from this are small enough that
we don't need to worry about that.

[commit-bot: I haz the power, 2017-02-06 23:14:52]

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

[commit-bot: I haz the power, 2017-02-06 23:14:52]

Dry run: This issue passed the CQ dry run.

[iclelland, 2017-02-07 13:48:47]

iclelland@chromium.org changed reviewers:
+ holte@chromium.org

[iclelland, 2017-02-07 13:48:47]

+r holte, can you PTAL at the histogram change?

Thanks!

[Steven Holte, 2017-02-07 23:36:51]

lgtm

[iclelland, 2017-02-07 23:53:26]

The CQ bit was checked by iclelland@chromium.org

[commit-bot: I haz the power, 2017-02-07 23:54:25]

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, 2017-02-07 23:58:14]

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

[commit-bot: I haz the power, 2017-02-07 23:58:15]

Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[iclelland, 2017-02-08 04:42:25]

The CQ bit was checked by iclelland@chromium.org

[iclelland, 2017-02-08 04:42:25]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org,
holte@chromium.org
Link to the patchset: https://codereview.chromium.org/2672823006/#ps60001
(title: "Rebase")

[commit-bot: I haz the power, 2017-02-08 04:42:39]

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, 2017-02-08 06:50:35]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1486528945154260,
"parent_rev": "cb3a29efa6f3e8fddbd6767c2e853687d4279ffc", "commit_rev":
"2906e0c1298431ea3cdb9fb3a8da8046d7c9e6bb"}

[commit-bot: I haz the power, 2017-02-08 06:52:17]

Description was changed from

==========
Add UseCounter to see how often the "allowfullscreen" attribute is modified.

Before implementing snapshotting of the iframe allowfullscreen attribute, we
should have some idea of how often the existing live attribute is being modified
after the frame is attached to a document. This adds a new UseCounter named
HTMLIFrameElementAllowfullscreenAttribute to track this.

BUG=682282
==========

to

==========
Add UseCounter to see how often the "allowfullscreen" attribute is modified.

Before implementing snapshotting of the iframe allowfullscreen attribute, we
should have some idea of how often the existing live attribute is being modified
after the frame is attached to a document. This adds a new UseCounter named
HTMLIFrameElementAllowfullscreenAttribute to track this.

BUG=682282

Review-Url: https://codereview.chromium.org/2672823006
Cr-Commit-Position: refs/heads/master@{#448923}
Committed:
https://chromium.googlesource.com/chromium/src/+/2906e0c1298431ea3cdb9fb3a8da...
==========

[commit-bot: I haz the power, 2017-02-08 06:52:18]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/2906e0c1298431ea3cdb9fb3a8da...