Closed
Bug 583109
Opened 14 years ago
Closed 14 years ago
Add Visibility notification NPAPI for plugins
Categories
(Core Graveyard :: Plug-ins, enhancement)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla2.0b8
People
(Reporter: jap, Assigned: benjamin)
References
Details
Attachments
(4 files, 9 obsolete files)
(deleted),
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
karlt
:
review+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•14 years ago
|
||
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).
Updated•14 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 2•14 years ago
|
||
(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.
Depends on: 343515
Reporter | ||
Comment 3•14 years ago
|
||
Assignee: nobody → jap
Attachment #461374 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #465372 -
Flags: review?
Reporter | ||
Comment 4•14 years ago
|
||
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.
Reporter | ||
Comment 5•14 years ago
|
||
@karlt: do you know who could review that patch?
Comment 6•14 years ago
|
||
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.
Attachment #465372 -
Flags: review?(karlt)
Attachment #465372 -
Flags: review?(bzbarsky)
Attachment #465372 -
Flags: review?
Comment 7•14 years ago
|
||
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.
Attachment #465372 -
Flags: review?(bzbarsky) → review+
Attachment #465372 -
Flags: review?(joshmoz)
Comment 8•14 years ago
|
||
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
Reporter | ||
Comment 9•14 years ago
|
||
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).
Attachment #465372 -
Attachment is obsolete: true
Attachment #465372 -
Flags: review?(karlt)
Attachment #465372 -
Flags: review?(joshmoz)
Reporter | ||
Updated•14 years ago
|
Attachment #467353 -
Flags: review?(karlt)
Reporter | ||
Updated•14 years ago
|
Attachment #467353 -
Flags: review?(joshmoz)
Comment 10•14 years ago
|
||
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)
Attachment #467353 -
Flags: review?(karlt) → review-
Comment 11•14 years ago
|
||
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.)
Attachment #467353 -
Flags: review?(roc)
+ 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.
Reporter | ||
Comment 13•14 years ago
|
||
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)
Attachment #467353 -
Attachment is obsolete: true
Attachment #475485 -
Flags: review?
Attachment #467353 -
Flags: review?(roc)
Attachment #467353 -
Flags: review?(joshmoz)
Reporter | ||
Updated•14 years ago
|
Attachment #475485 -
Flags: review? → review?(karlt)
Comment 14•14 years ago
|
||
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.
Attachment #475485 -
Flags: review?(joshmoz)
Comment 15•14 years ago
|
||
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.
Attachment #475485 -
Flags: review?(karlt) → review-
Reporter | ||
Comment 16•14 years ago
|
||
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.
Attachment #475485 -
Attachment is obsolete: true
Attachment #476554 -
Flags: review?(karlt)
Attachment #476554 -
Flags: feedback?(romaxa)
Attachment #475485 -
Flags: review?(joshmoz)
Comment 17•14 years ago
|
||
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.
Attachment #476554 -
Flags: review?(karlt)
Attachment #476554 -
Flags: review?(joshmoz)
Attachment #476554 -
Flags: review+
Reporter | ||
Comment 18•14 years ago
|
||
Fixed small remaining issues.
Attachment #476554 -
Attachment is obsolete: true
Attachment #476729 -
Flags: review?(karlt)
Attachment #476729 -
Flags: feedback?(romaxa)
Attachment #476554 -
Flags: review?(joshmoz)
Attachment #476554 -
Flags: feedback?(romaxa)
Reporter | ||
Updated•14 years ago
|
Attachment #476729 -
Flags: review?(joshmoz)
Comment 19•14 years ago
|
||
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•14 years ago
|
||
Comment on attachment 476729 [details] [diff] [review]
Updated patch v6
patch need to be updated
Attachment #476729 -
Flags: feedback?(romaxa) → feedback-
Reporter | ||
Comment 21•14 years ago
|
||
Fixed according to Oleg's feedback.
Attachment #476729 -
Attachment is obsolete: true
Attachment #476859 -
Flags: review?(karlt)
Attachment #476859 -
Flags: feedback?(romaxa)
Attachment #476729 -
Flags: review?(karlt)
Attachment #476729 -
Flags: review?(joshmoz)
Reporter | ||
Updated•14 years ago
|
Attachment #476859 -
Flags: review?(joshmoz)
Reporter | ||
Comment 22•14 years ago
|
||
(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•14 years ago
|
||
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.
Attachment #476859 -
Flags: review?(karlt) → review+
Comment 24•14 years ago
|
||
(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?
No. But mobile isn't drawWindowing everything anymore.
Comment 26•14 years ago
|
||
Comment on attachment 476859 [details] [diff] [review]
Updated patch v7
Ok, this looks ok, but fix karl comments.
Attachment #476859 -
Flags: feedback?(romaxa) → feedback+
Comment 27•14 years ago
|
||
> > 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
Attachment #476859 -
Flags: review?(joshmoz) → review+
Reporter | ||
Comment 28•14 years ago
|
||
Fix typo in comment and use a new more lightweight function nsPluginInstanceOwner::SetWindow() instead of nsObjectFrame::CallSetWindow().
Attachment #477220 -
Flags: review?(karlt)
Updated•14 years ago
|
Attachment #477220 -
Flags: review?(karlt) → review+
Comment 29•14 years ago
|
||
Jan/Oleg - what is the status of this -- do we need both patches? Do we haves tests
Reporter | ||
Updated•14 years ago
|
Attachment #476859 -
Attachment is obsolete: true
Reporter | ||
Comment 30•14 years ago
|
||
(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•14 years ago
|
||
jan, are there tests that we can run to verify this doesn't regress anything?
Reporter | ||
Comment 32•14 years ago
|
||
We should maybe run plugin tests. To verify there is no regression.
Reporter | ||
Comment 33•14 years ago
|
||
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?
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•14 years ago
|
||
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.
Assignee | ||
Comment 36•14 years ago
|
||
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...
Flags: in-testsuite?
Assignee | ||
Comment 37•14 years ago
|
||
Assignee | ||
Comment 38•14 years ago
|
||
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.
Assignee | ||
Comment 39•14 years ago
|
||
Attachment #487944 -
Flags: review?(karlt)
Assignee | ||
Comment 40•14 years ago
|
||
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).
Attachment #487944 -
Flags: review?(karlt)
Assignee | ||
Comment 41•14 years ago
|
||
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•14 years ago
|
||
(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].)
Assignee | ||
Comment 43•14 years ago
|
||
Assignee: jap → benjamin
Attachment #487942 -
Attachment is obsolete: true
Attachment #488463 -
Flags: review?(karlt)
Assignee | ||
Comment 44•14 years ago
|
||
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.
Attachment #487944 -
Attachment is obsolete: true
Attachment #488465 -
Flags: review?(karlt)
Comment 45•14 years ago
|
||
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•14 years ago
|
||
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.
Attachment #488465 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 47•14 years ago
|
||
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...
Assignee | ||
Comment 48•14 years ago
|
||
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•14 years ago
|
||
(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•14 years ago
|
||
(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•14 years ago
|
||
(In reply to comment #48)
> http://hg.mozilla.org/projects/maple/rev/949de4d79ea9 (tests)
Looks good, thanks.
Comment 52•14 years ago
|
||
... although part2 of xulbrowser_plugin_visibility.xul really should have the same treatment.
Comment 53•14 years ago
|
||
If for part2, you wait for plugin2.getPaintCount(), I think it would be safe to assume that the isVisible()s have changed.
Updated•14 years ago
|
Attachment #488463 -
Flags: review?(karlt) → review+
Assignee | ||
Comment 54•14 years ago
|
||
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
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Comment 55•14 years ago
|
||
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•14 years ago
|
||
Assignee | ||
Comment 57•14 years ago
|
||
That last bit is filed/fixed in bug 611593
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•