799078 - GetContentsScaleFactor should get values directly from the device context instead of guessing

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The current code is not only overly verbose, but gets things wrong when the device context has been set up to use a preffed value instead of widget->GetDefaultScale.

Comment 1

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: Expose nsContentUtils::FindPresShellForDocument

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This code should match the behavior of the code it replaces. However, not knowing enough about how Mac plugin coordinates work with this content scaling factr, it's possible UnscaledAppUnitsPerDevPixel should actually be AppUnitsPerDevPixel. The question is, should full-zoom affect the "content scale factor"? Do things currently behave correctly when you full-zoom in and out on a page with plugins?

Comment 4

[Jonathan Kew [:jfkthame]]

Comment on attachment 669102 [details] [diff] [review]
fix

Review of attachment 669102 [details] [diff] [review]:
-----------------------------------------------------------------

This looks ok to me, but we should wait for smichaud to take a look.

Full-zooming a youtube page running Flash seems to work as expected. A page using the Java plugin (I tried http://jrc313.com/projects/processing/cloth/index.html) doesn't scale so well, but that seems to be a pre-existing problem, not related to the HiDPI stuff; it behaves the same in pre-HiDPI releases. So we should probably file that as a separate issue, if it's not already known.

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +3732,5 @@
>    // pixels.
>  #if defined(XP_MACOSX)
> +  nsIPresShell* presShell = FindPresShellForDocument(mContent);
> +  if (presShell) {
> +    scaleFactor = double(nsPresContext::AppUnitsPerCSSPixel())/

space before the operator, please

Comment 5

[Steven Michaud [:smichaud] (Retired)]

> Full-zooming a youtube page running Flash seems to work as
> expected. A page using the Java plugin (I tried
> http://jrc313.com/projects/processing/cloth/index.html) doesn't
> scale so well

Jonathan, what do you mean by "full zoom"?

Comment 6

[Jonathan Kew [:jfkthame]]

i.e. zooming the page (with cmd-+) *without* having the Zoom Text Only option selected, so that the whole content zooms.

Comment 7

[Jonathan Kew [:jfkthame]]

And what I see with the http://jrc313.com/projects/processing/cloth/index.html page in that case is that the plugin rect gets bigger as expected (shown by the dotted outline when it is clicked), but the content drawn within that rect doesn't grow.

Comment 8

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #7)

I think this is actually correct.  In any case, your example (and most Java applets) work the same way in Safari and Chrome.

Interestingly, there are also some Java applets that zoom with the page:
http://www.eskimo.com/support/java/demo/ArcTest/example1.html

Comment 9

[Jonathan Kew [:jfkthame]]

Fair enough - I'll continue to ignore it, then. :)

Comment 10

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 669102 [details] [diff] [review]
fix

This patch doesn't work (or compile) as is, but the basic idea seems sound.

The previous code doesn't "guess" -- in fact it goes right to the source (the OS) for the information.  But it also doesn't allow this information to be overridden by layout.css.devPixelsPerPx -- which your patch does.

Otherwise (in my tests at least) the two patches are functionally equivalent, which is good.  I tested with current code and also with Jonathan's and my patches for bug 794038, switching between a display in HiDPI mode and another display not in HiDPI mode.

> +  nsIPresShell* presShell = FindPresShellForDocument(mContent);

This needs to be replaced by something like:

nsIPresShell *presShell = nullptr;
nsIDocument *doc = mContent->GetCurrentDoc();
if (doc) {
  presShell = nsContentUtils::FindPresShellForDocument(doc);
}

> + presShell->PresContext()->DeviceContext()->UnscaledAppUnitsPerDevPixel();

This needs to be:

presShell->GetPresContext()->DeviceContext()->UnscaledAppUnitsPerDevPixel();

Comment 11

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 669101 [details] [diff] [review]
Part 1: Expose nsContentUtils::FindPresShellForDocument

> +  return nsnull;

Should be:

return nullptr;

Comment 12

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #3)

> it's possible UnscaledAppUnitsPerDevPixel should actually be
> AppUnitsPerDevPixel. The question is, should full-zoom affect the
> "content scale factor"?

The answer to your question is "no".

Plugin coordinates probably need to be in "display pixels" on all
platforms.  At the very least window sizes and coordinates sent to the
plugin need to be in "display pixels", and not change when you change
from a HiDPI mode to a non-HiDPI mode or zoom the page.

Changing a plugin's size (as the plugin sees it) requires calling
NPP_SetWindow() on the plugin with the new size, and normally requires
the plugin to reinitialize itself (for example stopping any animation
or "movie" the plugin may be playing).

Plugins needn't know anything about HiDPI mode.  So it doesn't make
sense to force the plugin to reinitialize when the HiDPI state
changes.

And it'd be unfortunate if all plugins on a page needed to be
reinitialized every time you changed the page's zoom factor.  In any
case this isn't how Chrome and Safari behave (at least on OS X) --
both browsers preserve a plugin's "plugin coordinates" (as the plugin
sees them) when the page containing the plugin is zoomed.  (You can
see this by testing with my Debug Events Plugin from bug 441880.)

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 669101 [details] [diff] [review]
Part 1: Expose nsContentUtils::FindPresShellForDocument

r=mats

A few style nits:

>content/base/public/nsContentUtils.h
>+  static nsIPresShell *FindPresShellForDocument(nsIDocument *aDoc);

Move the '*' next to the type name.

>   static nsIWidget *WidgetForDocument(nsIDocument *aDoc);

ditto

>content/base/src/nsContentUtils.cpp
>+nsIPresShell *

Remove the space.

>+  if (shell)
>+    return shell;

Missing {}

>+  return nsnull;

nullptr

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Steven Michaud from comment #10)
> nsIPresShell *presShell = nullptr;
> nsIDocument *doc = mContent->GetCurrentDoc();
> if (doc) {
>   presShell = nsContentUtils::FindPresShellForDocument(doc);
> }

Replaced with mContent->GetOwnerDoc() (which is never null).

> > + presShell->PresContext()->DeviceContext()->UnscaledAppUnitsPerDevPixel();
> 
> This needs to be:
> 
> presShell->GetPresContext()->DeviceContext()->UnscaledAppUnitsPerDevPixel();

Thanks!

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0c8ac346e41b
https://hg.mozilla.org/integration/mozilla-inbound/rev/256882fd63c7

Comment 16

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0c8ac346e41b
https://hg.mozilla.org/mozilla-central/rev/256882fd63c7