583109 - Add Visibility notification NPAPI for plugins

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, enhancement)

Description

[Jan Arne Petersen]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US) AppleWebKit/534.3 (KHTML, like Gecko) Chrome/6.0.458.1 Safari/534.3
Build Identifier: 

There is the request to get notification via NPAPI if:
- Plug-in instance goes out of sight after page scrolling.
- Plug-in instance comes in sight after page scrolling.
- Plug-in instance goes out of sight after browser window goes background.
- Plug-in instance comes in sight after browser window goes foreground.

See thread about it at:
https://mail.mozilla.org/pipermail/plugin-futures/2010-July/000123.html

From this discussion it is not clear yet, how that notifications should be done.

Reproducible: Always

Comment 1

[Jan Arne Petersen]

Attachment: Using special clipRect [0, 0, 0, 0] for hidden notification. (WIP)

This patch is based on the proposal for using clipRect to notify about visibility:

https://mail.mozilla.org/pipermail/plugin-futures/2010-July/000131.html

From the discussion in the mailing list it is not clear if clipRect should really be used for that notification.

Currently in this patch is missing notification when tab changed (i need to find use how to get that notification/event) and it currently does not check if the browser shell is completely hidden (need to figure out how to get that info also).

Comment 2

[Karl Tomlinson (:karlt)]

