origin not set properly when SetWindow() called

VERIFIED FIXED in mozilla0.8

Status

()

Core
Plug-ins
P3
major
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: Nicholas Hart, Assigned: Brian Nesse (gone))

Tracking

({shockwave})

Trunk
mozilla0.8
PowerPC
Mac System 9.x
shockwave
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [branch accept], URL)

Attachments

(7 attachments)

(Reporter)

Description

17 years ago
The origin is set to the upper-left corner of the plugin for an updateEvt, but 
for all other events (including null events) this is not the case.  The legacy 
plugin API does set the origin before calling NPP_HandleEvent.

Install the attached plugin and go to 
http://pages.prognet.com/onesies/nhart/test.html

On an updateEvt the plugin draws a blue rectangle around the plugin's window's 
coordinates.
On a null event it draws a red rectangle.
On any other event it draws a yellow rectangle.

You should see a blue rectangle positioned properly on the page, and then a red 
rectangle in the upper-left corner of the browser with our plugin drawn inside 
it.  (We repaint the plugin on null events.)

Now, I can probably change our plugin to adjust the origin, but I'm not sure 
this is the proper solution.  It seems to me that the origin should *probably* 
be set to the upper-left corner of the plugin whenever any event is passed in 
to HandleEvent().

This isn't exactly a blocker for us, but I need to know ASAP if we need to fix 
it on our end or if this will be fixed in Mozilla.

Thanks.
(Reporter)

Comment 1

17 years ago
Created attachment 19001 [details]
stuffed example plugin
(Reporter)

Updated

17 years ago
Keywords: realplayer

Comment 2

17 years ago
Adding peterl and bnesse.
(Reporter)

Comment 3

17 years ago
Any update on this?  We can't proceed until we get some assistance on this.
Thanks.
(Assignee)

Comment 4

17 years ago
It seems that the problem stems from window->x, y, and clipRect not being setup 
correctly in nsObjectFrame::DidReflow() before calling inst->SetWindow(window);

If I change these to the correct values (offset for window->portx & porty) it 
does the initial placement correctly. Unfortunately, all future draws are then 
doubly offest and the remaining draws happen in the center of the screen.
(Reporter)

Comment 5

17 years ago
This is blocking me from making progress on the RealPlayer plugin for Mac.  If 
it is too late to fix the plugin API, then I could at least use some 
information about how I might be able to detect if the origin is incorrect and 
fix it up.

Thanks
(Reporter)

Comment 6

17 years ago
To clarify my previous comments a bit: I am looking for a little assistance so 
that I can detect and (where appropriate) adjust the origin in our HandleEvent
() function.

Thanks!

Comment 7

17 years ago
setting bug status to New
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 8

17 years ago
Peter, the code I sent you previously for fixing positioning problems (bug 54962 
I believe) did adjust the values in DidReflow(). Apparently your fix ended up 
taking a different route? Do you have any thoughts as to why the offsets would 
get doubled up as per my 2000-11-13 comments?

Reassigning to Peter because I believe this is simply an extension of work he has 
already done on Mac positioning.
Assignee: av → peterlubczynski

Comment 9

17 years ago
accepting...looking at it now....
Status: NEW → ASSIGNED

Comment 10

17 years ago
Brian,
We are calculating the origin with the view which is incorrect. The correct way 
is to use the widget which is what my code fixes up in Paint. I tried placing 
this in DidReflow and before events get sent but it didn't quite work perfectly. 
(Assignee)

Comment 11

17 years ago
Peter, upon further review, it appears that there is also an additional code path 
into this problem. SetWindow() also get called directly from 
nsPluginHostImpl::InstantiateEmbededPlugin(). It appears that in neither case, 
here or in nsObjectFrame::DidReflow(), is there a valid widget from which to 
obtain positioning data.

Comment 12

17 years ago
Exactly!! Because there is no widget when SetWindow gets called, the position of 
the plugin is incorrectly calculated and the wrong origin is set. If we don't do 
a SetWindow when we don't have a widget, the origin is set correctly, except I'm 
seeing the height of the chrome added twice only for update events. 
(Assignee)

Comment 13

17 years ago
I guess I'm seeing 3 options here then. The first is to insure that the widget is 
created before we call SetWindow(). The second is to set the clipRect to {0, 0, 
0, 0} until the widget is valid (and hope all plug-ins are good citizens and pay 
attention.) The third is to strongly recommend that drawing not happen from the 
SetWindow call.

Any other thoughts?

Comment 14

17 years ago
Created attachment 19583 [details] [diff] [review]
patch to nsObjectFrame to fix-up origin

Comment 15

17 years ago
Try the above patch and let me know if it does the trick. 
(Assignee)

Comment 16

17 years ago
The patch didn't fix anything for me but without context diffs to be certain, it 
seems that some of the diff placements were odd. Let's sort it out via email 
rather than here.
(Assignee)

Comment 17

17 years ago
As per discussion with nhart at Real...

59620 is no longer blocking. It appears that the issue is that somewhere 
along the line Netscape gives us the wrong coordinates for a window...

