Screenshots. One without calling setInitialScale and the other one with setInitialScale(160).

Deleted: Screenshot_20170110-130417.png 38.2 KB

	Screenshot_20170110-130417.png 38.2 KB View Download

Deleted: Screenshot_20170110-130401.png 132 KB

	Screenshot_20170110-130401.png 132 KB View Download

satyavathir@, the user reported in G+ that this issue only occurs in WebView beta, so we might have an M56 regression.  Can we work to repro and bisect please?  If this looks bad enough please add a stable blocking label and loop in the bug cop.

amineer@, we are working on this and will update the bisect range, if this issue repros.  

Issue reproducible on Nexus5/M4B30Z.

Bisect Range: https://chromium.googlesource.com/chromium/src/+log/56.0.2890.0..56.0.2891.0?pretty=fuller&n=10000

Logcat @ http://go/chrome-androidlogs1/6/679709

Bisected to CL: https://codereview.chromium.org/2376163002

Selim: who would know why WebView is reacting like this?

> Selim for visibility.

I wonder if visualViewport is wrong in WebView without setInitialScale? I'd recommend playing with the repro case (building a debug APK using WebView [1]) and then inspect it with chrome://inspect and see if you can find why WebView is behaving differently.

[1] https://developers.google.com/web/tools/chrome-devtools/remote-debugging/webviews

When I inspect it in the console on a Nexus 5, for that particular app showing a 520x256px image:

For portrait, the image dimensions are straight up wacky:
Height = 390146px
Width = 520px

Rotating to landscape makes the image show up as intended, but because the image is being shown in the main frame of the Android WebView it's shown with a black background:
Height = 256px
Width = 546px

Rotating back to portrait makes the height huge again:
Height = 390146px
Width = 520px

One more thing to note is that the height of the Android WebView is set to "wrap_content", so it's allowed to get as large as it wants, but I'm not sure why it doesn't expand like crazy in landscape, or why 390146 is what it ends up scaling up to.

Deleted: demo.mp4 4.4 MB

	demo.mp4 4.4 MB View Download

I guess the app is simply setting a giant webview?

Aelias is a good person to comment on these view things.

and sorry for missing the initial CC.

I haven't looked closely, but could we just disable the new behavior in WebView?  It makes sense that WebView would often be used to display single images on part of the screen and the centering behavior is not really appropriate for it.

I see ImageDocument::shouldShrinkToFit() already disables the behavior for subframes, and WebViews are quite similar to subframes.  An easy way to check for WebView that could be added there and be cherry-picked to release branch is the wideViewportQuirkEnabled setting (always true for WebView, always false elsewhere).

That'd be a change in behavior for all images displayed in WebViews, wouldn't it?  It'd mean that WebViews would no longer try to scale down anything to fit within the bounds of their containers and would just spill out.

Tima or Bo:

would it be possible for one of you guys to look at the proposed blink change and comment here. 

thanks

Yeah, that's true... perhaps then you could more specifically check WRAP_CONTENTS layout, which is expressed as ForceZeroLayoutHeight setting in Blink?

> Yeah, that's true... perhaps then you could more specifically check WRAP_CONTENTS layout, which is expressed as ForceZeroLayoutHeight setting in Blink?

I support that over investigating what's actually happening here.

So yeah, it's getting into the loop I was assuming:

 01-17 15:57:13.341 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 8, 44.875) 520, 256 
 01-17 15:57:13.457 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 256, 1.40234) 520, 370 
 01-17 15:57:13.497 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 370, 0.97027) 520, 535 
 01-17 15:57:13.602 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 535, 0.671028) 520, 774 
 01-17 15:57:13.656 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 774, 0.463824) 520, 1121 
 01-17 15:57:13.689 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 1121, 0.32025) 520, 1623 
 01-17 15:57:13.742 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 1623, 0.221195) 520, 2350 
 01-17 15:57:13.776 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 2350, 0.152766) 520, 3403 
 01-17 15:57:13.830 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 3403, 0.105495) 520, 4929 
 01-17 15:57:14.144 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 4929, 0.0728342) 520, 7139 
 01-17 15:57:14.335 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 7139, 0.0502872) 520, 10340 
 01-17 15:57:14.387 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 10340, 0.0347195) 520, 14977 
 01-17 15:57:14.415 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 14977, 0.0239701) 520, 21693 
 01-17 15:57:14.442 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 21693, 0.0165491) 520, 31421 
 01-17 15:57:14.483 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 31421, 0.0114255) 520, 45512 
 01-17 15:57:14.508 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 45512, 0.00788803) 520, 65922 
 01-17 15:57:14.550 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 65922, 0.00544583) 520, 95485 
 01-17 15:57:14.575 14964 15000 E chromium: [ERROR:ImageDocument.cpp(545)] dfalcantara -- windowSizeChanged: (359, 95485, 0.00375975) 520, 138306 

Blink gets a windowSizeChanged event causing the image document height to be recalculated, which results in the WebView changing its size, which results in Blink getting a windowSizeChanged event ad infinitum.

Investigating options.

Adding some context to #19:
The image is trying to be centered within the WebView, which requires setting a height of a div to match the height of the WebView exactly in ImageDocument.  This in turn causes WebView to resize itself, causing Blink to recalculate it and make the div big again.

