Implement ContentsScaleFactor for HiDPI (Retina) displays

RESOLVED FIXED

Status

()

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

People

(Reporter: BenWa, Assigned: BenWa)

Tracking

(Blocks: 1 bug)

Trunk
x86
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
https://wiki.mozilla.org/NPAPI:ContentsScaleFactor
(Assignee)

Comment 1

5 years ago
Created attachment 632314 [details] [diff] [review]
Stub

There's no arm in landing in stubs so that plugins can safely query this.
Assignee: nobody → bgirard
Status: NEW → ASSIGNED
Attachment #632314 - Flags: review?(smichaud)
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;
}
Attachment #632314 - Flags: review?(smichaud) → review-
(Assignee)

Comment 3

5 years ago
(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.
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;
}
(Assignee)

Comment 5

5 years ago
(In reply to Steven Michaud from comment #4)

You're right. I made the change quickly and it went in the wrong place.
Blocks: 785330
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?
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

5 years ago
(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?
Yes.
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.