...the original issue may or may not have been the origin when we get a null 
event-- the real problem was that we were simply drawing in the wrong location 
because the window we were given in the last SetWindow call was messed up...

The bug with incorrect window origin still exists. With some minor tweaks to 
Peter's patch, I *almost* have it working (y origin is correct, but not the x for 
some reason.)

This is still a major problem and needs to be addressed as it affects all plugin 
vendors. But it is no longer blocking real. Updating summary, status, and 
keywords to reflect the current state of affairs.
Severity: blocker → major
Keywords: flash, shockwave
Summary: origin not set properly for events other than updateEvt → origin not set properly when SetWindow() called

Comment 18

17 years ago
The testcase web URL is broken. Reporter, can you update?

Comment 19

17 years ago
setting target milestone to 0.8
Target Milestone: --- → mozilla0.8
(Assignee)

Comment 20

17 years ago
Created attachment 21867 [details] [diff] [review]
New and improved patch
(Assignee)

Comment 21

17 years ago
I have attached a patch which in effect insures that the clipRect of the window 
is { 0, 0, 0, 0 } until the widget is positioned correctly. This is similar to a 
fix I put into 4.x for a similar problem.

Comment 22

17 years ago
I see you removed the call to SetWindow() in DidReflow() which I think may have 
done the trick. However, I tried the Real test plugin and testcase attached to 
this bug and it still paints a blue and red rectangle over the chrome in the 
wrong places. I don't know what's causing these to be painted in the wrong place 
but perhpas FixUpPluginWindow() may need to be called in other places as well. 
I'll continue to do some more testing.
(Assignee)

Comment 23

17 years ago
Based on email exchanges with Real (see nhart's comments in my post on 2000-11-
30), I believe this to be the fault of the plug-in test case, not Mozilla. This 
test case is not valid. Try using the sample plugin and test case I sent you 
previously.
(Assignee)

Comment 24

17 years ago
Created attachment 22071 [details]
new testcase sitball.

Comment 25

17 years ago
Does Real believe that this patch will prevent them from being blocked?

I've tried this patch out on numerious testcases on Windows and Mac and it looks 
great.
(Assignee)

Comment 26

17 years ago
As of the 2000-11-30 comment real is no longer blocked by this. This is just to 
fix the inherent positioning problems.
(Assignee)

Comment 27

17 years ago
My patch, my bug I suppose :)
Assignee: peterlubczynski → bnesse
Status: ASSIGNED → NEW
(Assignee)

Comment 28

17 years ago
Created attachment 22163 [details] [diff] [review]
Same patch with tabs removed.

Comment 29

17 years ago
r=peterl

Comment 30

17 years ago
initial impressions:
1) it'd be nice if you commented what was going on, especially in 
platform-specific code.
2) for this line:
if (NS_OK == mInstanceOwner->GetInstance(inst))
shouldn't you use NS_SUCCEEDED?  And if the call does succeed, are you 
guaranteed to get a valid pointer?  Or is null a legal result?

To understand the rest of the patch, I'll have to apply it.  I'm updating my 
tree now, so I'll try to get to it soon.
(Assignee)

Comment 31

17 years ago
1) If you'd like, I will add comments and re-post the patch.

2) ? Are you referring to the code I removed?
nsPluginInstanceOwner::GetInstance(), which is declared in nsObjectFrame.cpp, 
returns NS_OK if there is a valid pointer or NS_ERROR_FAILURE if it is null.

I don't see any reason why NS_SUCCEEDED couldn't be used here, but if it is there 
are several other places in the code where the same change should be propagated.

Comment 32

17 years ago
It's a technical point, but checking against NS_OK directly is almost never
right.  Interface methods are free to return *any* non-negative value for
success (if done properly, using a "define success" macro that I don't recall
off the top of my head to generate the number), and the NS_SUCCEEDED macro
handles this perfectly. 
(Assignee)

Updated

17 years ago
Blocks: 62685
(Assignee)

Comment 33

17 years ago
Created attachment 22393 [details] [diff] [review]
New patch with comments, updated to ver 1.193 checkins.

Comment 34

17 years ago
new patch looks great.  the comments really help. sr=buster
(Assignee)

Comment 35

17 years ago
*** Bug 62685 has been marked as a duplicate of this bug. ***
(Assignee)

Updated

17 years ago
No longer blocks: 62685

Comment 36

17 years ago
Looks good to me. a=av.
(Assignee)

Comment 37

17 years ago
Checked in on Trunk (v 1.194)

Comment 38

17 years ago
Adding branch accept the status whiteboard.
.Angela...
Whiteboard: [branch accept]

Comment 39

17 years ago
Please check on to the branch A.S.A.P. Thanks!
.Angela...
(Assignee)

Comment 40

17 years ago
Checked into branch (v 1.181.4.9).

Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 41

17 years ago
Created attachment 22419 [details]
Test case for bug 62685 (duplicate bug.)

Comment 42

17 years ago
working fine with mac branch and trunk (0117). Marking verified.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.