[Blink] Don't center images in Android WebViews

[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}
Committed: https://chromium.googlesource.com/chromium/src/+/684507d88c125e0ea9077787e94328ced875eae3

[gone, 2017-01-18 23:44:46]

Description was changed from

==========
[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 should be
effectively treated as if they were not main frames,
disabling the new centering on black treatment that
main frames get.

BUG=679709
==========

to

==========
[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 should be
effectively treated as if they were not main frames,
disabling the new centering on black treatment that
main frames get.

To do that, this undoes a bit of https://codereview.chromium.org/2445413003
and brings back code that works directly on the <img>
element instead of the <div>.

BUG=679709
==========

[gone, 2017-01-18 23:48:45]

dfalcantara@chromium.org changed reviewers:
+ aelias@chromium.org, boliu@chromium.org, pdr@chromium.org

[gone, 2017-01-18 23:48:46]

What do you all think of this?  It cordons off the new presentation codepath for
Android WebViews so that you should end up with the old behavior.

[gone, 2017-01-18 23:59:53]

The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 00:00:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2017-01-19 00:23:49]

https://codereview.chromium.org/2646663002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):

https://codereview.chromium.org/2646663002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageDocument.cpp:568:
m_imageElement->setInlineStyleProperty(
Could you try instead returning false from ImageDocument::shouldShrinkToFit() if
page()->settings().getForceZeroLayoutHeight() is true?  I believe that should
avoid the problem in the WRAP_CONTENT WebView case while still allowing a
scale-down behavior in the MATCH_PARENT WebView case, without needing to
introduce a WebView-specific scaling path like this one.

[gone, 2017-01-19 01:02:31]

Description was changed from

==========
[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 should be
effectively treated as if they were not main frames,
disabling the new centering on black treatment that
main frames get.

To do that, this undoes a bit of https://codereview.chromium.org/2445413003
and brings back code that works directly on the <img>
element instead of the <div>.

BUG=679709
==========

to

==========
[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 should be
effectively treated as if they were not main frames,
disabling the new centering on black treatment that
main frames get.

BUG=679709
==========

[gone, 2017-01-19 01:04:39]

Description was changed from

==========
[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 should be
effectively treated as if they were not main frames,
disabling the new centering on black treatment that
main frames get.

BUG=679709
==========

to

==========
[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
==========

[gone, 2017-01-19 01:05:55]

Well, that works better than expected.  I was worried that m_shouldShrinkToFit
would be problematic, but AFAICT it's only used on desktop anyway.  Running
through the tests now.

[gone, 2017-01-19 01:06:02]

The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run

[gone, 2017-01-19 01:06:29]

https://codereview.chromium.org/2646663002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):

https://codereview.chromium.org/2646663002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageDocument.cpp:599: page() ?
page()->settings().getForceZeroLayoutHeight() : false;
Is there a better name for this variable here, btw?

[commit-bot: I haz the power, 2017-01-19 01:06:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2017-01-19 01:11:08]

Looks good, thanks.  Could you add a test case to ImageDocumentTest.cpp that
sets this setting?

https://codereview.chromium.org/2646663002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):

https://codereview.chromium.org/2646663002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageDocument.cpp:599: page() ?
page()->settings().getForceZeroLayoutHeight() : false;
On 2017/01/19 at 01:06:28, dfalcantara (load balance plz) wrote:
> Is there a better name for this variable here, btw?

I'd suggest isWrapContentWebView.

[gone, 2017-01-19 02:09:30]

The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 02:09:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[aelias_OOO_until_Jul13, 2017-01-19 03:05:58]

Hmm, since this is for M56 cherry-pick and time is running out, lgtm without a
test.  Still needs OWNERS approval from pdr@ though.

[pdr., 2017-01-19 03:17:32]

https://codereview.chromium.org/2646663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):

https://codereview.chromium.org/2646663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageDocument.cpp:599: page() ?
page()->settings().getForceZeroLayoutHeight() : false;
Is getForceZeroLayoutHeight causing the height to be zero which causes the
infinite loop of window size changes? Or is getForceZeroLayoutHeight only used
as a "isWebview" check?

[commit-bot: I haz the power, 2017-01-19 03:34:52]

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

[commit-bot: I haz the power, 2017-01-19 03:34:53]

Dry run: This issue passed the CQ dry run.

[aelias_OOO_until_Jul13, 2017-01-19 03:53:07]

https://codereview.chromium.org/2646663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):

https://codereview.chromium.org/2646663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageDocument.cpp:599: page() ?
page()->settings().getForceZeroLayoutHeight() : false;
On 2017/01/19 at 03:17:32, pdr. wrote:
> Is getForceZeroLayoutHeight causing the height to be zero which causes the
infinite loop of window size changes? Or is getForceZeroLayoutHeight only used
as a "isWebview" check?

It's used as isWebView (_in_WRAP_CONTENT_mode).  The reason in the first place
for the settings is because a lot of the rest of Blink layout is prone to the
same infinite loop of window size changes, and forcing the layout height to zero
proves a nice universal mitigation.  The code in this file uses visible height
instead of layout height so it needs a more explicit check.

[aelias_OOO_until_Jul13, 2017-01-19 04:08:22]

https://codereview.chromium.org/2646663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageDocumentTest.cpp (right):

https://codereview.chromium.org/2646663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageDocumentTest.cpp:188: if
(document().shouldShrinkToFit()) {
This test can never go red, even without your change.  It's not an integration
test, so there's no concept of running in a different environment with a
different setting.  Rather, the test itself determines its settings and it's
uniform across all platforms.  So please set the forceZeroLayoutHeight setting
during test initialization instead of having an if statement (or simply remove
the test if you don't have time, I'd rather have no test than a fake test).

[pdr., 2017-01-19 05:04:27]

LGTM % a comment and aelias's point about tests.

https://codereview.chromium.org/2646663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageDocument.cpp (right):

https://codereview.chromium.org/2646663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageDocument.cpp:599: page() ?
page()->settings().getForceZeroLayoutHeight() : false;
On 2017/01/19 at 03:53:07, aelias wrote:
> On 2017/01/19 at 03:17:32, pdr. wrote:
> > Is getForceZeroLayoutHeight causing the height to be zero which causes the
infinite loop of window size changes? Or is getForceZeroLayoutHeight only used
as a "isWebview" check?
> 
> It's used as isWebView (_in_WRAP_CONTENT_mode).  The reason in the first place
for the settings is because a lot of the rest of Blink layout is prone to the
same infinite loop of window size changes, and forcing the layout height to zero
proves a nice universal mitigation.  The code in this file uses visible height
instead of layout height so it needs a more explicit check.

Could we just add an isWebView() function instead of getWideViewportQuirkEnabled
and getForceZeroLayoutHeight? I see several of these calls throughout Blink.
That's out of scope for this patch though.

Can we add a comment here though? Something similar to the change description
like "WebView automatically resizes to match the contents which will cause an
infinite loop as the contents then resize to match the window. To prevent this,
we disable shrinking to fit for WebViews"

[gone, 2017-01-19 18:56:29]

Cool, thanks.  Can I get a sanity check on the last patch?

https://codereview.chromium.org/2646663002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/ImageDocumentTest.cpp (right):

https://codereview.chromium.org/2646663002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/ImageDocumentTest.cpp:188: if
(document().shouldShrinkToFit()) {
On 2017/01/19 04:08:22, aelias wrote:
> This test can never go red, even without your change.  It's not an integration
> test, so there's no concept of running in a different environment with a
> different setting.  Rather, the test itself determines its settings and it's
> uniform across all platforms.  So please set the forceZeroLayoutHeight setting
> during test initialization instead of having an if statement (or simply remove
> the test if you don't have time, I'd rather have no test than a fake test).

Yeah, I was conking out last night before I could figure out how to set up the
setting and uploaded this to see if I was on the right track with the bots. 
I've split it into two tests and updated the settings now.

[gone, 2017-01-19 18:56:33]

The CQ bit was checked by dfalcantara@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-19 18:57:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[pdr., 2017-01-19 19:10:59]

On 2017/01/19 at 18:56:29, dfalcantara wrote:
> Cool, thanks.  Can I get a sanity check on the last patch?
> 
>
https://codereview.chromium.org/2646663002/diff/60001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/core/html/ImageDocumentTest.cpp (right):
> 
>
https://codereview.chromium.org/2646663002/diff/60001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/html/ImageDocumentTest.cpp:188: if
(document().shouldShrinkToFit()) {
> On 2017/01/19 04:08:22, aelias wrote:
> > This test can never go red, even without your change.  It's not an
integration
> > test, so there's no concept of running in a different environment with a
> > different setting.  Rather, the test itself determines its settings and it's
> > uniform across all platforms.  So please set the forceZeroLayoutHeight
setting
> > during test initialization instead of having an if statement (or simply
remove
> > the test if you don't have time, I'd rather have no test than a fake test).
> 
> Yeah, I was conking out last night before I could figure out how to set up the
setting and uploaded this to see if I was on the right track with the bots. 
I've split it into two tests and updated the settings now.

LGTM

[commit-bot: I haz the power, 2017-01-19 20:33:35]

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

[commit-bot: I haz the power, 2017-01-19 20:33:36]

Dry run: This issue passed the CQ dry run.

[gone, 2017-01-19 20:53:37]

The CQ bit was checked by dfalcantara@chromium.org

[gone, 2017-01-19 20:53:38]

The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org
Link to the patchset: https://codereview.chromium.org/2646663002/#ps80001
(title: "Update test")

[commit-bot: I haz the power, 2017-01-19 20:53:57]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-19 20:58:28]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1484859217844060,
"parent_rev": "91822bd4b039b9151a54df63c387ae987e35fc59", "commit_rev":
"684507d88c125e0ea9077787e94328ced875eae3"}

[commit-bot: I haz the power, 2017-01-19 20:59:01]

Description was changed from

==========
[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
==========

to

==========
[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}
Committed:
https://chromium.googlesource.com/chromium/src/+/684507d88c125e0ea9077787e943...
==========

[commit-bot: I haz the power, 2017-01-19 20:59:03]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/684507d88c125e0ea9077787e943...