Replace RenderFullScreen with top layer

Replace RenderFullScreen with top layer

This patch makes the Fullscreen API use the top layer, as per the latest
Fullscreen spec:
http://fullscreen.spec.whatwg.org/

Some more details:
- HTMLPlugInElement now retains its widget during reattach. Otherwise,
a video would need to be reloaded when it enters fullscreen.
- The placeholder renderer that was in RenderFullScreen is removed. The
spec doesn't mention such a mechanism and it's better to avoid the
complexity if possible. It was originally added to prevent a flash of a
collapsed page when exiting fullscreen:
https://bugs.webkit.org/show_bug.cgi?id=61897
It's possible we still need it. Either way, it has to be moved from
RenderFullScreen.

Intent to ship:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/yJUanvg7d40

BUG=240576, 246077
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=165710

[falken, 2014-01-21 11:39:15]

Hi all,

Could you take a look please?

This is a continuation of https://codereview.chromium.org/18896003/

[esprehn, 2014-01-21 18:59:05]

https://codereview.chromium.org/139743005/diff/50001/Source/core/css/fullscre...
File Source/core/css/fullscreen.css (right):

https://codereview.chromium.org/139743005/diff/50001/Source/core/css/fullscre...
Source/core/css/fullscreen.css:5: *|*:not(:root):-webkit-full-screen {
remove the *|*, it's not needed and just complicates this. Not sure how I feel
about the :not() in the UA sheet either, but this is fine I guess.

https://codereview.chromium.org/139743005/diff/50001/Source/core/css/fullscre...
Source/core/css/fullscreen.css:14: height: 100%;
Why are you stretching the element to 100% size? I don't think Firefox does
this, the authors is supposed to do it.

https://codereview.chromium.org/139743005/diff/50001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/139743005/diff/50001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4799: m_activeModalDialog = 0;
Document::activeModalDialog() should just contain a loop, get rid of the field
and the updating below.

https://codereview.chromium.org/139743005/diff/50001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4814: return m_activeModalDialog;
This should contain the loop.

https://codereview.chromium.org/139743005/diff/50001/Source/core/dom/Fullscre...
File Source/core/dom/FullscreenElementStack.cpp (right):

https://codereview.chromium.org/139743005/diff/50001/Source/core/dom/Fullscre...
Source/core/dom/FullscreenElementStack.cpp:264: // layer.
file a bug and link here.

https://codereview.chromium.org/139743005/diff/50001/Source/core/dom/RenderTr...
File Source/core/dom/RenderTreeBuilder.cpp (right):

https://codereview.chromium.org/139743005/diff/50001/Source/core/dom/RenderTr...
Source/core/dom/RenderTreeBuilder.cpp:229: // Note: Adding newRenderer instead
of renderer(). renderer() may be a child of newRenderer.
This comment is also dead, the "child of" comment was about RenderFullScreen.

https://codereview.chromium.org/139743005/diff/50001/Source/core/html/HTMLDia...
File Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/139743005/diff/50001/Source/core/html/HTMLDia...
Source/core/html/HTMLDialogElement.cpp:119:
document().removeFromModalDialogStack(this);
Remove all this, I don't think you should need the special
removeFromModalDialogStack logic, just make the getter dynamic.

https://codereview.chromium.org/139743005/diff/50001/Source/core/html/HTMLDia...
File Source/core/html/HTMLDialogElement.h (right):

https://codereview.chromium.org/139743005/diff/50001/Source/core/html/HTMLDia...
Source/core/html/HTMLDialogElement.h:47: void setModal(bool modal) { m_modal =
modal; }
Hmm, this should be private? You can setModal(true) externally without the
dialog actually going modal.

[falken, 2014-01-22 06:08:01]

Thanks! PTAL

https://codereview.chromium.org/139743005/diff/50001/Source/core/css/fullscre...
File Source/core/css/fullscreen.css (right):

https://codereview.chromium.org/139743005/diff/50001/Source/core/css/fullscre...
Source/core/css/fullscreen.css:5: *|*:not(:root):-webkit-full-screen {
On 2014/01/21 18:59:05, esprehn wrote:
> remove the *|*, it's not needed and just complicates this. Not sure how I feel
> about the :not() in the UA sheet either, but this is fine I guess.

This was taken directly from the Fullscreen API spec. I guess we don't care
about namespaces though?

https://codereview.chromium.org/139743005/diff/50001/Source/core/css/fullscre...
Source/core/css/fullscreen.css:14: height: 100%;
On 2014/01/21 18:59:05, esprehn wrote:
> Why are you stretching the element to 100% size? I don't think Firefox does
> this, the authors is supposed to do it.

It's taken from the spec. IE 11 also follows the spec here. Firefox actually
forces width/height 100% without allowing it to be overriden by custom styling.
(Some basic test cases are here: http://falken-testing.appspot.com/)

https://codereview.chromium.org/139743005/diff/50001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/139743005/diff/50001/Source/core/dom/Document...
Source/core/dom/Document.cpp:4799: m_activeModalDialog = 0;
On 2014/01/21 18:59:05, esprehn wrote:
> Document::activeModalDialog() should just contain a loop, get rid of the field
> and the updating below.

Done.

https://codereview.chromium.org/139743005/diff/50001/Source/core/dom/Fullscre...
File Source/core/dom/FullscreenElementStack.cpp (right):

https://codereview.chromium.org/139743005/diff/50001/Source/core/dom/Fullscre...
Source/core/dom/FullscreenElementStack.cpp:264: // layer.
On 2014/01/21 18:59:05, esprehn wrote:
> file a bug and link here.

Done.

https://codereview.chromium.org/139743005/diff/50001/Source/core/dom/RenderTr...
File Source/core/dom/RenderTreeBuilder.cpp (right):

https://codereview.chromium.org/139743005/diff/50001/Source/core/dom/RenderTr...
Source/core/dom/RenderTreeBuilder.cpp:229: // Note: Adding newRenderer instead
of renderer(). renderer() may be a child of newRenderer.
On 2014/01/21 18:59:05, esprehn wrote:
> This comment is also dead, the "child of" comment was about RenderFullScreen.

Ah, I see. Done.

https://codereview.chromium.org/139743005/diff/50001/Source/core/html/HTMLDia...
File Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/139743005/diff/50001/Source/core/html/HTMLDia...
Source/core/html/HTMLDialogElement.cpp:119:
document().removeFromModalDialogStack(this);
On 2014/01/21 18:59:05, esprehn wrote:
> Remove all this, I don't think you should need the special
> removeFromModalDialogStack logic, just make the getter dynamic.

OK but we still need removedFrom to reset m_modal. Otherwise, if you remove a
modal dialog, then add it back and make it fullscreen, it will be treated as a
modal. The spec says when a dialog is removed, it loses modality.

https://codereview.chromium.org/139743005/diff/50001/Source/core/html/HTMLDia...
File Source/core/html/HTMLDialogElement.h (right):

https://codereview.chromium.org/139743005/diff/50001/Source/core/html/HTMLDia...
Source/core/html/HTMLDialogElement.h:47: void setModal(bool modal) { m_modal =
modal; }
On 2014/01/21 18:59:05, esprehn wrote:
> Hmm, this should be private? You can setModal(true) externally without the
> dialog actually going modal.

Yeah, without the Document::removeFromModalDialogStack stuff we don't need it
anymore.

[esprehn, 2014-01-22 07:32:18]

lgtm, wjmaclean@ this adds a hack that hopefully you'll be removing.

[esprehn, 2014-01-22 07:33:58]

btw I think you're failing real tests: 

unexpected_failures:
fast/dom/HTMLObjectElement/update-data.html
fullscreen/full-screen-is-in-top-layer.html
fast/dom/HTMLObjectElement/beforeload-set-text-crash.xhtml
fast/frames/crash-remove-iframe-during-object-beforeload.html

[falken, 2014-01-23 02:23:27]

On 2014/01/22 07:33:58, esprehn wrote:
> btw I think you're failing real tests: 
> 
> unexpected_failures:
> fast/dom/HTMLObjectElement/update-data.html
> fullscreen/full-screen-is-in-top-layer.html
> fast/dom/HTMLObjectElement/beforeload-set-text-crash.xhtml
> fast/frames/crash-remove-iframe-during-object-beforeload.html


Yeah, it looks like the protect widget hack is too aggressive. If <object>
element's data or type attribute dynamically changes, it causes a reattach and
the plugin must be reloaded, but the hack is preventing this. I think I must
change the hack to retain the widget only if these attributes haven't changed.
Other ideas welcome.

[esprehn, 2014-01-23 02:29:12]

On 2014/01/23 02:23:27, falken wrote:
> On 2014/01/22 07:33:58, esprehn wrote:
> > btw I think you're failing real tests: 
> > 
> > unexpected_failures:
> > fast/dom/HTMLObjectElement/update-data.html
> > fullscreen/full-screen-is-in-top-layer.html
> > fast/dom/HTMLObjectElement/beforeload-set-text-crash.xhtml
> > fast/frames/crash-remove-iframe-during-object-beforeload.html
> 
> 
> Yeah, it looks like the protect widget hack is too aggressive. If <object>
> element's data or type attribute dynamically changes, it causes a reattach and
> the plugin must be reloaded, but the hack is preventing this. I think I must
> change the hack to retain the widget only if these attributes haven't changed.
> Other ideas welcome.

You probably want to look at the needsWidgetUpdate() bit, I don't think you want
to look at individual attributes and such.

[falken, 2014-01-23 09:40:19]

On 2014/01/23 02:29:12, esprehn wrote:
> You probably want to look at the needsWidgetUpdate() bit, I don't think you
want
> to look at individual attributes and such.

I've updated the patch, PTAL.

The changes are:
- HTMLPlugInElement retains the widget only if the reattach alone would have
caused the plugin to be reloaded.
- Object elements weren't getting ::backdrops, because
RenderEmbeddedObject::canHaveChildren() returns false. I changed to
Element::needsPseudoElement to allow backdrop in that case and added a test.

[esprehn, 2014-01-23 18:44:57]

Allowing generated children, even backdrop, for a renderer that doesn't expect
it usually leads to security bugs and crashes since now the renderer has
children it didn't expect (and usually ones that return false are doing sketchy
things)... I'll have to think about that change.

https://codereview.chromium.org/139743005/diff/610001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/139743005/diff/610001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2796: if (pseudoId != BACKDROP &&
!renderer()->canHaveGeneratedChildren())
This is not safe, renderers that return false from canHaveGeneratedChildren() do
so because having them would lead to crashes or security bugs.

https://codereview.chromium.org/139743005/diff/610001/Source/core/html/HTMLDi...
File Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/139743005/diff/610001/Source/core/html/HTMLDi...
Source/core/html/HTMLDialogElement.cpp:118: m_modal = false;
What calls inertSubtreesChanged(document()) when you're removed? It seems like
closeDialog() should be invoked when a <dialog> is removed which would do this
for you.

[esprehn, 2014-01-23 19:01:01]

On 2014/01/23 18:44:57, esprehn wrote:
> Allowing generated children, even backdrop, for a renderer that doesn't expect
> it usually leads to security bugs and crashes since now the renderer has
> children it didn't expect (and usually ones that return false are doing
sketchy
> things)... I'll have to think about that change.
> 

