Fixes bug where cast_shell uses fraction of screen on ATV

Fixes bug where cast_shell uses fraction of screen on ATV

Recent changes to page scale initialization and defaults mean we ended
up with page scale = 0.25 (previously 1) on ATV.

BUG=internal b/20418423

Committed: https://crrev.com/1b125b2d8d09f8cb71b3199095817da66a25b505
Cr-Commit-Position: refs/heads/master@{#327084}

[halliwell, 2015-04-24 00:00:59]

halliwell@chromium.org changed reviewers:
+ gunsch@chromium.org, lcwu@chromium.org

[gunsch, 2015-04-24 00:04:38]

The fix itself LG, but a few questions/thoughts that could be reasonably
addressed in this CL as well:

1) Can you confirm <meta>-based device-scale changes in the page don't affect
it?

(can try at http://172.17.23.168/gtv/gunsch/fiddle.html )

2) Is there a planned follow up for setting different resolution per intent?
(see CastShellActivity.setResolution for the current logic).

3) Should we just remove the command-line-flag logic in
CastShellActivity.setResolution for now?

[halliwell, 2015-04-24 00:11:46]

On 2015/04/24 00:04:38, gunsch wrote:
> The fix itself LG, but a few questions/thoughts that could be reasonably
> addressed in this CL as well:
> 
> 1) Can you confirm <meta>-based device-scale changes in the page don't affect
> it?
> 
Yes, I confirmed this.  Without the 'override' call, they do.

> (can try at http://172.17.23.168/gtv/gunsch/fiddle.html )
> 
> 2) Is there a planned follow up for setting different resolution per intent?
> (see CastShellActivity.setResolution for the current logic).
> 

AFAIK, we still need device scale factor to cover the discrepancy between
physical display resolution, and our desired application resolution.  I don't
have a planned follow-up.

> 3) Should we just remove the command-line-flag logic in
> CastShellActivity.setResolution for now?

I think we still need this ...

[gunsch, 2015-04-24 00:18:04]

On 2015/04/24 00:11:46, halliwell wrote:
> On 2015/04/24 00:04:38, gunsch wrote:
> > The fix itself LG, but a few questions/thoughts that could be reasonably
> > addressed in this CL as well:
> > 
> > 1) Can you confirm <meta>-based device-scale changes in the page don't
affect
> > it?
> > 
> Yes, I confirmed this.  Without the 'override' call, they do.
> 
> > (can try at http://172.17.23.168/gtv/gunsch/fiddle.html )
> > 
> > 2) Is there a planned follow up for setting different resolution per intent?
> > (see CastShellActivity.setResolution for the current logic).
> > 
> 
> AFAIK, we still need device scale factor to cover the discrepancy between
> physical display resolution, and our desired application resolution.  I don't
> have a planned follow-up.
> 
> > 3) Should we just remove the command-line-flag logic in
> > CastShellActivity.setResolution for now?
> 
> I think we still need this ...

Discussed briefly online. It does also depend on a downstream hack we have. We
could consider using IPC or a command-line flag to communicate that information
to the render process, then call WebView::setDeviceScaleFactor ourselves on a
per-page basis.

Also, I don't completely understand the relation between device-scale factor and
page-scale factor, and I wonder if we could make do using only the page-scale
factor.

[halliwell, 2015-04-24 00:53:21]

On 2015/04/24 00:18:04, gunsch wrote:
> On 2015/04/24 00:11:46, halliwell wrote:
> > On 2015/04/24 00:04:38, gunsch wrote:
> > > The fix itself LG, but a few questions/thoughts that could be reasonably
> > > addressed in this CL as well:
> > > 
> > > 1) Can you confirm <meta>-based device-scale changes in the page don't
> affect
> > > it?
> > > 
> > Yes, I confirmed this.  Without the 'override' call, they do.
> > 
> > > (can try at http://172.17.23.168/gtv/gunsch/fiddle.html )
> > > 
> > > 2) Is there a planned follow up for setting different resolution per
intent?
> > > (see CastShellActivity.setResolution for the current logic).
> > > 
> > 
> > AFAIK, we still need device scale factor to cover the discrepancy between
> > physical display resolution, and our desired application resolution.  I
don't
> > have a planned follow-up.
> > 
> > > 3) Should we just remove the command-line-flag logic in
> > > CastShellActivity.setResolution for now?
> > 
> > I think we still need this ...
> 
> Discussed briefly online. It does also depend on a downstream hack we have. We
> could consider using IPC or a command-line flag to communicate that
information
> to the render process, then call WebView::setDeviceScaleFactor ourselves on a
> per-page basis.
> 
> Also, I don't completely understand the relation between device-scale factor
and
> page-scale factor, and I wonder if we could make do using only the page-scale
> factor.

