Fix an issue that fullscreen layer is scrollable

Fix an issue that fullscreen layer is scrollable

When an element enters fullscreen, main frame's content size may
not be equal to that of the fullscreen renderer. As a result, the
fullscreen layer is scrollable. This might cause some unnecessary
computations and some wierd artifacts:

For example, on android phones, the edge glow effect may come after
scrolling the fullscreen video page several times, rather than coming
immediately.

This CL sets the scroll clip layer to NULL, so we won't be able to scroll it.

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

[qinmin, 2014-03-14 22:39:45]

PTAL

[esprehn, 2014-03-14 23:40:48]

LGTM to me, but morrita should probably look at it too. Feels a bit hacky, but
I'm not sure of an alternative.

[Hajime Morrita, 2014-03-15 00:28:29]

On 2014/03/14 23:40:48, esprehn wrote:
> LGTM to me, but morrita should probably look at it too. Feels a bit hacky, but
> I'm not sure of an alternative.

I have no idea in this area, but seems like it worth covered by a test so that
we don't break it when better-looking idea arrives.

[esprehn, 2014-03-15 01:16:14]

Yeah can you add a test before landing?

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[qinmin, 2014-03-15 01:59:56]

Sure, will do

On 2014/03/15 01:16:14, esprehn wrote:
> Yeah can you add a test before landing?
> 
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:blink-reviews+unsubscribe@chromium.org.

[qinmin, 2014-03-17 21:49:09]

added a WebFrameTest to test the patch.

[qinmin, 2014-03-18 14:57:07]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-03-18 14:57:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/200943002/20001

[commit-bot: I haz the power, 2014-03-18 14:57:18]

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

[commit-bot: I haz the power, 2014-03-18 14:57:21]

Failed to apply patch for Source/core/page/scrolling/ScrollingCoordinator.cpp:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file Source/core/page/scrolling/ScrollingCoordinator.cpp
  Hunk #3 FAILED at 173.
  1 out of 3 hunks FAILED -- saving rejects to file
Source/core/page/scrolling/ScrollingCoordinator.cpp.rej

Patch:       Source/core/page/scrolling/ScrollingCoordinator.cpp
Index: Source/core/page/scrolling/ScrollingCoordinator.cpp
diff --git a/Source/core/page/scrolling/ScrollingCoordinator.cpp
b/Source/core/page/scrolling/ScrollingCoordinator.cpp
index
635f6aff9230355aa6a8985aeba565c893d22759..eed67f7d35d6052bdf3c910cc34eeae89ace3905
100644
--- a/Source/core/page/scrolling/ScrollingCoordinator.cpp
+++ b/Source/core/page/scrolling/ScrollingCoordinator.cpp
@@ -29,13 +29,19 @@
 
 #include "RuntimeEnabledFeatures.h"
 #include "core/dom/Document.h"
+#include "core/dom/FullscreenElementStack.h"
 #include "core/dom/Node.h"
 #include "core/dom/WheelController.h"
 #include "core/html/HTMLElement.h"
 #include "core/frame/FrameView.h"
 #include "core/frame/LocalFrame.h"
-#include "core/page/Page.h"
 #include "core/frame/Settings.h"
+#include "core/page/Page.h"
+#include "core/plugins/PluginView.h"
+#include "core/rendering/RenderGeometryMap.h"
+#include "core/rendering/RenderView.h"
+#include "core/rendering/compositing/CompositedLayerMapping.h"
+#include "core/rendering/compositing/RenderLayerCompositor.h"
 #include "platform/TraceEvent.h"
 #include "platform/exported/WebScrollbarImpl.h"
 #include "platform/exported/WebScrollbarThemeGeometryNative.h"
@@ -47,11 +53,6 @@
 #endif
 #include "platform/scroll/ScrollAnimator.h"
 #include "platform/scroll/ScrollbarTheme.h"
-#include "core/plugins/PluginView.h"
-#include "core/rendering/RenderGeometryMap.h"
-#include "core/rendering/RenderView.h"
-#include "core/rendering/compositing/CompositedLayerMapping.h"
-#include "core/rendering/compositing/RenderLayerCompositor.h"
 #include "public/platform/Platform.h"
 #include "public/platform/WebCompositorSupport.h"
 #include "public/platform/WebLayerPositionConstraint.h"
@@ -172,8 +173,16 @@ void ScrollingCoordinator::updateAfterCompositingChange()
 
     // The mainFrame view doesn't get included in the FrameTree below, so we
     // update its size separately.
-    if (WebLayer* scrollingWebLayer = frameView ?
scrollingWebLayerForScrollableArea(frameView) : 0)
-        scrollingWebLayer->setBounds(frameView->contentsSize());
+    if (WebLayer* scrollingWebLayer = frameView ?
scrollingWebLayerForScrollableArea(frameView) : 0) {
+        // If there is a fullscreen element, the content size should be set to
the size of that element.
+        Element* fullscreenElement =
FullscreenElementStack::fullscreenElementFrom(*(m_page->mainFrame()->document()));
+        if (fullscreenElement && fullscreenElement->renderer()) {
+            RenderBox* renderBox =
fullscreenElement->renderer()->enclosingBox();
+           
scrollingWebLayer->setBounds(blink::WebSize(renderBox->width().toInt(),
renderBox->height().toInt()));
+        } else {
+            scrollingWebLayer->setBounds(frameView->contentsSize());
+        }
+    }
 
     const FrameTree& tree = m_page->mainFrame()->tree();
     for (const LocalFrame* child = tree.firstChild(); child; child =
child->tree().nextSibling()) {

[qinmin, 2014-03-18 16:41:17]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-03-18 16:41:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/200943002/40001

[commit-bot: I haz the power, 2014-03-18 16:42:49]

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

[commit-bot: I haz the power, 2014-03-18 16:42:51]

Try jobs failed on following builders:
  tryserver.blink on mac_blink_compile_dbg

[qinmin, 2014-03-18 16:52:16]

+aelias for Source/web

[aelias_OOO_until_Jul13, 2014-03-18 19:22:10]

I don't think it's a good idea to hack the size either, and I don't like how the
test locks in the size hack instead of checking for the intended behavior
(nonscrollability).

Instead of this approach, how about just calling
scrollingWebLayerForScrollableArea(scrollableArea)->setUserScrollable(false,
false); ?

[qinmin, 2014-03-18 19:50:37]

On 2014/03/18 19:22:10, aelias wrote:
> I don't think it's a good idea to hack the size either, and I don't like how
the
> test locks in the size hack instead of checking for the intended behavior
> (nonscrollability).
> 
> Instead of this approach, how about just calling
> scrollingWebLayerForScrollableArea(scrollableArea)->setUserScrollable(false,
> false); ?

setUserScrollable() doesn't work. CC calculates the maxScrollOffset() from the
layer size, and it uses that to determine if a layer is scrollable or not.

[aelias_OOO_until_Jul13, 2014-03-18 20:32:41]

OK.  How about SetScrollClipLayerId(-1)?  Since there is the line:

bool scrollable() const { return scroll_clip_layer_id_ != INVALID_ID; }

[bokan, 2014-03-18 21:11:14]

On 2014/03/18 19:50:37, qinmin wrote:
> On 2014/03/18 19:22:10, aelias wrote:
> > I don't think it's a good idea to hack the size either, and I don't like how
> the
> > test locks in the size hack instead of checking for the intended behavior
> > (nonscrollability).
> > 
> > Instead of this approach, how about just calling
> > scrollingWebLayerForScrollableArea(scrollableArea)->setUserScrollable(false,
> > false); ?
> 
> setUserScrollable() doesn't work. CC calculates the maxScrollOffset() from the
> layer size, and it uses that to determine if a layer is scrollable or not.

It doesn't do anything as of r239675 as it blocked pinched in scrolling when
overflow: hidden was set. IMO, this is clearer but in the meantime aelias@'s
comment below should do the trick, perhaps leave a TODO?

[qinmin, 2014-03-18 23:02:39]

PTAL again. 
Changed this to use setScrollClipLayer(0) to do the trick.

[qinmin, 2014-03-18 23:03:24]

On 2014/03/18 23:02:39, qinmin wrote:
> PTAL again. 
> Changed this to use setScrollClipLayer(0) to do the trick.

pending chromium side change https://codereview.chromium.org/196573035/

[aelias_OOO_until_Jul13, 2014-03-19 01:02:20]

lgtm modulo nits

https://codereview.chromium.org/200943002/diff/60001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/200943002/diff/60001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:167: // TODO(qinmin): find a
better way to handle the fullscreen case.
nit: please remove this TODO, it doesn't provide useful information and this
approach may be OK for a while.

https://codereview.chromium.org/200943002/diff/60001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:170:
scrollingWebLayer->setScrollClipLayer(0);
Blink style is to use "nullptr".

[qinmin, 2014-03-19 02:26:38]

https://codereview.chromium.org/200943002/diff/60001/Source/core/page/scrolli...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/200943002/diff/60001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:167: // TODO(qinmin): find a
better way to handle the fullscreen case.
On 2014/03/19 01:02:21, aelias wrote:
> nit: please remove this TODO, it doesn't provide useful information and this
> approach may be OK for a while.

Done.

https://codereview.chromium.org/200943002/diff/60001/Source/core/page/scrolli...
Source/core/page/scrolling/ScrollingCoordinator.cpp:170:
scrollingWebLayer->setScrollClipLayer(0);
On 2014/03/19 01:02:21, aelias wrote:
> Blink style is to use "nullptr".

I think nullptr was used for smart pointers, not for dumb pointers like here
unless we change the WebLayer interface.

[aelias_OOO_until_Jul13, 2014-03-19 02:31:11]

On 2014/03/19 02:26:38, qinmin wrote:
> On 2014/03/19 01:02:21, aelias wrote:
> > Blink style is to use "nullptr".
> 
> I think nullptr was used for smart pointers, not for dumb pointers like here
> unless we change the WebLayer interface.

Yeah, I noticed that after I said that.  Fine to leave it as 0.

[cimamoglu (inactive), 2014-03-19 14:23:06]

https://chromiumcodereview.appspot.com/200943002/diff/80001/Source/web/tests/...
File Source/web/tests/data/fullscreen_div.html (right):

https://chromiumcodereview.appspot.com/200943002/diff/80001/Source/web/tests/...
Source/web/tests/data/fullscreen_div.html:14: <body">
remove the quote symbol "

[qinmin, 2014-03-19 16:44:57]

https://codereview.chromium.org/200943002/diff/80001/Source/web/tests/data/fu...
File Source/web/tests/data/fullscreen_div.html (right):

https://codereview.chromium.org/200943002/diff/80001/Source/web/tests/data/fu...
Source/web/tests/data/fullscreen_div.html:14: <body">
On 2014/03/19 14:23:07, cimamoglu wrote:
> remove the quote symbol "

Done.

[qinmin, 2014-03-19 16:45:09]

The CQ bit was checked by qinmin@chromium.org

[commit-bot: I haz the power, 2014-03-19 16:45:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/qinmin@chromium.org/200943002/100001

[commit-bot: I haz the power, 2014-03-19 17:58:41]

Change committed as 169566