120168 - [OSX/NP_FULL/PDF] Call to NPP_SetWindow in plugins has incorrect window frame when resizing window

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P2)

Description

[John Kinder]

This is a follow on to Bug # 97656, which has been fixed.

Altough Mozilla sets the topleft coordinate correctly in it's initial call to
NPP_SetWindow, when the user resizes the window subsequent calls to
NPP_SetWindow have the topleft coordinate set to {0,0} instead of being offset
to the correct port origin.  This can cause PDFViewer to overwrite the Mozilla
Navigation and personal toolbars.

Reproducible: Always
Steps to Reproduce:
I've been told that I shouldn't distribute my OS X version of PDFViewer, unless
absolutely necessary, so hopefully you can reproduce this without the plugin.

Actual Results:  topleft coordinate of window is at 0,0.

Expected Results:  topleft coordinate of window to be in correct location.

Comment 1

[av (gone)]

This is full-page plugin, right? Do you mean resize and scroll?

Comment 2

[Peter Lubczynski]

Mac --> peterl

Comment 3

[Peter Lubczynski]

Other PDF painting over chrome issues may be a dup of this bug.

Reporter: can you e-mail me PDFViewer for OS 10.1 or can I download it somewhere?

Comment 4

[Joel Chan]

Free download for PDFViewer 5.0.5 is at

http://www.adobe.com/products/acrobat/readstep2.html

Comment 5

[lmcquarr]

Acrobat 5.05 does not contain the view in the browser on OS X solution. 
Peter -- I will ping John Kinder to provide some test code to you.

Comment 6

[Peter Lubczynski]

nominating nsbeta1

Comment 7

[lmcquarr]

Folks --

When you are ready to work on this, please ping John Kinder ([e-mail])
for a development version of Acrobat to test with. 

Comment 8

[Brian Nesse (gone)]

Peter, I don't know if this is a similar issue but here's another page that
doesn't deal with resizing well.

http://www.media-division.com/flashapps/flashchess/chess.htm

Comment 9

[Peter Lubczynski]

I've recieved the development version of Acrobat for OSX and it's on my desk.
Just have not had a chance to look at it in detail yet.

Comment 10

[Winnie Lam]

adding adt1 to status whiteboard as per discussion with beppe as text can flow
outside of the plugin window.

Comment 11

[Peter Lubczynski]

I tried the software I recieved on CD from Adobe and Acrobat seems to be 
working fine in a current Mozilla build. I do not see any problems and the 
plugin paints in the correct location. 

Reporter:
What are the steps needed to reproduce this bug?

Comment 12

[rubydoo123]

downgrading from ADT1 until we get a valid test case and are able to reproduce 
this on a consistent basis.

Comment 13

[Peter Lubczynski]

Okay, I see the problem now and also the ASSERTION in the Acrobat plugin. When
the window that contains a full page plugin is resized, the origin passed into
SetWindow does not have the widget offset calculated in. We do this fix up in
the object frame and I'll see about doing the same in |PluginViewerImpl::SetBounds|.

Does anyone happen to know if this also happen on OS 9.x too?

Comment 14

[John Kinder]

It does not happen with Netscape 4.x, but I believe it did happen in the 
Mozilla OS 9 builds.  Is that what you were asking?

Comment 15

[Peter Lubczynski]

Attachment: patch to FixupPluginWindow

Here's a patch that should fix this problem along with a few others:

1. Call FixupPluginWindow () anytime we pass the window to the plugin so that
we can add-in the widget offset
2. NPN_UserAgent was being called BEFORE gServiceMgr was initialized.
3. Fixes warnings on other platforms by placing the declaration of
FixupPluginWindow() in #ifdef XP_MAC

This patch works good with Acrobat and Quicktime on both Carbon and Classic.

Comment 16

[Brian Nesse (gone)]

Is there any reason to retain the global gServiceMgr ns ns4xPlugin anymore? With
this change, we are down to one place (_reloadplugins) where it is used... is
there any reason why that shouldn't just do a do_GetService() there as well?

Comment 17

[Peter Lubczynski]

Attachment: removing gServiceMgr

Yup, gServiceMgr should be removed...Seeing reviews/comments

Comment 18

[Brian Nesse (gone)]

Comment on attachment 77088 [details] [diff] [review]
removing gServiceMgr

-  gServiceMgr = serviceMgr;

Can we remove the |serviceMgr| parameter from the constructor too? :)

We are doing the same basic thing in CreatePlugin. Should clean that up too.

+  nsCOMPtr<nsIPluginManager> pm = do_GetService(kPluginManagerCID);
+    pm->ReloadPlugins(reloadPages);

Seem to have missed the
+  if (pm)

Comment 19

[Peter Lubczynski]

Attachment: more cleanup

Comment 20

[Brian Nesse (gone)]

Comment on attachment 77128 [details] [diff] [review]
more cleanup

Hmm, I don't see nsIClassicPluginFactory.h here... did you miss it?

Comment 21

[Peter Lubczynski]

Attachment: missing file included

Comment 22

[Peter Lubczynski]

Attachment: patch v.5

now using nsCOMPtr

Comment 23

[Brian Nesse (gone)]

Comment on attachment 77191 [details] [diff] [review]
patch v.5

Much cleaner. :) r=bnesse.

Comment 24

[Peter Lubczynski]

Attachment: patch v.6

oops, forgot to include important nsPluginViewer.cpp diffs in last patch

Comment 25

[Brian Nesse (gone)]

Comment on attachment 77263 [details] [diff] [review]
patch v.6

Heh. Those were good from the start... I stopped looking for 'em. r=bnesse

Comment 26

[Patrick C. Beard]

Comment on attachment 77263 [details] [diff] [review]
patch v.6

sr=beard
There's a lot of non-essential fixes in this patch, but you are doing the RIGHT
THING™

Comment 27

[Peter Lubczynski]

We can go with just the nsPluginViewer.cpp changes, but the ASSERTION in
NPN_UserAgent bothered me.

Comment 28

[Asa Dotzler [:asa]]

Comment on attachment 77263 [details] [diff] [review]
patch v.6

a=asa (on behalf of drivers) for checkin to the 1.0 trunk

Comment 29

[rubydoo123]

forgot to readd teh ADT1

Comment 30

[Jaime Rodriguez, Jr.]

This is change to XP code. IS this happening on other platforms too? If so, have
we tested on other platforms.

Comment 31

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin to 1.0 to address this Mac issue. 

Pls file another bug for the assertion/user agent issue.

Comment 32

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin to 1.0 to address this Mac issue. 

Pls file another bug for the assertion/user agent issue.

Comment 33

[Jaime Rodriguez, Jr.]

adt1.0.0+ (on ADT's behalf) approval for checkin to 1.0 to address this Mac issue. 

Pls file another bug for the assertion/user agent issue.

Comment 34

[Peter Lubczynski]

Patch in trunk (before branching), marking FIXED. Bug 79062 re-opened on
ASSERTION problem.

Comment 35

[shrirang khanzode]

verif fixd.