Yep - as discussed, will try this out and see what happens :)  Let's hold off on
this review till I've done that.

[lcwu1, 2015-04-24 00:54:21]

https://codereview.chromium.org/1075043004/diff/1/chromecast/renderer/cast_co...
File chromecast/renderer/cast_content_renderer_client.cc (right):

https://codereview.chromium.org/1075043004/diff/1/chromecast/renderer/cast_co...
chromecast/renderer/cast_content_renderer_client.cc:155:
webview->setInitialPageScaleOverride(1.f);
Do we need both? ISTM the first API call should be able to restore the behavior
on M41. And if we don't make the first call, the 2nd API call should be able to
override the default scale setting.

[halliwell, 2015-04-24 01:02:39]

https://codereview.chromium.org/1075043004/diff/1/chromecast/renderer/cast_co...
File chromecast/renderer/cast_content_renderer_client.cc (right):

https://codereview.chromium.org/1075043004/diff/1/chromecast/renderer/cast_co...
chromecast/renderer/cast_content_renderer_client.cc:155:
webview->setInitialPageScaleOverride(1.f);
On 2015/04/24 00:54:21, lcwu1 wrote:
> Do we need both? ISTM the first API call should be able to restore the
behavior
> on M41. And if we don't make the first call, the 2nd API call should be able
to
> override the default scale setting.

I believe we want the second call to override the meta viewport tag.  If we
don't do this, we will get inconsistent behaviour between ATV and other
platforms (where meta viewport is ignored).

I think you may be right that we don't need the first call though.  I will
check.

[lcwu1, 2015-04-24 01:05:59]

https://codereview.chromium.org/1075043004/diff/1/chromecast/renderer/cast_co...
File chromecast/renderer/cast_content_renderer_client.cc (right):

https://codereview.chromium.org/1075043004/diff/1/chromecast/renderer/cast_co...
chromecast/renderer/cast_content_renderer_client.cc:155:
webview->setInitialPageScaleOverride(1.f);
On 2015/04/24 01:02:38, halliwell wrote:
> On 2015/04/24 00:54:21, lcwu1 wrote:
> > Do we need both? ISTM the first API call should be able to restore the
> behavior
> > on M41. And if we don't make the first call, the 2nd API call should be able
> to
> > override the default scale setting.
> 
> I believe we want the second call to override the meta viewport tag.  If we
> don't do this, we will get inconsistent behaviour between ATV and other
> platforms (where meta viewport is ignored).
> 
> I think you may be right that we don't need the first call though.  I will
> check.

This sounds correct to me, as the order seems to be:

1. default scale
2. page scale (meta viewport)
3. override

So doing #3 (which is your 2nd API) should be good enough. But please do
try/test it out.

[halliwell, 2015-04-27 17:44:11]

On 2015/04/24 00:18:04, gunsch wrote:
> On 2015/04/24 00:11:46, halliwell wrote:
> > On 2015/04/24 00:04:38, gunsch wrote:
> > > The fix itself LG, but a few questions/thoughts that could be reasonably
> > > addressed in this CL as well:
> > > 
> > > 1) Can you confirm <meta>-based device-scale changes in the page don't
> affect
> > > it?
> > > 
> > Yes, I confirmed this.  Without the 'override' call, they do.
> > 
> > > (can try at http://172.17.23.168/gtv/gunsch/fiddle.html )
> > > 
> > > 2) Is there a planned follow up for setting different resolution per
intent?
> > > (see CastShellActivity.setResolution for the current logic).
> > > 
> > 
> > AFAIK, we still need device scale factor to cover the discrepancy between
> > physical display resolution, and our desired application resolution.  I
don't
> > have a planned follow-up.
> > 
> > > 3) Should we just remove the command-line-flag logic in
> > > CastShellActivity.setResolution for now?
> > 
> > I think we still need this ...
> 
> Discussed briefly online. It does also depend on a downstream hack we have. We
> could consider using IPC or a command-line flag to communicate that
information
> to the render process, then call WebView::setDeviceScaleFactor ourselves on a
> per-page basis.
> 
> Also, I don't completely understand the relation between device-scale factor
and
> page-scale factor, and I wonder if we could make do using only the page-scale
> factor.

Ok, I played around more with page scale and device scale today, and I don't
think we can use page scale to replace device scale factor.  It does give the
desired values for JS window.innerWidth/innerHeight, but fails to handle CSS
width/height=100% correctly.  Youtube TV interface uses this to fill its window,
so it ends up with a 1080p resolution, and then applying page scale 1.5 means we
get a zoomed-in view of a portion of the page.

