Closed
Bug 88998
Opened 23 years ago
Closed 13 years ago
Current plug-in files need to be reviewed and updated to reflect the current state and status of each file, making adjustments and modifications as demed appropriate (formally titled: [meta] Plugin code cleanup tracking bug
Categories
(Core Graveyard :: Plug-ins, defect, P2)
Core Graveyard
Plug-ins
Tracking
(Not tracked)
RESOLVED
INVALID
mozilla1.4beta
People
(Reporter: waterson, Assigned: peterl-bugs)
References
Details
(Keywords: topembed-, Whiteboard: [PL:BRANCH])
Attachments
(1 file)
So, I spent the day today trying to figure out why the plugin code can't properly handle a reframe. I started by discovering that nsObjectFrame doesn't implement nsIStatefulFrame, and wound up realizing that there is quite a bit of stuff that could use some rework here. Some rework is major, some is fairly minor; for example, - We appear to have lost any notion of abstraction; nsObjectFrame, nsPluginInstanceOwner, nsPluginHostImpl, and nsPluginInstance all seem to be randomly inter-twined. These objects should be teased apart so that (e.g.) nsObjectFrame isn't twiddling the plugin instance directly. - We should be using nsIStatefulFrame rather than maintaining a list of stopped plugin instances in the plugin host object. - nsIObjectFrame should only have one method, |GetPluginInstance()| Filing this bug as a placeholder to track cleanup, simplification, and refactoring of the plugin code.
Reporter | ||
Comment 1•23 years ago
|
||
Reporter | ||
Comment 2•23 years ago
|
||
The above patch 1. Removes all the unnecessary methods from nsIObjectFrame 2. Separates nsPluginInstanceOwner into its own file, mostly for my own sanity. 3. Makes a first stab at pulling all the insanity out of nsObjectFrame::Reflow(), while cleaning up string-fu and poor style at the same time. 4. Dribbles stylistic cleanup and XXX comments throughout.
Status: NEW → ASSIGNED
Comment 3•23 years ago
|
||
Wow what a patch! Since youv'e sperated nsPluginInstanceOwner, nsPluginViewer.cpp could really use that version instead of the duplicate code inside the file now.
Reporter | ||
Comment 4•23 years ago
|
||
- The way we handle <object>s that are images is really gross. nsObjectFrame should really be a leaf, but it's a container because it might contain an image frame. Ick. I wonder if we should merge nsImageFrame and nsObjectFrame, or make each inherit from a common base class (nsReplacedFrame)? - Seems like the "clsid:browser" hack (does this even work?) should be moved out to the plugin module, and dealt with ``cleanly'' by registering a plugin handler for that clsid that instantiates a nested browser.
Comment 5•23 years ago
|
||
There is also some old ActiveX stuff that can be removed. cc:ing Karanze as he had fixed up nsObjectFrame::Reflow() before, but those changes caused some regressions.
Reporter | ||
Comment 7•23 years ago
|
||
Yeah, that patch above is a work-in-progress. It breaks Java, at a minimum (although I'm not sure why). Anyway, I'm going to try to break it out into bite- sized patches. On another tangent, I'm wondering why we need to have a separate object implementing nsIPluginInstanceOwner at all? One obvious reason would be if the back-end needed a refcountable object to hang its hat on (e.g., because the nsObjectFrame can go away). - nsPluginStreamListenerPeer wants to hold a strong ref to the owner, apparently so that it can instantiate the real plugin when the stream starts to arrive. This seems like the toughest case to handle, because >1 stream could be created per plugin instance; but it also seems like the only time that it really _needs_ the owner is when its the first inbound stream, and the plugin hasn't yet been instantiated. - The nsPluginStreamToFile also wants an owner, so that it can get the URL (but it seems like this could be computed at creation time). - The nsPluginInstancePeer obviously has an owner, but it seems like it doesn't need to hold an owning reference, and clearing its back-pointer on frame destruction would be sufficient to make it work right.
Reporter | ||
Comment 8•23 years ago
|
||
On another tangent, do we have a good set of plugin regression tests?
Comment 9•23 years ago
|
||
I don't know of any formal regression tests. I think we could really use some. You can find some tests at: http://slip.mcom.com/shrir http://panther.mcom.com/testcases I think it's hard to have automated regression tests for plugins because events and timing play a big role in reprouding bugs. Also the 4.x API is very strict, even a small could change the order of calls and break fragile plugins like Shockwave which do weird things like swap our plugin entry points while running and installing. I don't quite understand what you mean about nsIPluginInstanceOwner? One base class for both full-page and embeded plugins would be ideal instead of the dup code today. But I do think that nsIPluginInstanceOwner is needed. It's sort of a bridge between the plugin instances and layout. There is also the byte range case to handle, which is kind of tricky. In this case, any byte range streams for any plugin instances must stay open until either the plugin kills the stream or the plugin is stopped (for like a page transition).
Comment 10•23 years ago
|
||
Adding... bug 90268 object frame should be able to withstand a reframe bug 90256 Can't script Fullpage Plugins (Jazmin should attach a patch that claims to remove the need for dual nsPluginInstanceOwners)
Comment 11•23 years ago
|
||
adding... bug 83186 nsIPluginStreamListener2 should be removed
Depends on: 83186
Comment 12•23 years ago
|
||
The meta tracking plugin clean up bug has the the starter patch by Waterson to split nsPluginInstanceOwner to it's own file. -->Andrei..
Assignee: waterson → av
Status: ASSIGNED → NEW
Comment 13•23 years ago
|
||
based on the milestone for bug 89077, moving to future
Priority: -- → P3
Target Milestone: --- → Future
Comment 14•22 years ago
|
||
*** Bug 39907 has been marked as a duplicate of this bug. ***
Updated•22 years ago
|
Whiteboard: [PL2:P2]
Target Milestone: Future → mozilla1.2beta
Comment 15•22 years ago
|
||
Since the Summary on this bug has created much anxiety for a few folks, I am updating the Summary so as to help with that issue. Please note, this is the bug that the plug-ins team will use to annotate issues in regard to files within the plug-in module.
Keywords: meta
Summary: [meta] plugin code cleanup tracking bug → Current plug-in files need to be reviewed and updated to reflect the current state and status of each file, making adjustments and modifications as demed appropriate (formally titled: [meta] Plugin code cleanup tracking bug
Updated•22 years ago
|
Priority: P3 → P2
Comment 16•22 years ago
|
||
batch: adding topembed per Gecko2 document http://rocknroll.mcom.com/users/marek/publish/Gecko/Gecko2Tasks.html
Keywords: topembed
Updated•22 years ago
|
Comment 17•22 years ago
|
||
Peter, I guess this is a kind of duplicate of what you are working on now.
Assignee: av → peterl
Comment 18•22 years ago
|
||
moving to 1.4 beta: plug-in branch work
Whiteboard: [PL2:P2] → [PL:BRANCH]
Target Milestone: mozilla1.2beta → mozilla1.4beta
Updated•15 years ago
|
QA Contact: shrir → plugins
Comment 19•13 years ago
|
||
This bug is no longer useful for anything.
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → INVALID
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•