Last Comment Bug 763918 - Implement ContentsScaleFactor for HiDPI (Retina) displays
: Implement ContentsScaleFactor for HiDPI (Retina) displays
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Plug-ins (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal with 11 votes (vote)
: ---
Assigned To: Benoit Girard (:BenWa)
:
Mentors:
Depends on:
Blocks: osx-hidpi 674373
  Show dependency treegraph
 
Reported: 2012-06-12 07:25 PDT by Benoit Girard (:BenWa)
Modified: 2012-10-06 15:03 PDT (History)
22 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Stub (1.44 KB, patch)
2012-06-12 10:37 PDT, Benoit Girard (:BenWa)
smichaud: review-
Details | Diff | Splinter Review

Description Benoit Girard (:BenWa) 2012-06-12 07:25:43 PDT
https://wiki.mozilla.org/NPAPI:ContentsScaleFactor
Comment 1 Benoit Girard (:BenWa) 2012-06-12 10:37:45 PDT
Created attachment 632314 [details] [diff] [review]
Stub

There's no arm in landing in stubs so that plugins can safely query this.
Comment 2 Steven Michaud [:smichaud] (Retired) 2012-06-12 12:14:19 PDT
Comment on attachment 632314 [details] [diff] [review]
Stub

This is	wrong in several ways.

First, there's no point	in making any call from	the browser to the
plugin's NPP_SetValue()	to change NPNVcontentsScaleFactor if the
browser doesn't yet support changing the scale factor.

Second,	your change to PluginInstanceChild.cpp isn't the right way to
implement this.	 You'd need to model your code on how we call a
plugin's NPP_SetValue() to change NPNVprivateModeBool.

So let's just drop your	changes	to PluginInstanceChild.cpp.

Third, the correct way to implement your changes to nsNPAPIPlugin.cpp
is as follows:

case NPNVcontentsScaleFactor: {
  // Until bug 674373 is fixed we only support a scale factor of 1.0.
  *(double*) result = 1.0;

  return NPERR_NO_ERROR;
}
Comment 3 Benoit Girard (:BenWa) 2012-06-12 12:27:46 PDT
(In reply to Steven Michaud from comment #2)
> Comment on attachment 632314 [details] [diff] [review]
> Stub
> 
> This is	wrong in several ways.
> 
> First, there's no point	in making any call from	the browser to the
> plugin's NPP_SetValue()	to change NPNVcontentsScaleFactor if the
> browser doesn't yet support changing the scale factor.
> 

And this patch doesn't to this. It lets the plugin query the scale factor. Which we will unconditionally return 1.0 until it is implemented properly.


> Second,	your change to PluginInstanceChild.cpp isn't the right way to
> implement this.	 You'd need to model your code on how we call a
> plugin's NPP_SetValue() to change NPNVprivateModeBool.
> 

There's no need to call SetValue if we don't change the contentscale during the lifetime of the plugins.

> So let's just drop your	changes	to PluginInstanceChild.cpp.
> 
> Third, the correct way to implement your changes to nsNPAPIPlugin.cpp
> is as follows:
> 
> case NPNVcontentsScaleFactor: {
>   // Until bug 674373 is fixed we only support a scale factor of 1.0.
>   *(double*) result = 1.0;
> 
>   return NPERR_NO_ERROR;
> }

You're right, the return value is incorrect.
Comment 4 Steven Michaud [:smichaud] (Retired) 2012-06-12 12:54:07 PDT
PluginInstanceChild::NPN_SetValue() (where you put your changes) is called (from a plugin, indirectly) to *change* a value in the browser, not to *query* a value.

But I took another look and it seems PluginInstanceChild.cpp does need changes -- just not the ones you made, and not where you made them.

Add the following to XP_MACOSX part of PluginInstanceChild::NPN_GetValue():

case NPNVcontentsScaleFactor: {
  *((double*)aValue) = 1.0;
  return NPERR_NO_ERROR;
}
Comment 5 Benoit Girard (:BenWa) 2012-06-12 13:42:42 PDT
(In reply to Steven Michaud from comment #4)

You're right. I made the change quickly and it went in the wrong place.
Comment 6 Justin Dolske [:Dolske] 2012-09-18 15:36:40 PDT
Is there value in keeping this bug open? Sounds like it's the wrong approach, and bug 785667 will take care of making plugins work for HiDPI? --> INVALID/DUPE?
Comment 7 Steven Michaud [:smichaud] (Retired) 2012-09-18 15:55:52 PDT
I'll "fix" this bug as part of my work at bug 785667 -- I'll return an appropriate value to plugins that query NPNVcontentsScaleFactor.

In fact my current patches already do that.

Though, as best I can tell, the plugins that query this value behave the same regardless of the answer they get, or whether or not the query is recognized.
Comment 8 FX 2012-10-06 14:43:46 PDT
(In reply to Steven Michaud from comment #7)
> I'll "fix" this bug as part of my work at bug 785667 -- I'll return an
> appropriate value to plugins that query NPNVcontentsScaleFactor.

Given that bug 785667 is RESOLVED FIXED, is this one resolved too?
Comment 9 Steven Michaud [:smichaud] (Retired) 2012-10-06 15:03:50 PDT
Yes.

Note You need to log in before you can comment on or make changes to this bug.