Reland r165710 "Replace RenderFullScreen with top layer"

Reland r165710 "Replace RenderFullScreen with top layer"

It broke browser_test OutOfProcessPPAPITest.Fullscreen by tripping an
assert reached by calling pluginWidget in HTMLPluginElement::detach
which causes a layout. The solution is to get the existing widget
without causing a layout.

Original code review: https://codereview.chromium.org/139743005/

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

[falken, 2014-01-27 11:52:05]

Elliott, could you take a look again?

Patchset 1 is the commit that was reverted (synced to head).

The Windows failure was caused by the stack trace below. pluginWidget()
always causes a layout, which is probably not what we want to do in detach.

ASSERTION FAILED: !inStyleRecalc()
Backtrace:
Still waiting for the following processes to finish:
        "out\Debug\browser_tests.exe" --allow-file-access --gtest_also_run_disab
led_tests --gtest_filter=OutOfProcessPPAPITest.Fullscreen --no-sandbox --rendere
r-startup-dialogg --single_process --user-data-dir="D:\src\tmp\scoped_dir7944_20
219\d7944_4664"
        WebCore::Document::incDOMTreeVersion [0x17231A74+68] (d:\src\chrome\src\
third_party\webkit\source\core\dom\document.h:807)
        WebCore::Element::attributeChanged [0x172E5D13+163] (d:\src\chrome\src\t
hird_party\webkit\source\core\dom\element.cpp:978)
        WebCore::Element::didModifyAttribute [0x172F633B+43] (d:\src\chrome\src\
third_party\webkit\source\core\dom\element.cpp:3155)
        WebCore::Element::setAttributeInternal [0x172E5B11+513] (d:\src\chrome\s
rc\third_party\webkit\source\core\dom\element.cpp:948)
        WebCore::Element::setAttribute [0x172E5638+872] (d:\src\chrome\src\third
_party\webkit\source\core\dom\element.cpp:902)
        blink::WebElement::setAttribute [0x15C0AC25+165] (d:\src\chrome\src\thir
d_party\webkit\source\web\webelement.cpp:105)
        content::PepperPluginInstanceImpl::ResetSizeAttributesAfterFullscreen [0
x12D012A0+224] (d:\src\chrome\src\content\renderer\pepper\pepper_plugin_instance
_impl.cc:3039)
        content::PepperPluginInstanceImpl::ViewChanged [0x12CF16DC+732] (d:\src\
chrome\src\content\renderer\pepper\pepper_plugin_instance_impl.cc:1130)
        content::PepperWebPluginImpl::updateGeometry [0x12D33741+305] (d:\src\ch
rome\src\content\renderer\pepper\pepper_webplugin_impl.cc:177)
        blink::WebPluginContainerImpl::reportGeometry [0x15C7CC2B+283] (d:\src\c
hrome\src\third_party\webkit\source\web\webplugincontainerimpl.cpp:407)
        blink::WebPluginContainerImpl::widgetPositionsUpdated [0x15C7BCE4+52] (d
:\src\chrome\src\third_party\webkit\source\web\webplugincontainerimpl.cpp:216)
        WebCore::RenderWidget::widgetPositionsUpdated [0x1823139F+79] (d:\src\ch
rome\src\third_party\webkit\source\core\rendering\renderwidget.cpp:345)
        WebCore::FrameView::updateWidgetPositions [0x17BF5596+246] (d:\src\chrom
e\src\third_party\webkit\source\core\frame\frameview.cpp:1196)
        WebCore::FrameView::performPostLayoutTasks [0x17BFA6C1+641] (d:\src\chro
me\src\third_party\webkit\source\core\frame\frameview.cpp:2050)
        WebCore::FrameView::scheduleOrPerformPostLayoutTasks [0x17BF2BAE+158] (d
:\src\chrome\src\third_party\webkit\source\core\frame\frameview.cpp:873)
        WebCore::FrameView::layout [0x17BF3967+3223] (d:\src\chrome\src\third_pa
rty\webkit\source\core\frame\frameview.cpp:1066)
        WebCore::FrameView::scrollbarExistenceDidChange [0x17BF8D43+131] (d:\src
\chrome\src\third_party\webkit\source\core\frame\frameview.cpp:1748)
        WebCore::ScrollView::updateScrollbars [0x1DA0258F+95] (d:\src\chrome\src
\third_party\webkit\source\platform\scroll\scrollview.cpp:344)
        WebCore::ScrollView::setScrollbarModes [0x1DA00FA3+179] (d:\src\chrome\s
rc\third_party\webkit\source\platform\scroll\scrollview.cpp:133)
        WebCore::FrameView::layout [0x17BF3513+2115] (d:\src\chrome\src\third_pa
rty\webkit\source\core\frame\frameview.cpp:999)
        WebCore::Document::updateLayout [0x1727328E+382] (d:\src\chrome\src\thir
d_party\webkit\source\core\dom\document.cpp:1811)
        WebCore::Document::updateLayoutIgnorePendingStylesheets [0x17273BDA+106]
 (d:\src\chrome\src\third_party\webkit\source\core\dom\document.cpp:1863)
        WebCore::HTMLPlugInElement::renderWidgetForJSBindings [0x176040D0+32] (d
:\src\chrome\src\third_party\webkit\source\core\html\htmlpluginelement.cpp:331)
        WebCore::HTMLPlugInElement::pluginWidget [0x17603AFC+60] (d:\src\chrome\
src\third_party\webkit\source\core\html\htmlpluginelement.cpp:264)
        WebCore::HTMLPlugInElement::detach [0x1760347B+107] (d:\src\chrome\src\t
hird_party\webkit\source\core\html\htmlpluginelement.cpp:161)
        WebCore::Node::reattach [0x1733B3D5+85] (d:\src\chrome\src\third_party\w
ebkit\source\core\dom\node.cpp:979)
        WebCore::Element::recalcOwnStyle [0x172EAE07+487] (d:\src\chrome\src\thi
rd_party\webkit\source\core\dom\element.cpp:1621)
        WebCore::Element::recalcStyle [0x172EAAAD+317] (d:\src\chrome\src\third_
party\webkit\source\core\dom\element.cpp:1588)
        WebCore::Element::recalcChildStyle [0x172EB470+592] (d:\src\chrome\src\t
hird_party\webkit\source\core\dom\element.cpp:1687)
        WebCore::Element::recalcStyle [0x172EAADF+367] (d:\src\chrome\src\third_
party\webkit\source\core\dom\element.cpp:1595)
        WebCore::Element::recalcChildStyle [0x172EB470+592] (d:\src\chrome\src\t
hird_party\webkit\source\core\dom\element.cpp:1687)
        WebCore::Element::recalcStyle [0x172EAADF+367] (d:\src\chrome\src\third_
party\webkit\source\core\dom\element.cpp:1595)
        WebCore::Element::recalcChildStyle [0x172EB470+592] (d:\src\chrome\src\t
hird_party\webkit\source\core\dom\element.cpp:1687)
        WebCore::Element::recalcStyle [0x172EAADF+367] (d:\src\chrome\src\third_
party\webkit\source\core\dom\element.cpp:1595)
        WebCore::Element::recalcChildStyle [0x172EB470+592] (d:\src\chrome\src\t
hird_party\webkit\source\core\dom\element.cpp:1687)
        WebCore::Element::recalcStyle [0x172EAADF+367] (d:\src\chrome\src\third_
party\webkit\source\core\dom\element.cpp:1595)
        WebCore::Document::recalcStyle [0x1727253C+956] (d:\src\chrome\src\third
_party\webkit\source\core\dom\document.cpp:1734)
        WebCore::Document::updateStyleIfNeeded [0x17272E3F+335] (d:\src\chrome\s
rc\third_party\webkit\source\core\dom\document.cpp:1769)
        WebCore::Document::styleRecalcTimerFired [0x17271456+22] (d:\src\chrome\
src\third_party\webkit\source\core\dom\document.cpp:1563)