I see, BACKDROP is always a child of the RenderView, since it's always in the
top layer, and RenderView allows generated children so this is fine. You can
ignore that comment, the != BACKDROP check is fine, but you might add a comment
about why it's safe.

[falken, 2014-01-24 01:50:44]

Thanks! I addressed the comments below and added the required NeedsRebaseline
lines to TestExpectations. I think it's ready to land.

https://codereview.chromium.org/139743005/diff/610001/Source/core/dom/Element...
File Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/139743005/diff/610001/Source/core/dom/Element...
Source/core/dom/Element.cpp:2796: if (pseudoId != BACKDROP &&
!renderer()->canHaveGeneratedChildren())
On 2014/01/23 18:44:58, esprehn wrote:
> This is not safe, renderers that return false from canHaveGeneratedChildren()
do
> so because having them would lead to crashes or security bugs.

Added a comment.

https://codereview.chromium.org/139743005/diff/610001/Source/core/html/HTMLDi...
File Source/core/html/HTMLDialogElement.cpp (right):

https://codereview.chromium.org/139743005/diff/610001/Source/core/html/HTMLDi...
Source/core/html/HTMLDialogElement.cpp:118: m_modal = false;
On 2014/01/23 18:44:58, esprehn wrote:
> What calls inertSubtreesChanged(document()) when you're removed? It seems like
> closeDialog() should be invoked when a <dialog> is removed which would do this
> for you.

Oh great point. This is an existing bug, we won't update inert subtrees in the
current code either. I added a FIXME and will fix in a next patch. We can't
just call closeDialog since we shouldn't reset open or fire a close event.

[commit-bot: I haz the power, 2014-01-24 04:39:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/falken@chromium.org/139743005/1150001

[commit-bot: I haz the power, 2014-01-24 07:23:20]

Change committed as 165710

[Alpha Left Google, 2014-01-25 00:18:06]

A revert of this CL has been created in
https://codereview.chromium.org/137233017/ by hclam@chromium.org.

The reason for reverting is: This patch seems to be causing crashs on windows 7
debug build.
http://build.chromium.org/p/chromium.webkit/builders/Win7%20%28dbg%29/builds/....