OJI plugin needs a way to detect scrolling

RESOLVED FIXED in mozilla0.9.8

Status

()

Core
Layout
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: Patrick C. Beard, Assigned: Brian Nesse (gone))

Tracking

Trunk
mozilla0.9.8
PowerPC
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
Created attachment 53308 [details] [diff] [review]
Implements new scrolling events.
(Reporter)

Comment 2

17 years ago
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.
(Reporter)

Updated

17 years ago
Keywords: review
(Reporter)

Comment 3

17 years ago
This patch needs reviewing.
Status: NEW → ASSIGNED

Comment 4

17 years ago
I feel funny about turning off double buffering on Classic. Hows does it work
with plugins like Shockwave?
(Reporter)

Comment 5

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

Comment 6

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

ah, in that case r=peterl
Attachment #53308 - Flags: review+

Comment 7

17 years ago
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.
(Reporter)

Comment 8

17 years ago
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.

Updated

17 years ago
Blocks: 110094
(Reporter)

Updated

17 years ago
Blocks: 108019

Comment 9

17 years ago
>> 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.
(Assignee)

Comment 10

17 years ago
Created attachment 63413 [details] [diff] [review]
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.
Attachment #53308 - Attachment is obsolete: true
(Assignee)

Comment 11

17 years ago
-> me.
Assignee: beard → bnesse
Status: ASSIGNED → NEW

Comment 12

17 years ago
Brian, do you think we should make this patch not only for Mac but for 
windowless plugins on Windows too?
(Assignee)

Comment 13

17 years ago
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

17 years ago
For a simple Windows windowless plugin, see:
http://lxr.mozilla.org/mozilla/source/modules/plugin/tools/sdk/samples/winless/windows/
(Assignee)

Updated

17 years ago
Target Milestone: --- → mozilla0.9.8
(Assignee)

Updated

17 years ago
Blocks: 76085

Comment 15

17 years ago
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.
(Assignee)

Comment 16

17 years ago
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.
(Assignee)

Comment 17

17 years ago
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

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

Comment 19

17 years ago
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

17 years ago
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.
(Assignee)

Comment 21

17 years ago
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.
(Assignee)

Comment 22

17 years ago
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

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

Comment 24

17 years ago
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.

(Assignee)

Comment 25

17 years ago
>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

17 years ago
Also, what about full-page plugins?
(Assignee)

Comment 27

17 years ago
Created attachment 65180 [details] [diff] [review]
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?
Attachment #63413 - Attachment is obsolete: true
(Assignee)

Comment 28

17 years ago
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

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

r=peterl
Attachment #65180 - Flags: review+

Updated

17 years ago
Blocks: 120222
(Assignee)

Comment 30

17 years ago
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

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

sr=sfraser
Attachment #65180 - Flags: superreview+
a=blizzard on behalf of drivers for 0.9.8
Keywords: mozilla0.9.8+

Comment 33

17 years ago
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.
(Assignee)

Comment 34

17 years ago
Fix checked in.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.