Closed Bug 1171182 Opened 9 years ago Closed 8 years ago

Implement browser zoom query in NPAPI

Categories

(Core Graveyard :: Plug-ins, defect)

defect
Not set
normal

Tracking

(firefox41 affected, firefox47 affected, firefox48 fixed)

RESOLVED FIXED
mozilla48
Tracking Status
firefox41 --- affected
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: qdot, Assigned: qdot)

References

Details

Attachments

(2 files, 5 obsolete files)

Plugins using NPAPI should be able to query the current zoom factor of the browser.
Attaches zoom information for the current document to the NPWindow/NPRemoteWindow struct. This works on the assumption that browser zoom will trigger a window reset, so we'll always call SetWindow (either on plugin initialization or browser zoom change), meaning this can just be passed along through that structure instead of having to add new messages to the ipdl protocol.
Tests initial fetch of browser zoom value, as well as value change (triggered by a window.onresize event).
Attachment #8634457 - Attachment is obsolete: true
Attachment #8699544 - Flags: review?(benjamin)
Attachment #8634458 - Attachment is obsolete: true
Attachment #8699545 - Flags: review?(benjamin)
Comment on attachment 8699544 [details] [diff] [review]
Patch 1 (v2) - Browser Zoom Query for NPAPI

Where are these properties documented? I don't see the npapi changes here reflected in the npapi headers (https://github.com/mozilla/npapi-sdk/blob/master/headers/npapi.h).

Is Flash already using these properties, or will they?

