Closed Bug 785667 Opened 7 years ago Closed 7 years ago

Make plugins work with HiDPI mode on the Mac

Categories

(Core :: Plug-ins, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: smichaud, Assigned: smichaud)

References

(Blocks 2 open bugs)

Details

Attachments

(4 files, 13 obsolete files)

785.12 KB, image/png
Details
804.17 KB, image/png
Details
719.03 KB, image/jpeg
Details
85.33 KB, patch
smichaud
: review+
Details | Diff | Splinter Review
This bug is spun off from bug 674373.

Currently there are many problems using plugins work properly when the browser supports HiDPI mode.  For example, plugin mouse events have the wrong coordinates (so clicking on a plugin's controls won't activate them).  And some plugins (e.g. Java plugins) display at a reduced size.

For the last week I've been working on a patch to fix these problems (to be applied on top of Jonathan Kew's patches for bug 674373).  I've been making progress, but I have to fight for every inch of it.  I hope to have a preliminary patch that works with at least most plugins by the end of next week.
Assignee: nobody → smichaud
Duplicate of this bug: 781567
Is this change going to use https://wiki.mozilla.org/NPAPI:ContentsScaleFactor, or are we doing all that scaling internally and transparently to the plugin?
(In reply to comment #2)

Yes.  It turns out that Flash, Silverlight and Java all "support" it -- at least they use NPN_GetValue to query its value.

However this doesn't solve the scaling problem with any of these plugins.  So (presumably) we'll have to scale the CGContextRef we pass to plugins in the NPCocoaEventDrawRect event.

I'm working on it.
I finally broke the back of this problem!

I've now got OOP plugins displaying at the correct size, with controls working correctly and HiDPI-level sharpness (even in those plugins that don't support NPNVcontentsScaleFactor).  It was a simple trick, but *very* counterintuitive.  You'll see when I post my patch (probably next Tuesday or Wednesday).

I've still got some things to clean up (including support for non-OOP plugins like Java).  But that should (I hope) be pretty straightforward.
At long last here's my patch.  It applies on top of Jonathan Kew's screen points patch from bug 674373 (updated to current code).

It's not yet complete, and has only been lightly tested.  But I think it should work reasonably well with all plugins, whether run in process or out-of-process.

A tryserver build will follow in a few hours.
For reference, here's Jonathan Kew's screen points patch from bug 674373, updated to current code.

I did as little work as possible updating this patch.  For example, I dealt with the issue mentioned at bug 674373 comment #255 by simply reverting nsDOMWindowUtils::GetScreenPixelsPerCSSPixel to the code from Jonathan's patch.
Attachment #659526 - Attachment description: Patch on top of screenpoints patch for bug 674373 → Patch on top of screenpoints patch for bug 674373, v0.5
Here's a tryserver build made with the patches from comment #5 and comment #6:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-531774024566/try-macosx64/firefox-18.0a1.en-US.mac.dmg

