[Chromecast] Handle device scale factor correctly

[Chromecast] Handle device scale factor correctly

On ATV, this removes the forced device scale factor hack; it
can have its intended natural value for the platform.

On Linux, we use device scale factor of 1.5 when we can use
a 1080p device resolution.

With this, all applications should get a 720p viewport (in
CSS pixels).  Applications that wish to provide higher-res
assets can use CSS or JS to detect the higher scale factor.

BUG=internal b/28199180

Committed: https://crrev.com/b7f20ad97366a9392432f65ef330545a40d71674
Cr-Commit-Position: refs/heads/master@{#394034}

[halliwell, 2016-05-11 01:08:26]

Description was changed from

==========
[Chromecast] Use television viewport to force 720p layout

Previous television viewport css had no effect; setting min-width=1280
did nothing since we always run with a display width >= 1280.

In the new approach, we'll run with layout res = device res by default,
with an exception for the case of apps that need to force 720p layout.
This path will be activated by internal code (based on app cfg)
specifying app resolution in WebContents user data.

This is a short-term approach; longer-term we will eliminate resolution
dependencies from applications and can then just run apps at the native
display resolution.

BUG=internal b/28199180
==========

to

==========
[Chromecast] Use television viewport to force 720p layout

Previous television viewport css had no effect; setting min-width=1280
did nothing since we always run with a display width >= 1280.

In the new approach, we'll run with layout res = device res by default,
with an exception for the case of apps that need to force 720p layout.
This path will be activated by internal code (based on app cfg)
specifying app resolution in WebContents user data.

This is a short-term approach; longer-term we will eliminate resolution
dependencies from applications and can then just run apps at the native
display resolution.

BUG=internal b/28199180
==========

[halliwell, 2016-05-11 01:08:26]

halliwell@chromium.org changed reviewers:
+ alokp@chromium.org, derekjchow@chromium.org, sanfin@chromium.org

[halliwell, 2016-05-11 01:09:08]

PTAL.  As you'll see from the internal CL, I haven't finished the ATV codepath
yet, let alone tested it.  On Linux, more testing is needed but it looks
promising so far.

[alokp, 2016-05-11 16:46:54]

https://codereview.chromium.org/1972433002/diff/1/chromecast/browser/cast_con...
File chromecast/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/1972433002/diff/1/chromecast/browser/cast_con...
chromecast/browser/cast_content_browser_client.cc:293: prefs->viewport_enabled =
false;
This seems risky to me. I am not sure what else we might break by disabling
viewport on Android. You should ask blink folks for advice.

I was under the impression that we would attempt the opposite - enable viewport
on non-android platforms. That gives applications most freedom to either use the
default viewport or define their own.

https://codereview.chromium.org/1972433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/css/viewportTelevision.css (right):

https://codereview.chromium.org/1972433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/css/viewportTelevision.css:8: width: 1280px;
Is the window size still bigger than that of layout viewport when you account
for device-scale-factor? IIRC it is usually >=1.5 for Android devices, in which
case this change should not be required?

[halliwell, 2016-05-11 17:56:56]

On 2016/05/11 16:46:54, alokp wrote:
>
https://codereview.chromium.org/1972433002/diff/1/chromecast/browser/cast_con...
> File chromecast/browser/cast_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/1972433002/diff/1/chromecast/browser/cast_con...
> chromecast/browser/cast_content_browser_client.cc:293: prefs->viewport_enabled
=
> false;
> This seems risky to me. I am not sure what else we might break by disabling
> viewport on Android. You should ask blink folks for advice.
> 
> I was under the impression that we would attempt the opposite - enable
viewport
> on non-android platforms. That gives applications most freedom to either use
the
> default viewport or define their own.
> 
>
https://codereview.chromium.org/1972433002/diff/1/third_party/WebKit/Source/c...
> File third_party/WebKit/Source/core/css/viewportTelevision.css (right):
> 
>
https://codereview.chromium.org/1972433002/diff/1/third_party/WebKit/Source/c...
> third_party/WebKit/Source/core/css/viewportTelevision.css:8: width: 1280px;
> Is the window size still bigger than that of layout viewport when you account
> for device-scale-factor? IIRC it is usually >=1.5 for Android devices, in
which
> case this change should not be required?

Thanks Alok.  Quick summary of our offline conversation:
* For devices with 1080p back buffers, we should have device-scale-factor=1.5. 
For devices with 720p back buffers, this scale should be 1.
* This means css change can be reverted; min-width=1280 should give a 720p
viewport.  Apps that want a 1080p viewport should opt into it with viewport tag.
* IMAX should use viewport width=device-width to get the native resolution of
the platform they're running on.

I will update the patch in this direction.

[halliwell, 2016-05-11 21:39:50]

Description was changed from

==========
[Chromecast] Use television viewport to force 720p layout

Previous television viewport css had no effect; setting min-width=1280
did nothing since we always run with a display width >= 1280.

In the new approach, we'll run with layout res = device res by default,
with an exception for the case of apps that need to force 720p layout.
This path will be activated by internal code (based on app cfg)
specifying app resolution in WebContents user data.

This is a short-term approach; longer-term we will eliminate resolution
dependencies from applications and can then just run apps at the native
display resolution.

BUG=internal b/28199180
==========

to

==========
[Chromecast] Enable viewport on all platforms

720p platforms should now set device scale factor = 1; 1080p platforms should
use 1.5.  Together with using TV UA stylesheet and enabling viewport, this means
default app behaviour is to get a 720p layout viewport on all platforms
(upscaled on 1080p).

If an application can handle both 720p and 1080p resolutions, it should now set
viewport width = device-width.

BUG=internal b/28199180
==========

[halliwell, 2016-05-11 21:40:59]

Updated with a first pass at new approach ... still needs testing though :)

