465732 - Don't invalidate plugin after scrolling

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

Invalidation should be handled elsewhere, in view/ and widget/.

Or can anyone think of a reason why we need the invalidation?

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

Because ScrollPositionDidChange is called even if plugin isn't in the visible
part of the scroll port, we call Invalidate() way too often, especially
on some flash-heavy pages like http://www.mtv3.fi.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think that should not be needed.

Comment 3

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 348974 [details] [diff] [review]
patch

Just looking at the code, I've no idea whether or not this will work.
So I'll need to test this patch (using my testing plugin from bug
441880).  I should be able to get to it tomorrow.

I won't be able to do similar testing on Windows or Linux.  But I'll
assume I'm being asked to do a Mac-specific review.

Comment 4

[Steven Michaud [:smichaud] (Retired)]

> I won't be able to do similar testing on Windows or Linux.  But I'll
> assume I'm being asked to do a Mac-specific review.

Oops.  Just realized that this patch *is* Mac-specific.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

I've tested the patch on some flash heavy pages and also with quicktime.

Java applets behave as badly as currently - no repainting while scrolling.
I hope that isn't a regression from ff3. At least it is unrelated to this bug.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #5)
> Java applets behave as badly as currently - no repainting while scrolling.
> I hope that isn't a regression from ff3. At least it is unrelated to this bug.
FF3 behaves the same way.

Comment 7

[Steven Michaud [:smichaud] (Retired)]

(In reply to comment #5 and comment #6)

The Java Embedding Plugin (which I wrote and which provides FF's Java
support on the Mac) deliberately stops updating its "port" whenever
the user scrolls (or resizes the window).

It was just too much trouble to get updates working properly under
those circumstances :-)

Comment 8

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 348974 [details] [diff] [review]
patch

This patch is fine with me.  In fact I really like it!

I tested it the same way I initially tested my patch for bug 459319:

1) Copy the latest version of the DebugEventsPlugin (from bug 441880)
   to /Library/Internet Plug-Ins.

2) Load the plugin distro's test.html into Minefield.

3) Make the browser window smaller (vertically) than the plugin
   display (so that a vertical scrollbar appears).

4) Click once on the scrollbar's up or down arrow, and observe the
   events logged to the system console.

   Without the patch you see two calls to NPP_SetWindow(), followed by
   two update events.  Since the plugin's display didn't change
   between these two update events, one of them is superfluous.

   With the patch you see two calls to NPP_SetWindow() and one update
   event -- as you should.

This patch is more narrowly targetted than my patch for bug 459319,
and therefore doesn't provide all its benefits (for example this patch
(of course) makes no difference in the test described in bug 459319
comment #2).  But my patch for bug 459319 is controversial -- as this
patch isn't.  And I think this patch does provide substantial
benefits, with (as best I can tell) no downside whatsoever.

So let's take it!

Comment 9

[Steven Michaud [:smichaud] (Retired)]

By the way, I tested a combination of this patch and my patch for bug
459319, using the test from comment #8, and got exactly the same
results.  So this patch and the patch for bug 459319 don't seem to
interfere with each other.

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 348974 [details] [diff] [review]
patch

a191=beltzner

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Fixed on trunk and 1.9.1.