The details are as follows: if device scale factor is 1, then
ContentViewCoreImpl::GetViewportSizeDip returns 1080p resolution.  This is used
by RWHVAndroid::GetViewBounds, to RWHVBase::GetRequestedRendererSize, to
RenderWidgetHostImpl::GetResizeParams, which populates the resize IPC message
and gets passed through to RenderWidget, and on to Blink code.  We end up with
1080p as WebViewImpl::m_size.

This also shows why we need to update the device scale factor in the browser
process.  So I think for now, that downstream hack remains a TODO item, separate
to this CL.

[halliwell, 2015-04-27 17:44:20]

https://codereview.chromium.org/1075043004/diff/1/chromecast/renderer/cast_co...
File chromecast/renderer/cast_content_renderer_client.cc (right):

https://codereview.chromium.org/1075043004/diff/1/chromecast/renderer/cast_co...
chromecast/renderer/cast_content_renderer_client.cc:155:
webview->setInitialPageScaleOverride(1.f);
On 2015/04/24 01:05:58, lcwu1 wrote:
> On 2015/04/24 01:02:38, halliwell wrote:
> > On 2015/04/24 00:54:21, lcwu1 wrote:
> > > Do we need both? ISTM the first API call should be able to restore the
> > behavior
> > > on M41. And if we don't make the first call, the 2nd API call should be
able
> > to
> > > override the default scale setting.
> > 
> > I believe we want the second call to override the meta viewport tag.  If we
> > don't do this, we will get inconsistent behaviour between ATV and other
> > platforms (where meta viewport is ignored).
> > 
> > I think you may be right that we don't need the first call though.  I will
> > check.
> 
> This sounds correct to me, as the order seems to be:
> 
> 1. default scale
> 2. page scale (meta viewport)
> 3. override
> 
> So doing #3 (which is your 2nd API) should be good enough. But please do
> try/test it out.

I tried this, and it does work (setting initial scale only).  However, I think
there's an argument for setting the min/max as well.  In principle, the override
value is only an _initial_ override, and Blink is free to change it within the
min/max range according to its logic.  In practice, it probably only changes it
based on pinch/zoom events, but still, if we want to fix the page scale at 1, it
seems correct to also set the min/max range.

Let me know if you agree or not :)

[lcwu1, 2015-04-27 17:51:54]

On 2015/04/27 17:44:20, halliwell wrote:
>
https://codereview.chromium.org/1075043004/diff/1/chromecast/renderer/cast_co...
> File chromecast/renderer/cast_content_renderer_client.cc (right):
> 
>
https://codereview.chromium.org/1075043004/diff/1/chromecast/renderer/cast_co...
> chromecast/renderer/cast_content_renderer_client.cc:155:
> webview->setInitialPageScaleOverride(1.f);
> On 2015/04/24 01:05:58, lcwu1 wrote:
> > On 2015/04/24 01:02:38, halliwell wrote:
> > > On 2015/04/24 00:54:21, lcwu1 wrote:
> > > > Do we need both? ISTM the first API call should be able to restore the
> > > behavior
> > > > on M41. And if we don't make the first call, the 2nd API call should be
> able
> > > to
> > > > override the default scale setting.
> > > 
> > > I believe we want the second call to override the meta viewport tag.  If
we
> > > don't do this, we will get inconsistent behaviour between ATV and other
> > > platforms (where meta viewport is ignored).
> > > 
> > > I think you may be right that we don't need the first call though.  I will
> > > check.
> > 
> > This sounds correct to me, as the order seems to be:
> > 
> > 1. default scale
> > 2. page scale (meta viewport)
> > 3. override
> > 
> > So doing #3 (which is your 2nd API) should be good enough. But please do
> > try/test it out.
> 
> I tried this, and it does work (setting initial scale only).  However, I think
> there's an argument for setting the min/max as well.  In principle, the
override
> value is only an _initial_ override, and Blink is free to change it within the
> min/max range according to its logic.  In practice, it probably only changes
it
> based on pinch/zoom events, but still, if we want to fix the page scale at 1,
it
> seems correct to also set the min/max range.
> 
> Let me know if you agree or not :)

Sounds reasonable to me.

lgtm

[gunsch, 2015-04-27 17:52:15]

lgtm, +1 to setting both. Thanks for digging so far into this.

[halliwell, 2015-04-27 18:07:08]

The CQ bit was checked by halliwell@chromium.org

[commit-bot: I haz the power, 2015-04-27 18:08:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1075043004/1

[commit-bot: I haz the power, 2015-04-27 18:36:58]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2015-04-27 18:38:15]

Patchset 1 (id:??) landed as
https://crrev.com/1b125b2d8d09f8cb71b3199095817da66a25b505
Cr-Commit-Position: refs/heads/master@{#327084}