This happened before. The solution was ForceZeroLayoutHeight, ie ignore the height from webview during blink layout. Does that not work anymore?

I'm not familiar with that; this is relatively new Blink code in ImageDocument.cpp::windowSizeChanged().  Where would that flag need to be set (if it isn't already) or checked?

It's set already as a result of WRAP_CONTENT in this example.  I suggest checking it in ImageDocument::shouldShrinkToFit() and returning false if it's set.

Err, not familiar with blink.. but I think you should not need to check it, and just use layout height instead of window height?

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/web/WebViewImpl.cpp?q=getForceZeroLayoutHeight+file:third_party/webkit&sq=package:chromium&l=3226&dr=C

Right, it might be cleaner alter ImageDocument::windowSizeChanged() to check FrameView layout width/height instead, and bail out if the height is zero (since the aspect ratio computation doesn't make sense in that case anyway anyway).  visual viewport size probably shouldn't be used for a layout-like operation like this anyway.

Are you proposing changing just the place in windowSizeChanged() that's causing the resize loop or are you proposing changing all of the calculations throughout the file to use the layout width & height?  Just changing it for windowSizeChanged() seems to revert to the older behavior for WebViews (I'm still testing), and might be a safer choice for an M56 cherry-pick.

I guess the correct set of fixes for WebViews is to:
1) Keep the shrink to fit behavior for WebViews
2) Don't display those images on black
3) Don't center them

Is there a specific flag I could check to make sure this affects only WebViews?  Would have thought it was covered by the main frame case.

Oh also
4) Use the layout size (which seems to match the viewport sizes in my manual testing just now)

> Is there a specific flag I could check to make sure this affects only WebViews?

wideViewportQuirkEnabled mentioned earlier?

Actually, it seems that the layout height and the visual viewport height can differ in Clank proper because of the toolbars.  We need to use the visual viewport height so that the image is properly centered on screen when the toolbar scrolls onto or off of the screen.

Checking the flag seems to work, btw.  I'm guessing we don't want to do any sort of shadowbox in the WebView case, so I'll try and guard off all the cases where this new behavior exists instead of switching to the layout height.

Uploaded a potential CL here:
https://codereview.chromium.org/2646663002/

Indentation changes make it look larger than it really is.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/684507d88c125e0ea9077787e94328ced875eae3

commit 684507d88c125e0ea9077787e94328ced875eae3
Author: dfalcantara <dfalcantara@chromium.org>
Date: Thu Jan 19 20:58:25 2017

[Blink] Don't center images in Android WebViews

Android WebViews currently get into a loop where Blink's
ImageDocument::windowSizeChanged() is called, causing
WebViews to be resized larger, causing Blink's
windowSizeChanged() to be called.

Instead of using a different height for the calculation,
it was decided on the bug that Android WebViews shouldn't
return true for shouldShrinkToFit(), treating them similarly
to non-main frames.

BUG= 679709 

Review-Url: https://codereview.chromium.org/2646663002
Cr-Commit-Position: refs/heads/master@{#444842}

[modify] https://crrev.com/684507d88c125e0ea9077787e94328ced875eae3/third_party/WebKit/Source/core/html/ImageDocument.cpp
[modify] https://crrev.com/684507d88c125e0ea9077787e94328ced875eae3/third_party/WebKit/Source/core/html/ImageDocument.h
[modify] https://crrev.com/684507d88c125e0ea9077787e94328ced875eae3/third_party/WebKit/Source/core/html/ImageDocumentTest.cpp

Merge-requesting because it's a P1 RBS.

Your change meets the bar and is auto-approved for M56. Please go ahead and merge the CL to branch 2924 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), gkihumba@(cros), bustamante@(desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d7252652724ed7adf1c560348596d826d34f1fe2

commit d7252652724ed7adf1c560348596d826d34f1fe2
Author: dfalcantara@chromium.org <dfalcantara@chromium.org>
Date: Thu Jan 19 21:45:09 2017

[Blink] Don't center images in Android WebViews

Android WebViews currently get into a loop where Blink's
ImageDocument::windowSizeChanged() is called, causing
WebViews to be resized larger, causing Blink's
windowSizeChanged() to be called.

Instead of using a different height for the calculation,
it was decided on the bug that Android WebViews shouldn't
return true for shouldShrinkToFit(), treating them similarly
to non-main frames.

BUG= 679709 
TBR=aelias,pdr

Original-Review-Url: https://codereview.chromium.org/2646663002
Original-Cr-Commit-Position: refs/heads/master@{#444842}
Review-Url: https://codereview.chromium.org/2647633004 .
Cr-Commit-Position: refs/branch-heads/2924@{#805}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/d7252652724ed7adf1c560348596d826d34f1fe2/third_party/WebKit/Source/core/html/ImageDocument.cpp
[modify] https://crrev.com/d7252652724ed7adf1c560348596d826d34f1fe2/third_party/WebKit/Source/core/html/ImageDocument.h
[modify] https://crrev.com/d7252652724ed7adf1c560348596d826d34f1fe2/third_party/WebKit/Source/core/html/ImageDocumentTest.cpp

Going to mark as fixed given this is landed on trunk & branch, please reopen if you have further work.

Thanks dfalcantara@!

Verified fix on LG G5 MRA58K version 56.0.2924.78.

Verified fix on Pixel XL N MR2 version 57.0.2987.9.