[alokp, 2016-05-11 21:48:48]

This looks pretty good. Still need to verify the theory that:
1. device-scale-factor will obviate the need for setting max viewport width.
2. 1080p apps have a way to change viewport or do something functionally
equivalent with device-pixel-ratio.

https://codereview.chromium.org/1972433002/diff/20001/chromecast/browser/cast...
File chromecast/browser/cast_content_browser_client.cc (right):

https://codereview.chromium.org/1972433002/diff/20001/chromecast/browser/cast...
chromecast/browser/cast_content_browser_client.cc:279: #if !defined(OS_ANDROID)
You might want to just set viewport_enabled and viewport_meta_enabled to true
without any OS_ANDROID guard and remove those DCHECKs.

[halliwell, 2016-05-12 21:08:30]

Description was changed from

==========
[Chromecast] Enable viewport on all platforms

720p platforms should now set device scale factor = 1; 1080p platforms should
use 1.5.  Together with using TV UA stylesheet and enabling viewport, this means
default app behaviour is to get a 720p layout viewport on all platforms
(upscaled on 1080p).

If an application can handle both 720p and 1080p resolutions, it should now set
viewport width = device-width.

BUG=internal b/28199180
==========

to

==========
[Chromecast] Handle device scale factor correctly

On ATV, this removes the forced device scale factor hack; it
can have its intended natural value for the platform.

On Linux, we use device scale factor of 1.5 when we can use
a 1080p device resolution.

With this, all applications should get a 720p viewport (in
CSS pixels).  Applications that wish to provide higher-res
assets can use CSS or JS to detect the higher scale factor.

BUG=internal b/28199180
==========

[halliwell, 2016-05-12 21:10:12]

On 2016/05/11 21:48:48, alokp wrote:
> This looks pretty good. Still need to verify the theory that:
> 1. device-scale-factor will obviate the need for setting max viewport width.
> 2. 1080p apps have a way to change viewport or do something functionally
> equivalent with device-pixel-ratio.
> 
>
https://codereview.chromium.org/1972433002/diff/20001/chromecast/browser/cast...
> File chromecast/browser/cast_content_browser_client.cc (right):
> 
>
https://codereview.chromium.org/1972433002/diff/20001/chromecast/browser/cast...
> chromecast/browser/cast_content_browser_client.cc:279: #if
!defined(OS_ANDROID)
> You might want to just set viewport_enabled and viewport_meta_enabled to true
> without any OS_ANDROID guard and remove those DCHECKs.

Ok, after talking with Alok last night - another tweak to the approach.  We came
to the conclusion that there is no need for the application's viewport to be
higher-res in CSS pixels; instead, we should always have 720p viewports in CSS
pixels, and set the device scale factor to allow apps to provide higher-res
imagery and so on if they wish.

PTAL!

[alokp, 2016-05-12 21:35:16]

lgtm % nits

https://codereview.chromium.org/1972433002/diff/40001/chromecast/browser/cast...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/1972433002/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.cc:371: #if defined(USE_AURA)
nit: move the logic to determine display size into a function

https://codereview.chromium.org/1972433002/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.cc:372: gfx::Size display_size(1280,
720);
nit: make this more explicit by defining a kDefaultDisplaySize here or at the
top of this file.

https://codereview.chromium.org/1972433002/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.cc:375:
base::CommandLine::ForCurrentProcess()->argv()))
nit: braces for multi-line if blocks

https://codereview.chromium.org/1972433002/diff/40001/chromecast/browser/cast...
File chromecast/browser/cast_content_window.cc (right):

https://codereview.chromium.org/1972433002/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_content_window.cc:74:
CHECK(display::Screen::GetScreen());
Can this be a DCHECK?

https://codereview.chromium.org/1972433002/diff/40001/chromecast/media/base/v...
File chromecast/media/base/video_plane_controller.h (right):

https://codereview.chromium.org/1972433002/diff/40001/chromecast/media/base/v...
chromecast/media/base/video_plane_controller.h:55: void
SetGraphicsPlaneResolution(const Size& resolution);
Now that we pass graphics resolution in the constructor that does not change, do
we still need this method?

[halliwell, 2016-05-13 23:48:03]

This patchset contains changes to get it working on ATV.  In particular:
* It didn't compile to start with :)
* Found a line that still set forced device scale factor = 1 on ATV
* ATV appears to default to page-scale=0.25 and a window with huge dimensions. 
I restored the lines that set page scale to 1, which appear to fix this,
although I wonder if the solution should be elsewhere (e.g. should it be in
television css file).  I don't fully understand where the 0.25 comes from.

https://codereview.chromium.org/1972433002/diff/40001/chromecast/browser/cast...
File chromecast/browser/cast_browser_main_parts.cc (right):

https://codereview.chromium.org/1972433002/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.cc:371: #if defined(USE_AURA)
On 2016/05/12 21:35:16, alokp wrote:
> nit: move the logic to determine display size into a function

Done (also moved it into CastScreen source file).

https://codereview.chromium.org/1972433002/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.cc:372: gfx::Size display_size(1280,
720);
On 2016/05/12 21:35:16, alokp wrote:
> nit: make this more explicit by defining a kDefaultDisplaySize here or at the
> top of this file.

no need now it's in a helper.

https://codereview.chromium.org/1972433002/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_browser_main_parts.cc:375:
base::CommandLine::ForCurrentProcess()->argv()))
On 2016/05/12 21:35:16, alokp wrote:
> nit: braces for multi-line if blocks

Done.

https://codereview.chromium.org/1972433002/diff/40001/chromecast/browser/cast...
File chromecast/browser/cast_content_window.cc (right):

https://codereview.chromium.org/1972433002/diff/40001/chromecast/browser/cast...
chromecast/browser/cast_content_window.cc:74:
CHECK(display::Screen::GetScreen());
On 2016/05/12 21:35:16, alokp wrote:
> Can this be a DCHECK?

