HiDPI: Scale viewport for pepper flash fullscreen

HiDPI: Scale viewport for pepper flash fullscreen

Pepper flash fullscreen mode was not rendering correctly in HiDPI mode. The problem was that we weren't taking into account the display scale factor when setting the view port.

This CL pipes the screen info to RenderWidgetFullscreenPepper so that the display scale factor can be used.

BUG=130323
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=148508

[sail, 2012-07-19 18:06:45]

Hey Nico and Josh. My patch doesn't quite work. Here's what I'm seeing:
  - laptop with 2x display connected to an external monitor with 1x display
  - open a browser window on the external display (any URL, newtab is fine)
  - RenderWidget::RenderWidget() sets device_scale_factor_ to 2.0.
I would expect that the scale factor would be 1.0. Am I doing something wrong?

The webpage looks fine but the scale factor end up making the pepper flash
fullscreen twice as big as it should be.

Thanks.

[sail, 2012-07-19 18:30:03]

On 2012/07/19 18:06:45, sail wrote:
> Hey Nico and Josh. My patch doesn't quite work. Here's what I'm seeing:
>   - laptop with 2x display connected to an external monitor with 1x display
>   - open a browser window on the external display (any URL, newtab is fine)
>   - RenderWidget::RenderWidget() sets device_scale_factor_ to 2.0.
> I would expect that the scale factor would be 1.0. Am I doing something wrong?
> 
> The webpage looks fine but the scale factor end up making the pepper flash
> fullscreen twice as big as it should be.
> 
> Thanks.


Actually, never mind. This looks like an unrelated bug. Navigating to another
page correctly changes the display scale factor to 1.0.

I can also reproduce the bug outside of fullscreen by opening a page on launch.
For example:
   - Chromium.app/Content/MacOS/Chromium
    - newtab page looks fine
    - navigate to http://www.youtube.com/watch?v=bWpK0wsnitc&feature=g-logo-xit,
everything looks fine
     - quit
      - Chromium.app/Contents/MacOS/Chromium
http://www.youtube.com/watch?v=bWpK0wsnitc&feature=g-logo-xit
      - notice that the webpage is zoomed in at 2x, navigating to another page
fixes it

Is this a known issue or should I file a bug for this? Thanks.

[Nico, 2012-07-21 16:52:22]

On Thu, Jul 19, 2012 at 11:30 AM,  <sail@chromium.org> wrote:
> On 2012/07/19 18:06:45, sail wrote:
>>
>> Hey Nico and Josh. My patch doesn't quite work. Here's what I'm seeing:
>>    - laptop with 2x display connected to an external monitor with 1x
>> display
>>    - open a browser window on the external display (any URL, newtab is
>> fine)
>>    - RenderWidget::RenderWidget() sets device_scale_factor_ to 2.0.
>> I would expect that the scale factor would be 1.0. Am I doing something
>> wrong?
>
>
>> The webpage looks fine but the scale factor end up making the pepper flash
>> fullscreen twice as big as it should be.
>
>
>> Thanks.
>
>
>
> Actually, never mind. This looks like an unrelated bug. Navigating to
> another
> page correctly changes the display scale factor to 1.0.
>
> I can also reproduce the bug outside of fullscreen by opening a page on
> launch.
> For example:
>    - Chromium.app/Content/MacOS/Chromium
>     - newtab page looks fine
>     - navigate to
> http://www.youtube.com/watch?v=bWpK0wsnitc&feature=g-logo-xit,
> everything looks fine
>      - quit
>       - Chromium.app/Contents/MacOS/Chromium
> http://www.youtube.com/watch?v=bWpK0wsnitc&feature=g-logo-xit
>       - notice that the webpage is zoomed in at 2x, navigating to another
> page
> fixes it
>
> Is this a known issue or should I file a bug for this? Thanks.

Please file.

[Nico, 2012-07-21 16:55:15]

https://chromiumcodereview.appspot.com/10807039/diff/1/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10807039/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:1709: routing_id_, plugin, active_url,
screen_info_);
As far as I can tell, this doesn't handle DPI changes at run time (this happens
e.g. when you switch between a regular resolution and a hidpi resolution in
system preferences on a lodpi mbp with hidpi resolutions enabled in
quartzdebug).

