104298 - remove obsolete interfaces from the plugin directory

Status: VERIFIED FIXED

(Core Graveyard :: Plug-ins, defect, P3)

Description

[Judson Valeski]

The following files need to be removed from the tree.

nsIMalloc.h
nsIJRILiveConnectPlugInstPeer
nsIJRILiveConnectPlugin
nsILiveConnectPlugInstPeer
nsILiveConnectPlugin

Comment 1

[av (gone)]

--- Mass reassigning Unix bugs to serge ---

Comment 2

[serge (gone)]

OS == All

Comment 3

[av (gone)]

I should probably take this one as long it is not Unix specific. Unless you are
ready to fix it right now...

Comment 4

[scottputterman]

Moving Netscape owned 0.9.9 and 1.0 bugs that don't have an nsbeta1, nsbeta1+,
topembed, topembed+, Mozilla0.9.9+ or Mozilla1.0+ keyword.  Please send any
questions or feedback about this to [e-mail].  You can search for
"Moving bugs not scheduled for a project" to quickly delete this bugmail.

Comment 5

[Judson Valeski]

plussing this one. leaving these around just confuses plugin authors.

Comment 6

[rubydoo123]

The plug-ins triage team (av, beppe, peterl, serge and shrir) have reviewed this
issue and have made the following determination:

This clean-up will be addressed in the next milestone.

Comment 7

[av (gone)]

Attachment: list of files -- candidates for removal from the tree

In cleaning up the plugin module folders I followed these condiderations,
please let me know if I am wrong:

1. source file (either .cpp or .idl) is not used -- can be removed
2. header file is not included anywhere -- can be removed
3. interface is not used anywhere -- its header can be removed
4. I am also going to move nsIPluginHost.h, nsIPluginInstanceOwner.h and
nsPluginsCID.h from /src to /public

I tried to check all dependecies on all platforms, but could've missed
something, please review the attached list and let me know what should get
back.

Comment 8

[av (gone)]

Attachment: patch v.1

This patches the files that need to be updated due to this clean up.

Comment 9

[av (gone)]

Looks like the absense of those two will break Mac:

nsIPluginManager2.idl
nsIPluginStreamListener.idl

What else?

Another thing. Should not all interfaces be originated from idls rather than .h
files? Why do we have both in almost all occasions? Maybe this is a good time to
get rid of all nsI*.h files from the tree in favor of idls?

Comment 10

[av (gone)]

Comment on attachment 93451 [details] [diff] [review]
patch v.1

I am going to come up with another patch switching to .idl files completely.

Comment 11

[av (gone)]

Attachment: patch v.2

This is the full version of the patch which can be applied and tried.

Comment 12

[av (gone)]

Attachment: compact version of the patch v.2

This is for convenience only, it only contains modified files, not newly added
or removed files. I merely omit -N option in the cvs diff command.

Comment 13

[av (gone)]

Attachment: list of added and removed files

This is just a list of the files which are going to be cvs removed or cvs added
by the patch v.2.

Comment 14

[av (gone)]

I heard there are problems applying the patch. Somehow new files don't find
their way to the right location. I think the best way would be to remove old
files and add new files by hand and apply the light version of the patch which
is for existing files.

Comment 15

[av (gone)]

Attachment: patch v.2.1

This patch is identical codewise to patch v.2, I just performed more correct
cvs diff, so that full paths now appear in the Index: statement for each file.
This patch should apply nicely if you do it from /mozilla folder.

Comment 16

[av (gone)]

Somehow the patch utility needs some special instructions. Please use the
following command (or equivalent) when applying the patch: 

patch -p 0 -i patch.txt

Comment 17

[serge (gone)]

Comment on attachment 98517 [details] [diff] [review]
patch v.2.1

tested on linux, r=serge

Comment 18

[Brian Nesse (gone)]

Looking at this at av's request... This patch has a few issues.

 - the file /plugin/base/src/MANIFEST needs to be cvs removed too.
 - the reference to it in MozillaBuildList.pm needs to be removed also.
 - you can't change nsIJRILiveConnectPlugInstPeer.h to .idl the name is too long
   for the Mac file system (.h was already the max. supported length)
 - nspluginroot.idl needs a newline at the end of the file (cvs warning)

