104445 - OJI plugin needs a way to detect scrolling

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Patrick C. Beard]

The MRJ plugin for Mac OS X does asynchronous drawing, which needs to be
inhibited during scrolling, otherwise the applet draws in the wrong place. To
work around this, nsObjectFrame needs to implement the nsIScrollPositionListener
interface, and needs to deliver two new events to plugins, one indicating that
scrolling is about to happen, the other indicating scrolling has happened.
Upcoming patch introduces these new events and sends them when scrolling occurs.

Comment 1

[Patrick C. Beard]

Attachment: Implements new scrolling events.

Comment 2

[Patrick C. Beard]

The patch also synchronizes the extended set of events provided by the XPCOM
plugin API with the NPAPI, and introduces the new enum NPEventType in
mozilla/include/npapi.h.

We could get by just defining a single scrolling event, but that would be
inconsistent with the preexisting getFocusEvent/loseFocusEvent pair.

Comment 3

[Patrick C. Beard]

This patch needs reviewing.

Comment 4

[Peter Lubczynski]

I feel funny about turning off double buffering on Classic. Hows does it work
with plugins like Shockwave?

Comment 5

[Patrick C. Beard]

That #if 0 was mistakenly included in the patch, I won't check it in that way.

Comment 6

[Peter Lubczynski]

Comment on attachment 53308 [details] [diff] [review]
Implements new scrolling events.

ah, in that case r=peterl

Comment 7

[Simon Fraser [no longer active]]

Comments:
+enum NPEventType {
+  NPEventType_GetFocusEvent = (osEvt + 16),

Isn't there a risk that this list will grow to collide with kHighLevelEvent (23)?

Also, why are you keeping the seemingly redundant #defines:
 #define getFocusEvent     (osEvt + 16)
 #define loseFocusEvent    (osEvt + 17)

+  } else if (aIID.Equals(NS_GET_IID(nsIScrollPositionListener))) {
+    *aInstancePtr = NS_STATIC_CAST(nsIScrollPositionListener*,this);
+    return NS_OK;
   } else if (aIID.Equals(NS_GET_IID(nsISupports))) {

Can you just macro-ize the entire QueryInterface?

 NS_IMETHODIMP_(nsrefcnt) nsObjectFrame::AddRef(void)
 {
-  NS_WARNING("not supported for frames");
+  // NS_WARNING("not supported for frames");
   return 1;
 }
@@ -359,5 +365,5 @@
 NS_IMETHODIMP_(nsrefcnt) nsObjectFrame::Release(void)
 {
-  NS_WARNING("not supported for frames");
+  // NS_WARNING("not supported for frames");
   return 1;
 }

Other folks may not like this.

+#ifdef XP_MAC
+  // install as a scroll position listener.

Can it be installed on all platforms, moving the #ifdef into the listener code? 
That would seem cleaner, and less prone to bustage.

+nsresult nsObjectFrame::ScrollPositionWillChange(nsIScrollableView* aScrollable, 
nscoord aX, nscoord aY)
+{
+#ifdef XP_MAC
+    if (mInstanceOwner)
+        mInstanceOwner->ScrollPositionWillChange(aScrollable, aX, aY);
+#endif
+    return NS_OK;
+}
This should be a NS_IMETHODIMP, no? Otherwise Windows will break. Also, you do 
have an #ifdef XP_MAC here, which seems redundant.

Comment 8

[Patrick C. Beard]

Simon, thanks for the quick turnaround review. I will address your 
concerns and resubmit the patch. Here are a couple of explanations:

>Comments:
>+enum NPEventType {
>+  NPEventType_GetFocusEvent = (osEvt + 16),
>
>Isn't there a risk that this list will grow to collide with kHighLevelEvent 
(23)?

These aren't new. We have shipping plugin code using them. Yes, any 
NEW events should probably start up at 128 or greater. Do you have a 
preference? I'll move the scrolling events up higher.

>Also, why are you keeping the seemingly redundant #defines:
> #define getFocusEvent     (osEvt + 16)
> #define loseFocusEvent    (osEvt + 17)

Didn't want to break anything in the rest of the tree yet.

As far as removing the NS_WARNING calls in the AddRef/Release, I did 
this because it was generating too much noise. There are some other 
frames in the tree that implement the nsIScrollPositionListener, and they 
don't warn in those refCounting methods. I'd just as soon remove the 
warnings rather than comment them out.

Comment 9

[Peter Lubczynski]

>> Can you just macro-ize the entire QueryInterface?

The QI's in frames also seem to use nsIFrameDebug but only in a debug build so
it'd be hard to use the standard macros. Most frames don't use the macros.

Comment 10

[Brian Nesse (gone)]

Attachment: Revised patch

Revised patch based on sfrasers comments. Also added code to the DidChange
method which passes an update event to the plugin if it doesn't handle (i.e.
recognize) the scroll event. This fixes bug 76085, a long standing Flash
scrolling bug.

Comment 11

[Brian Nesse (gone)]

-> me.

Comment 12

[av (gone)]

Brian, do you think we should make this patch not only for Mac but for 
windowless plugins on Windows too?

Comment 13

[Brian Nesse (gone)]

Would a windowless plugin ever be scrolled? I guess I'm not entirely certain
what defines a windowless plugin. With the current patch it would be easy enough
to add the support if there is a reason to do so.

Comment 14

[Peter Lubczynski]

For a simple Windows windowless plugin, see:
http://lxr.mozilla.org/mozilla/source/modules/plugin/tools/sdk/samples/winless/windows/

Comment 15

[Ari Berger]

Yes- our windowless plugin can get scrolled.  We still have origin coordinates 
on the page which change when the page is scrolled.  By being windowless and 
transparent we can draw over the page more easily when we chose to.

Comment 16

[Brian Nesse (gone)]

After talking with av I tried to implement this by sending WM_H/VSCROLL messages
on windows. After some investigation this appears to be a fruitless endeavor.
When the nsScrollPortView calls ScrollPositionWillChange() and
ScrollPositionDidChange() methods it passes the same information in the aX and
aY arguments both times. Both sets of data appear to describe the "scroll to"
location. This means that I can neither 1) determine the amount of the scroll,
nor 2) determine whether the scroll was vertical or horizontal. Given these
issues, I can not send a meaningfull Windows message. Any thoughts or ideas
would be greatly appreciated.

