Mac HiDPI: Fix scale factor on external display

Mac HiDPI: Fix scale factor on external display

On Mac OS 10.8, opening a page on external monitor would cause the page to be rendered at the scale factor of the primary display.

The problem is in RenderViewHostImpl::CreateRenderView. It uses the view to calculate the display scale factor but this doesn't work because the view hasn't been added to a window yet.

My fix is to update the scale factor when -[RenderWidgetHostViewCocoa viewDidMoveToWindow:] is called. This CL also caches the scale factor.

Note, for some reason I can't reproduce the bug on 10.7. On 10.7 the scale factor is also incorrect but for some reason the page is displayed correctly. I'm not sure what's going on.

BUG=138039
TBR=sky

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

[Nico, 2012-08-07 23:36:59]

Should the rwhvmac remember the dpi it last sent to the web, and only do this in
viewDidMoveToWindow: when it changed? that might catch the issue in more cases
and be a bit more robust.

[Avi (use Gerrit), 2012-08-07 23:47:39]

That's a long description...

I'm not sure viewDidMoveToWindow is where we want to put it. Right now we fire
off dpi notifications when views become visible. My question is, should we be
creating views with is_hidden = true, and giving them an initial Show? That's a
deeper change, but if we're really creating them hidden but they don't think
they are, that's a deeper mismatch of reality.

[Nico, 2012-08-07 23:50:55]

On 2012/08/07 23:47:39, Avi wrote:
> That's a long description...
> 
> I'm not sure viewDidMoveToWindow is where we want to put it. Right now we fire
> off dpi notifications when views become visible. My question is, should we be
> creating views with is_hidden = true, and giving them an initial Show? That's
a
> deeper change, but if we're really creating them hidden but they don't think
> they are, that's a deeper mismatch of reality.

I think the underlying issue isn't that we create them hidden, but that we
create them with a nil window. Maybe changing that would be good eventually, but
it sounds like viewDidMoveToWindow: is what's called once the view's window
changes from nil to something real

[Avi (use Gerrit), 2012-08-08 00:18:25]

First, I don't want to add anything to viewDidMoveToWindow. We already override
viewWillMoveToWindow, so if we're going to do anything let's do it there.

Our current approach is apparently broken. The approach is:
- for visible views we fire on a notification
- for hidden views we fire when they're shown

That fails because new views are created "shown", and there's no change once
they're added to the window. That says to me that we should fire when we add a
view to the window, and remove the part where we fire when hidden views are
shown because that would be subsumed by the "adding to the window" bit.

Am I misunderstanding?

[sail, 2012-08-08 00:22:57]

On 2012/08/08 00:18:25, Avi wrote:
> First, I don't want to add anything to viewDidMoveToWindow. We already
override
> viewWillMoveToWindow, so if we're going to do anything let's do it there.

We currently override both will move and did move. I don't think it hurts
anything to post the signal in did move and it makes the code simpler (since we
don't have to pass a window parameter).

> 
> Our current approach is apparently broken. The approach is:
> - for visible views we fire on a notification
> - for hidden views we fire when they're shown
> 
> That fails because new views are created "shown", and there's no change once
> they're added to the window. That says to me that we should fire when we add a
> view to the window, and remove the part where we fire when hidden views are
> shown because that would be subsumed by the "adding to the window" bit.

I can remove the updateTabBackingStoreScaleFactor call from WashShown as a part
of this patch.

> Am I misunderstanding?

[Avi (use Gerrit), 2012-08-08 00:29:34]

On 2012/08/08 00:22:57, sail wrote:
> We currently override both will move and did move.

We do? We do.

> I don't think it hurts
> anything to post the signal in did move and it makes the code simpler (since
we
> don't have to pass a window parameter).

OK.

> I can remove the updateTabBackingStoreScaleFactor call from WashShown as a
part
> of this patch.

If I remember right, the tab removes and reinserts the WebContents from the view
hierarchy when they're hidden/shown, so that if we update on didmove that should
obviate the need for wasshown. I'm not sure off-hand where to verify that memory
in the code.

[sail, 2012-08-08 00:33:49]

Moved to viewDidMoveToWindow.

[Nico, 2012-08-08 00:35:16]

this looks much better imho, thanks

https://chromiumcodereview.appspot.com/10837154/diff/8001/content/browser/ren...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://chromiumcodereview.appspot.com/10837154/diff/8001/content/browser/ren...
content/browser/renderer_host/render_widget_host_view_mac.mm:2996: [self
updateTabBackingStoreScaleFactor];
why only for key views?

[sail, 2012-08-08 00:45:00]

https://chromiumcodereview.appspot.com/10837154/diff/8001/content/browser/ren...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://chromiumcodereview.appspot.com/10837154/diff/8001/content/browser/ren...
content/browser/renderer_host/render_widget_host_view_mac.mm:2996: [self
updateTabBackingStoreScaleFactor];
On 2012/08/08 00:35:17, Nico wrote:
> why only for key views?

Oops. Fixed.

[Nico, 2012-08-08 00:47:52]

lgtm

https://chromiumcodereview.appspot.com/10837154/diff/10001/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://chromiumcodereview.appspot.com/10837154/diff/10001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_mac.mm:2984: if ([self
window] != lastWindow_)
nit: I'd just say "if ([self window])", as lastWindow_ is only updated for when
this is a key view (assuming that this suggested change doesn't affect behavior)

[Avi (use Gerrit), 2012-08-08 01:01:28]

https://chromiumcodereview.appspot.com/10837154/diff/10001/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://chromiumcodereview.appspot.com/10837154/diff/10001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_mac.mm:2052: // active, in
WasShown().
This comment is no longer true. Please fix.

https://chromiumcodereview.appspot.com/10837154/diff/10001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_mac.mm:2984: if ([self
window] != lastWindow_)
Grrr. I really dislike lastWindow_; isn't that what viewWillMove is for?

[sail, 2012-08-09 00:25:18]

Addressed review comments.

I also updated layout_mac.mm/GetScaleFactorScaleForNativeView() so that it's
implementation matches WebScreenInfoFactory.mm/deviceScaleFactor().

I also ran my latest changes on 10.7 and 10.8 and verified that everything works
correctly.

https://chromiumcodereview.appspot.com/10837154/diff/10001/content/browser/re...
File content/browser/renderer_host/render_widget_host_view_mac.mm (right):

https://chromiumcodereview.appspot.com/10837154/diff/10001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_mac.mm:2052: // active, in
WasShown().
On 2012/08/08 01:01:28, Avi wrote:
> This comment is no longer true. Please fix.

Done.

https://chromiumcodereview.appspot.com/10837154/diff/10001/content/browser/re...
content/browser/renderer_host/render_widget_host_view_mac.mm:2984: if ([self
window] != lastWindow_)
On 2012/08/08 00:47:52, Nico wrote:
> nit: I'd just say "if ([self window])", as lastWindow_ is only updated for
when
> this is a key view (assuming that this suggested change doesn't affect
behavior)

Ahh makes sense.

[Nico, 2012-08-09 00:56:16]

lgtm!

[Avi (use Gerrit), 2012-08-09 04:11:50]

lgtm!

[commit-bot: I haz the power, 2012-08-09 04:14:32]

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

[commit-bot: I haz the power, 2012-08-09 04:14:38]

Presubmit check for 10837154-5 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    ui

[sail, 2012-08-09 04:32:20]

+sky for ui/base OWNERS review

[Nico, 2012-08-10 00:54:52]

sky tbr

[commit-bot: I haz the power, 2012-08-10 00:55:06]

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

[commit-bot: I haz the power, 2012-08-10 02:00:50]

Change committed as 150973