I can not test any farther until the filename issue is resolved, but at a 
minimum, the /macbuild/pluginIDL.xml will also require changes.

Also, in nsplugindefs.h please (please, please, please) :) change the definintion 
of nsPluginRect
struct nsPluginRect {
    PRUint16            top;
    PRUint16            left;
    PRUint16            bottom;
    PRUint16            right;
};
from PRUint16's to PRInt16's. Rectangles can and do have negative coordinate 
values. So far, I have only found this to be an issue in the debugger, but it 
could be a potential opportunity for bugs.

Comment 19

[av (gone)]

Brian, thank you very much for trying this out and commenting! I really
appreciate this. I will correct the patch for the comments.

The only problem is nsPluginRect. I think it has been defined this way in
nsplugindefs.h to mimic the corresponding definition in the old npapi.h. Is
there a danger that we can break legacy plugins if we do that? Also, I don't
think we should touch this file at all, it is a part of the deprecated API, if
we really want to address this issue we should probably revisit npapi.h but I
doubt this needs to a part of the current patch.

Comment 20

[Brian Nesse (gone)]

Unfortunately is seems that you are correct. The old plugin SDK does indeed
define these as uint16's... however incorrect that may be.

You are also correct that it does not need to be a part of this patch. Forget
about the request for now.

Comment 21

[av (gone)]

Attachment: list of files to be added or removed

Comment 22

[av (gone)]

Attachment: patch v.3

Brians comments addressed. I also renamed nsIJRILiveConnectPlugInstPeer.idl to
nsIJRILiveConnectPIPeer.idl.

Comment 23

[av (gone)]

Attachment: patch v.3.1

Forgot BackwardAdapter.cpp, updated patch.

Comment 24

[Brian Nesse (gone)]

Attachment: Some mac additions and fixes

I got the plugins to build with a little work. Attached are the xml project
changes and a change required in ns4xPluginsInstance.h. There is a problem with
having npupp.h as the first include file... it causes GENERATINGCFM to be
defined before the Mac system includes get a change to define it.

Part of your patch currently "removes" nsIJRILiveConnectPlugInstPeer.idl... you
should remove that since the file doesn't actually exist in CVS.

The next issue is that this totally breaks the MRJ plugin build. There are some
methods (RegisterWindow, UnregisterWindow, AllocateMenuID, DeallocateMenuID,
and HasAllocatedMenuID) which were defined in nsIPluginManager2.h, but are not
define in nsIPluginManager2.idl.

Comment 25

[av (gone)]

Attachment: patch v.4

Brian, thanks for trying the patch and comments! This patch addresses
everything you mentioned except your comment about removing a file which is not
in the repository. I think it still is:
http://lxr.mozilla.org/seamonkey/source/modules/plugin/base/public/nsIJRILiveConnectPlugInstPeer.idl


Please review.

Comment 26

[Brian Nesse (gone)]

My bad. I thought that was one of the new files you had created. I'll try and
look at the new patch soon.

Comment 27

[Brian Nesse (gone)]

Attachment: new pluginIDL.xml changes

With the changes av and I discussed offline and these changes to the mac idl
project, this builds on the mac.

Comment 28

[Brian Nesse (gone)]

Tested against all major plugins on Mac. All appear to work as before.

Comment 29

[av (gone)]

Attachment: patch v.5

Updated patch. nsIPluginInputStream.idl is added as it is needed by the
backward adapter. pluginIDL.xml is also updated to reflect this.

Comment 30

[Brian Nesse (gone)]

Comment on attachment 101310 [details] [diff] [review]
patch v.5

r=bnesse.

Comment 31

[Patrick C. Beard]

Comment on attachment 101310 [details] [diff] [review]
patch v.5

Some of the changes look gratuitous (e.g. parameter name changes in
nsIEventHandler.idl). I wonder if anybody is using nsIFileUtilities (I didn't
even know it existed, or forgot about it). sr=beard

Comment 32

[av (gone)]

Thanks for sr, Patrick. As to changing parameter names I was trying to follow
our naming convention when 'a' prefix is for 'argument', I remember reading this
on Mozilla.org.

Comment 33

[av (gone)]

In the trunk, marking fixed.

Comment 34

[shrirang khanzode]

.