Comment 17

[Brian Nesse (gone)]

Given the issues described above, I'd like to go ahead and land this as a Mac
only patch for the 0.9.8 milestone. It will address current Mac issues and does
not preclude a future Windows solution. Can I get an r/sr on the current patch
please?

Comment 18

[Ari Berger]

The patch on 110094 actually fixes the scrolling problem that our Viewpoint 
plugin had.  I guess this bug doesn't really block that one?

Comment 19

[Brian Nesse (gone)]

No, this started out as a Mac only fix. I think Peter believed this might be a
possible route to a solution for your bug.

Comment 20

[Simon Fraser [no longer active]]

Comments:

+  NPEventType_GetFocusEvent = (osEvt + 16),

I'm a little worried about these custom events types. Do any Apple docs say which 
ranges are reserved?

+        ::OSEventAvail(0, &scrollEvent);
OSEventAvail is not availble in Carbon.

Comment 21

[Brian Nesse (gone)]

OSEventAvail is declared via:
#if TARGET_CARBON
inline Boolean OSEventAvail(EventMask mask, EventRecord* event) { return 
EventAvail(mask, event); }
#endif

I'm not entirely sure why it was done this way. Is there any reason why we 
shouldn't just use EventAvail always?

I can check Apple's documentation, but the defines for get/loseFocusEvent and 
AdjustCursorEvent exist in the 4.x plug-in SDK. Changing these at this point 
risks breaking backwards compatibility. I could potentially change the 
MenuCommandEvent and the ClippingChangedEvent, as these came into existance with 
Mozilla, but there is still a chance we may break some existing plugin.

Comment 22

[Brian Nesse (gone)]

Apple doesn't leave any room for definition of private events:

http://developer.apple.com/techpubs/mac/Toolbox/Toolbox-45.html

"Note that in System 7, event types with the values 9 through 14 are undefined
and reserved for future use by Apple. All other values for the what field are
also reserved for use by Apple."

Comment 23

[Simon Fraser [no longer active]]

Ha, so we've been bad all along. Fair enough. sr=sfraser on the patch.

Comment 24

[Peter Lubczynski]

Now thinking about it, maybe the nsIScrollPositionListener interface should be
implemented on nsPluginInstanceOwner instead of the object frame because:

a) it's refcounted
b) all the other listeners are there
c) frames may go away, but the instance owner wrapper will be there

Also note, in bug 118789 I think I'm forced to use a seperate class for the
nsIDOMContextMenuListner.

Comment 25

[Brian Nesse (gone)]

>Now thinking about it, maybe the nsIScrollPositionListener interface...
Yeah, that does make sense and removes a useless level of indirection.

>Also note, in bug 118789... a seperate class for the nsIDOMContextMenuListner.
I'll watch for problems, but as it seems ok now, I don't anticipate this being
an issue.

Comment 26

[Peter Lubczynski]

Also, what about full-page plugins?

Comment 27

[Brian Nesse (gone)]

Attachment: Patch addressing Peter's comments.

This patch removes the nsObjectFrame portion of the patch, and therefore a
large chunk of the patch. I also removed the changes to the npapi.h file in the
embedding directory so as not to perpetuate the false belief that it is
current.

There is simply no easy way to integrate these changes into the fullpage
plug-in right now. I need access to objects that simply don't appear to be
available from the PluginViewerImpl object. I recommend waiting on this until
the two pluginInstanceOwner classes are integrated.

Can I get r/sr on this latest patch please?

Comment 28

[Brian Nesse (gone)]

BTW: I am currently having some unexplained issues with double keypresses on my 
flash test case (http://slip/shrir/edittext4.html.) I have verified that it isn't 
related to my patch, but rather somehow my build. I can not reproduce the problem 
on this mornings verification build. Right now I think it may be because I'm 
building with CW 7.1. I'm going to build a completely new tree with 7.0 to see if 
I can verify this and/or eliminate the problem.

Comment 29

[Peter Lubczynski]

Comment on attachment 65180 [details] [diff] [review]
Patch addressing Peter's comments.

r=peterl

Comment 30

[Brian Nesse (gone)]

I have verified that the double keypress thing is not related to CodeWarrior
either. As I still can not reproduce it in the daily builds, I am now thinking
maybe it's a debug only thing...

Comment 31

[Simon Fraser [no longer active]]

Comment on attachment 65180 [details] [diff] [review]
Patch addressing Peter's comments.

sr=sfraser

Comment 32

[Christopher Blizzard (:blizzard)]

a=blizzard on behalf of drivers for 0.9.8

Comment 33

[Asa Dotzler [:asa]]

I saw a checkin comment referencing this bug "01/18/2002 12:12
bnesse%netscape.com mozilla/ modules/ plugin/ samples/ default/ mac/
NullPlugin.cpp 1.8 1/1  Fix for bug 104445. Make nsPluginInstance a scrollbar
listener so it can notify plugins they are scrolling. Also fixes bug 76085.
r=peterl, sr=sfraser, a=blizzard"

Is this resolved? can it be marked Fixed to pull it off the "0.9.8 outstanding"
list? Thanks.

Comment 34

[Brian Nesse (gone)]

Fix checked in.