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

RESOLVED FIXED in mozilla19

Status

()

Core
Plug-ins
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: roc, Assigned: roc)

Tracking

18 Branch
mozilla19
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

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.
Created attachment 669101 [details] [diff] [review]
Part 1: Expose nsContentUtils::FindPresShellForDocument
Assignee: nobody → roc
Attachment #669101 - Flags: review?
Attachment #669101 - Flags: review? → review?(matspal)
Attachment #669102 - Flags: review?(jfkthame)
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 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
Attachment #669102 - Flags: review?(jfkthame) → review+
> 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"?
i.e. zooming the page (with cmd-+) *without* having the Zoom Text Only option selected, so that the whole content zooms.
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.
(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
Fair enough - I'll continue to ignore it, then. :)
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();
Attachment #669102 - Flags: review?(smichaud) → review+
Comment on attachment 669101 [details] [diff] [review]
Part 1: Expose nsContentUtils::FindPresShellForDocument

> +  return nsnull;

Should be:

return nullptr;
(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 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
Attachment #669101 - Flags: review?(matspal) → review+
(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!
https://hg.mozilla.org/mozilla-central/rev/0c8ac346e41b
https://hg.mozilla.org/mozilla-central/rev/256882fd63c7
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.