Closed
Bug 135737
Opened 23 years ago
Closed 23 years ago
[viewpoint] windowless plugin receives incorrect SetWindow calls
Categories
(Core Graveyard :: Plug-ins, defect, P2)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla1.0
People
(Reporter: aberger, Assigned: peterl-bugs)
References
()
Details
(Keywords: topembed+, Whiteboard: [ADT1 RTM][ETA: 5/15],custrtm-)
Attachments
(1 file, 9 obsolete files)
6.39 KB,
patch
|
peterlubczynski-bugs
:
review+
peterlubczynski-bugs
:
superreview+
jesup
:
approval+
|
Details | Diff | Splinter Review |
From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)
BuildID: trunk or .9.9 branch
when the plugin is started, we get correct coordinates in the SetWindow call.
(For this content, something about (8,8) is reasonable).
Then we start getting SetWindow calls before each paint message, and the
numbers don't make any sense. Example (-137, -58) for the top-left corner.
Reproducible: Always
Steps to Reproduce:
1.Install Viewpoint plugin (http://cole.viewpoint.com/~aberger/builds/vpt.zip)
2. View content:
http://cole.viewpoint.com/~aberger/primitive/
Actual Results: we get incorrect window coordinates. Mouse interactivity is
confused.
Updated•23 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 1•23 years ago
|
||
assigning to av and setting to 1.0.1
Assignee: beppe → av
Priority: -- → P2
Target Milestone: --- → mozilla1.0.1
Comment 2•23 years ago
|
||
-->taking bug
This is a serious problem with the Viewpoint plugin (and other windowless
plugins) with painting and will be important to embedding.
This problem maybe caused because now on the trunk we recreate the context each
time we tell the plugin to paint and we are trying to do a translation to tell
it to paint in the right place:
http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsObjectFrame.cpp#1699
Comment 3•23 years ago
|
||
Just had a conversation with peterl in regards to this problem. It seems that
what happens is that the plug-in writes all over the window, including hte
chrome. That can cause all sorts of unexpected behavior, including the
possibility of not being able to access objects under the plug-in. This is
probably not limited to ViewPoint, but rather global in nature. That would mean
that the exposure is to a very large audience (think PDF file viewing). Due to
the large impact of this issue, I am setting to ADT1
Comment 4•23 years ago
|
||
This problem was introduced by the fix to bug 118721 . It should only affect
windowless plugins and I'm pretty sure that the PDF plugin doesn't use
windowless mode. There are not many plugins around that use windowless mode at
all, my company has not released a new windowless plugin that works with
Mozilla (our currently released NS4.x plugin will work with Mozilla in
windowless mode, but none of the scripting works so it is basically useless). As
far as I know the only current windowless plugin for Mozilla is Viewpoint's.
The reason for this problem is that Mozilla has an optimised redraw process, if
only one region has been invalidated then a DC is created for that region alone
and the plugin is only asked to repaint that region.
I haven't got access to a debug build at the moment, so can you confirm that the
plugin draws correctly and that the problem is that you have no way of
determining the position of the mouse coordinates relative to your windowless
plugin?
The difficulty with this problem is that in NS4.x this never happened, the
drawing process was not so well optimised, so looking at the docs it is not
clear what the correct way of doing things is.
NPWindow is defined here:
http://developer.netscape.com/docs/manuals/communicator/plugin/refstrc.htm#npwindow
The line:
"x, y The x and y coordinates for the top left corner of the plug-in relative to
the page (and hence, relative to the origin of the drawable). These values
should not be modified by the plug-in."
the "and hence" bit no longer makes sense as it is not necessarily the case that
the origin of the drawable is the same as the origin of the page. So, how do we
pass the info to the plugin to tell it what the origin of the drawable is?
One solution is to leave this as it is now implemented so that SetWinow passes
the origin of the plugin relative to the drawable which leads to the negative
coordinates that you are seeing. And change the windowless mouse event handling
functions to fixup the coordinates so that they are relative to the drawable
passed on the last paint. Your mouse handling code may even work unchanged with
this approach?
Reporter | ||
Comment 5•23 years ago
|
||
Your explanation makes a lot of sense. You are correct that we seem to draw in
the right place but our mousing is wrong.
Let me make sure I understand. Before this fix, SetWindow had the origin of
the plugin relative to the page. Paint messages would have the offset within
the window where we should paint. After the fix, we receive SetWindow messages
before each paint. We always Paint at (0,0), and the SetWindow gives us the
offset FROM the paint DC to the origin.
Mouse events used to be (and still are) relative to the origin of the plugin
embed (so 0,0 would be a mouse event at the topleft of the embed). However,
now, we think that the embed is half off the page (because its we got SetWindow
with a negative origin) and a (0,0) mouse event doesn't make too much sense.
Is that correct?
The other problem that I have to look into is our efficiency. I think (but I
need to check) that we may assume SetWindow is being called to tell us that the
window has really moved (scrolled, probably) and invalidate our entire window.
I need to look into this.
Comment 6•23 years ago
|
||
>Your explanation makes a lot of sense. You are correct that we seem to draw in
>the right place but our mousing is wrong.
>Let me make sure I understand. Before this fix, SetWindow had the origin of
>the plugin relative to the page. Paint messages would have the offset within
>the window where we should paint.
Do you mean with the offset that it is contained in the NPRect that is passed
via the lParam of the WM_PAINT message. If so then yes that's right.
>After the fix, we receive SetWindow messages
>before each paint. We always Paint at (0,0), and the SetWindow gives us the
>offset FROM the paint DC to the origin.
I don't think that the lParam will always specify a rect at (0,0). I still
haven't got access to a debug build or my plugin source to verify the actual
behaviour, but this is what I'd guess would happen. Say your plugin is at (50,
50) and is 100x100 in size.
On first draw:
SetWindow will pass (50, 50, 100, 100) for (x, y, width, height) and the dirty
rect will be (50, 50, 150, 150) for (left, top, right, bottom)
If the plugin called InvalidateRect on its whole region then:
SetWindow will pass (0, 0, 100, 100) and the dirty rect will be (0, 0, 100, 100)
If something were dragged over the bottom right corner of the plugin from 50,
50 then:
SetWindow will pass (-50, -50, 100, 100) and the dirty rect will be (0, 0, 50,
50)
>Mouse events used to be (and still are) relative to the origin of the plugin
>embed (so 0,0 would be a mouse event at the topleft of the embed). However,
>now, we think that the embed is half off the page (because its we got
>SetWindow with a negative origin) and a (0,0) mouse event doesn't make too
>much sense.
>Is that correct?
That's correct.
Comment 7•23 years ago
|
||
It seems like we have three possible solutions to this:
1) Switch of optimised drawing on the view containing the plugin. Would this be
achieved by disabling double-buffering? See, this code for how it's done on the mac:
http://lxr.mozilla.org/mozilla/source/layout/html/base/src/nsObjectFrame.cpp#753
It would be a bit of a shame to do this as it would remove the benefit of the
optimised drawing.
2) Call SetWindow (again!) with the offset of the plugin before firing a mouse
events.
This could be quite ineffecient, it depends what processing gets done in
SetWindow by the plugin.
3) Modify the mouse event that gets passed to the plugin so that it is relative
to the window.x and window.y last passed to SetWindow.
This would lead to some weird values for the mouse coords being passed through
to the plugin. But, the code in a typical windowless plugin would subtract the
last value passed in window.x and window.y from the mouse coords, which would
magically produce the correct mouse coordinate.
Any opinions?
Comment 8•23 years ago
|
||
Well, I think we've got two choices:
1) Go back to the way we used to do it and pass a DC the size of the page
2) Relnote this issue and provide a way for windowless plugins to find out their
absolute location
For #1, here's a possibility of what we can do:
1. When a windowless plugin is told to paint, create a scratch buffer the size
of the document -- like we did in 0.9.4 and 4.x.
2. Copy the translated bits of the current backbuffer into this new scratch bufffer
3. Pass the scratch buffer to the windowless plugin to Paint (call
SetWindow/Paint like before)
4. Copy and translate the bits from the scratch buffer back over the old back buffer
For #2, if the plugin couldn't determine it's location with just math, maybe
implement a new NPN_GetValue variable or possibly add info to the NPWindow struct.
The question is what is the right thing to do here: closer compatibility or
optimized painting?
Status: NEW → ASSIGNED
Reporter | ||
Comment 9•23 years ago
|
||
To expand on Peter's point #2:
The reason that we need to know the offset of where our plugin is on the page
is that sometimes we don't get mouse messages from the browser (these should be
offset correctly).
The problem is that for various reasons, we sometimes need to capture the
mouse. In this case we get the events from the OS (relative to screen). To
find the location relative to the plugin embed, we subtract 2 offsets: the
browser view window (which we get from GetWindowRect() on the window we get
from NPN_GetValue(NPNVnetscapeWindow)) and the offset of the plugin from the
browser (which is what USED to come in from SetWindow).
To make this work, we would need to get that value- the offst of the plugin
relative to the browser window- in some other way.
There are 2 ways we can get it:
1) You could provide a NPN_GetValue for it (however, we would rather not have
to ask all the time)
2) We could do the math based on the offsets we get in SetWindow and Paint and
calculate it. However, what if we get a SetWindow not followed by a Paint (or
what if the OS sends us a mouse message between setwindow and paint)?
I will continue to look at this issue.
Comment 10•23 years ago
|
||
Ari, having had a chance to look at our code again I now understand the problem
with SetCapture.
Instead of NPN_GetValue being called all the time how about we use NPN_SetValue
instead and pass a pointer to an NPRect, if this value has been set by the
plugin then the browser should fill it with the offset of the plugin (and update
it every time this offset changes). If we did this then we could also write it
so that if this value isn't set then optimised drawing is also disabled. That
way we get the best of both worlds, compatibility and optimised drawing if you
choose it.
Reporter | ||
Comment 11•23 years ago
|
||
that sounds like a great solution to me.
Comment 12•23 years ago
|
||
I think that this patch disables optimised drawing for windowless plugins - I
haven't tested it that much. I am working on a complete patch which includes
the ability for the plugin to re-enable optimised drawing and pass in a pointer
to a rectangle to get the plugin position. That's going to take a few more days
though.
Reporter | ||
Comment 13•23 years ago
|
||
I applied the patch.
It does solve the mouse problem- things behave properly. However, the drawing
pipe is not correct. Everything is in the correct place, but there is some
double-blitting going on. Every time we draw, our content blinks. It looks
like you blit to the screen before we get a chance to draw.
Updated•23 years ago
|
Comment 14•23 years ago
|
||
Ari, thanks for testing that.
I also tried using the following in the same place (not fully understanding what
it was meant to do!):
viewMan->SetViewBitBltEnabled(view, PR_FALSE);
but that doesn't seem to make any difference at all.
I'm afraid I'm out of ideas as to how we disable optimised drawing.
Comment 15•23 years ago
|
||
hm....I've been taking example from nsWindow::OnPaint() and trying to do
something with CopyOffScreenBits:
nsRect bounds;
nsCOMPtr<nsDrawingSurfaceWin> myBuffer;
aPresContext->GetVisibleBounds(bounds);
aRenderingContext.CreateDrawingSurface(&bounds,
NS_CREATEDRAWINGSURFACE_FOR_PIXEL_ACCESS, (void*&)*getter_AddRefs(myBuffer));
HDC hDC;
myBuffer->GetDC(&hDC);
plugin->Paint(hDC, 0.9.4 window math);
aRenderingContext.CopyOffScreenBits(myBuffer, aDirtyRect.x, aDirtyRect.y,
NS_COPYBITS_TO_BACK_BUFFER);
aRenderingContext.DestroyDrawingSurface(myBuffer);
It's not working quite yet. Maybe it would be better just to hand back an NPRect?
Comment 16•23 years ago
|
||
I think that passing an NPRect is probably the way to go. Given that as AFAIK
the only thing that is broken by this bug is getting the mouse position when it
is captured. Adding all that code for creating a new surface etc. seems like
overkill.
Reporter | ||
Comment 17•23 years ago
|
||
Additional info regarding the patch:
I do not always see the double blitting problem. In fact, when my dirty rect
(the area that I invalidate) is entirely within the embed window, things work
perfectly: drawing and mousing.
Once any part of the dirty rect extends outside the embed window, I see the
flickering.
To see our dirty rects, open the MetaStreamConfig.ini file in the Program
Files\Viewpoint Media Player (or Viewpoint Experience Technology) folder. In
the [ComponentMgrOptions] section, add DirtyRects=1. I don't know if this
helps.
Comment 18•23 years ago
|
||
This patch adds a new way through NPN_GetValue for plugins to get back a
pointer to an NPRect struct which has their location. I'm adding
NPNVpluginWindowLocation/nsPluginInstancePeerVariable_PluginWindowLocation.
Please test this out to see if it fits your needs.
Comment 19•23 years ago
|
||
So I talked with Ari and I think I'm going to try a little simplier approach. In
DidReflow and ScrollPositionChange I'm going to pass an NPRect through
HandleEvent. That should keep the plugin notified of where it is (the old
purpose of SetWindow) and be close to the old behavior.
Whiteboard: [ADT1] → [ADT1][ETA: 4/19]
Comment 20•23 years ago
|
||
Gota run to a meeting, I haven't had time to test it out, but here's a patch
that sends an NPRect in lParam through HandleEvent when either we scroll of
have finished (re)layout. Let me know if you find any issues.
Comment 21•23 years ago
|
||
Comment on attachment 79904 [details] [diff] [review]
patch to send WM_WINDOWPOSCHANGED events v.1
I don't think this patch is calculating the offset from the widget correctly
and I don't think it's working as desired with scrolling.
Attachment #79904 -
Flags: needs-work+
Comment 22•23 years ago
|
||
I think that you need to use the code that was backed out in bug 132262 and call
it from inside Paint to check if the plugin has moved. I think that DidReflow
doesn't get called if the plugin is in a table (see bug 114667).
Comment 23•23 years ago
|
||
Okay, this patch sends rects just like before (0.9.4) except now through
HandleEvent. This patch looks like it's doing the right thing, even when
scrolling.
Reporter | ||
Comment 24•23 years ago
|
||
OK, this looks like it will work for us.
Do you need to send us a HandleEvent WM_WINDOWPOSCHANGED with every paint
though?
We used to get a Paint message to paint, now we get a WM_WINDOWPOSCHANGED,
SetWindow, and then Paint.
Would it make sense to pass us this event only when the window has actually
changed?
I need to play with it some more, see what it means efficiency-wise for us, but
the fix works.
Reporter | ||
Comment 25•23 years ago
|
||
One problem- the window you send in through handleevent is not clipped as I
would expect.
If you resize the browser window so the plugin embed is clipped, you still pass
us the full original size of the window, not the actual size.
Reporter | ||
Comment 26•23 years ago
|
||
One problem:
In nsObjectFrame.cpp:
NPRect rect = {origin.y, origin.x, origin.y + window->height, origin.x + window-
>width};
NPRect is a set of uint16, origin is int32. Negative origins don't behave
properly.
Comment 27•23 years ago
|
||
Ooops, can't use NPRect. Okay, how about this:
* I pass you an NPWindow* in lParam through HandleEvent
* I cache a nsPluginWindow, only calling you when changed (not on every paint)
* I intersect your pluign window rect with the visible rect to get right clipping
Comment 28•23 years ago
|
||
Changing this to [ADT1 RTM]. Let's get it on the trunk, and let it bake, then
take it for RTM.
Whiteboard: [ADT1][ETA: 4/19] → [ADT1 RTM][ETA: 4/19]
Comment 29•23 years ago
|
||
Just so you know, we need this fixed asap, meaning this week.
Jaime: I will talk to you about this issue offline.
Comment 30•23 years ago
|
||
Here's a better patch, however I think we are incorrectly calculating the
visible width. I'll try to ask the layout folks on how to get the right values.
Let me know if there are any other problems.
Attachment #80019 -
Attachment is obsolete: true
Reporter | ||
Comment 31•23 years ago
|
||
Thanks Peter- I will try this.
If there is time preasure to get this checked in, the width is not a big deal.
I noticed that we seem to get the wrong width on IE in many cases- that is the
most minor piece. You have to know what you are looking for to see something
is wrong. The key is having the right origin- with that we can calculate
offsets properly. Since we only need this for mousing and not drawing, the
width isn't crucial.
The other 2 (especially the nprect vs npwindow) are the important part (for us
at least- it would obviously be good to get this right in general).
Reporter | ||
Comment 32•23 years ago
|
||
Looks great! I tried tables, divs, scrolling- it all looks good. The height
and width are wrong, but those are pretty unimportant.
Comment 33•23 years ago
|
||
Updated patch: Now I'm getting the bounds of the root widget as my intersected
visible area.
Ari, let me know if this patch fixes the width/height problems.
Attachment #81093 -
Attachment is obsolete: true
Reporter | ||
Comment 34•23 years ago
|
||
Height and width look perfect.
Comment 35•23 years ago
|
||
New patch addressing Andrei's comments that we should be passing a WINDOWPOS
struct instead of an NPWindow. Ari, is that going to work for you?
Reporter | ||
Comment 36•23 years ago
|
||
Sure- anything with an x, y, height, and width will work fine.
Let's get this checked in.
Comment 37•23 years ago
|
||
Comment on attachment 81908 [details] [diff] [review]
patch v.5
Peter, did you mean instead of
+ winpos.cx = NS_STATIC_CAST(PRUint32,
mWindowlessWindow.width);
+ winpos.cy = NS_STATIC_CAST(PRUint32,
mWindowlessWindow.height);
to write
+ winpos.cx = NS_STATIC_CAST(PRInt32,
mWindowlessWindow.width);
+ winpos.cy = NS_STATIC_CAST(PRInt32,
mWindowlessWindow.height);
It is |int| in the Windows struct and |PRUint32| in our nsPluginWindow struct.
If I got it right and you fix it, r=av. We can probably also fill the hwnd
field of this struct.
Attachment #81908 -
Flags: review+
Comment 38•23 years ago
|
||
Oh right, thanks Andrei. Here's an updated patch. I added a memset but could
not fill in hwnd because, naturally, windowless plugins don't get a
widget/window.
Attachment #78542 -
Attachment is obsolete: true
Attachment #79667 -
Attachment is obsolete: true
Attachment #79904 -
Attachment is obsolete: true
Attachment #81749 -
Attachment is obsolete: true
Attachment #81908 -
Attachment is obsolete: true
Updated•23 years ago
|
Attachment #81948 -
Flags: review+
Comment 39•23 years ago
|
||
Comment on attachment 81948 [details] [diff] [review]
patch v.6
r=av
Comment 40•23 years ago
|
||
Comment on attachment 81948 [details] [diff] [review]
patch v.6
sr=attinasi
Attachment #81948 -
Flags: superreview+
Comment 41•23 years ago
|
||
New patch: No need to cache a whole nsPluginWindow if all we need is what an
nsRect can provide. Also gets rid of compiler warnings and the NS_STATIC_CAST.
Attachment #81948 -
Attachment is obsolete: true
Comment 42•23 years ago
|
||
Attachment #82055 -
Attachment is obsolete: true
Comment 43•23 years ago
|
||
Comment on attachment 82057 [details] [diff] [review]
patch 7.1
Talked with Marc, copying reviews from previous patch.
Attachment #82057 -
Flags: superreview+
Attachment #82057 -
Flags: review+
Reporter | ||
Comment 44•23 years ago
|
||
Does this have the problems mentioned in comment 26- NPRect is a set of uints,
we need ints?
Comment 45•23 years ago
|
||
|nsRect| unlike |NPRect| has it all |int32|, so it should be fine.
Comment 46•23 years ago
|
||
Patch in trunk, marking FIXED and nominating adt1.0.0.
Comment 47•23 years ago
|
||
Let's get this one in after RC2. adt1.0.0- [adt1 RTM]
Reporter | ||
Comment 48•23 years ago
|
||
I've tried the nightly trunk build and it is working great. Our QA has started
looking at it and has found no problems. We'd love to see this in the branch
as soon as possible- without this bug fixed, our plugin doesn't work properly.
Comment 49•23 years ago
|
||
*** Bug 143142 has been marked as a duplicate of this bug. ***
Comment 50•23 years ago
|
||
looks ok on trunk 0509.
Comment 51•23 years ago
|
||
Justification for the branch:
Viewpoint is a windowless plugin and is important to embedding. This patch only
touches windowless plugins. The way we paint has changed (we recreate drawing
surfaces dynamically) and this fix is important so that ALL windowless plugins
know where they are on the screen. Otherwise the plug-in content will draw
incorrectly, and in many cases will overlay the window controls.
Comment 52•23 years ago
|
||
Adding adt1.0.0+ for 1.0 branch checkin approval. Please get drivers approval
before checking in.
Comment 53•23 years ago
|
||
This is fixed on Win XP trunk build 2002051408
Comment 54•23 years ago
|
||
Comment on attachment 82057 [details] [diff] [review]
patch 7.1
a=rjesup@wgate.com for branch checkin under the assurance from valeski that the
fact that the patch basically only touches XP_WIN is ok - no other platforms
need this fix.
Attachment #82057 -
Flags: approval+
Comment 56•23 years ago
|
||
Ari,
Is there a new version of your plugin that uses this new event that our QA can
grab to test and verify this bug with?
Thanks!
Reporter | ||
Comment 57•23 years ago
|
||
I'll toss one up on our server for testing purposes first thing Monday morning.
Comment 58•23 years ago
|
||
Adding custrtm-; no impact on customization.
Comment 59•23 years ago
|
||
has been long verified..needed to add kwd.
Status: RESOLVED → VERIFIED
Keywords: verified1.0.0
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
•