sure, done.

https://codereview.chromium.org/1972433002/diff/40001/chromecast/media/base/v...
File chromecast/media/base/video_plane_controller.h (right):

https://codereview.chromium.org/1972433002/diff/40001/chromecast/media/base/v...
chromecast/media/base/video_plane_controller.h:55: void
SetGraphicsPlaneResolution(const Size& resolution);
On 2016/05/12 21:35:16, alokp wrote:
> Now that we pass graphics resolution in the constructor that does not change,
do
> we still need this method?

Yep, not needed.

[alokp, 2016-05-14 05:15:32]

still lgtm

https://codereview.chromium.org/1972433002/diff/80001/chromecast/browser/cast...
File chromecast/browser/cast_content_window.cc (right):

https://codereview.chromium.org/1972433002/diff/80001/chromecast/browser/cast...
chromecast/browser/cast_content_window.cc:76: gfx::Size display_size =
Ideally we should query this size from WebContents NativeView.

https://codereview.chromium.org/1972433002/diff/80001/chromecast/browser/cast...
chromecast/browser/cast_content_window.cc:105:
CHECK(display::Screen::GetScreen());
I wonder why this is a member function for this class (not related to this
patch).

[halliwell, 2016-05-16 15:48:48]

https://codereview.chromium.org/1972433002/diff/80001/chromecast/browser/cast...
File chromecast/browser/cast_content_window.cc (right):

https://codereview.chromium.org/1972433002/diff/80001/chromecast/browser/cast...
chromecast/browser/cast_content_window.cc:76: gfx::Size display_size =
On 2016/05/14 05:15:32, alokp wrote:
> Ideally we should query this size from WebContents NativeView.

WebContents is currently not aware of this size.  Does this mean we should get
the NativeView and call SetBounds immediately after creating the WebContents
below?

https://codereview.chromium.org/1972433002/diff/80001/chromecast/browser/cast...
chromecast/browser/cast_content_window.cc:105:
CHECK(display::Screen::GetScreen());
On 2016/05/14 05:15:32, alokp wrote:
> I wonder why this is a member function for this class (not related to this
> patch).

Good question, I don't know; maybe some historical reasons.

[alokp, 2016-05-16 16:26:14]

https://codereview.chromium.org/1972433002/diff/80001/chromecast/browser/cast...
File chromecast/browser/cast_content_window.cc (right):

https://codereview.chromium.org/1972433002/diff/80001/chromecast/browser/cast...
chromecast/browser/cast_content_window.cc:76: gfx::Size display_size =
On 2016/05/16 15:48:48, halliwell wrote:
> On 2016/05/14 05:15:32, alokp wrote:
> > Ideally we should query this size from WebContents NativeView.
> 
> WebContents is currently not aware of this size.  Does this mean we should get
> the NativeView and call SetBounds immediately after creating the WebContents
> below?

We already pass the size in WebContents::CreateParams. I would expect that it
the NativeView bounds would already be of the correct size. Here we are just
creating a parent window of the same size as the content window.

[halliwell, 2016-05-16 20:42:20]

https://codereview.chromium.org/1972433002/diff/80001/chromecast/browser/cast...
File chromecast/browser/cast_content_window.cc (right):

https://codereview.chromium.org/1972433002/diff/80001/chromecast/browser/cast...
chromecast/browser/cast_content_window.cc:76: gfx::Size display_size =
On 2016/05/16 16:26:14, alokp wrote:
> On 2016/05/16 15:48:48, halliwell wrote:
> > On 2016/05/14 05:15:32, alokp wrote:
> > > Ideally we should query this size from WebContents NativeView.
> > 
> > WebContents is currently not aware of this size.  Does this mean we should
get
> > the NativeView and call SetBounds immediately after creating the WebContents
> > below?
> 
> We already pass the size in WebContents::CreateParams. I would expect that it
> the NativeView bounds would already be of the correct size. Here we are just
> creating a parent window of the same size as the content window.