(In reply to comment #1)
> Currently in this patch is missing notification when tab changed (i need to
> find use how to get that notification/event) and it currently does not check if
> the browser shell is completely hidden (need to figure out how to get that info
> also).

Bug 343515 may be helpful here.

Comment 3

[Jan Arne Petersen]

Attachment: Updated patch.

Comment 4

[Jan Arne Petersen]

This patch uses new API from Bug 343515.

To indicate non active (hidden) plugin clipRect value of [0, 0, 0, 0] is sent to plugin.

Comment 5

[Jan Arne Petersen]

@karlt: do you know who could review that patch?

Comment 6

[Karl Tomlinson (:karlt)]

Comment on attachment 465372 [details] [diff] [review]
Updated patch.

(In reply to comment #5)
Try bz on the PresShell changes.
I think I can review the nsObjectFrame changes.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 465372 [details] [diff] [review]
Updated patch.

You need to rev the IIDs of both nsIPresShell and nsIObjectFrame.

SetIsActive doesn't have to be pure-virtual, does it?  It could be just virtual, and the impl can be nsIPresShell::SetIsActive.  mDocument and mIsActive both live on nsIPresShell.

You should probably null-check mDocument before dereferencing it.

With those nits r=me on the presshell bits.

Comment 8

[Karl Tomlinson (:karlt)]

Jan, can you attach a patch with function names and more context please?
Mercurial can be configured to do this automatically as in the [diff] section here:
https://developer.mozilla.org/en/Installing_Mercurial#Configuration

Comment 9

[Jan Arne Petersen]

Attachment: Updated Patch v3

After bug 512260 is landed, I rebased on top of it, which removes most of the PresShell changes. Fixed also some small bugs in the previous patch.

(In reply to comment #7)
> You need to rev the IIDs of both nsIPresShell and nsIObjectFrame.

nsIPresShell is not changed anymore and nsIObjectFrame does not have an IID.

> SetIsActive doesn't have to be pure-virtual, does it?  It could be just
> virtual, and the impl can be nsIPresShell::SetIsActive.  mDocument and
> mIsActive both live on nsIPresShell.

There is a PresShell::SetIsActive from bug 512260 now (it looks like it could have been done there with nsIPresShell::SetIsActive also, but I do not want to change that in this patch).

> You should probably null-check mDocument before dereferencing it.

Done.

(In reply to comment #8)
> Jan, can you attach a patch with function names and more context please?

Done (and thanks for the link).

Comment 10

[Karl Tomlinson (:karlt)]

Comment on attachment 467353 [details] [diff] [review]
Updated Patch v3

For some crazy reason much of Gecko still prefers PRBool to bool.
As there are no other bools in this file, better stick with PRBool.

>+#include "nsIBaseWindow.h"

Is this still needed?

>@@ -996,16 +1000,18 @@ nsObjectFrame::FixupWindow(const nsSize&
> 
>   NPWindow *window;
>   mInstanceOwner->GetWindow(window);
> 
>   NS_ENSURE_TRUE(window, /**/);
> 
> #ifdef XP_MACOSX
>   mInstanceOwner->FixUpPluginWindow(ePluginPaintDisable);
>+#else
>+  mInstanceOwner->UpdateWindowClipRect();
> #endif

FixupWindow will later modifiy window->ClipRect, but see discussion below.

>@@ -1040,16 +1047,18 @@ nsObjectFrame::CallSetWindow()
>       !pi ||
>       NS_FAILED(rv = mInstanceOwner->GetWindow(win)) || 
>       !win)
>     return;
> 
>   nsPluginNativeWindow *window = (nsPluginNativeWindow *)win;
> #ifdef XP_MACOSX
>   mInstanceOwner->FixUpPluginWindow(ePluginPaintDisable);
>+#else
>+  mInstanceOwner->UpdateWindowClipRect();
> #endif

CallSetWindow will call SetWindow again, so it would be better if
UpdateWindowClipRect merely set mPluginWindow->clipRect rather than
also calling SetWindow.

CallSetWindow from DidReflow always follows FixupWindow from Reflow.

The other callers of FixupWindow are the Instantiate methods.  The aMimeType
version calls nsObjectFrame::CallSetWindow.  The nsIChannel version doesn't
seem to call nsObjectFrame::CallSetWindow, but I'm sure SetWindow will get
called somewhere.

There don't seem to be any other CallSetWindow users that matter, so I don't
think CallSetWindow needs to call UpdateWindowClipRect, as FixupWindow would
have been called.

I suggest replacing the window->ClipRect code in FixupWindow with a call to
UpdateWindowClipRect.

For the other UpdateWindowClipRect callers, one solution could be that
UpdateWindowClipRect returns a PRBool to indicate whether the clip rect
changed, so that the other callers can call instance->SetWindow.

> void
> nsObjectFrame::DidSetWidgetGeometry()
> {
> #if defined(XP_MACOSX)
>   if (mInstanceOwner) {
>     mInstanceOwner->FixUpPluginWindow(ePluginPaintEnable);
>   }
>+#else
>+  if (mInstanceOwner) {
>+    mInstanceOwner->UpdateWindowClipRect();
>+  }
> #endif

I don't think this is currently helping at all, because whether the plugin
is visible is currently based solely on root scroll frame position and scroll
position changes are already considered.

A more thorough analysis of visibility could be done by registering even
windowless plugins with this DidSetWidgetGeometry system and picking up
results possibly through PluginHideEnumerator in nsPresContext.cpp.
However, I'm not familiar enough with that code to make recommendations, and
that would be better left for another patch.

>+nsObjectFrame::UpdatePluginActive()
>+{
>+#ifndef MAC_CARBON_PLUGINS
>+  mInstanceOwner->UpdateWindowClipRect();
>+#endif
>+}

Can we be sure mInstanceOwner is non-NULL here?  (I don't know.)

>-  if (!invalidRect && sizeChanged) {
>-    NPRect newClipRect;
>+  NPRect newClipRect;
>+  if (IsWindowHidden()) {
>+    newClipRect.left = 0;
>+    newClipRect.top = 0;
>+    newClipRect.right = 0;
>+    newClipRect.bottom = 0;
>+  } else {
>     newClipRect.left = 0;
>     newClipRect.top = 0;
>     newClipRect.right = window->width;
>     newClipRect.bottom = window->height;

It doesn't seem right to me to give the plugin an empty clip rect to tell the
plugin that it is not visible in NativeImageDraw.  If NativeImageDraw is being
called and has passed the "absPosHeight == 0 || absPosWidth == 0" test then I
assume the plugin is visible.

I don't think the changes to NativeImageDraw are needed.

Similarly Renderer::DrawWithXlib would not be called if the plugin were not
visible.

If you are making a distinction between a plugin visible in a normal window
and a plugin painted through a tab preview or other canvas DrawWindow, then I
think what you want to do is provide the non-empty clip for the paint.
(Requesting a paint with an empty clip doesn't make sense.)  If the clip was
empty before the paint, then, after the paint, check IsWindowHidden and if
hidden do another SetWindow with the empty clip, probably from
nsPluginInstanceOwner::Paint.

>+bool nsPluginInstanceOwner::IsWindowHidden()

There is an nsObjectFrame::IsHidden() which means something different.
How about IsVisible() (with negated return value)? 

>+  // Calculate new clip rectangle
>+  nsRect clip;
>+
>+  return !clip.IntersectRect(r, scrollPortRect);

return r.Intersects(scrollPortRect)

Comment 11

[Karl Tomlinson (:karlt)]

Comment on attachment 467353 [details] [diff] [review]
Updated Patch v3

I think we should get roc to look at nsPluginInstanceOwner::IsWindowHidden().

The analysis there differs from the list of nsIScrollableFrames in Init() but
this might be an acceptably safe upper bound on visibility.
(If not, the scroll position detection could be done in a follow-up patch.)

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+ nsRect r = mObjectFrame->GetRect() + mObjectFrame->GetOffsetTo(rootFrame);

This line is wrong. GetRect() is relative to mObjectFrame's parent.

Checking the scrollport like this is OK, but a better approach would be to register windowless plugins with RegisterPluginForGeometryUpdates. Then nsObjectFrame::ComputeWidgetGeometry will be called every time something changes that might affect the content around the plugin. For windowless plugins, just don't add anything to aConfigurations. You'll know whether you're visible based on whether aRegion is empty or not. With this approach, you'll know if you've been hidden by any kind of clipping, or even if you've been hidden by opaque content over the plugin.

Comment 13

[Jan Arne Petersen]

Attachment: Updated patch v4

I was on vacation, so it took some time for the next patch update.

Modified in this patch:
* Use PRBool instead of bool
* Remove unneeded include
* Only call SetWindow in UpdateWindowClipRect when aSetWindow argument is true
* Rename IsWindowHidden to IsWindowVisible
* Remove UpdateWindowClipRect calls from Draw functions
* Use ComputeWidgetGeometry to check if plugin is hidden or not (instead of checking the scrollport)

Comment 14

[Karl Tomlinson (:karlt)]

Comment on attachment 475485 [details] [diff] [review]
Updated patch v4

This is looking much better, thanks.
I want to look at some things more carefully tomorrow, but we should get Josh's approval on the new clipRect use.

Comment 15

[Karl Tomlinson (:karlt)]

Comment on attachment 475485 [details] [diff] [review]
Updated patch v4

I'm not really clear on how things work now with async painting and AsyncSetWindow.  (Bug 556487 just landed.)  We might need Oleg and/or roc to comment on whether that is an issue here.

My review here is based mainly on sync painting.  I've had a quick look at the
async painting and it feels like things should mostly continue to work if the
plugin doesn't invalidate while hidden, but it's probably worth verifying that
things still work as expected.

roc tells me that, since bug 130078 is now fixed,
EnumerateFreezableElements(UpdateIsActiveElement, nsnull) and
presShell->IsActive() are no longer necessary as
RegisterPluginForGeometryUpdates will provide sufficient notifications.

The header file for for RegisterPluginForGeometryUpdates says "Callers must
call UnregisterPluginForGeometryUpdates before the aPlugin frame is
destroyed."  This is called for windowed plugins and can be called similarly
for windowless plugins.

>+  mPluginWindowVisible = PR_TRUE;

I would initialize this to PR_FALSE.  That would be consistent with the zero
size for windowed plugins in nsPluginInstanceOwner::CreateWidget.  And I think
that would work better because RequestUpdatePluginGeometry is not initially
called (and doesn't need to be).  If the plugin is initially visible, the
clipRect will be set correctly on the first paint, but if the plugin is not
initially visible, I'm not sure that the ComputeWidgetGeometry will get called
to tell the frame that it is not visible.

>+  if (IsWindowVisible()) {
>+    mPluginWindow->clipRect.right = mPluginWindow->width;
>+    mPluginWindow->clipRect.bottom = mPluginWindow->height;
>+  } else {
>+    mPluginWindow->clipRect.right = 0;
>+    mPluginWindow->clipRect.bottom = 0;
>+  }
>+
>+  if (aSetWindow &&
>+      (mPluginWindow->clipRect.left   != oldClipRect.left   ||
>+       mPluginWindow->clipRect.top    != oldClipRect.top    ||
>+       mPluginWindow->clipRect.right  != oldClipRect.right  ||
>+       mPluginWindow->clipRect.bottom != oldClipRect.bottom)) {
>+    mInstance->SetWindow(mPluginWindow);
>+  }

For windowless plugins, I think the clipRect should only be set (and SetWindow
only called) here when not visible.  This is because the clipRect will get set
(probably differently) when the plugin is painted, so setting it here will
cause an additional SetWindow (or maybe two).

For windowed plugins, calling SetWindow both when visible and when not visible
is right. 

>+  mInstanceOwner->UpdateWindowClipRect(PR_FALSE);

>+  mInstanceOwner->UpdateWindowClipRect(PR_FALSE);

I think UpdateWindowClipRect should only need to be called from either
FixupWindow or from CallSetWindow; both should not be necessary.
AFAICS calling from FixupWindow should be sufficient.

Comment 16

[Jan Arne Petersen]

Attachment: Updated patch v5

Just use RegisterPluginForGeometryUpdates for this patch. It works fine in firefox but it is broken in mobile. But I guess we should rather fix mobile to get the correct RegisterPluginForGeometryUpdates calls also instead of using EnumerateFreezableElements(UpdateIsActiveElement, nsnull) and
presShell->IsActive() directly.

Also removed unneeded UpdateWindowClipRect call in CallSetWindow, unneeded clip updates for windowless plugins and added UnregisterPluginForGeometryUpdates call for windowless plugins.

Added Oleg for feedback about issues regarding this patch and async painting.

Comment 17

[Karl Tomlinson (:karlt)]

Comment on attachment 476554 [details] [diff] [review]
Updated patch v5

>+void nsPluginInstanceOwner::UpdateWindowClipRect(PRBool aSetWindow)
>+{
>+  if (!mPluginWindow || !mInstance || !mObjectFrame)
>+    return;

I don't think the mObjectFrame test is necessary.

>+  const NPRect oldClipRect = mPluginWindow->clipRect;
>+
>+  mPluginWindow->clipRect.left = 0;
>+  mPluginWindow->clipRect.top = 0;
>+
>+  if (aSetWindow && !mWidget && mPluginWindowVisible)
>+    return;

Move the early return to before the oldClipRect declaration/initialization.
If the window clipRect were to be modified here, the plugin would need to be told about the change, as change won't necessarily be detected later.

Can you add a comment explaining that a non-empty clip rect will be passed to SetWindow during Paint, please?

I fear that some async-painting integration is going to be necessary, but that's probably best done in a separate patch, so I'll mark this patch r+ assuming the above changes.

We still need to get Josh's approval on the plugin API.

Comment 18

[Jan Arne Petersen]

Attachment: Updated patch v6

Fixed small remaining issues.

Comment 19

[Oleg Romashin (:romaxa)]

Comment on attachment 476729 [details] [diff] [review]
Updated patch v6


>+
>+  if (!aSetWindow)
>+    return;
>+
>+  if ((mPluginWindow->clipRect.left   != oldClipRect.left   ||
>+       mPluginWindow->clipRect.top    != oldClipRect.top    ||
>+       mPluginWindow->clipRect.right  != oldClipRect.right  ||
>+       mPluginWindow->clipRect.bottom != oldClipRect.bottom)) {
>+    mInstance->SetWindow(mPluginWindow);

Don't call setwindow directly... because this will not work with layers.

it is better to call "CallSetWindow"
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#1124

Also I'm not sure how often we going to call setWindow (UpdateClip) when plugin on the border of screen.... are we going to have thousands SetWindow calls?

Comment 20

[Oleg Romashin (:romaxa)]

Comment on attachment 476729 [details] [diff] [review]
Updated patch v6

patch need to be updated

Comment 21

[Jan Arne Petersen]

Attachment: Updated patch v7

Fixed according to Oleg's feedback.

Comment 22

[Jan Arne Petersen]

(In reply to comment #19)
> Also I'm not sure how often we going to call setWindow (UpdateClip) when plugin
> on the border of screen.... are we going to have thousands SetWindow calls?

SetWindow is only called once when there is a change in the clipt rect. So there will only be thousands of calls when the plugin becomes visible/invisible thousand times.

Comment 23

[Karl Tomlinson (:karlt)]

Comment on attachment 476859 [details] [diff] [review]
Updated patch v7

>+  // passed to the plugian during paint, an additional update

plugin

(In reply to comment #19)
> Don't call setwindow directly... because this will not work with layers.
> 
> it is better to call "CallSetWindow"
> http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsObjectFrame.cpp#1124

"CallSetWindow" is probably a poor name as it does a bunch of extra
calculations that are not necessary.  We don't really want to reset offsets
and drawable id here.  I'd be inclined to

if (UseLayers()) mInstance->AsyncSetWindow(mPluginWindow);
else mInstance->SetWindow(mPluginWindow);

(formatted according to file style)
Maybe this could be in an nsPluginInstanceOwner::SetWindow method.
I don't really mind.

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to comment #16)
> Just use RegisterPluginForGeometryUpdates for this patch. It works fine in
> firefox but it is broken in mobile. But I guess we should rather fix mobile to
> get the correct RegisterPluginForGeometryUpdates calls also instead of using
> EnumerateFreezableElements(UpdateIsActiveElement, nsnull) and
> presShell->IsActive() directly.

I'm not clear what to recommend for mobile, so I'm just throwing out ideas here.

I don't know whether RootPresContext can know whether the browser front-end is going to drawWindow any particular portion of the page.

Does the Fennec still use nsIObjectFrameSetAbsoluteScreenPosition?  Maybe that can be used as a hint.

OTOH, this problem seems similar to the decision on whether to retain layers for offscreen documents.  Has that problem been solved?

Comment 25

[Robert O'Callahan (:roc) (email my personal email if necessary)]

No. But mobile isn't drawWindowing everything anymore.

Comment 26

[Oleg Romashin (:romaxa)]

Comment on attachment 476859 [details] [diff] [review]
Updated patch v7

Ok, this looks ok, but fix karl comments.

Comment 27

[Oleg Romashin (:romaxa)]

> > Just use RegisterPluginForGeometryUpdates for this patch. It works fine in

Don't understand how this will work with windowless plugins... try force opaque mode... and pobably it will stop working for firefox too

For non-layers mode (less interested) we should use scroll-Listener (same as mac-osx)

For layers I think it is enough to track ::BuildLayer 
  nsRect area = GetContentRect() + aBuilder->ToReferenceFrame(GetParent());
  gfxRect r = nsLayoutUtils::RectToGfxRect(area, PresContext()->AppUnitsPerDevPixel());

area.... I believe it should be changed when plugins are changed...

But I also believe that layer-manager  should provide some notification for layers overflow state changes... which would help to drop currently invisible surfaces and optimize memory usage

Comment 28

[Jan Arne Petersen]

Attachment: Updated patch v8

Fix typo in comment and use a new more lightweight function nsPluginInstanceOwner::SetWindow() instead of nsObjectFrame::CallSetWindow().

Comment 29

[Doug Turner (:dougt)]

Jan/Oleg - what is the status of this -- do we need both patches?  Do we haves tests

Comment 30

[Jan Arne Petersen]

(In reply to comment #29)
> Jan/Oleg - what is the status of this -- do we need both patches?  Do we haves
> tests

v7 was obsolete (forgot to mark it).There are no tests yet.

Comment 31

[Doug Turner (:dougt)]

jan, are there tests that we can run to verify this doesn't regress anything?

Comment 32

[Jan Arne Petersen]

We should maybe run plugin tests. To verify there is no regression.

Comment 33

[Jan Arne Petersen]

Is there some test-suite which could be run to test that there is no regressions for plugins with this patch?

Or how can we proceed with this patch?

Comment 34

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think what Doug is saying is that we need to add a mochitest to ensure that this visibility API keeps working. That's pretty easy to do; add support to the nptest plugin to detect these visibility events and an API on the plugin object so that our test JS code can query the plugin to find out what the current visibility status is. See modules/plugin/test/testplugin/. Then add a test to modules/plugin/test/mochitest/.

Comment 35

[Oleg Romashin (:romaxa)]

no, Doug only care about patches status, and how safe they are...

To make sure that patch not breaking anything Jan send this patch to try server:
https://wiki.mozilla.org/ReleaseEngineering/TryServer
I hope we have some tests which are testing window->clipRect data for Mac...

In order to make sure that exactly this functionality will not be broken in future we should create some new test, similar to this one http://mxr.mozilla.org/mozilla-central/source/modules/plugin/test/mochitest/test_painting.html?force=1
but more simple and only for clip/visibility testing. follow to comment 34.

Test changes should be in different path/bug, but first try server.

Comment 36

[Benjamin Smedberg]

I do not want this patch to land without tests. The plugin code is already fragile enough without accidentally breaking this sort of new API.

AFAICT the testplugin doesn't test the bitmap-drawing codepaths at all...

Comment 37

[Benjamin Smedberg]

Attachment: v8, updated

Comment 38

[Benjamin Smedberg]

Attachment: Test

I got this test to pass locally with async plugins on Windows: I'm running it through tryserver to see what we come up with. It has one timeout that I don't like, waiting for the visible->invisible reflow to happen. Karl, suggestions for a deterministic way to wait for that reflow are welcome.

Comment 39

[Benjamin Smedberg]

Attachment: Propagate API additions for asynchronous plugins, rev. 1

Comment 40

[Benjamin Smedberg]

Comment on attachment 487944 [details] [diff] [review]
Propagate API additions for asynchronous plugins, rev. 1

This patch is not sufficient. There are some weird ordering issues with nsObjectFrame::BuildLayer and nsDisplayPlugin::GetWidgetConfiguration (we build a layer for a plugin before we inform it that it is visible).

Comment 41

[Benjamin Smedberg]

I'm getting unusual/bad results for this on Windows in some tests. When loading some pages such as file:///c:/builds/async-plugins/src/modules/plugin/test/reftest/plugin-alpha-zindex.html

The first time we try to paint the plugin, nsPluginInstanceOwner.mPluginWindowVisible is false (the default state). This is because we paint/build the layer, *then* call nsPluginInstanceOwner::UpdateWindowVisibility at this stack:

>	xul.dll!nsObjectFrame::ComputeWidgetGeometry(aRegion={...}, aPluginOrigin={...}, aConfigurations=0x0023c7d0)  Line 1301	C++
 	xul.dll!nsDisplayPlugin::GetWidgetConfiguration(aBuilder=0x0023c460, aConfigurations=0x0023c7d0)  Line 1295	C++
 	xul.dll!RecoverPluginGeometry(aBuilder=0x0023c460, aList=0x071e9c0c, aClosure=0x0023c760)  Line 2478	C++
	xul.dll!nsRootPresContext::GetPluginGeometryUpdates(aChangedSubtree=0x05033ba0, aConfigurations=0x0023c7d0)  Line 2550	C++
 	xul.dll!nsRootPresContext::UpdatePluginGeometry()  Line 2644	C++
 	xul.dll!PresShell::DidPaint()  Line 7408	C++
 	xul.dll!nsViewManager::CallDidPaintOnObservers()  Line 1679	C++
 	xul.dll!nsViewManager::DispatchEvent(aEvent=0x0023cbb4, aView=0x050040f8, aStatus=0x0023c91c)  Line 956	C++
 	xul.dll!AttachedHandleEvent(aEvent=0x0023cbb4)  Line 194	C++
 	xul.dll!nsWindow::DispatchEvent(event=0x0023cbb4, aStatus=nsEventStatus_eIgnore)  Line 3499	C++
 	xul.dll!nsWindow::DispatchWindowEvent(event=0x0023cbb4)  Line 3526	C++
 	xul.dll!nsWindow::OnPaint(aDC=0x00000000, aNestingLevel=0x00000000)  Line 777	C++
 	xul.dll!nsWindow::ProcessMessage(msg=0x0000000f, wParam=0x00000000, lParam=0x00000000, aRetValue=0x0023d38c)  Line 4748	C++
 	xul.dll!nsWindow::WindowProcInternal(hWnd=0x003e1b88, msg=0x0000000f, wParam=0x00000000, lParam=0x00000000)  Line 4339	C++
 	xul.dll!nsWindow::WindowProc(hWnd=0x003e1b88, msg=0x0000000f, wParam=0x00000000, lParam=0x00000000)  Line 4291	C++

This leaves at least a very noticeable delay painting the plugin compared with the rest of the content.

Comment 42

[Karl Tomlinson (:karlt)]

(In reply to comment #38)
> It has one timeout that I don't
> like, waiting for the visible->invisible reflow to happen. Karl, suggestions
> for a deterministic way to wait for that reflow are welcome.

Given the asynchronous nature of the plugin notification, the best option I can think of is to repeatedly wait and poll isVisible until the expected state is reached (or some absurdly large time or number of iterations is reached).
(That's the approach I took for attachment 488410 [details] [diff] [review].)

Comment 43

[Benjamin Smedberg]

Attachment: Test with both visibility: hidden and tab-switching

Comment 44

[Benjamin Smedberg]

Attachment: Factor async plugin painting to deal correctly with visibility notifications, rev. 2

This patch is a bit more complicated than I would like, but it works really well and passes all tests.

When the OOPP receives AsyncSetWindow for an invisible plugin, it doesn't ever paint. When it receives AsyncSetWindow for a visible plugin, it starts sending paint updates immediately. The PaintFinished event is no longer required (we can start sending paints before the layer is built in some cases).

The geometry notifications are fired after painting, so when we receive either a paint call or a BuildLayer call, we automatically switch back to "visible" mode.

Comment 45

[Karl Tomlinson (:karlt)]

Comment on attachment 488463 [details] [diff] [review]
Test with both visibility: hidden and tab-switching

>+    p.style.visibility = 'hidden';
>+
>+    // Can we know exactly when the reflow/AsyncSetWindow actually happen?
>+    setTimeout(part4, 500);

If there were a reflow pending, we could force it with getBoundingClientRect
or similar, but visibility changes should not cause a reflow, and that would
not deal with the asynchronous plugin notification.  The only reliable methods
are polling as indicated in comment 42 or providing a "hiddenscript" or
"setwindowscript" argument to the plugin.

>+      is(plugin1.getPaintCount(), 1 * doubleForDoublePass(),
>+         "Plugin in tab 1 should have painted once.");

I don't follow why the plugin must have painted at this point.
(I wouldn't expect nsIWebProgressListener to care about painting.)

>+bool isVisible(NPObject* npobj, const NPVariant* args, uint32_t argCount, NPVariant* result)
>+{
>+  NPP npp = static_cast<TestNPObject*>(npobj)->npp;
>+  InstanceData* id = static_cast<InstanceData*>(npp->pdata);
>+
>+  BOOLEAN_TO_NPVARIANT(id->window.clipRect.top != 0 ||
>+		       id->window.clipRect.left != 0 ||
>+		       id->window.clipRect.bottom != 0 ||
>+		       id->window.clipRect.right != 0, *result);

I think it would make more sense to think of an empty rectangle as invisible,
irrespective of position.

i.e. isVisible would be
clipRect.top != clipRect.bottom && clipRect.left != clipRect.right

Comment 46

[Karl Tomlinson (:karlt)]

Comment on attachment 488465 [details] [diff] [review]
Factor async plugin painting to deal correctly with visibility notifications, rev. 2

(In reply to comment #44)
> When the OOPP receives AsyncSetWindow for an invisible plugin, it doesn't ever
> paint. When it receives AsyncSetWindow for a visible plugin, it starts sending
> paint updates immediately.

Looks good.

> The PaintFinished event is no longer required (we
> can start sending paints before the layer is built in some cases).

There are some advantages in this, though there are also disadvantages in
letting the plugin paint more than is useful.  I guess we can try this and see
how it goes.

> The geometry notifications are fired after painting,

I think that's probably related to the notification system being designed
for windowed plugins and complications of moving windows during painting.

> so when we receive either a paint call or a BuildLayer call, we
> automatically switch back to "visible" mode.

I wonder whether this might happen during a tab preview or similar.  The
effect might be that the plugin gets told it is visible, even though it
doesn't really need to paint for some time.  However, I think that problem
already existed with sync painting, and I don't really have any ideas re when
to tell the plugin again that it is hidden.

>-        mPendingForcePaint = true;

Please remove now unused mPendingForcePaint.

>+    if (mWindow.clipRect.top != aWindow.clipRect.top ||
>+        mWindow.clipRect.left != aWindow.clipRect.left ||
>+        mWindow.clipRect.bottom != aWindow.clipRect.bottom ||
>+        mWindow.clipRect.right != aWindow.clipRect.right)
> 
>     mWindow.x = aWindow.x;
>     mWindow.y = aWindow.y;

Something's not right here.
I assume the test is unnecessary.

>+    bool IsVisible() {
>+        return mWindow.clipRect.top != 0 ||
>+            mWindow.clipRect.left != 0 ||
>+            mWindow.clipRect.bottom != 0 ||
>+            mWindow.clipRect.right != 0;
>+    }
>+

As above, an empty rectangle seems a more intuitive indicator of hidden state.

>+#if 0
>   nsCOMPtr<nsIPluginInstance> pi;
>   mInstanceOwner->GetInstance(*getter_AddRefs(pi));
>   // Give plugin info about layer paint
>   if (pi) {
>     if (NS_FAILED(pi->NotifyPainted())) {
>       return nsnull;
>     }
>   }
>+#endif

Don't forget to remove this.

>         if (reinterpret_cast<HDC>(window->window) != hdc ||
>             window->x != dest.left || window->y != dest.top) {
>           window->window = hdc;
>           window->x = dest.left;
>           window->y = dest.top;
>+          window->clipRect.left = 0;
>+          window->clipRect.top = 0;
>+          // if we're painting, we're visible.
>+          window->clipRect.right = window->width;
>+          window->clipRect.bottom = window->height;

>+          // changes, but the hdc probably changes on every paint.

I wonder whether that's still the case with retained layers, because that's
what is required so that this block is getting executed and the clipRect
updated.  It would be clearer to either test clipRect, or always call SetWindow.

Comment 47

[Benjamin Smedberg]

As far as I know, the [0,0,0,0] rect is special according to existing NPAPI usage on mac and is therefore the correct test...

Comment 48

[Benjamin Smedberg]

I believe I have addressed the review comments with these commits:

http://hg.mozilla.org/projects/maple/rev/d12c46b9e27f (code)
http://hg.mozilla.org/projects/maple/rev/949de4d79ea9 (tests)

I've left the full 0,0,0,0 check in because I believe that is what is specified in the NPAPI discussion.

Comment 49

[Karl Tomlinson (:karlt)]

(In reply to comment #48)
> I believe I have addressed the review comments with these commits:
> 
> http://hg.mozilla.org/projects/maple/rev/d12c46b9e27f (code)

(In reply to comment #46)
> >         if (reinterpret_cast<HDC>(window->window) != hdc ||
> >             window->x != dest.left || window->y != dest.top) {
> >           window->window = hdc;
> >           window->x = dest.left;
> >           window->y = dest.top;
> >+          window->clipRect.left = 0;
> >+          window->clipRect.top = 0;
> >+          // if we're painting, we're visible.
> >+          window->clipRect.right = window->width;
> >+          window->clipRect.bottom = window->height;
> 
> >+          // changes, but the hdc probably changes on every paint.
> 
> I wonder whether that's still the case with retained layers, because that's
> what is required so that this block is getting executed and the clipRect
> updated.  It would be clearer to either test clipRect, or always call
> SetWindow.

My comment here was about the
  "if (reinterpret_cast<HDC>(window->window) != hdc ||
       window->x != dest.left || window->y != dest.top)"
block, not the "if (mWindowlessRect != winlessRect)" block.

The mWindowlessRect != winlessRect test was correct AFAIK, but will only be executed if the test on the outer block passes.

What I'm concerned about is that the new clipRect won't be provided if the test on the outer block does not pass.

Comment 50

[Karl Tomlinson (:karlt)]

(In reply to comment #47)
> As far as I know, the [0,0,0,0] rect is special according to existing NPAPI
> usage on mac and is therefore the correct test...

I don't know any references for existing usage on Mac.

First mention of the empty rect I see is here:
https://mail.mozilla.org/pipermail/plugin-futures/2010-July/000129.html
and confirmed here:
https://mail.mozilla.org/pipermail/plugin-futures/2010-July/000143.html

The notion of [0,0,0,0] being special was introduced here AFAICT:
https://mail.mozilla.org/pipermail/plugin-futures/2010-July/000163.html

I don't know that it really matters, though.

Comment 51

[Karl Tomlinson (:karlt)]

(In reply to comment #48)
> http://hg.mozilla.org/projects/maple/rev/949de4d79ea9 (tests)

Looks good, thanks.

Comment 52

[Karl Tomlinson (:karlt)]

... although part2 of xulbrowser_plugin_visibility.xul really should have the same treatment.

Comment 53

[Karl Tomlinson (:karlt)]

If for part2, you wait for plugin2.getPaintCount(), I think it would be safe to assume that the isVisible()s have changed.

Comment 54

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/24a2e0ac428f
http://hg.mozilla.org/mozilla-central/rev/6ff3fcbb7845
http://hg.mozilla.org/mozilla-central/rev/df0cf7e8e9e6
http://hg.mozilla.org/mozilla-central/rev/d12c46b9e27f
http://hg.mozilla.org/mozilla-central/rev/949de4d79ea9
http://hg.mozilla.org/mozilla-central/rev/cff6e330a3de
http://hg.mozilla.org/mozilla-central/rev/30b383d73524

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

So because of this PluginInstanceChild::InvalidateRectDelayed doesn't 
always clear mCurrentInvalidateTask, and if I read the code right, 
that may point to a deleted object later.

Comment 56

[Olli Pettay [:smaug][[e-mail]]]

tn got this crash http://crash-stats.mozilla.com/report/index/bp-ec73d190-31a7-4e5b-be46-d725f2101114

Comment 57

[Benjamin Smedberg]

That last bit is filed/fixed in bug 611593