There were a bunch of test failures, so I've definitely got more work to do.
Did some poking around with this build around plugins I could remember were broken. Seems to be working well.
Not sure if it's a problem with this patch, or with Nightly in general, but drop-down lists that provide suggestions (either from Firefox, or via JavaScript) add a bunch of whitespace and make them way too long.
(In reply to comment #9)

I see this too, though not in current nightlies.

But it also happens just with the patch for bug 674373, so it's not caused by my patch.

But I don't see it until I start loading plugins.  So it may still (somehow) be plugin-related.  I'll dig around to see what I can find.
Attached image Plugin content too small? (obsolete) —
Screenshot of a Flickr video player on Reddit. The doodads in the player are all smaller and at different positions than in Chrome (or stock Nighly w/o any hidpi support). Seems there should be some pixel-doubling doing on?
(last comment was with Try build from https://tbpl.mozilla.org/?tree=Try&rev=6b773f738a34)
You mentioned this in email, but I'm having problems clicking on anything in Flash (YouTube, Flicker, Vimeo, various Facebook games). Have Flash 11.4 r402 installed.

Things with a "click anywhere to begin" kind of action will start, but attempting to interact with any specific control doesn't seem no do anything.
Simple demo (with source, which might be useful):
  http://active.tutsplus.com/tutorials/games/create-a-basic-drawing-application-in-flash/
  This is completely non-functional for me, can't interact at all

Helpful demo?

  http://www.zefrank.com/scribbler/

  There's a weird mouse offset here, if I click-and-move to draw in the lower-right
  of the drawing area, the actual drawing appears to the upper-left...

  http://cl.ly/image/3K2e3p131002
  This is me doing so along the lower-right edges of the drawing area. Looks like
  there's some confusion with coords-within-plugin vs coords-within-window.
Justin, you need to test with the tryserver build from comment #7.

I just checked, and it's still available.
> https://tbpl.mozilla.org/?tree=Try&rev=6b773f738a34

This is Jonathan's latest non-screen-points patch from bug 674373, without my plugin patch.

I haven't yet rewritten my patch for that version of Jonathan's patch (only for his older screen points patch).  It (plus a tryserver build) should be available sometime tomorrow.

In the meantime please test with the tryserver build from comment #7.
Oh my. Yes, testing with the right build is infinitely better. I only saw a few small issues after retesting:

* On http://www.zefrank.com/scribbler/, if while drawing (mousedown) you move outside the plugin, you sometimes can't start drawing a new path or there is a multi-second lag after which your line shows up again. Chrome/Safari/Nightly seem ok.

* On a Facebook game (Idle Worship), a two-finger scroll up/down was both making the game do it's zoomin/zoomout view (expected), as well as made the page in the tab scroll up and down (unexpected). Works in Chrome, but ah-ha is broken in Nightly too.
> * On http://www.zefrank.com/scribbler/, if while drawing (mousedown)
>   you move outside the plugin, you sometimes can't start drawing a
>   new path or there is a multi-second lag after which your line
>   shows up again. Chrome/Safari/Nightly seem ok.

I can reproduce this with my latest build (on top of bug 674373's
screen points patch) ... or something very close.  I can't reproduce
it in recent mozilla-central nightlies.

What I actually see is the following:

The lines I draw keep appearing (as soon as I've drawn them) as long
as I don't lift the mouse button.  If I let go the mouse button for
more than a few seconds, the lines I draw (after pressing the mouse
button again) don't appear while I draw them (though they *do* appear
when I unfocus the plugin, by clicking elsewhere on the page).

Very puzzling.  When I unfocus the plugin, all lines that I've drawn
appear in the correct locations.

I probably won't be able to figure this out anytime soon.  Let's hope
the problem disappears when I rewrite my patch for bug 674373's
non-screen-points patch :-)

By the way, I used my Debug Events Plugin from bug 441880 to confirm
that the mouse events (including drag events) we send to plugins have
the correct coordinates.  So that can't be the problem.
Here's a revision of my patch, intended to be applied on top of Jonathan's "cocoa points" patch for bug 674373.

A tryserver build should be available in a few hours.
Attachment #660340 - Attachment is obsolete: true
And here's an equivalent revision of my earlier screenpoints patch.

I'm also doing a tryserver build of this.  But the cocoapoints patch is really the one you should test (since it looks like that version of the patch for bug 674373 is strongly preferred).
Attachment #659526 - Attachment is obsolete: true
Attachment #659527 - Attachment is obsolete: true
(Following up comment #18)

The "scribbler" problem doesn't happen with the combination of Jonathan's and my "cocoapoints" patches.
Here's the tryserver build made with my cocoapoints patch from comment #19:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-79505bfaf1d1/try-macosx64/firefox-18.0a1.en-US.mac.dmg

As with the screenpoints patch, there were lots of test failures.  I suspect I'll have to rewrite a bunch of tests, next week.
I'm currently making a list of all the (apparently) "legitimate" test failures, by running mochitests locally.  That's *very* slow, so it'll be a while.  Once I have the list I'll post it here.

I expect most of these failures will require rewriting the test.  But I think I should postpone asking for review until I've found which failures (if any) are due to problems with my patch(es).
The test plugin fails to load, with the following error:

Windowless mode not supported by the browser, windowed mode not supported by the plugin!

So virtually all plugin tests fail.

I'll be working on this.
Running your tryserver build, Steven - nice stuff, Flash plugins seem so far to be quite happy.
I've noticed that in the sidebar, the text isn't positioned (or sized?) correctly inside things like the search box - e.g. open the History sidebar, see cursor positioning in search box.
Also with the TagSieve extension, certain elements are wrongly-sized under the text.
(In reply to comment #25)

Thanks, Peter, for testing.

I've noticed similar problems, plus another one -- vertical and horizontal scrollbars (and all their elements) are half the size they should be.  These problems also happen without my patch (i.e. just with bug 674373's cocoapoints patch).  I think I know how to fix them -- port bug 674373's screenpoints' patch changes to nsNativeThemeCocoa to the cocoapoints patch.  I'll try this sometime in the next few days.
The "native theme" patch (attachment 635246 [details] [diff] [review]) in bug 674373 is independent of the main hi-dpi content changes; it works with either the screenpoints or cocoapoints patches. If you look at my tryserver builds there, you'll see that it is used in both versions.
(Following up comment #24)

I've found how to get the test plugin loading, and have fixed a few tests.  I'm now tracking down the remainder of the test failures.  I hope to be done with this sometime tomorrow, and to be able to post new patches (and start the review process).
(Following up comment #27)

Oh.  I've just been testing with builds I made from your attachment 660067 [details] [diff] [review] patch ("patch v2, implement HiDPI rendering in Cocoa widget code").  I'll do a build that combines that and the "native theme" patch.  Anything else I need to add? :-)
I don't think so. I'm still battling with multi-screen / mixed-resolution issues there, and hope to have an update soon, but I don't think that will affect your plugins work.
Here's a revision of my cocoapoints patch that fixes practically all of the test failures I saw previously.

And here's a tryserver build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-249d8a884799/try-macosx64/firefox-18.0a1.en-US.mac.dmg

This build now includes the native theme patch (the changes to nsNativeThemeCocoa).

There are still some failures in the plugin reftests.  These happen more often when the tests are run in an HiDPI mode, but also happen (intermittently) in non-HiDPI modes.  Since my patch doesn't change plugin behavior in non-HiDPI modes, I can't figure out why.  Maybe these failures are already happening, but just haven't been noticed yet.

In any case, I don't see why they should block this bug.  If they persist, I'll open a new bug on them.
Attachment #661393 - Attachment is obsolete: true
Attachment #663145 - Flags: review?(bgirard)
Attached image Latest tryserver is broken again (obsolete) —
Sorry, but the latest tryserver actually regresses. Please don't judge me on the choice of YouTube video, I just picked one from the front page.

You can see that the plugins are not filling the area where they should be. This worked in the previous tryserver.
Attachment #663146 - Attachment description: Bug 674373's screen points and native theme patches, updated to current trunk → Bug 674373's cocoapoints and native theme patches, updated to current trunk
Jonathan, please post the URL to your testcase.
(In reply to Steven Michaud from comment #34)
> Jonathan, please post the URL to your testcase.

https://www.youtube.com/watch?v=OL0adkkYIh0&feature=g-logo-xit
https://www.youtube.com/watch?v=tEJcnaFMWQk&feature=g-vrec

Looks like any YouTube video that isn't using HTML 5 is not scaling correctly.
Yes, this build is broken.  It behaves as if it doesn't have my patch at all.

I'll figure it out and post another.
Ok, I'll test it out when you have it :)
Comment on attachment 663145 [details] [diff] [review]
Patch on top of cocoapoints patch for bug 674373, v1.1

Interestingly, the current build (and patch) from comment #31 works fine with Java and my Debug Events Plugin from bug 441880.  It's broken with Flash and Silverlight.

Seems this was triggered by something changing (elsewhere) on trunk sometime this week (since Monday).  I'll need to figure out what, and revise my patch accordingly.

In the meantime I'll cancel my review request.
Attachment #663145 - Flags: review?(bgirard)
(Following up comment #38)

Here's the patch that triggered this problem:
http://hg.mozilla.org/mozilla-central/rev/1b9d7c2c4310

Tomorrow I'll start trying to figure out what to do about this.
Turns out the problem caused by http://hg.mozilla.org/mozilla-central/rev/1b9d7c2c4310 was easy to work around.  Here's a very slightly revised new patch.

And here's a tryserver build made from it, on top of the previous version of Jonathan's cocoapoints patch for bug 674373:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-a31c50ade454/try-macosx64/firefox-18.0a1.en-US.mac.dmg

I've started another build on top of Jonathan's latest cocoapoints patch.  It should be available in a few hours.
Attachment #659573 - Attachment is obsolete: true
Attachment #661397 - Attachment is obsolete: true
Attachment #663145 - Attachment is obsolete: true
Attachment #663146 - Attachment is obsolete: true
Attachment #663150 - Attachment is obsolete: true
Attachment #663551 - Flags: review?(bgirard)
> I've started another build on top of Jonathan's latest cocoapoints patch.  It
> should be available in a few hours.

Here it is:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-f6b0273bb250/try-macosx64/firefox-18.0a1.en-US.mac.dmg
www.apple.com front page seems to render completely jumbled with this build
Nevermind, closed and re-opened the browser, and now it renders fine (except for the low-res image). My apologies for the false alarm.
Oh, OK.

> except for the low-res image

Which low-res image?  I don't see one.
Antonio claims (in email to me) that the entire page at http://www.apple.com is low-res in the build from comment #41.  (Other pages are hi-res.)

I don't see this.  That page (and other pages) look identical to me in the build from comment #41 and Safari.  I tested on a Retina MBP running OS X 10.7.4.

Antonio, please attach screen shots for comparison -- one from Safari and one from my build from comment #41.
Actually I have one problem to report myself:

My Debug Events Plugin from bug 441880 doesn't work running out-of-process (the default) with my build from comment #41, though it works just fine in a build I did (with the same code) locally.  The Debug Events Plugin never receives any NPCocoaEventDrawRect events.

If anybody knows of any plugins that use Core Graphics drawing mode, please test with them.  (Flash, Silverlight and Java all use Core Animation or Invalidating Core Animation drawing mode.)
(Following up comment #46)

Never mind.  I just "fixed" the problem by rebuilding my Launch Services database (/System/Library/Frameworks/CoreServices.framework/Frameworks/LaunchServices.framework/Support/lsregister -kill -r -domain local -domain system -domain user).
Antonio, we need *two* screenshots, one from my build and one from Safari.
We also need others to test with http://www.apple.com.
Here is the requested Safari screenshot.

Also note that I can reproduce the Firefox screenshot with the comment #41 build.  It's definitely not rendering apple.com in high resolution.
Before the patch for bug 564815 landed, we had a reason for Apple's pages not rendering in high resolution.  But it's now landed (as of the 2012-07-08 nightlies).  See bug 674373 comment #76.
> as of the 2012-07-08 nightlies

As of the 2012-09-08 nightlies.
> Also note that I can reproduce the Firefox screenshot with the comment #41 build.
> It's definitely not rendering apple.com in high resolution.

Actually I can now reproduce this too, though I have to zoom in a lot to see the difference between Safari and my comment #41 build.

So I suppose the patch for bug 564815 didn't solve the problem, or didn't solve it fully.

I think this problem is relatively minor, and shouldn't block landing the patch for bug 674373.  (Someone will need to open a new bug on the subject after it lands.)

Also note that this problem has nothing to do with plugins, and therefore nothing to do with my patch for this bug.
The large image, the large text associated with the large image, and the text in the top menu bar all render in low-res. Oddly though, the text in the top menu bar renders in hi-res on all other apple.com pages.
To fix the problem with Apple's pages still displaying at low-res, we may also need to fix bug 765914.
(In reply to Steven Michaud from comment #41)
This is working really well so far after a few days on my MBPr.  I'm going to try and attach a screenshot of one oddity in nested toolbar sizing that might be in the earlier nightly, or not.  I notice the same issue on menus from some add-ons like no script.  Flash seems to be working really well though for me so far, and a huge improvement, thank you.
> http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/smichaud@pobox.com-
> f6b0273bb250/try-macosx64/firefox-18.0a1.en-US.mac.dmg
The one funny thing I can't do with this build is use my mozilla persona, won't click on the button at all.  I have to use Safari to log in here, this is the only time I had to revert to Safari in the last few days of using this latest build.  Ironic.
Everyone:

Don't report problems here that you can also reproduce in current trunk (mozilla-central) nightlies.

Also note that problems that don't involve plugins (like problems with personas), even if they can't be reproduced in current mozilla-central nightlies, are probably caused by the patches for bug 674373 alone.
Comment on attachment 663551 [details] [diff] [review]
Patch on top of cocoapoints patch for bug 674373, v1.2

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

Nice work. I'm a bit concern about handling dynamic switching while you have surfaces being drawn and sent from one process to another so I'd like discuss that case before r+.

::: dom/plugins/base/nsIPluginInstanceOwner.idl
@@ +123,5 @@
> +  /**
> +   * Get the contents scale factor for the screen the plugin is
> +   * drawn on.
> +   */
> +  double getContentsScaleFactor();

I don't see this being used?

::: dom/plugins/base/nsNPAPIPluginInstance.cpp
@@ +1735,5 @@
> +{
> +  nsresult rv = NS_OK;
> +  double scaleFactor = 1.0;
> +  if (mOwner) {
> +    rv = mOwner->GetContentsScaleFactor(&scaleFactor);

NIT: I think it should be up to GetContentsScaleFactor to preserve the value of scaleFactor if it fails. This would save 11 instances of NS_FAILED(rv) scaleFactor = 1.0.

::: dom/plugins/base/nsPluginInstanceOwner.cpp
@@ +3754,5 @@
> +        }
> +      }
> +    }
> +  }
> +  if (scaleFactor <= 0) {

Is this expected during a normal run? If not I would rather have this fail hard so we can catch it early.

::: dom/plugins/ipc/PluginInstanceChild.cpp
@@ +469,5 @@
> +
> +    case NPNVcontentsScaleFactor: {
> +        double v = 1.0;
> +        NPError result;
> +        if (!CallNPN_GetValue_NPNVcontentsScaleFactor(&v, &result)) {

Here we have the child process calling into the parent process. The plugin process may already have a surface at a particular scale factor. Shouldn't we just return the scale factor of the surface the plugin is drawing to? We should already have the constraint that these two will never fall out of sync.

::: dom/plugins/ipc/PluginInstanceParent.cpp
@@ +613,5 @@
> +        double scaleFactor = 1.0;
> +        mNPNIface->getvalue(mNPP, NPNVcontentsScaleFactor, &scaleFactor);
> +
> +        RefPtr<MacIOSurface> newIOSurface =
> +          MacIOSurface::LookupSurface(iodesc.surfaceId(), scaleFactor);

How do we handle dynamic switching of scaleFactor? Here we (seem to?) run the risk of the IOSurface id being of scaleFactor X, the user switches the scaleFactor to Y, GetValue returns Y and we lookup a surface using the value Y mismatching its internal value.

::: gfx/2d/MacIOSurface.h
@@ +36,5 @@
>    IOSurfaceID GetIOSurfaceID();
>    void *GetBaseAddress();
>    size_t GetWidth();
>    size_t GetHeight();
> +  double GetContentsScaleFactor() { return mContentsScaleFactor; }

Can you add a single line to clarify whether width/height in the functions and getters need to be scaled by the content factor or if that's handled internally.

::: gfx/2d/QuartzSupport.mm
@@ +353,5 @@
>    return ioSurface.forget();
>  }
>  
> +TemporaryRef<MacIOSurface> MacIOSurface::LookupSurface(IOSurfaceID aIOSurfaceID,
> +                                                       double aContentsScaleFactor) { 

I don't have like needing to provide a scale factor when looking up a surface. Sadly IOSurfaceSetValue recommends to avoid using it. Maybe we could turn IOSurfaceID into a structure to hide the fact that we need an internal iosurface id+scale pair.

::: layout/generic/test/plugin_focus_helper.html
@@ +33,5 @@
>  
>      var element = document.getElementById(id);
>      var bounds = element.getBoundingClientRect();
> +    var x = (bounds.left + window.mozInnerScreenX + 10);
> +    var y = (bounds.top + window.mozInnerScreenY + 10);

It's not clear to me why we no longer have to scale by screenPixelsPerCSSPixel. Perhaps there should be a comment here.
Attachment #663551 - Flags: review?(bgirard) → review-
(In reply to comment #61)

> How do we handle dynamic switching of scaleFactor? Here we (seem
> to?) run the risk of the IOSurface id being of scaleFactor X, the
> user switches the scaleFactor to Y, GetValue returns Y and we lookup
> a surface using the value Y mismatching its internal value.

You're right, I think.  Let me see what I can do about this.

> Here we have the child process calling into the parent process. The
> plugin process may already have a surface at a particular scale
> factor. Shouldn't we just return the scale factor of the surface the
> plugin is drawing to? We should already have the constraint that
> these two will never fall out of sync.

Let me also see what I can do about this.

I'll post a new patch later today.  When I do I'll answer the rest of
your comments.
> I'll post a new patch later today.

Looks like it'll be tomorrow.
Attached patch Cocoapoints patch v1.4 (obsolete) — Splinter Review
Here's a revised and cleaned up patch.

I followed most of Benoit's suggestions, and added some changes of my own.

With my current patch, we never call from the child process to the parent process to get the current contents scale factor.  Instead I now pass the information from the parent to the child with all calls (and their variants) to NPP_SetWindow and NPP_HandleEvent.

I believe this will accommodate dynamic switching to and from HiDPI mode.  But I can't know for sure until we start working on bug 794038.  It's conceivable we'll need to explicitly call NPP_SetWindow (from the parent) every time the contents scale factor changes.

>:: dom/plugins/base/nsIPluginInstanceOwner.idl
>@@ +123,5 @@
>> +  /**
>> +   * Get the contents scale factor for the screen the plugin is
>> +   * drawn on.
>> +   */
>> +  double getContentsScaleFactor();
>
> I don't see this being used?

It's used by nsPluginInstance::GetContentsScaleFactor().

>::: layout/generic/test/plugin_focus_helper.html
>@@ +33,5 @@
>>  
>>      var element = document.getElementById(id);
>>      var bounds = element.getBoundingClientRect();
>> +    var x = (bounds.left + window.mozInnerScreenX + 10);
>> +    var y = (bounds.top + window.mozInnerScreenY + 10);
>
> It's not clear to me why we no longer have to scale by
> screenPixelsPerCSSPixel. Perhaps there should be a comment here.

nsIDOMWindowUtils.screenPixelsPerCSSPixel was previously almost always used to find the zoom ratio.  These instances have now been replaced by Jonathan's new nsIDOMWindowUtils.zoomRatio.

In the two cases I found where nsIDOMWindowUtils.screenPixelsPerCSSPixel wasn't used for the zoom ratio, I don't think it makes sense to use it at all.  Both are plugin tests, and we always use CSS pixels when "talking" to plugins.

>::: gfx/2d/QuartzSupport.mm
>@@ +353,5 @@
>>    return ioSurface.forget();
>>  }
>>
>> +TemporaryRef<MacIOSurface>
>> +   MacIOSurface::LookupSurface(IOSurfaceID aIOSurfaceID,
>> +                               double aContentsScaleFactor) {
>
> I don't have like needing to provide a scale factor when looking up
> a surface. Sadly IOSurfaceSetValue recommends to avoid using
> it. Maybe we could turn IOSurfaceID into a structure to hide the
> fact that we need an internal iosurface id+scale pair.

We actually aren't using the scale factor to look up an IOSurface --
only the id.  We only use the scale factor to create a MacIOSurface
from the IOSurface we find.

As best I can tell, the only way this can cause trouble is if the
scale factor of the MacIOSurface we create doesn't match the
(implicit) scale factor of the IOSurface we find.  I don't think this
is possible in current code.  And in any case current code doesn't
check that the IOSurface we find has the "right" width or height.
Attachment #663551 - Attachment is obsolete: true
Attachment #664643 - Flags: review?(bgirard)
I'm seeing fairly consistent failures in /tests/dom/plugins/test/test_bug539565-1.html that look related to my patch.  I'll need to address those before my patch lands (probably by revising my patch).
(Following up comment #64)

> I don't have like needing to provide a scale factor when looking up
> a surface. Sadly IOSurfaceSetValue recommends to avoid using
> it.

I didn't see any value for "contents scale factor" among the symbols like kIOSurfaceWidth and kIOSurfaceHeight in the IOSurface framework, which I take to mean that there isn't even any undocumented support for it.

I did consider adding an an "unsupported" value (for contents scale factor) to the CFDictionary we use when calling IOSurfaceCreate() -- one that we could query ourselves.  But this may be risky, and I don't think it's really necessary.

(By the way, I played with changing kIOSurfaceElementHeight and kIOSurfaceElementWidth from their default values of '1'.  But this wasn't helpful.)
Comment on attachment 664643 [details] [diff] [review]
Cocoapoints patch v1.4

The test failures mentioned in comment #65 show some of my v1.4 changes were mistaken.  I'll post another revision soon (this evening or tomorrow).
Attachment #664643 - Flags: review?(bgirard)
Attached patch Cocoapoints patch v1.5 (obsolete) — Splinter Review
Here's another revision of my patch that fixes the test failures described in comment #65.

In its comments, it introduces the notion of "display pixels", which are the smallest fully addressable part of a display (or screen).  In a sane world these are the same as device pixels, but in HiDPI modes they're not.

My previous patch had comments talking about CSS pixels instead of display pixels.  These, too, are usually the same.  But not when (for example) the user zooms the display.  My previous patch assumed they *are* the same, which caused the test failures.

Even in HiDPI modes, most programs (like plugins and simple Cocoa apps) use (and work on) display pixels.  I imagine the same is true of at least some extensions.  If so, we'll need to do something for them similar to what I've done for plugins.

I'd really have liked to use the term "screen pixels" for what I'm calling "display pixels".  But the Mozilla tree already uses (I'd say misuses) "screen pixels" as a synonym for "device pixels".  So (to avoid confusion) I had to come up with another term.
Attachment #664643 - Attachment is obsolete: true
Attachment #664763 - Flags: review?(bgirard)
Attached patch Cocoapoints patch v1.6 (obsolete) — Splinter Review
This revision drops my previous patches' changes to /layout/style/test/test_media_queries.html.

Those changes were unrelated to the rest of my patch, and just today started causing some of those tests to fail.  The tests now pass if left unchanged.

The changes have been in my patches for a while, and didn't previously cause test failures.  I don't know why the situation changed, but I've decided not to worry about it (at least for the time being).
Attachment #664763 - Attachment is obsolete: true
Attachment #664763 - Flags: review?(bgirard)
Attachment #665156 - Flags: review?(bgirard)
(In reply to Steven Michaud from comment #64)
> I believe this will accommodate dynamic switching to and from HiDPI mode. 
> But I can't know for sure until we start working on bug 794038.  It's
> conceivable we'll need to explicitly call NPP_SetWindow (from the parent)
> every time the contents scale factor changes.

There are now some work-in-progress patches at bug 794038 that you could use to test this, I think.
I've discovered an oddball problem with my patch, which is probably ultimately an OS bug:  Plugins that use the Core Graphics drawing model don't display out-of-process (in HiDPI mode) when building with the 10.6 SDK (though they display just fine when building with the 10.7 SDK).

All of the major plugins that I'm aware of use the Core Animation or Invalidating Core Animation drawing models.  So I'm not sure how important this is.

Interestingly, the Core Animation drawing model works even when building with the 10.6 SDK if you don't make the plugin itself use HiDPI.

I'll see if I can find a way to hack around this.
> Interestingly, the Core Animation drawing model works even when building with the
> 10.6 SDK if you don't make the plugin itself use HiDPI.

Of course, here I mean the Core Graphics drawing model.
(In reply to Jonathan Kew (:jfkthame) from comment #71)
> (In reply to Steven Michaud from comment #64)
> > I believe this will accommodate dynamic switching to and from HiDPI mode. 
> > But I can't know for sure until we start working on bug 794038.  It's
> > conceivable we'll need to explicitly call NPP_SetWindow (from the parent)
> > every time the contents scale factor changes.
> 
> There are now some work-in-progress patches at bug 794038 that you could use
> to test this, I think.

I've tried moving a YouTube window with Flash playing between HiDPI and non-HiDPI displays, using the patches from bug 794038, and it does break up after the move, which makes me suspect that "we'll need to explicitly call NPP_SetWindow (from the parent) every time the contents scale factor changes" as suggested above.

(The plugin didn't crash or anything - in fact, zooming it to full-screen on the new display made it render fine again - but at its original size on the web page the display was thoroughly garbled.)
Steven, your work on this is much appreciated. :)

This patch works well for me, except for the following issue: whenever I double-tap to open the context menu of a Flash object and then double-tap outside of the Flash object, the context menu of the Flash object appears for a second time.
(In reply to comment #75)

"Double-tap" doesn't do anything for me -- in Firefox or in Safari.  And I can't find anything (in the Trackpad system pref panel) to turn it on (on either OS X 10.7.5 or 10.8.1).

I can't reproduce what you report with a right-click or a two-finger click.

That said, I've noticed other problems testing with my Debug Events Plugin from bug 441880 that seem to be focus related (and which also happen with only the patches for bug 674373).  At some point I'll open a bug on them.
Attachment #665156 - Flags: review?(bgirard) → review+
My patch has been broken by something that landed on mozilla-central in the last few days.  What's broken is that plugin updating no longer works properly.

I'll need to figure this out and update my patch before I can land it.

Sigh :-(
Perhaps bug 795796 fixes it.
> Perhaps bug 795796 fixes it.

Nope.

Still digging.  I probably won't be able to find a fix until sometime tomorrow.

I'll probably have to tear apart and put back together everything having to do with plugin updates.
Interestingly, the problem also effects the patch for bug 674373 alone ... but not in yesterday's mozilla-central nightly (only today's).

So the regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a680fd777c3b&tochange=beee809b7ade
Here are a couple of Flash "movies" I've been testing with:
http://mirrors.creativecommons.org/
>> Perhaps bug 795796 fixes it.
>
> Nope.

In fact it's the patch for bug 795796 that triggers/causes the problem:
http://hg.mozilla.org/mozilla-central/rev/beee809b7ade

This problem still happens with HiDPI support turned off (with gfx.hidpi.enabled set to '-1').  It doesn't happen on Windows (Windows XP).

I'll open a new bug on this (if one hasn't been opened already).
I've opened bug 796183.
Here's my patch updated to current trunk, with a workaround for the problem described in comment #72.

I'm carrying forward Benoit's r+.
Attachment #665156 - Attachment is obsolete: true
Attachment #667119 - Flags: review+
Comment on attachment 667119 [details] [diff] [review]
Cocoapoints patch v1.7

Landed on mozilla-inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d6f8b964ca27

Bug 796183 still breaks plugin updating (with or without my patch).  But it's big and bad enough that it should get fixed soon.
https://hg.mozilla.org/mozilla-central/rev/d6f8b964ca27
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
This makes me incredibly happy, thank you!
Not wishing to spam this bug, but with the latest Nightly, YouTube and Vimeo videos are showing up as black for me - no video, but audio works fine. I'm using the latest Adobe Flash - 11.4.402.287.
Sounds like you should file a new bug for that.
Sure, just a bit wary of submitting bugs for Nightlies, wanted to check if it's known or not. I'll have a search and do so, thanks.
Why are you wary of submitting bugs for nightlies? I don't think you need to be.
when will this bug fix be backported to firefox stable version?
on  my 13' macbook pro, firefox 17 , firefox 18 still don't work and flash version is 11.6.602.167.
by the way safari, chrome and even firefox nightly all works fine.
(In reply to Darren from comment #92)
> when will this bug fix be backported to firefox stable version?

This bug (general HiDPI support for plugins) is fixed in Fx18, although there were HiDPI related follow-up bugs. 
Please file a new bug with your symptoms if you can't find one specific to what you're seeing.
You need to log in before you can comment on or make changes to this bug.