[esprehn, 2014-01-28 03:57:45]

lgtm, I'm glad my new ASSERT caught this.

https://codereview.chromium.org/134753003/diff/30001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/134753003/diff/30001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:883: /* It's safe to have a backdrop
since its renderer is always a child of RenderView. */
C++ style comments.

// It's safe ...

We don't really use C /* comments.

[falken, 2014-01-28 04:18:09]

thanks

https://codereview.chromium.org/134753003/diff/30001/Source/core/css/resolver...
File Source/core/css/resolver/StyleResolver.cpp (right):

https://codereview.chromium.org/134753003/diff/30001/Source/core/css/resolver...
Source/core/css/resolver/StyleResolver.cpp:883: /* It's safe to have a backdrop
since its renderer is always a child of RenderView. */
On 2014/01/28 03:57:45, esprehn wrote:
> C++ style comments.
> 
> // It's safe ...
> 
> We don't really use C /* comments.

Done.

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

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

[commit-bot: I haz the power, 2014-01-28 07:53:10]

Change committed as 165904

[falken, 2014-01-28 11:15:57]

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

The reason for reverting is: Looks like it's causing failures on:
virtual/android/fullscreen/anonymous-block-merge-crash.html
fullscreen/anonymous-block-merge-crash.html
virtual/android/fullscreen/full-screen-iframe-legacy.html
plugin-document-back-forward.html

