114921 - [viewpoint] NPP_HANDLE_EVENT for WM_PAINT doesn't pass in the updateRect

Status: VERIFIED FIXED

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

Description

[Ari Berger]

From Bugzilla Helper:
User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 5.0)
BuildID:    

In Netscape 4.x, we received the updaterect for WM_PAINT messages in the 
lparam.  Mozilla passes us NULL.
This has all sorts of bad consequences, including performance problems, and 
(more seriously) drawing errors when part of the plugin is covered by another 
window.

Reproducible: Always
Steps to Reproduce:
1.You should be able to reproduce this with any content on any plugin- the code 
in nsObjectFrame.cpp always sets the lparam to NULL
2.To see the negative consequences is somewhat more involved- install our 
plugin (http://cole.viewpoint.com/~aberger/setwindowbug/viewpointplugin.zip)
3.View http://cole.viewpoint.com/~aberger/setwindowbug/index.html
(note: viewing this content depends on having the patch from 114667)
4. Partially cover the plugin area with another window and then move it away
5. interact with the object- you should see that our image is stuck in the 
background


Because we don't know which part of the window you redrew, we assume you redrew 
the whole thing.  We assume that our image is gone, so we redraw our image.  
But you have not really redrawn the whole plugin window, just part of it.  We 
need to know which part.

Comment 1

[Peter Lubczynski]

Andrei, can you take a look at this?

Comment 2

[Judson Valeski]

nom'ing

Comment 3

[av (gone)]

Is passing the rectangle documented anywhere?

Comment 4

[Ari Berger]

I have not seen it in documentation.  In fact, the Netscape 4.x API 
documentation just says that it will windows message, which doesn't pass it in.
However, Netscape 4.x passed it in, and it is very important for us.
If you have only redrawn part of the window (and didn't clear the rest of it), 
and we paint the whole window, you will see a sort of double-image- we are 
still drawn on the screen (from the last time we painted) and we will draw 
again on top of it.
Normally when you get a WM_PAINT message, you can call GetUpdateRect. If I try 
that when you tell me to paint, I get back 0 (meaning there is no update rect).

Comment 5

[av (gone)]

Just to confirm. 4x passed the rectangle in the form of (top, left, bottom, 
right) rather than (x, y, width, height) and this is what we want, is that 
right?

Comment 6

[Ari Berger]

Correct- they passed (top, left, bottom,right).

Comment 7

[av (gone)]

Attachment: patch v.1

Ari, I also assume that the rect struct is Windows style, i.e. all fields are
of LONG type, rather than NPRect with uint16 fields.

This patch passes whatever dirty rectangle we have in
nsPluginInstanceOwner::Paint assuming it is correct. I still some painting
problems but not sure what they are related to.

This patch also includes "alternative patch" from bug 114667
(http://bugzilla.mozilla.org/showattachment.cgi?attach_id=61428)

Peter, Patrick, please have a look. Ari, can you please test it?

Comment 8

[av (gone)]

Peter, in this patch I basically reused on Windows what we already have on Mac 
for calculating the dirty rectanle, excluding FixUpWindow.

Comment 9

[Peter Lubczynski]

That sounds correct because windowless plugins events work similar to Mac. I'd
like to try this out, but I'm a bit backlogged on testing patches....

Comment 10

[Peter Lubczynski]

Attachment: patch resolving conflicts for 0.9.4 branch

...for those that only have that branch...

Comment 11

[Ari Berger]

Andrei-
I tried the patch out.  It is part of the way there, but not all the way.
The rect that I am getting seems to be the correct size (at least, at looks 
reasonable) but it is offset vertically. For example, on the piece of content I 
was viewing (i will try it on the simplified content that i reference in this 
bug tomorrow), I get the following results:
On the first paint call (when I would expect to be told to paint the entire 
window, I get:
My entire window is (462, 45) -(818,345) (or height, width = 300,356)
the rect you pass me is (462,-240) -(818,60) (or height, width = 300,356)

As I move around a window in front of us to see what we get told to paint, the 
bottom coordinate stays at 60, the top changes.

So again, looks like correct dimensions, wrong origin.

I hope this helps.  Tomorrow we are doing a new build of our plugin which I can 
make available, and I will try this against the test content that I have posted.

Comment 12

[Ari Berger]

If you want to try to reproduce the problem that I am seeing, I put up a new 
version of our plugin.  If you listen with a debugger, I send out debugstrings 
with the rect that I get in setwindow and the rects that I get from you.
Same content URL:
http://cole.viewpoint.com/~aberger/setwindowbug/index.html

If you already have our plugin installed, you can just replace the dll and xpt 
from:
http://cole.viewpoint.com/~aberger/setwindowbug/npViewpoint.dll 
http://cole.viewpoint.com/~aberger/setwindowbug/npViewpoint.xpt
(note that we renamed our dll- you should probably delete the old one).
If you haven't installed our plugin, you also need to run the executable:
http://cole.viewpoint.com/~aberger/setwindowbug/VMPFullInstall_3_0_8_201.exe

Comment 13

[av (gone)]

Thanks, Ari, I'll give it a try. One should also probably nuke xpti.dat and 
components.reg when switching to the new plugin.

Comment 14

[av (gone)]

Comment on attachment 62056 [details] [diff] [review]
patch resolving conflicts for 0.9.4 branch

New patch is coming.

Comment 15

[av (gone)]

Comment on attachment 62041 [details] [diff] [review]
patch v.1

New patch is coming.

Comment 16

[av (gone)]

Hm... I mean all previous patches are now obsolete, I have a new patch which 
passes the correct dirty rectangle. Will pass it tomorrow.

Comment 17

[av (gone)]

Attachment: patch v.2

This patch presumably calculates the dirty rectangle correctly. It also assumes
that fixes for bug 116093 and bug 116108 are already applied.

Please try and review.

Comment 18

[Ari Berger]

Preliminary testing looks great! I'll run it through a bunch more test cases, 
but it looks like this does the trick for me.

Comment 19

[Peter Lubczynski]

Comment on attachment 62409 [details] [diff] [review]
patch v.2

r=peterl

Comment 20

[Michael Dunn]

Plusing for 094 based on Ari's feedback

Comment 21

[Marc Attinasi]

Comment on attachment 62409 [details] [diff] [review]
patch v.2

sr=attinasi

Comment 22

[av (gone)]

Setting milestone to be on par with the two bugs this one depends on.

Comment 23

[av (gone)]

In the trunk. Nominating for 0.9.7.

Comment 24

[av (gone)]

In 0.9.4, marking fixed.

Comment 25

[shrirang khanzode]

reso/fxd. will verify asap.

Comment 26

[shrirang khanzode]

verif on 1226 trunk and 1225 branch builds that this works. Arun clarified that 
changing the useragent string for commercial browser might get this testcase to 
work (which is correct). 

Comment 27

[shrirang khanzode]

adding keyword 'verified0.9.4'