The NativeView's bounds appear to be 0x0 at this point, so I don't think we can
use it.

[halliwell, 2016-05-16 23:07:21]

The CQ bit was checked by halliwell@chromium.org

[commit-bot: I haz the power, 2016-05-16 23:07:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972433002/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972433002/80001

[commit-bot: I haz the power, 2016-05-16 23:29:29]

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

[commit-bot: I haz the power, 2016-05-16 23:29:30]

Try jobs failed on following builders:
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)

[halliwell, 2016-05-17 00:07:00]

The CQ bit was checked by halliwell@chromium.org

[halliwell, 2016-05-17 00:07:01]

The patchset sent to the CQ was uploaded after l-g-t-m from alokp@chromium.org
Link to the patchset: https://codereview.chromium.org/1972433002/#ps100001
(title: "Add missing include")

[commit-bot: I haz the power, 2016-05-17 00:07:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972433002/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972433002/100001

[commit-bot: I haz the power, 2016-05-17 00:22:57]

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

[commit-bot: I haz the power, 2016-05-17 00:22:58]

Try jobs failed on following builders:
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[halliwell, 2016-05-17 01:52:20]

The CQ bit was checked by halliwell@chromium.org

[halliwell, 2016-05-17 01:52:21]

The patchset sent to the CQ was uploaded after l-g-t-m from alokp@chromium.org
Link to the patchset: https://codereview.chromium.org/1972433002/#ps120001
(title: "Fix test compile error")

[commit-bot: I haz the power, 2016-05-17 01:52:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1972433002/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1972433002/120001

[commit-bot: I haz the power, 2016-05-17 03:09:17]

Description was changed from

==========
[Chromecast] Handle device scale factor correctly

On ATV, this removes the forced device scale factor hack; it
can have its intended natural value for the platform.

On Linux, we use device scale factor of 1.5 when we can use
a 1080p device resolution.

With this, all applications should get a 720p viewport (in
CSS pixels).  Applications that wish to provide higher-res
assets can use CSS or JS to detect the higher scale factor.

BUG=internal b/28199180
==========

to

==========
[Chromecast] Handle device scale factor correctly

On ATV, this removes the forced device scale factor hack; it
can have its intended natural value for the platform.

On Linux, we use device scale factor of 1.5 when we can use
a 1080p device resolution.

With this, all applications should get a 720p viewport (in
CSS pixels).  Applications that wish to provide higher-res
assets can use CSS or JS to detect the higher scale factor.

BUG=internal b/28199180
==========

[commit-bot: I haz the power, 2016-05-17 03:09:18]

Committed patchset #7 (id:120001)

[commit-bot: I haz the power, 2016-05-17 03:10:25]

Description was changed from

==========
[Chromecast] Handle device scale factor correctly

On ATV, this removes the forced device scale factor hack; it
can have its intended natural value for the platform.

On Linux, we use device scale factor of 1.5 when we can use
a 1080p device resolution.

With this, all applications should get a 720p viewport (in
CSS pixels).  Applications that wish to provide higher-res
assets can use CSS or JS to detect the higher scale factor.

BUG=internal b/28199180
==========

to

==========
[Chromecast] Handle device scale factor correctly

On ATV, this removes the forced device scale factor hack; it
can have its intended natural value for the platform.

On Linux, we use device scale factor of 1.5 when we can use
a 1080p device resolution.

With this, all applications should get a 720p viewport (in
CSS pixels).  Applications that wish to provide higher-res
assets can use CSS or JS to detect the higher scale factor.

BUG=internal b/28199180

Committed: https://crrev.com/b7f20ad97366a9392432f65ef330545a40d71674
Cr-Commit-Position: refs/heads/master@{#394034}
==========

[commit-bot: I haz the power, 2016-05-17 03:10:26]

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