See:
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux/builds/26047
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Linux%20(dbg)/b...
http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Mac10.6%20%28db....

[esprehn, 2014-01-30 04:48:59]

Please remove the Crash status.

https://codereview.chromium.org/134753003/diff/150001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/134753003/diff/150001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:689: Bug(falken)
fullscreen/parent-flow-inline-with-block-child.html [ Pass ImageOnlyFailure
Crash NeedsManualRebaseline ]
Remove Crash, it definitely shouldn't crash.

[falken, 2014-01-30 05:00:20]

Thanks!

https://codereview.chromium.org/134753003/diff/150001/LayoutTests/TestExpecta...
File LayoutTests/TestExpectations (right):

https://codereview.chromium.org/134753003/diff/150001/LayoutTests/TestExpecta...
LayoutTests/TestExpectations:689: Bug(falken)
fullscreen/parent-flow-inline-with-block-child.html [ Pass ImageOnlyFailure
Crash NeedsManualRebaseline ]
On 2014/01/30 04:48:59, esprehn wrote:
> Remove Crash, it definitely shouldn't crash.

Did you see the comment above? This is the existing expectation with
NeedsManualRebaseline added. From bug crbug.com/331576, it looks like it's
flakily crashing due to a font cache issue. It might be my patch will fix it,
but I'm not sure, so I'll keep it for now. If it turns out the flaky crash
disappears after landing, I'll remove it during the manual rebaseline.

[esprehn, 2014-01-30 05:10:34]

On 2014/01/30 05:00:20, falken wrote:
> Thanks!
> 
>
https://codereview.chromium.org/134753003/diff/150001/LayoutTests/TestExpecta...
> File LayoutTests/TestExpectations (right):
> 
>
https://codereview.chromium.org/134753003/diff/150001/LayoutTests/TestExpecta...
> LayoutTests/TestExpectations:689: Bug(falken)
> fullscreen/parent-flow-inline-with-block-child.html [ Pass ImageOnlyFailure
> Crash NeedsManualRebaseline ]
> On 2014/01/30 04:48:59, esprehn wrote:
> > Remove Crash, it definitely shouldn't crash.
> 
> Did you see the comment above? This is the existing expectation with
> NeedsManualRebaseline added. From bug crbug.com/331576, it looks like it's
> flakily crashing due to a font cache issue. It might be my patch will fix it,
> but I'm not sure, so I'll keep it for now. If it turns out the flaky crash
> disappears after landing, I'll remove it during the manual rebaseline.

Yeah I missed the comment, disregard me. :)