Is there any advantage in passing through screen_info_ instead of just the
display scale factor? (i don't mind either way, just curious)

[sail, 2012-07-25 18:05:24]

On 2012/07/21 16:52:22, Nico wrote:
> On Thu, Jul 19, 2012 at 11:30 AM,  <mailto:sail@chromium.org> wrote:
> > On 2012/07/19 18:06:45, sail wrote:
> >>
> >> Hey Nico and Josh. My patch doesn't quite work. Here's what I'm seeing:
> >>    - laptop with 2x display connected to an external monitor with 1x
> >> display
> >>    - open a browser window on the external display (any URL, newtab is
> >> fine)
> >>    - RenderWidget::RenderWidget() sets device_scale_factor_ to 2.0.
> >> I would expect that the scale factor would be 1.0. Am I doing something
> >> wrong?
> >
> >
> >> The webpage looks fine but the scale factor end up making the pepper flash
> >> fullscreen twice as big as it should be.
> >
> >
> >> Thanks.
> >
> >
> >
> > Actually, never mind. This looks like an unrelated bug. Navigating to
> > another
> > page correctly changes the display scale factor to 1.0.
> >
> > I can also reproduce the bug outside of fullscreen by opening a page on
> > launch.
> > For example:
> >    - Chromium.app/Content/MacOS/Chromium
> >     - newtab page looks fine
> >     - navigate to
> > http://www.youtube.com/watch?v=bWpK0wsnitc&feature=g-logo-xit,
> > everything looks fine
> >      - quit
> >       - Chromium.app/Contents/MacOS/Chromium
> > http://www.youtube.com/watch?v=bWpK0wsnitc&feature=g-logo-xit
> >       - notice that the webpage is zoomed in at 2x, navigating to another
> > page
> > fixes it
> >
> > Is this a known issue or should I file a bug for this? Thanks.
> 
> Please file.

Looks like there was already a bug filed: http://crbug.com/138039
I added my repro steps to that bug.

[sail, 2012-07-25 18:16:02]

https://chromiumcodereview.appspot.com/10807039/diff/1/content/renderer/rende...
File content/renderer/render_view_impl.cc (right):

https://chromiumcodereview.appspot.com/10807039/diff/1/content/renderer/rende...
content/renderer/render_view_impl.cc:1709: routing_id_, plugin, active_url,
screen_info_);
On 2012/07/21 16:55:15, Nico wrote:
> As far as I can tell, this doesn't handle DPI changes at run time (this
happens
> e.g. when you switch between a regular resolution and a hidpi resolution in
> system preferences on a lodpi mbp with hidpi resolutions enabled in
> quartzdebug).

From what I can tell RenderWidget should get an updated device scale factor when
the DPI changes.

Here's what I tried:
   - on a retina scree at 2x scale factor play a youtube video
   - switch the display to 1x scale factor. Verified that the youtube page still
looks correct
    - switch to fullscreen, verify everything looks correct
    - while still in fullscreen change to 1x scale factor. Verified that the
fullscreen youtube video still looks correct.

> Is there any advantage in passing through screen_info_ instead of just the
> display scale factor? (i don't mind either way, just curious)

RenderWidgetFullscreenPepper derives from RenderWidget which takes a ScreenInfo
object. RenderWidget::RenderWidget() uses the screen info to calculate the
initial device scale factor.

[Nico, 2012-07-25 18:19:55]

Ah, I get it now. LGTM, thanks.

(OnSetDeviceScaleFactor() is called due to the pepper classes being RenderWidget
subclasses, which handles dpi changes)

[commit-bot: I haz the power, 2012-07-25 18:25:36]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10807039/1

[commit-bot: I haz the power, 2012-07-25 18:25:40]

Presubmit check for 10807039-1 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Messages **
If this change requires manual test instructions to QA team, add
TEST=[instructions].

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    content/renderer

Presubmit checks took 1.3s to calculate.

[sail, 2012-07-25 18:48:56]

Hi Antoine, could you do a OWNERS review of this change. It fixes the flapper
fullscreen bug I talked to you about last week. Thanks!

[piman, 2012-07-25 19:14:52]

OWNERS lgtm

[commit-bot: I haz the power, 2012-07-25 19:16:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10807039/1

[commit-bot: I haz the power, 2012-07-25 22:26:19]

Try job failure for 10807039-1 (retry) on win_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-07-25 22:38:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sail@chromium.org/10807039/1

[commit-bot: I haz the power, 2012-07-26 03:47:33]

Change committed as 148508