Closed Bug 104445 Opened 23 years ago Closed 23 years ago

OJI plugin needs a way to detect scrolling

Categories

(Core :: Layout, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla0.9.8

People

(Reporter: beard, Assigned: bnesse)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
Attached patch Implements new scrolling events. (obsolete) — Splinter Review
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.
Keywords: review
This patch needs reviewing.
Status: NEW → ASSIGNED
I feel funny about turning off double buffering on Classic. Hows does it work
with plugins like Shockwave?
That #if 0 was mistakenly included in the patch, I won't check it in that way.
Comment on attachment 53308 [details] [diff] [review]
Implements new scrolling events.

ah, in that case r=peterl
Attachment #53308 - Flags: review+
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.
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.
Blocks: 110094
Blocks: 108019
>> 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.
Attached patch Revised patch (obsolete) — Splinter Review
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
-> me.
Assignee: beard → bnesse
Status: ASSIGNED → NEW
Brian, do you think we should make this patch not only for Mac but for 
windowless plugins on Windows too?
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.
Target Milestone: --- → mozilla0.9.8
Blocks: 76085
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.
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.
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?
The patch on 110094 actually fixes the scrolling problem that our Viewpoint 
plugin had.  I guess this bug doesn't really block that one?
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.
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.
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.
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."
Ha, so we've been bad all along. Fair enough. sr=sfraser on the patch.
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.

>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.
Also, what about full-page plugins?
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
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 on attachment 65180 [details] [diff] [review]
Patch addressing Peter's comments.

r=peterl
Attachment #65180 - Flags: review+
Blocks: 120222
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 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+
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.
Fix checked in.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: