Closed
Bug 104445
Opened 23 years ago
Closed 23 years ago
OJI plugin needs a way to detect scrolling
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla0.9.8
People
(Reporter: beard, Assigned: bnesse)
References
Details
Attachments
(1 file, 2 obsolete files)
7.33 KB,
patch
|
peterlubczynski-bugs
:
review+
sfraser_bugs
:
superreview+
|
Details | Diff | Splinter Review |
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•23 years ago
|
||
Reporter | ||
Comment 2•23 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.
Comment 4•23 years ago
|
||
I feel funny about turning off double buffering on Classic. Hows does it work with plugins like Shockwave?
Reporter | ||
Comment 5•23 years ago
|
||
That #if 0 was mistakenly included in the patch, I won't check it in that way.
Comment 6•23 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•23 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•23 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.
Comment 9•23 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•23 years ago
|
||
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
Comment 12•23 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•23 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•23 years ago
|
||
For a simple Windows windowless plugin, see: http://lxr.mozilla.org/mozilla/source/modules/plugin/tools/sdk/samples/winless/windows/
Assignee | ||
Updated•23 years ago
|
Target Milestone: --- → mozilla0.9.8
Comment 15•23 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•23 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•23 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•23 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•23 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•23 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•23 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•23 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•23 years ago
|
||
Ha, so we've been bad all along. Fair enough. sr=sfraser on the patch.
Comment 24•23 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•23 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•23 years ago
|
||
Also, what about full-page plugins?
Assignee | ||
Comment 27•23 years ago
|
||
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•23 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•23 years ago
|
||
Comment on attachment 65180 [details] [diff] [review] Patch addressing Peter's comments. r=peterl
Attachment #65180 -
Flags: review+
Assignee | ||
Comment 30•23 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•23 years ago
|
||
Comment on attachment 65180 [details] [diff] [review] Patch addressing Peter's comments. sr=sfraser
Attachment #65180 -
Flags: superreview+
Comment 33•23 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•23 years ago
|
||
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.
Description
•