Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
911083002:
Carry out a resize even if no layout has been performed. (Closed)
DescriptionCarry out a resize even if no layout has been performed.
Add a FrameView resize in WebviewImpl::performResize if no layout has been
performed. This to fix the case where a layout is not performed because the
layout height is forced to zero (and therefore no layouts are performed when
the actual height is changed).
BUG=457159
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=190424
Patch Set 1 #
Total comments: 2
Patch Set 2 : Different approach #Patch Set 3 : Made needsLayout() true when forcing zero-height and added unit test #
Total comments: 11
Patch Set 4 : Reverting to old to check trybots #Patch Set 5 : Removed new unit test to be able to run trybots on old code #Patch Set 6 : Readded the forceZeroLayoutHeight changes and use a new setting instead of ViewportDescriptionChang… #Patch Set 7 : Added null-check to unittest to avoid crashes #Patch Set 8 : Added comment to unit test #
Total comments: 1
Patch Set 9 : New approach: carry out frameview resize if no layout performed #Patch Set 10 : Removed some artifacts originating from earlier patch sets. #
Total comments: 4
Patch Set 11 : Minor changes in unit test #
Messages
Total messages: 30 (5 generated)
gsennton@chromium.org changed reviewers: + boliu@chromium.org
I added the change suggested by Bo, does this seem OK or will it break stuff?
On 2015/02/10 12:32:31, gsennton wrote: > I added the change suggested by Bo, does this seem OK or will it break stuff? Thanks! I put the change on Blink trybots.
boliu@chromium.org changed reviewers: + aelias@chromium.org
+aelias I have no power here
hush@chromium.org changed reviewers: + hush@chromium.org
https://codereview.chromium.org/911083002/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/911083002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:1825: if (view->needsLayout() || (page()->settings().forceZeroLayoutHeight() && pinchVirtualViewportEnabled())) I think the initial purpose for forceZeroLayoutHeight is to make needsLayout() false here... See mkosiba's initial code review here: https://codereview.chromium.org/453893002/diff/1/Source/web/tests/WebFrameTes...
So, I think it's OK to trigger a single unnecessary layout to get the FrameView size updated, but it's ugly to have this kind of "always layout even if needsLayout is false" policy. I'm thinking a clean way to get the behavior we want here would be to plumb the forceZeroLayoutHeight setting to WebCore settings and make it clobber the height of the return value of FrameView::layoutSize(), instead of affecting it at setting time. That way, FrameView::contentsResized() will get triggered when the height changes which should fix this bug. Please also write a new WebFrameTest for this. A nice way to write the test would be to actually send a tap event to a button and verify it got hit properly. https://codereview.chromium.org/911083002/diff/1/Source/web/WebViewImpl.cpp File Source/web/WebViewImpl.cpp (right): https://codereview.chromium.org/911083002/diff/1/Source/web/WebViewImpl.cpp#n... Source/web/WebViewImpl.cpp:1825: if (view->needsLayout() || (page()->settings().forceZeroLayoutHeight() && pinchVirtualViewportEnabled())) On 2015/02/10 at 18:21:04, hush wrote: > I think the initial purpose for forceZeroLayoutHeight is to make needsLayout() false here... > See mkosiba's initial code review here: > https://codereview.chromium.org/453893002/diff/1/Source/web/tests/WebFrameTes... No, the initial purpose is to avoid weird layout infinite loops between the Android View system and Blink, as I understand it. The absence of needsLayout was a minor side effect I think.
Here is the fix aelias suggested (if I understood it correctly). There is no new unit test yet and the potential fix breaks an earlier test.
Hmm, I guess this has the reverse problem of not causing a layout when the setting is activated. I'd suggest notifying FrameView when the settings changed, with a new method like FrameView::didChangeForceLayoutHeightMode that just calls contentsResized().
On 2015/02/11 23:56:07, aelias wrote: > Hmm, I guess this has the reverse problem of not causing a layout when the > setting is activated. I'd suggest notifying FrameView when the settings > changed, with a new method like FrameView::didChangeForceLayoutHeightMode that > just calls contentsResized(). (Argg, I told Gustav some plainly wrong information this morning in chat. Sorry!) Before this change, toggling forceZeroLayoutHeight did indeed set needsLayout. This call path goes from "invalidate=ViewportDescription" bit in Settings.in, which generates code calling Page::settingsChanged(ViewportDescriptionChange), which eventually calls WebViewImpl::updatePageDefinedViewportConstraints and WebViewImpl::updateMainFrameLayoutSize. This broke in PS2 because FrameView::setLayoutSizeInternal early outs at the m_layoutSize == size check. Expanding Alex's suggestion more concretely, you can add a new enum type to Page::settingsChanged, and then call FrameView::didChangeForceLayoutHeightMode from there. (Alex: sounds reasonable?)
I see. I don't think we need to add a new settings category, we could just make the existing ViewportDescriptionChange always do that.
Added a unit test for this issue (which breaks when the internal layout height is set to zero as opposed to only the return height value of layoutSize being zero). I did not add another setting in Page::settingsChanged, instead ViewportDescriptionChange is used (as suggested by aelias).
https://codereview.chromium.org/911083002/diff/40001/Source/core/frame/FrameV... File Source/core/frame/FrameView.h (right): https://codereview.chromium.org/911083002/diff/40001/Source/core/frame/FrameV... Source/core/frame/FrameView.h:135: void didChangeForceLayoutHeightMode(); Please rename this to didChangeViewportDescriptionSettings since it ended up tied to the more generic setting. https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/WebFram... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:1375: webViewHelper.webView()->resize(WebSize(viewportWidth, 0)); I don't think we should be setting 0 viewport height here, that doesn't match what happens in reality and the WebFrameTest.SetForceZeroLayoutHeight test above doesn't do it. https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:1385: RefPtrWillBeRawPtr<Node> hitNode = webViewHelper.webViewImpl()->mainFrameImpl()->frame()->eventHandler().hitTestResultAtPoint(hitPoint, HitTestRequest::ReadOnly | HitTestRequest::Active).innerNode(); Could you send down an input event via handleGestureEvent, make the button do something and verify that it happened instead? That will give this test better coverage. https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/data/bu... File Source/web/tests/data/button.html (right): https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/data/bu... Source/web/tests/data/button.html:4: <meta name='button' content='width=device-width'/> If this is supposed to be a viewport tag, the name has to be "viewport". As it is, this HTML is a desktop page.
https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/WebFram... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:1375: webViewHelper.webView()->resize(WebSize(viewportWidth, 0)); On 2015/02/12 19:49:17, aelias wrote: > I don't think we should be setting 0 viewport height here, that doesn't match > what happens in reality and the WebFrameTest.SetForceZeroLayoutHeight test above > doesn't do it. It does happen in reality. Some apps with wrap-contents webview will set the webview size to 0 before anything is loaded. It's probably something like: * setForceZeroLayoutHeight(true) * resize(viewportWidth, 0) * load content * layout * resize(viewportWidth, viewportHeight) * layout check input is not clipped If this is initialized sized to viewportWidth x viewportHeight, I don't think the test would fail without the fix.
https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/WebFram... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:1375: webViewHelper.webView()->resize(WebSize(viewportWidth, 0)); On 2015/02/12 20:04:43, boliu wrote: > On 2015/02/12 19:49:17, aelias wrote: > > I don't think we should be setting 0 viewport height here, that doesn't match > > what happens in reality and the WebFrameTest.SetForceZeroLayoutHeight test > above > > doesn't do it. > > It does happen in reality. OK then, fine to leave as is, thanks for the clarification.
https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/WebFram... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:1375: webViewHelper.webView()->resize(WebSize(viewportWidth, 0)); On 2015/02/12 20:08:37, aelias wrote: > On 2015/02/12 20:04:43, boliu wrote: > > On 2015/02/12 19:49:17, aelias wrote: > > > I don't think we should be setting 0 viewport height here, that doesn't > match > > > what happens in reality and the WebFrameTest.SetForceZeroLayoutHeight test > > above > > > doesn't do it. > > > > It does happen in reality. > > OK then, fine to leave as is, thanks for the clarification. Should add some comments explaining this. No one will remember these details in a week..
https://codereview.chromium.org/911083002/diff/40001/Source/core/frame/FrameV... File Source/core/frame/FrameView.h (right): https://codereview.chromium.org/911083002/diff/40001/Source/core/frame/FrameV... Source/core/frame/FrameView.h:135: void didChangeForceLayoutHeightMode(); On 2015/02/12 19:49:17, aelias wrote: > Please rename this to didChangeViewportDescriptionSettings since it ended up > tied to the more generic setting. This is no longer tied to the more general setting (since that produced a lot of flakiness) and the name is therefore unchanged. https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/WebFram... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:1375: webViewHelper.webView()->resize(WebSize(viewportWidth, 0)); On 2015/02/12 20:10:27, boliu wrote: > On 2015/02/12 20:08:37, aelias wrote: > > On 2015/02/12 20:04:43, boliu wrote: > > > On 2015/02/12 19:49:17, aelias wrote: > > > > I don't think we should be setting 0 viewport height here, that doesn't > > match > > > > what happens in reality and the WebFrameTest.SetForceZeroLayoutHeight test > > > above > > > > doesn't do it. > > > > > > It does happen in reality. > > > > OK then, fine to leave as is, thanks for the clarification. > > Should add some comments explaining this. No one will remember these details in > a week.. Done. https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/WebFram... Source/web/tests/WebFrameTest.cpp:1385: RefPtrWillBeRawPtr<Node> hitNode = webViewHelper.webViewImpl()->mainFrameImpl()->frame()->eventHandler().hitTestResultAtPoint(hitPoint, HitTestRequest::ReadOnly | HitTestRequest::Active).innerNode(); On 2015/02/12 19:49:17, aelias wrote: > Could you send down an input event via handleGestureEvent, make the button do > something and verify that it happened instead? That will give this test better > coverage. Done. https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/data/bu... File Source/web/tests/data/button.html (right): https://codereview.chromium.org/911083002/diff/40001/Source/web/tests/data/bu... Source/web/tests/data/button.html:4: <meta name='button' content='width=device-width'/> On 2015/02/12 19:49:17, aelias wrote: > If this is supposed to be a viewport tag, the name has to be "viewport". As it > is, this HTML is a desktop page. Done.
https://codereview.chromium.org/911083002/diff/140001/Source/core/page/Page.cpp File Source/core/page/Page.cpp (right): https://codereview.chromium.org/911083002/diff/140001/Source/core/page/Page.c... Source/core/page/Page.cpp:462: case SettingsDelegate::ForceZeroLayoutHeightChange: A new changeType was added here since we would like to call contentsResized() when the forceZeroLayoutHeightChange setting is updated. Since the previously used changeType ViewportDescriptionChange was used by other settings updating those settings would also caused a call to contentsResized(). This extra call seems to have been the source of the flakiness in the earlier patch sets.
On 2015/02/13 19:41:07, gsennton wrote: > https://codereview.chromium.org/911083002/diff/140001/Source/core/page/Page.cpp > File Source/core/page/Page.cpp (right): > > https://codereview.chromium.org/911083002/diff/140001/Source/core/page/Page.c... > Source/core/page/Page.cpp:462: case > SettingsDelegate::ForceZeroLayoutHeightChange: > A new changeType was added here since we would like to call contentsResized() > when the forceZeroLayoutHeightChange setting is updated. > Since the previously used changeType ViewportDescriptionChange was used by other > settings updating those settings would also caused a call to contentsResized(). > This extra call seems to have been the source of the flakiness in the earlier > patch sets. You can see linux_blink_rel on PS3 had hundreds of flaky tests (amusingly bot still passed): http://build.chromium.org/p/tryserver.blink/builders/linux_blink_rel/builds/4... My suspicion was those tests were toggling other invalidate=ViewportDescription settings, which triggered a layout earlier than the test had expected.
OK, it seems my "cleanup" suggestion sent us down a painful path here. I'm
thinking let's throw this approach out and start over from first principles :/.
The problem is that performResize doesn't resize because A) it needs the result
of a layout if the width changed, and B) it expects a layout will always happen
because of the resize. In this case we have a condition where a layout actually
will not happen, therefore it doesn't need the result of the layout either,
therefore we can just go ahead and perform the layout in performResize() as
before.
So how about splitting off the code that's currently in layoutUpdated() as:
void WebViewImpl::postLayoutResize() {
if (pinchVirtualViewportEnabled()) {
if (webframe == mainFrame()) {
view->resize(mainFrameSize());
} else {
view->resize(webframe->frameView()->layoutSize());
}
}
}
then in WebView::performResize add the lines:
if (!needsLayout())
postLayoutResize();
I think it will still cause the test expectation in
WebFrameTest::SetForceZeroLayoutHeightWorksWithWrapContentsMode to fail, but
that change doesn't seem harmful so you can just change the expectation.
On 2015/02/13 at 20:52:22, aelias wrote: > therefore we can just go ahead and perform the layout in performResize() as before. I meant "perform the FrameView resize", not "perform the layout" here, sorry.
lgtm modulo test nits https://codereview.chromium.org/911083002/diff/180001/Source/web/tests/WebFra... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/911083002/diff/180001/Source/web/tests/WebFra... Source/web/tests/WebFrameTest.cpp:1394: RefPtrWillBeRawPtr<Element> element = static_cast<PassRefPtrWillBeRawPtr<Element>>(webViewHelper.webView()->mainFrame()->document().getElementById(id)); Is this casting really necessary? I see other tests in this file just do "Element* element = document->getElementById()", for example WebFrameTest.RemoveSpellingMarkers. https://codereview.chromium.org/911083002/diff/180001/Source/web/tests/WebFra... Source/web/tests/WebFrameTest.cpp:1397: if (element) { Instead of doing this, write ASSERT_NE(nullptr, element); above. Unlike EXPECT, that will terminate the test if it fails.
New patchsets have been uploaded after l-g-t-m from aelias@chromium.org
https://codereview.chromium.org/911083002/diff/180001/Source/web/tests/WebFra... File Source/web/tests/WebFrameTest.cpp (right): https://codereview.chromium.org/911083002/diff/180001/Source/web/tests/WebFra... Source/web/tests/WebFrameTest.cpp:1394: RefPtrWillBeRawPtr<Element> element = static_cast<PassRefPtrWillBeRawPtr<Element>>(webViewHelper.webView()->mainFrame()->document().getElementById(id)); On 2015/02/17 20:27:14, aelias wrote: > Is this casting really necessary? I see other tests in this file just do > "Element* element = document->getElementById()", for example > WebFrameTest.RemoveSpellingMarkers. Done. https://codereview.chromium.org/911083002/diff/180001/Source/web/tests/WebFra... Source/web/tests/WebFrameTest.cpp:1397: if (element) { On 2015/02/17 20:27:14, aelias wrote: > Instead of doing this, write ASSERT_NE(nullptr, element); above. Unlike EXPECT, > that will terminate the test if it fails. Done.
The CQ bit was checked by gsennton@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/911083002/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001) as https://src.chromium.org/viewvc/blink?view=rev&revision=190424
Message was sent while issue was closed.
A revert of this CL (patchset #11 id:200001) has been created in https://codereview.chromium.org/945853005/ by marcheu@chromium.org. The reason for reverting is: Speculative revert for: https://code.google.com/p/chromium/issues/detail?id=460294. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
