Move Document.fullscreenElement to DocumentOrShadowRoot

Move Document.fullscreenElement to DocumentOrShadowRoot

This change aims to prevent nodes in shadow tree from
leaking via global API (document.fullscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

BUG=622585
TBR=ericwilligers@chromium.org for tools/metrics/histograms/histograms.xml

Committed: https://crrev.com/7025a706a1b4d382e5241905904ca803808000f9
Cr-Commit-Position: refs/heads/master@{#427610}

[kochi, 2016-09-15 09:41:29]

Description was changed from

==========
Split Document.fullscreenElement to Document and ShadowRoot

BUG=622585
==========

to

==========
Split Document.fullscreenElement to Document and ShadowRoot

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

BUG=622585
==========

[kochi, 2016-09-15 09:41:29]

kochi@chromium.org changed reviewers:
+ foolip@chromium.org, hayato@chromium.org

[kochi, 2016-09-15 10:23:23]

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

[commit-bot: I haz the power, 2016-09-15 10:23:44]

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

[kochi, 2016-09-15 10:32:46]

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

[commit-bot: I haz the power, 2016-09-15 10:33:16]

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

[kochi, 2016-09-15 11:00:57]

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

[commit-bot: I haz the power, 2016-09-15 11:01:24]

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

[kochi, 2016-09-15 11:05:41]

Description was changed from

==========
Split Document.fullscreenElement to Document and ShadowRoot

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

BUG=622585
==========

to

==========
Split Document.fullscreenElement to Document and ShadowRoot

This change aims to prevent node leak nodes in shadow tree
via global API (document.fulscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

This change intentionally preserve the behavior of
document.webkitFulscreenElement, which is already
exposed to the web.

BUG=622585
==========

[kochi, 2016-09-15 11:06:08]

Description was changed from

==========
Split Document.fullscreenElement to Document and ShadowRoot

This change aims to prevent node leak nodes in shadow tree
via global API (document.fulscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

This change intentionally preserve the behavior of
document.webkitFulscreenElement, which is already
exposed to the web.

BUG=622585
==========

to

==========
Split Document.fullscreenElement to Document and ShadowRoot

This change aims to prevent node leak nodes in shadow tree
via global API (document.fulscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

This change intentionally preserve the behavior of
document.webkitFulscreenElement, which is already
exposed to the web.

BUG=622585
==========

[kochi, 2016-09-15 11:07:46]

Description was changed from

==========
Split Document.fullscreenElement to Document and ShadowRoot

This change aims to prevent node leak nodes in shadow tree
via global API (document.fulscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

This change intentionally preserve the behavior of
document.webkitFulscreenElement, which is already
exposed to the web.

BUG=622585
==========

to

==========
Move Document.fullscreenElement to DocumentOrShadowRoot

This change aims to prevent nodes in shadow tree from
leaking via global API (document.fulscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

This change intentionally preserves the behavior of
document.webkitFulscreenElement, which is already
exposed to the web.

BUG=622585
==========

[kochi, 2016-09-15 11:09:22]

PTAL

[foolip, 2016-09-15 11:58:53]

Description was changed from

==========
Move Document.fullscreenElement to DocumentOrShadowRoot

This change aims to prevent nodes in shadow tree from
leaking via global API (document.fulscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

This change intentionally preserves the behavior of
document.webkitFulscreenElement, which is already
exposed to the web.

BUG=622585
==========

to

==========
Move Document.fullscreenElement to DocumentOrShadowRoot

This change aims to prevent nodes in shadow tree from
leaking via global API (document.fullscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

This change intentionally preserves the behavior of
document.webkitFullscreenElement, which is already
exposed to the web.

BUG=622585
==========

[commit-bot: I haz the power, 2016-09-15 12:25:38]

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

[commit-bot: I haz the power, 2016-09-15 12:25:39]

Dry run: This issue passed the CQ dry run.

[foolip, 2016-09-15 16:07:36]

High level I think it's actually important that we keep the prefixed and
unprefixed API in sync, so that webkitFullscreenElement start returning the
shadow host in these cases. Otherwise we always risk being stuck with the
difference forever, and differences like this increase the risk for a web
developer to transition to the unprefixed API once it's shipped.

There's also webkitCurrentFullScreenElement which unfortunately isn't always the
same as webkitFullscreenElement which will need the same treatment, because I'm
working on making them aliases.

[kochi, 2016-09-20 11:10:26]

Description was changed from

==========
Move Document.fullscreenElement to DocumentOrShadowRoot

This change aims to prevent nodes in shadow tree from
leaking via global API (document.fullscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

This change intentionally preserves the behavior of
document.webkitFullscreenElement, which is already
exposed to the web.

BUG=622585
==========

to

==========
Move Document.fullscreenElement to DocumentOrShadowRoot

This change aims to prevent nodes in shadow tree from
leaking via global API (document.fullscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

BUG=622585
==========

[kochi, 2016-09-20 11:11:26]

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

[commit-bot: I haz the power, 2016-09-20 11:11:37]

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

[kochi, 2016-09-20 11:12:57]

On 2016/09/15 16:07:36, foolip wrote:
> High level I think it's actually important that we keep the prefixed and
> unprefixed API in sync, so that webkitFullscreenElement start returning the
> shadow host in these cases. Otherwise we always risk being stuck with the
> difference forever, and differences like this increase the risk for a web
> developer to transition to the unprefixed API once it's shipped.
> 
> There's also webkitCurrentFullScreenElement which unfortunately isn't always
the
> same as webkitFullscreenElement which will need the same treatment, because
I'm
> working on making them aliases.

Thanks for the comment.
I discussed with Hayato that we can also make the same change for
.webkitFullscreenElement as unprefixed version.

Also made the same treatment for Mozilla version
(webkitCurrentFullScreenElement)
as well.

Could you take another look?

[foolip, 2016-09-20 13:44:22]

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentFullscreen.idl (right):

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentFullscreen.idl:32:
[MeasureAs=PrefixedDocumentIsFullscreen] readonly attribute boolean
webkitIsFullScreen;
If you change DocumentOrShadowRoot.cpp to simply forward to
Fullscreen::fullscreenElement(Document/ShadowRoot&) and
Fullscreen::currentFullScreenElement(Document/ShadowRoot&), I think these
aliases in the IDL won't need to change, and more of what the Fullscreen spec
says is implemented in Fullscreen.cpp.

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.h (right):

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.h:91:
UseCounter::count(document, UseCounter::DocumentFullscreenElementInV0Shadow);
If we go the path of gating the change in behavior on FullscreenUnprefixed, then
I think the use counter won't be necessary.

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl (right):

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl:24: // Fullscreen
API
Does our IDL compiler support partial interfaces for mixins? If so
DocumentOrShadowRootFullscreen.idl would be nice, I have been thinking that
maybe fullscreen could move into core/fullscreen or modules/fullscreen after the
merge into top layer.

If not, that's OK too.

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl:27:
[MeasureAs=PrefixedDocumentCurrentFullScreenElement,
ImplementedAs=currentFullScreenElement] readonly attribute Element
webkitCurrentFullScreenElement;
I was unclear/misleading with "important that we keep the prefixed and
unprefixed API in sync". I think that the document.fullscreenElement and
.webkitFullscreenElement should be plain aliases, but for ShadowRoot I see two
unfortunate options:

1. Add prefixed attributes, as in this CL.
2. Don't add them, and make it impossible to find the real fullscreen element.

I am working on unprefixing, so how about gating the change in behavior of the
prefixed attributes on the FullscreenUnprefixed flag? That increases the compat
risk of shipping FullscreenUnprefixed, but the other two options seem worse.

https://codereview.chromium.org/2340263003/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/shadow-dom/fullscreen-element-in-shadow.html
(right):

https://codereview.chromium.org/2340263003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/shadow-dom/fullscreen-element-in-shadow.html:14:
<canvas></canvas>
Can you turn this into the minimal tree needed for the test? The simplest test
would be just a single di, with a shadow tree with a single div.

https://codereview.chromium.org/2340263003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/shadow-dom/fullscreen-element-in-shadow.html:84:
canvas.requestFullscreen();
Tests for nested fullscreen cases and when the :-webkit-full-screen selector
matches would also be great. It would be fantastic to have the tests in the
style of LayoutTests/fullscreen/model/fully-exit-fullscreen-nested.html so that
I can also upstream to web-platform-tests more easily.

[kochi, 2016-10-07 10:28:20]

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

[commit-bot: I haz the power, 2016-10-07 10:28:32]

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

[kochi, 2016-10-07 11:08:34]

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

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

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-10-07 14:13:47]

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

[commit-bot: I haz the power, 2016-10-07 14:13:48]

Dry run: This issue passed the CQ dry run.

[kochi, 2016-10-07 14:26:42]

Sorry for not updating for a while.

Could you take a look?

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentFullscreen.idl (right):

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentFullscreen.idl:32:
[MeasureAs=PrefixedDocumentIsFullscreen] readonly attribute boolean
webkitIsFullScreen;
On 2016/09/20 13:44:21, foolip wrote:
> If you change DocumentOrShadowRoot.cpp to simply forward to
> Fullscreen::fullscreenElement(Document/ShadowRoot&) and
> Fullscreen::currentFullScreenElement(Document/ShadowRoot&), I think these
> aliases in the IDL won't need to change, and more of what the Fullscreen spec
> says is implemented in Fullscreen.cpp.

Done. Now I see clearly that the original intention for separating
DocumentFullscreen
and Fullscreen.

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.h (right):

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.h:91:
UseCounter::count(document, UseCounter::DocumentFullscreenElementInV0Shadow);
On 2016/09/20 13:44:21, foolip wrote:
> If we go the path of gating the change in behavior on FullscreenUnprefixed,
then
> I think the use counter won't be necessary.

Done.

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl (right):

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl:24: // Fullscreen
API
On 2016/09/20 13:44:21, foolip wrote:
> Does our IDL compiler support partial interfaces for mixins? If so
> DocumentOrShadowRootFullscreen.idl would be nice, I have been thinking that
> maybe fullscreen could move into core/fullscreen or modules/fullscreen after
the
> merge into top layer.
> 
> If not, that's OK too.

Confirmed with our IDL compiler people and partial interface for mixin does not
work yet.
So let me go in this file.

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl:27:
[MeasureAs=PrefixedDocumentCurrentFullScreenElement,
ImplementedAs=currentFullScreenElement] readonly attribute Element
webkitCurrentFullScreenElement;
On 2016/09/20 13:44:21, foolip wrote:
> I was unclear/misleading with "important that we keep the prefixed and
> unprefixed API in sync". I think that the document.fullscreenElement and
> .webkitFullscreenElement should be plain aliases, but for ShadowRoot I see two
> unfortunate options:
> 
> 1. Add prefixed attributes, as in this CL.
> 2. Don't add them, and make it impossible to find the real fullscreen element.
> 
> I am working on unprefixing, so how about gating the change in behavior of the
> prefixed attributes on the FullscreenUnprefixed flag? That increases the
compat
> risk of shipping FullscreenUnprefixed, but the other two options seem worse.

I may be still confused. Correct my if I misunderstand.

So for webkit-prefixed attributes, we don't add them in ShadowRoot, and leave
them
in Document, and in the implementation we do the new behavior gated by
FullscreenUnprefixed flag?

Original motivation for this was not to leak nodes inside shadow, but
as it is already shipped, changing this has some compat risk.

So effectively we do not change .webkitCurrentFullScreenElement and
.webkitFullscreenElement, and only make changes in .fullscreenElement,
as it is specced in whatwg fullscreen spec.

https://codereview.chromium.org/2340263003/diff/160001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/shadow-dom/fullscreen-element-in-shadow.html
(right):

https://codereview.chromium.org/2340263003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/shadow-dom/fullscreen-element-in-shadow.html:14:
<canvas></canvas>
On 2016/09/20 13:44:21, foolip wrote:
> Can you turn this into the minimal tree needed for the test? The simplest test
> would be just a single di, with a shadow tree with a single div.

This tests cases where some trees are involved and sibling shadow tree can
return
null for its fullscreenElement.

Created another simplest test case in a separate file.

https://codereview.chromium.org/2340263003/diff/160001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/shadow-dom/fullscreen-element-in-shadow.html:84:
canvas.requestFullscreen();
On 2016/09/20 13:44:21, foolip wrote:
> Tests for nested fullscreen cases and when the :-webkit-full-screen selector
> matches would also be great. It would be fantastic to have the tests in the
> style of LayoutTests/fullscreen/model/fully-exit-fullscreen-nested.html so
that
> I can also upstream to web-platform-tests more easily.

Changed to use the function in fullscreen/trusted-event.js.

For :-webkit-full-screen test, I'd like to work on it in another CL, for
supporting changes in the new :fullscreen for shadow hosts.
For nested cases, also I'd like to work on a separate CL for crbug.com/535164.

[foolip, 2016-10-10 12:13:41]

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl (right):

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl:24: // Fullscreen
API
On 2016/10/07 14:26:42, kochi_ooo_until_oct_14 wrote:
> On 2016/09/20 13:44:21, foolip wrote:
> > Does our IDL compiler support partial interfaces for mixins? If so
> > DocumentOrShadowRootFullscreen.idl would be nice, I have been thinking that
> > maybe fullscreen could move into core/fullscreen or modules/fullscreen after
> the
> > merge into top layer.
> > 
> > If not, that's OK too.
> 
> Confirmed with our IDL compiler people and partial interface for mixin does
not
> work yet.
> So let me go in this file.

Very well. Is there a bug for that?

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl:27:
[MeasureAs=PrefixedDocumentCurrentFullScreenElement,
ImplementedAs=currentFullScreenElement] readonly attribute Element
webkitCurrentFullScreenElement;
On 2016/10/07 14:26:42, kochi_ooo_until_oct_14 wrote:
> On 2016/09/20 13:44:21, foolip wrote:
> > I was unclear/misleading with "important that we keep the prefixed and
> > unprefixed API in sync". I think that the document.fullscreenElement and
> > .webkitFullscreenElement should be plain aliases, but for ShadowRoot I see
two
> > unfortunate options:
> > 
> > 1. Add prefixed attributes, as in this CL.
> > 2. Don't add them, and make it impossible to find the real fullscreen
element.
> > 
> > I am working on unprefixing, so how about gating the change in behavior of
the
> > prefixed attributes on the FullscreenUnprefixed flag? That increases the
> compat
> > risk of shipping FullscreenUnprefixed, but the other two options seem worse.
> 
> I may be still confused. Correct my if I misunderstand.
> 
> So for webkit-prefixed attributes, we don't add them in ShadowRoot, and leave
> them
> in Document, and in the implementation we do the new behavior gated by
> FullscreenUnprefixed flag?
> 
> Original motivation for this was not to leak nodes inside shadow, but
> as it is already shipped, changing this has some compat risk.
> 
> So effectively we do not change .webkitCurrentFullScreenElement and
> .webkitFullscreenElement, and only make changes in .fullscreenElement,
> as it is specced in whatwg fullscreen spec.

Yes, I'm suggesting no web exposed changes at all right now, but rather to
change the implementation of the (internal, non-static)
Fullscreen::fullscreenElement() and ::currentFullScreenElement() to return the
retargeted element if RuntimeEnabledFeatures::fullscreenUnprefixedEnabled().

https://codereview.chromium.org/2340263003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentFullscreen.h (right):

https://codereview.chromium.org/2340263003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentFullscreen.h:42: static Element*
webkitFullscreenElement(Document&);
If prefixed and unprefixed are kept in sync, these changes should not be needed.

https://codereview.chromium.org/2340263003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentFullscreen.idl (right):

https://codereview.chromium.org/2340263003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentFullscreen.idl:33:
[MeasureAs=PrefixedDocumentIsFullscreen,
ImplementedAs=webkitCurrentFullScreenElement] readonly attribute boolean
webkitIsFullScreen;
Ditto.

https://codereview.chromium.org/2340263003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.h (right):

https://codereview.chromium.org/2340263003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.h:79: return
Fullscreen::fullscreenElementForBindingFrom(document);
I think it's probably fine to not have a special code path for bindings, better
to have the internal APIs behave like bindings and have internal methods
ignoring shadow boundaries if needed.

https://codereview.chromium.org/2340263003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl (right):

https://codereview.chromium.org/2340263003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl:26: // W3C version
Drop "W3C version", in DocumentFullscreen.idl it's used to distinguish two forms
of the prefixed API.

[kochi, 2016-10-17 11:04:30]

Patchset #13 (id:240001) has been deleted

[kochi, 2016-10-17 11:04:41]

Patchset #13 (id:260001) has been deleted

[kochi, 2016-10-17 11:04:50]

Patchset #13 (id:280001) has been deleted

[kochi, 2016-10-17 11:14:27]

Sorry for the long turnaround, I'm back from ooo.
Could you review this?

foolip@, for .webkitCurrentFullScreenElement, will it be
unprefixed?  If it will not be included in WHATWG fullscreen
spec, I don't think we shouldn't add ShadowRoot version,
but probably it makes sense to retarget when it is unprefixed.

This version still have 
DocumentFullscreen::webkitFullscreenElement()
and
DocumentOrShadowRoot::fullscreenElement()
which are identical but defined separately.

As .idl files are separate, we can't make the one of them
an alias for the other?

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl (right):

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl:24: // Fullscreen
API
On 2016/10/10 12:13:40, foolip wrote:
> On 2016/10/07 14:26:42, kochi_ooo_until_oct_14 wrote:
> > On 2016/09/20 13:44:21, foolip wrote:
> > > Does our IDL compiler support partial interfaces for mixins? If so
> > > DocumentOrShadowRootFullscreen.idl would be nice, I have been thinking
that
> > > maybe fullscreen could move into core/fullscreen or modules/fullscreen
after
> > the
> > > merge into top layer.
> > > 
> > > If not, that's OK too.
> > 
> > Confirmed with our IDL compiler people and partial interface for mixin does
> not
> > work yet.
> > So let me go in this file.
> 
> Very well. Is there a bug for that?

Filed http://crbug.com/656517

https://codereview.chromium.org/2340263003/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl:27:
[MeasureAs=PrefixedDocumentCurrentFullScreenElement,
ImplementedAs=currentFullScreenElement] readonly attribute Element
webkitCurrentFullScreenElement;
On 2016/10/10 12:13:40, foolip wrote:
> On 2016/10/07 14:26:42, kochi_ooo_until_oct_14 wrote:
> > On 2016/09/20 13:44:21, foolip wrote:
> > > I was unclear/misleading with "important that we keep the prefixed and
> > > unprefixed API in sync". I think that the document.fullscreenElement and
> > > .webkitFullscreenElement should be plain aliases, but for ShadowRoot I see
> two
> > > unfortunate options:
> > > 
> > > 1. Add prefixed attributes, as in this CL.
> > > 2. Don't add them, and make it impossible to find the real fullscreen
> element.
> > > 
> > > I am working on unprefixing, so how about gating the change in behavior of
> the
> > > prefixed attributes on the FullscreenUnprefixed flag? That increases the
> > compat
> > > risk of shipping FullscreenUnprefixed, but the other two options seem
worse.
> > 
> > I may be still confused. Correct my if I misunderstand.
> > 
> > So for webkit-prefixed attributes, we don't add them in ShadowRoot, and
leave
> > them
> > in Document, and in the implementation we do the new behavior gated by
> > FullscreenUnprefixed flag?
> > 
> > Original motivation for this was not to leak nodes inside shadow, but
> > as it is already shipped, changing this has some compat risk.
> > 
> > So effectively we do not change .webkitCurrentFullScreenElement and
> > .webkitFullscreenElement, and only make changes in .fullscreenElement,
> > as it is specced in whatwg fullscreen spec.
> 
> Yes, I'm suggesting no web exposed changes at all right now, but rather to
> change the implementation of the (internal, non-static)
> Fullscreen::fullscreenElement() and ::currentFullScreenElement() to return the
> retargeted element if RuntimeEnabledFeatures::fullscreenUnprefixedEnabled().

It does not work, as Fullscreen::fullscreenElement() is also called
by other non-binding users (e.g. PaintLayerCompositor.cpp).
So we cannot rely on RuntimeEnabledFeatures to determine the retargeting
behavior in internal fullscreenElement() API.

So I still left the logic for retargeting or not in *ForBinding* functions.

https://codereview.chromium.org/2340263003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.h (right):

https://codereview.chromium.org/2340263003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.h:79: return
Fullscreen::fullscreenElementForBindingFrom(document);
On 2016/10/10 12:13:40, foolip wrote:
> I think it's probably fine to not have a special code path for bindings,
better
> to have the internal APIs behave like bindings and have internal methods
> ignoring shadow boundaries if needed.

The motivation I introduced *ForBinding* was that there are some other
users of Fullscreen::fullscreenElementFrom(), and I think what they need
is shadow boundary-agnostic one (the original), and shadow boundary-aware
one is only needed for bindings.

https://codereview.chromium.org/2340263003/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl (right):

https://codereview.chromium.org/2340263003/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentOrShadowRoot.idl:26: // W3C version
On 2016/10/10 12:13:41, foolip wrote:
> Drop "W3C version", in DocumentFullscreen.idl it's used to distinguish two
forms
> of the prefixed API.

Done.

[kochi, 2016-10-24 15:07:19]

foolip@, could you review?

Now I can spend more cycles on this CL so it will not take as much time as
previous reviews.

[foolip, 2016-10-24 15:15:58]

On 2016/10/24 15:07:19, kochi wrote:
> foolip@, could you review?
> 
> Now I can spend more cycles on this CL so it will not take as much time as
> previous reviews.

Oh sorry, I saw this last week but it got buried in my inbox. Looking now.

[foolip, 2016-10-24 15:29:42]

On 2016/10/17 11:14:27, kochi wrote:
> Sorry for the long turnaround, I'm back from ooo.
> Could you review this?
> 
> foolip@, for .webkitCurrentFullScreenElement, will it be
> unprefixed?  If it will not be included in WHATWG fullscreen
> spec, I don't think we shouldn't add ShadowRoot version,
> but probably it makes sense to retarget when it is unprefixed.

The plan is that webkitCurrentFullScreenElement should be an alias of
webkitFullscreenElement, see issue 402421. That's blocking unprefixing of
fullscreen.

> This version still have 
> DocumentFullscreen::webkitFullscreenElement()
> and
> DocumentOrShadowRoot::fullscreenElement()
> which are identical but defined separately.
> 
> As .idl files are separate, we can't make the one of them
> an alias for the other?

Right, some extra code will be needed, but [ImplementedAs=fullscreenElement]
should still work.

[foolip, 2016-10-24 15:30:13]

lgtm % nits

https://codereview.chromium.org/2340263003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentFullscreen.h (right):

https://codereview.chromium.org/2340263003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentFullscreen.h:42: static Element*
webkitFullscreenElement(Document&);
Can you keep ImplementedAs in the IDL file? These three static methods are
called exclusively from bindings, all internal code paths use
Fullscreen::something().

https://codereview.chromium.org/2340263003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2340263003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:223: // For Shadow DOM V0
compatibility: We allow returning an element in V0
Do you want to attempt all of these changes at once? If it's fine to do them
one-by-one, then just not having this code and tying the behavior change to
unprefixed Fullscreen seems OK. If not, why not the same logic for
Fullscreen::currentFullScreenElementForBindingFrom below?

[kochi, 2016-10-25 10:06:33]

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

[commit-bot: I haz the power, 2016-10-25 10:06:51]

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

[kochi, 2016-10-25 10:08:04]

Thanks for the review!
The updated patch uploaded.

hayato@, could you also take a look?

https://codereview.chromium.org/2340263003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/DocumentFullscreen.h (right):

https://codereview.chromium.org/2340263003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/DocumentFullscreen.h:42: static Element*
webkitFullscreenElement(Document&);
On 2016/10/24 15:30:12, foolip wrote:
> Can you keep ImplementedAs in the IDL file? These three static methods are
> called exclusively from bindings, all internal code paths use
> Fullscreen::something().

Done.

https://codereview.chromium.org/2340263003/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2340263003/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:223: // For Shadow DOM V0
compatibility: We allow returning an element in V0
On 2016/10/24 15:30:12, foolip wrote:
> Do you want to attempt all of these changes at once? If it's fine to do them
> one-by-one, then just not having this code and tying the behavior change to
> unprefixed Fullscreen seems OK. If not, why not the same logic for
> Fullscreen::currentFullScreenElementForBindingFrom below?

Ah good catch! I forgot to add this logic to
Fullscreen::currentFullScreenElementForBindingFrom below.
(the layout test case didn't catch this unfortunately, as
document.adjustedElement() does also skip V0 shadow root.)

[commit-bot: I haz the power, 2016-10-25 10:08:39]

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

[commit-bot: I haz the power, 2016-10-25 10:08:40]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  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...)

[kochi, 2016-10-25 10:30:28]

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

[commit-bot: I haz the power, 2016-10-25 10:30:50]

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-10-25 12:02:08]

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

[commit-bot: I haz the power, 2016-10-25 12:02:10]

Dry run: This issue passed the CQ dry run.

[hayato, 2016-10-26 03:15:56]

lgtm

https://codereview.chromium.org/2340263003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2340263003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:230: } else {
else if

https://codereview.chromium.org/2340263003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:231: if
(!toShadowRoot(scope).isV1())
Could we avoid toShadowRoot(treeScope)?
I am not comfortable with this cast. I think toShadowRoot(scope.rootNode())
would be consistent with other files.

[kochi, 2016-10-26 04:51:32]

Description was changed from

==========
Move Document.fullscreenElement to DocumentOrShadowRoot

This change aims to prevent nodes in shadow tree from
leaking via global API (document.fullscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

BUG=622585
==========

to

==========
Move Document.fullscreenElement to DocumentOrShadowRoot

This change aims to prevent nodes in shadow tree from
leaking via global API (document.fullscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

BUG=622585
TBR=ericwilligers@chromium.org for tools/metrics/histograms/histograms.xml
==========

[kochi, 2016-10-26 04:51:51]

kochi@chromium.org changed reviewers:
+ ericwilligers@chromium.org

[kochi, 2016-10-26 04:52:12]

Thanks for the review!

https://codereview.chromium.org/2340263003/diff/360001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Fullscreen.cpp (right):

https://codereview.chromium.org/2340263003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:230: } else {
On 2016/10/26 03:15:56, hayato wrote:
> else if

Done.

https://codereview.chromium.org/2340263003/diff/360001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Fullscreen.cpp:231: if
(!toShadowRoot(scope).isV1())
On 2016/10/26 03:15:56, hayato wrote:
> Could we avoid toShadowRoot(treeScope)?
> I am not comfortable with this cast. I think toShadowRoot(scope.rootNode())
> would be consistent with other files.

Done.

[kochi, 2016-10-26 04:53:13]

The CQ bit was checked by kochi@chromium.org

[kochi, 2016-10-26 04:53:14]

The patchset sent to the CQ was uploaded after l-g-t-m from foolip@chromium.org,
hayato@chromium.org
Link to the patchset: https://codereview.chromium.org/2340263003/#ps400001
(title: "Fix for hayato's comments.")

[commit-bot: I haz the power, 2016-10-26 04:53:35]

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-10-26 05:02:32]

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

[commit-bot: I haz the power, 2016-10-26 05:02:33]

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

[kochi, 2016-10-26 05:05:30]

The CQ bit was checked by kochi@chromium.org

[kochi, 2016-10-26 05:05:30]

The patchset sent to the CQ was uploaded after l-g-t-m from hayato@chromium.org,
foolip@chromium.org
Link to the patchset: https://codereview.chromium.org/2340263003/#ps420001
(title: "Style nit")

[commit-bot: I haz the power, 2016-10-26 05:05:49]

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-10-26 06:33:29]

Description was changed from

==========
Move Document.fullscreenElement to DocumentOrShadowRoot

This change aims to prevent nodes in shadow tree from
leaking via global API (document.fullscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

BUG=622585
TBR=ericwilligers@chromium.org for tools/metrics/histograms/histograms.xml
==========

to

==========
Move Document.fullscreenElement to DocumentOrShadowRoot

This change aims to prevent nodes in shadow tree from
leaking via global API (document.fullscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

BUG=622585
TBR=ericwilligers@chromium.org for tools/metrics/histograms/histograms.xml
==========

[commit-bot: I haz the power, 2016-10-26 06:33:30]

Committed patchset #19 (id:420001)

[commit-bot: I haz the power, 2016-10-26 06:35:22]

Description was changed from

==========
Move Document.fullscreenElement to DocumentOrShadowRoot

This change aims to prevent nodes in shadow tree from
leaking via global API (document.fullscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

BUG=622585
TBR=ericwilligers@chromium.org for tools/metrics/histograms/histograms.xml
==========

to

==========
Move Document.fullscreenElement to DocumentOrShadowRoot

This change aims to prevent nodes in shadow tree from
leaking via global API (document.fullscreenElement).

The change is discussed at:
https://github.com/w3c/webcomponents/issues/180

And spec is being updated at:
https://github.com/whatwg/fullscreen/pull/54

BUG=622585
TBR=ericwilligers@chromium.org for tools/metrics/histograms/histograms.xml

Committed: https://crrev.com/7025a706a1b4d382e5241905904ca803808000f9
Cr-Commit-Position: refs/heads/master@{#427610}
==========

[commit-bot: I haz the power, 2016-10-26 06:35:23]

Patchset 19 (id:??) landed as
https://crrev.com/7025a706a1b4d382e5241905904ca803808000f9
Cr-Commit-Position: refs/heads/master@{#427610}