I always want to review commit messages, and this doesn't have a commit message yet. Is this API designed for automatic (High DPI) zoom? Or does this change with "manual" zoom as what a user might choose with Control-+/- ?
Attachment #8699544 - Flags: review?(benjamin) → review-
Attachment #8699545 - Flags: review?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #5)
> Where are these properties documented? I don't see the npapi changes here
> reflected in the npapi headers
> (https://github.com/mozilla/npapi-sdk/blob/master/headers/npapi.h).

Ah, ok, didn't realize they needed to be updated there too. Will do that.

> Is Flash already using these properties, or will they?

They will be. I've been working with adobe on this, they've been running test builds against a beta of their plugin.

> I always want to review commit messages, and this doesn't have a commit
> message yet. 

Ah, dumped the patches using the wrong parameters to format-patch, will fix that.

> Is this API designed for automatic (High DPI) zoom? Or does
> this change with "manual" zoom as what a user might choose with Control-+/- ?

This changes manual zoom. HiDPI is handled via ContentScaleFactor, which already works on OS X, and which I'm working on for windows in bug 820831.
Spec posted to

https://wiki.mozilla.org/NPAPI:BrowserZoomLevelQuery

Will bring this up on the plugin-futures list once I get accepted by the moderator.
Converted patch to use NPP_SetValue instead of passing via window traits. We now update the zoom value whenever the plugin frame display list is updated, which should always happen when zoom changes. It's the same code path as content scaling changes, which means the logic stays grouped through the code.

Starting reviews for this with josh to make sure we're on track for NPAPI spec integration, then I'll ask for more reviews from there.
Attachment #8699544 - Attachment is obsolete: true
Attachment #8715410 - Flags: review?(jaas)
Tests for updates of zoom values via NPP_SetValue, as well as fetching values with GetValue.
Attachment #8699545 - Attachment is obsolete: true
Any chance you can look at this soon Josh?
Flags: needinfo?(jaas)
Header update has been merged to npapi-sdk.
Flags: needinfo?(jaas)
Comment on attachment 8715410 [details] [diff] [review]
Patch 1 (v3) - Browser Zoom Query for NPAPI

Good to go from a spec POV.
Attachment #8715410 - Flags: review?(jaas) → review+
Attachment #8715410 - Flags: review?(benjamin)
Attachment #8715414 - Flags: review?(benjamin)
Comment on attachment 8715410 [details] [diff] [review]
Patch 1 (v3) - Browser Zoom Query for NPAPI

>diff --git a/dom/plugins/base/nsNPAPIPluginInstance.cpp b/dom/plugins/base/nsNPAPIPluginInstance.cpp

>+nsresult nsNPAPIPluginInstance::CSSZoomFactorChanged(float aCSSZoomFactor)

style nit, nsresult on its own line

>+{
>+  if (RUNNING != mRunning)
>+    return NS_OK;
>+
>+  PLUGIN_LOG(PLUGIN_LOG_NORMAL, ("nsNPAPIPluginInstance informing plugin of CSS Zoom Factor change this=%p\n",this));
>+
>+  if (!mPlugin || !mPlugin->GetLibrary())
>+    return NS_ERROR_FAILURE;
>+
>+  NPPluginFuncs* pluginFunctions = mPlugin->PluginFuncs();
>+
>+  if (!pluginFunctions->setvalue)
>+    return NS_ERROR_FAILURE;
>+
>+  PluginDestructionGuard guard(this);
>+
>+  NPError error;
>+  double value = static_cast<double>(aCSSZoomFactor);
>+  NS_TRY_SAFE_CALL_RETURN(error, (*pluginFunctions->setvalue)(&mNPP, NPNVCSSZoomFactor, &value), this,
>+                          NS_PLUGIN_CALL_UNSAFE_TO_REENTER_GECKO);
>+  return (error == NPERR_NO_ERROR) ? NS_OK : NS_ERROR_FAILURE;
>+}
>+
> nsresult
> nsNPAPIPluginInstance::GetJSObject(JSContext *cx, JSObject** outObject)
> {
>@@ -1805,6 +1829,16 @@ nsNPAPIPluginInstance::GetContentsScaleFactor()
>   return scaleFactor;
> }
> 
>+float
>+nsNPAPIPluginInstance::GetCSSZoomFactor()
>+{
>+  float zoomFactor = 1.0;
>+  if (mOwner) {
>+    mOwner->GetCSSZoomFactor(&zoomFactor);
>+  }
>+  return zoomFactor;
>+}
>+
> nsresult
> nsNPAPIPluginInstance::GetRunID(uint32_t* aRunID)
> {
>diff --git a/dom/plugins/base/nsNPAPIPluginInstance.h b/dom/plugins/base/nsNPAPIPluginInstance.h
>index 7e425a2..d78fba9 100644
>--- a/dom/plugins/base/nsNPAPIPluginInstance.h
>+++ b/dom/plugins/base/nsNPAPIPluginInstance.h
>@@ -101,6 +101,7 @@ public:
>   nsresult GetDrawingModel(int32_t* aModel);
>   nsresult IsRemoteDrawingCoreAnimation(bool* aDrawing);
>   nsresult ContentsScaleFactorChanged(double aContentsScaleFactor);
>+  nsresult CSSZoomFactorChanged(float aCSSZoomFactor);
>   nsresult GetJSObject(JSContext *cx, JSObject** outObject);
>   bool ShouldCache();
>   nsresult IsWindowless(bool* isWindowless);
>@@ -307,6 +308,9 @@ public:
>   // Returns the contents scale factor of the screen the plugin is drawn on.
>   double GetContentsScaleFactor();
> 
>+  // Returns the css zoom factor of the document the plugin is drawn on.
>+  float GetCSSZoomFactor();
>+
>   nsresult GetRunID(uint32_t *aRunID);
> 
>   static bool InPluginCallUnsafeForReentry() { return gInUnsafePluginCalls > 0; }
>diff --git a/dom/plugins/base/nsPluginInstanceOwner.cpp b/dom/plugins/base/nsPluginInstanceOwner.cpp
>index 28518c5..6167ac0 100644
>--- a/dom/plugins/base/nsPluginInstanceOwner.cpp
>+++ b/dom/plugins/base/nsPluginInstanceOwner.cpp
>@@ -347,6 +347,7 @@ nsPluginInstanceOwner::nsPluginInstanceOwner()
>   mLastScaleFactor = 1.0;
>   mShouldBlurOnActivate = false;
> #endif
>+  mLastCSSZoomFactor = 1.0;
>   mContentFocused = false;
>   mWidgetVisible = true;
>   mPluginWindowVisible = false;
>@@ -3517,17 +3518,6 @@ nsPluginInstanceOwner::SendWindowFocusChanged(bool aIsActive)
> }
> 
> void
>-nsPluginInstanceOwner::ResolutionMayHaveChanged()
>-{
>-  double scaleFactor = 1.0;
>-  GetContentsScaleFactor(&scaleFactor);
>-  if (scaleFactor != mLastScaleFactor) {
>-    ContentsScaleFactorChanged(scaleFactor);
>-    mLastScaleFactor = scaleFactor;
>-   }
>-}
>-
>-void
> nsPluginInstanceOwner::HidePluginWindow()
> {
>   if (!mPluginWindow || !mInstance) {
>@@ -3598,6 +3588,28 @@ nsPluginInstanceOwner::UpdateWindowVisibility(bool aVisible)
> #endif // XP_MACOSX
> 
> void
>+nsPluginInstanceOwner::ResolutionMayHaveChanged()
>+{
>+#ifdef XP_MACOSX
>+  double scaleFactor = 1.0;
>+  GetContentsScaleFactor(&scaleFactor);
>+  if (scaleFactor != mLastScaleFactor) {
>+    ContentsScaleFactorChanged(scaleFactor);
>+    mLastScaleFactor = scaleFactor;
>+  }
>+#endif
>+  float zoomFactor = 1.0;
>+  GetCSSZoomFactor(&zoomFactor);
>+  if (zoomFactor != mLastCSSZoomFactor) {
>+    if (mInstance) {
>+      mInstance->CSSZoomFactorChanged(zoomFactor);
>+    }
>+    mLastCSSZoomFactor = zoomFactor;
>+  }
>+
>+}
>+
>+void
> nsPluginInstanceOwner::UpdateDocumentActiveState(bool aIsActive)
> {
>   PROFILER_LABEL_FUNC(js::ProfileEntry::Category::OTHER);
>@@ -3676,6 +3688,18 @@ nsPluginInstanceOwner::GetContentsScaleFactor(double *result)
>   return NS_OK;
> }
> 
>+void
>+nsPluginInstanceOwner::GetCSSZoomFactor(float *result)
>+{
>+  nsCOMPtr<nsIContent> content = do_QueryReferent(mContent);
>+  nsIPresShell* presShell = nsContentUtils::FindPresShellForDocument(content->OwnerDoc());
>+  if (presShell) {
>+    *result = presShell->GetPresContext()->DeviceContext()->GetFullZoom();
>+  } else {
>+    *result = 1.0;
>+  }
>+}
>+
> void nsPluginInstanceOwner::SetFrame(nsPluginFrame *aFrame)
> {
>   // Don't do anything if the frame situation hasn't changed.
>diff --git a/dom/plugins/base/nsPluginInstanceOwner.h b/dom/plugins/base/nsPluginInstanceOwner.h
>index 69bb08d..d1e16e8 100644
>--- a/dom/plugins/base/nsPluginInstanceOwner.h
>+++ b/dom/plugins/base/nsPluginInstanceOwner.h
>@@ -138,7 +138,6 @@ public:
>   enum { ePluginPaintEnable, ePluginPaintDisable };
> 
>   void WindowFocusMayHaveChanged();
>-  void ResolutionMayHaveChanged();
> 
>   bool WindowIsActive();
>   void SendWindowFocusChanged(bool aIsActive);
>@@ -160,6 +159,7 @@ public:
>   void UpdateWindowVisibility(bool aVisible);
> #endif // XP_MACOSX
> 
>+  void ResolutionMayHaveChanged();
>   void UpdateDocumentActiveState(bool aIsActive);
> 
>   void SetFrame(nsPluginFrame *aFrame);
>@@ -273,6 +273,7 @@ public:
>            const mozilla::widget::CandidateWindowPosition& aPosition);
>   bool RequestCommitOrCancel(bool aCommitted);
> 
>+  void GetCSSZoomFactor(float *result);
> private:
>   virtual ~nsPluginInstanceOwner();
> 
>@@ -323,7 +324,7 @@ private:
>   // True if, the next time the window is activated, we should blur ourselves.
>   bool                                      mShouldBlurOnActivate;
> #endif
>-
>+  double                                    mLastCSSZoomFactor;
>   // Initially, the event loop nesting level we were created on, it's updated
>   // if we detect the appshell is on a lower level as long as we're not stopped.
>   // We delay DoStopPlugin() until the appshell reaches this level or lower.
>diff --git a/dom/plugins/ipc/PPluginInstance.ipdl b/dom/plugins/ipc/PPluginInstance.ipdl
>index ff09c7f..a5381f5 100644
>--- a/dom/plugins/ipc/PPluginInstance.ipdl
>+++ b/dom/plugins/ipc/PPluginInstance.ipdl
>@@ -97,6 +97,8 @@ child:
>   intr NPP_GetValue_NPPVpluginNativeAccessibleAtkPlugId()
>     returns (nsCString plug_id, NPError result);
> 
>+  intr NPP_SetValue_NPNVCSSZoomFactor(double value) returns (NPError result);
>+
>   intr NPP_SetValue_NPNVmuteAudioBool(bool muted) returns (NPError result);
> 
>   intr NPP_HandleEvent(NPRemoteEvent event)
>diff --git a/dom/plugins/ipc/PluginInstanceChild.cpp b/dom/plugins/ipc/PluginInstanceChild.cpp
>index 27b6634..63a74fe 100644
>--- a/dom/plugins/ipc/PluginInstanceChild.cpp
>+++ b/dom/plugins/ipc/PluginInstanceChild.cpp
>@@ -543,6 +543,10 @@ PluginInstanceChild::NPN_GetValue(NPNVariable aVar,
>     }
> #endif /* XP_MACOSX */
> 
>+    case NPNVCSSZoomFactor: {
>+        *static_cast<double*>(aValue) = mCSSZoomFactor;
>+        return NPERR_NO_ERROR;
>+    }
> #ifdef DEBUG
>     case NPNVjavascriptEnabledBool:
>     case NPNVasdEnabledBool:
>@@ -818,6 +822,21 @@ PluginInstanceChild::AnswerNPP_SetValue_NPNVprivateModeBool(const bool& value,
> }
> 
> bool
>+PluginInstanceChild::AnswerNPP_SetValue_NPNVCSSZoomFactor(const double& value,
>+                                                          NPError* result)
>+{
>+    if (!mPluginIface->setvalue) {
>+        *result = NPERR_GENERIC_ERROR;
>+        return true;
>+    }
>+
>+    mCSSZoomFactor = value;
>+    double v = value;
>+    *result = mPluginIface->setvalue(GetNPP(), NPNVCSSZoomFactor, &v);
>+    return true;
>+}
>+
>+bool
> PluginInstanceChild::AnswerNPP_SetValue_NPNVmuteAudioBool(const bool& value,
>                                                           NPError* result)
> {
>diff --git a/dom/plugins/ipc/PluginInstanceChild.h b/dom/plugins/ipc/PluginInstanceChild.h
>index 6ddd751..d049544 100644
>--- a/dom/plugins/ipc/PluginInstanceChild.h
>+++ b/dom/plugins/ipc/PluginInstanceChild.h
>@@ -89,6 +89,8 @@ protected:
>     AnswerNPP_SetValue_NPNVprivateModeBool(const bool& value, NPError* result) override;
>     virtual bool
>     AnswerNPP_SetValue_NPNVmuteAudioBool(const bool& value, NPError* result) override;
>+    virtual bool
>+    AnswerNPP_SetValue_NPNVCSSZoomFactor(const double& value, NPError* result) override;
> 
>     virtual bool
>     AnswerNPP_HandleEvent(const NPRemoteEvent& event, int16_t* handled) override;
>@@ -403,6 +405,7 @@ private:
> #if defined(XP_DARWIN)
>     double mContentsScaleFactor;
> #endif
>+    double mCSSZoomFactor;
>     int16_t               mDrawingModel;
> 
>     NPAsyncSurface* mCurrentDirectSurface;
>diff --git a/dom/plugins/ipc/PluginInstanceParent.cpp b/dom/plugins/ipc/PluginInstanceParent.cpp
>index 34e6547..4d52072 100644
>--- a/dom/plugins/ipc/PluginInstanceParent.cpp
>+++ b/dom/plugins/ipc/PluginInstanceParent.cpp
>@@ -1500,6 +1500,13 @@ PluginInstanceParent::NPP_SetValue(NPNVariable variable, void* value)
> 
>         return result;
> 
>+    case NPNVCSSZoomFactor:
>+        if (!CallNPP_SetValue_NPNVCSSZoomFactor(*static_cast<double*>(value),
>+                                                &result))
>+            return NPERR_GENERIC_ERROR;
>+
>+        return result;
>+
>     default:
>         NS_ERROR("Unhandled NPNVariable in NPP_SetValue");
>         MOZ_LOG(GetPluginLog(), LogLevel::Warning,
>diff --git a/layout/generic/nsPluginFrame.cpp b/layout/generic/nsPluginFrame.cpp
>index 2c04e83..38a12c0 100644
>--- a/layout/generic/nsPluginFrame.cpp
>+++ b/layout/generic/nsPluginFrame.cpp
>@@ -1198,8 +1198,9 @@ nsPluginFrame::BuildDisplayList(nsDisplayListBuilder*   aBuilder,
> #endif
> 
>   if (aBuilder->IsForPainting() && mInstanceOwner) {
>-#ifdef XP_MACOSX
>+    // Update plugin frame for both content scaling and full zoom changes.
>     mInstanceOwner->ResolutionMayHaveChanged();
>+#ifdef XP_MACOSX
>     mInstanceOwner->WindowFocusMayHaveChanged();
> #endif
>     if (mInstanceOwner->UseAsyncRendering()) {
>-- 
>2.5.0.491.g0029c49
>
Attachment #8715410 - Flags: review?(benjamin) → review+
Attachment #8715414 - Flags: review?(benjamin) → review+
Wow. Ok. This ONLY fails on Win7 opt. I have no idea why.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3d39453d3206
This sounds a lot like uninitialized memory/variables and/or mismatched pointer types through NPAPI. So something is reading a double and somebody else expects a float. We've mostly seen this with 1-byte/4-byte bools.
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #17)
> This sounds a lot like uninitialized memory/variables and/or mismatched
> pointer types through NPAPI. So something is reading a double and somebody
> else expects a float. We've mostly seen this with 1-byte/4-byte bools.

It's a little simpler in this case. We're racing the value getting updated in the test, but it only shows up on windows release. If I just set it to loop on the test until it receives the expected value, things work.

However, the test is also not actually testing the SetValue version of this (it called GetValue twice, due to copy/paste accident :( ), so I need to fix that up too.
Have race condition in test fixed, but am now running into very weird problem where only one of the two functions I added to nptest is being called correctly, the other returns false. Have a feeling it has something to do with linking, which function works changes depending on things like ordering in the static function array. Trying to figure that out now, but it's taking a while.
Fixed issues with tests racing on windows, and SetValue test not getting called.
Attachment #8715414 - Attachment is obsolete: true
And while I'm continuing to do things in reverse order, here's the try I ran to verify the tests. Windows 7 took almost 16 hours to run due to the testing backup, so I haven't gotten to retrigger.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=87b043e65e6d&filter-tier=1&filter-tier=2&filter-tier=3
https://hg.mozilla.org/mozilla-central/rev/e32d4804b676
https://hg.mozilla.org/mozilla-central/rev/4a3d5109a73e
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment on attachment 8715410 [details] [diff] [review]
Patch 1 (v3) - Browser Zoom Query for NPAPI

Approval Request Comment
[Feature/regressing bug #]: None
[User impact if declined]: Browser Zoom won't work
[Describe test coverage new/current, TreeHerder]: Mochitests
[Risks and why]: Adobe has NPAPI BrowserZoom integrated in FP21, would like to get that into release ASAP.
[String/UUID change made/needed]: None
Attachment #8715410 - Flags: approval-mozilla-aurora?
Hi Kyle, could you elaborate the test coverage a bit? Was this change tested using Nightly and Adobe FP21 to see if the browser zoom does not degrade the resolution? Please let me know.
Flags: needinfo?(kyle)
Comment on attachment 8715410 [details] [diff] [review]
Patch 1 (v3) - Browser Zoom Query for NPAPI

Still waiting for Kyle to get back to me on the test coverage. Please renominate once you have clarified the test coverage for this uplift.
Attachment #8715410 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora-
It seems to test ok for me, but I've sent another request for testing at Adobe, which I'll wait for before requesting uplift.
Flags: needinfo?(kyle)
(In reply to Kyle Machulis [:kmachulis] [:qdot] from comment #27)
> It seems to test ok for me, but I've sent another request for testing at
> Adobe, which I'll wait for before requesting uplift.

Thank you for the due diligence.
Ok, talked to Adobe, they're having to update flash to accommodate some of the changes that were made in NPAPI before this landed. Once that's done and confirmed, I'll re-request uplift.
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.