202151 - Freeze OJI interfaces used by plugin

Status: RESOLVED WONTFIX

(Core Graveyard :: Java: OJI, defect, P1)

Description

[Peter Lubczynski]

We have several vendors using unfrozen interfaces in their plugins. It would be
nice if these interfaces would be marked somehow as to prevent future accidental
bustage.

OJI interfaces used by Java Plugin:
/modules/oji/public/nsIJRIPlugin.h
/modules/oji/public/nsIJVMConsole.h
/modules/oji/public/nsIJVMManager.idl
/modules/oji/public/nsIJVMPlugin.h
/modules/oji/public/nsIJVMPluginInstance.idl
/modules/oji/public/nsIJVMPluginTagInfo.h
/modules/oji/public/nsIJVMPrefsWindow.h
/modules/oji/public/nsISymantecDebugManager.h
/modules/oji/public/nsISymantecDebugger.h

New interface from bug 60304.
/modules/oji/public/nsIJVMAuthTools.idl 

XPCOM Plugin API interfaces:
/modules/plugin/base/public/nsIEventHandler.idl
/modules/plugin/base/public/nsIFileUtilities.idl
/modules/plugin/base/public/nsIJRILiveConnectPlugin.idl
/modules/plugin/base/public/nsIPlugin.idl
/modules/plugin/base/public/nsIPluginInstance.idl
/modules/plugin/base/public/nsIPluginInstancePeer.idl
/modules/plugin/base/public/nsIPluginManager.idl
/modules/plugin/base/public/nsIPluginManager2.idl
/modules/plugin/base/public/nsIPluginTagInfo.idl
/modules/plugin/base/public/nsIPluginTagInfo2.idl
/modules/plugin/base/public/nsIPluginTagInfo.idl
/modules/plugin/base/public/nsIPluginTagInfo2.idl
/modules/plugin/base/public/nsIWindowlessPlugInstPeer.idl
/modules/plugin/base/public/nsplugin.h
/modules/plugin/base/public/nsplugindefs.h

Comment 1

[Joshua Xia]

The process of freezing an interface is described here:
http://www.mozilla.org/projects/embedding/HowToFreeze.html

Louie, please help me to process this process,

Thanks!
->Louie

Comment 2

[Louie Zhao]

Attachment: patch version 1

patch to freeze interfaces of oji and plugin

Comment 3

[Doug Turner (:dougt)]

Louie, I can not r= that patch.  we need to sit down and formally review all of
these interfaces -- some may just get rubber stamped, but others have issues.

jjmata - can you set up a meeting for some time next week (in the afternoon) to
discuss these interfaces?

Comment 4

[Juan José Mata]

Sorry for the delay, setting up meeting shortly.  Watch the newsgroups for the
announcement as usual.

Comment 5

[timeless]

This is *very* disconcerting:
  * nsIPluginTagInfo2
  *
- * @status DEPRECATED
+ * @status FROZEN

Please allow me at least a week to read through your changes (before a meeting).
I'm currently on vacation but i might be forced to abandon it to address this
bug. :-(

Comment 6

[Doug Turner (:dougt)]

I agree.  That change is bothersome.  However, i think we should plan to meet
this week to discuss what the plan of action should be.  We will be posting what
we decide.  

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 120798 [details] [diff] [review]
patch version 1

Well the most lovely thing about the lines timeless mentioned is the next line:

- * @status DEPRECATED
+ * @status FROZEN
  *
  * Originally published XPCOM Plugin API is now deprecated

whichever way you go, you should not contradict yourself in the next line...

Comment 8

[Boris Zbarsky [:bzbarsky]]

> However, i think we should plan to meet this week

As you plan, keep in mind that people who are not working on Mozilla full-time
typically need some warning to get out of whatever they normally do and attend
such meetings.  For example, scheduling a meeting today for day after tomorrow
would be an excellent way to guarantee that no such people would be able to attend.

Comment 9

[Jack Lu]

Even though XPCOM plug-in API is deprecated, we should still support it since
the current OJI plug-in is essentially a XPCOM component. If we can't freeze
those interfaces, OJI plug-in will still be in a unstable state. 

If we can't freeze those deprecated interface, does that mean OJI plug-in should
be rewritten to be a NPAPI plugin?

Comment 10

[Louie Zhao]

>- * @status DEPRECATED
>+ * @status FROZEN
>  *
>  * Originally published XPCOM Plugin API is now deprecated
>whichever way you go, you should not contradict yourself in the next >line...

Thanks for pointing this out. After confirming what interface should be frozen
this week, I will update the patch.

Comment 11

[Louie Zhao]

Attachment: update patch

This is the update version of the patch for "OJI interface freeze". In this
patch, I have adjusted the coding format of these IDL files to make them more
readable.

I have 1 question which needs help:

Most of interfaces in "plugin" module has been "DEPRECATED" because the plugin
which uses them need the knowledge of XPCOM. NPAPI is suggested to use instead
of such interfaces because it's designed following C standard. My question is 
Can one interface have both "DEPRECATED" status  and "FROZEN" status?
I think it's reasonable because it will mark such interface both "can't be
modified" and "not suggested to use". The 2 status won't confict.
 
Alec, I have gone through the latest "IDL interface rules" of Mozilla. Here I
should mention some issues related this patch.

Most of interfaces in "OJI" and "Plugin" module have been checked in for a long
time. Many plugins provided by third-part vendor depend on these interfaces. If
we change the methods and properties in such interfaces, those plugin won't
work. The IDL rule is reasonable and should be implied for new interfaces, but
I think we can preserve the old interfaces in "OJI" and "Plugin" module.

For example,  According to rule "User interCaps for naming", we should change
the name of some methods and properties . But then the plugins won't recognize
such interfaces and won't work unless they also change accordingly.

For rule "Use ACString to represent ASCII strings", we should change argument 
type "string" to "nsACString". "string" will be converted to "char *" which
plugin (e.g. JPI) can recognize. But JPI has no idea about class "nsACString".

Some interfaces put CID in IDL file (e.g. nsIJVMManager.idl), that is for
convinence. For example, JPI can include only 1 interface definition file and
then use it. The rule "Avoid CID and ContractID declaration in IDL" is
reasonable inside Mozilla. But I think it can be flexible when it is used to
interfaces targetd to outside of mozillla.

Below is some examples for the questiones above, most of the interfaces to be
frozen have these issues. So My advise is to preserve these interface although
it won't fit all the "interface rules".

Alec, how about your opinion?

http://lxr.mozilla.org/seamonkey/source/modules/oji/public/nsIJVMPluginInstance.idl#61

http://lxr.mozilla.org/seamonkey/source/modules/oji/public/nsIJVMManager.idl#60

Comment 12

[timeless]

since plugins are c/c++ which would use .h files generated from .idl files
and since the idl compiler converts interCaps (.idl) to InitialCaps (.h)
changing the .idl declarations to interCaps should not cause any problems, but
you should certainly check :)

Comment 13

[Alec Flett]

Louie, we discussed this in the meeting, sorry if I didn't communicate this well
- since these interfaces are used by shipping products that we really care about
(i.e. JVMs from all sorts of vendors) we should freeze the interfaces themselves
as is... 

however, the rest of the "interface rules" such as predeclaring interfaces,
avoiding excess #includes, avoiding %{C++ %} and so forth, should still be
followed.. it looks like you are doing some of this already! Basically we want
to maintain binary compatibility, but build-time compatibility should not be as
much of an issue.

You wrote:
> Some interfaces put CID in IDL file (e.g. nsIJVMManager.idl), that is for
> convinence. For example, JPI can include only 1 interface definition file and
> then use it. The rule "Avoid CID and ContractID declaration in IDL" is
> reasonable inside Mozilla. But I think it can be flexible when it is used to
> interfaces targetd to outside of mozillla.

I think that here you should still try to follow the rules.. we are trying to
have a consistent API story (which includes interfaces targeted to outside
mozilla) and OJI is only a piece of this larger puzzle. What you can do is make
a more global header (like nsJVMManager.h) which #includes nsIJVMManager.h.
Consumers (such as Sun's JVM) will have to update their source the next time
they try to do a build against mozilla 1.4, but binary compatibility will not be
broken.

The rules you should probably make sure you ARE following are:
- Avoid excess #includes
- Try to #include only other .idl files.
- Avoid CID and ContractID declarations in IDL or C++ interface declarations

It may be that this patch covers all of these issues - but I wanted to
explicitly point out the ones that you can still follow without breaking binary
compatibility.

overall this patch looks great.. I'd just like to see the CIDs and other C++
moved into C++ header files.

Comment 14

[Louie Zhao]

Attachment: patch following alec's advise

Following Alec's comments, this patch abide by below IDL rules:

- Avoid excess #includes
- Try to #include only other .idl files.
- Avoid CID and ContractID declarations in IDL or C++ interface declarations

This patch won't break binary compatibility. The shipping products who use the
new IDL & head file to build their source will change their source code
accordingly -- fortunately, not much.

Alec, in this patch, there is only 1 special case.
In nsIJVMManager.idl, the original code has defined "GetCID" in nsIJVMManager,
which needs CID. Since JPI code has used nsIJVMManager::GetCID, I can't get rid
of CID and "GetCID" from nsIJVMManager.idl completely. Then I move CID to
nsOJICID.h and include nsOJICID.h in nsIJVMManager.idl.

Alec, do you have any suggestion about "DEPRECATED" and "FROZEN" I mentioned in
the previous comment?

zhengyu, I found NS_JVMPREFWINDOW_CID and NS_LIVECONNECTMANAGER_CID have
definition, but have never been used both in Mozilla and JPI side. So I delete
them directly. Can you confirm this? Thanks.

Thanks for help.

Comment 15

[Louie Zhao]

Alec, nsplugin.h and nsplugindefs.h are not IDL file and contain no interface
definition. nsplugindefs.h is used by other IDL file. nsplugin.h is a
"collection" of other IDL head file. Should they both be frozen, or stay unfrozen?

Comment 16

[Joshua Xia]

Please somebody help to r/sr this patch ASAP because Sun Java Plugin team want
to check in some fix on JPI　side, they expect to freeze the interfaces that JPI
used.

Thanks a lot! 

Comment 17

[Alec Flett]

Comment on attachment 123767 [details] [diff] [review]
patch following alec's advise

adding to my request queue

Comment 18

[Alec Flett]

I'll try to get to this today, but next time please request the review sooner -
you can't expect a one-day turnaround!

Comment 19

[Alec Flett]

Comment on attachment 123767 [details] [diff] [review]
patch following alec's advise

+#include "nsOJICID.h"

this is the right idea, to put the cids/contractids into one header, but they
still shouldn't be included from any of the .idl files. The burden needs to be
on the consumers. yeah, I know this will break existing builds, but as we
agreed above, we could allow for build-time breakage as long as we maintain
binary compatibility.

-    nsIAuthenticationInfo GetAuthenticationInfo(in string protocol,
-						 in string host,
+    nsIAuthenticationInfo GetAuthenticationInfo(in string  protocol,

these are only used by C++ at the moment, right? if that's the case, lets still
use interCaps - this should become getAuthenticationInfo - the C++ api will
stay the same, but we'll be consistent with our IDL. (The only thing I'd worry
about is if an OJI plugin used xpconnect to call back through this interface,
because to xpconnect, this is an effective name change. Can you double-check?)

same goes for other methods like SetAuthenticationIssue()

+ @status FROZEN

do you mind if we mark these UNDER_REVIEW so at least the sun java team can try
building against these headers and make sure we don't need any final tweaking?
I'm reluctant to go directly from unfrozen->FROZEN without at least someone
exercising the changes.


Also, I notice you're freezing a LOT of interfaces. do we have a definitive
list of interfaces that sun needs? I'm reluctant to freeze say nsIFileUtilities
if it is something that mozilla's OJI impl. uses internally.

this is really close.. we just have to finish hashing out these last issues!

Comment 20

[Louie Zhao]

Attachment: updated patch

updated the previous patch following alecf's comments.

> +#include "nsOJICID.h"
> they still shouldn't be included from any of the .idl files.

I know it breaks the XPCOM rule, but it's a special case for JPI. JPI use
nsIJVMManager::GetCID() to get CID from interface. "nsIJVMManager.idl" uses
macro

      NS_DEFINE_STATIC_CID_ACCESSOR(NS_JVMMANAGER_CID)

to provide "nsIJVMManager::GetCID()". That's why we need "#include "nsOJICID.h"
in "nsIJVMManager.idl", or we won't maintain binary compatibility.

> these are only used by C++ at the moment, right?

I have changed (S)GetAuthenticationInfo to (s)getAuthenticationInfo in this
patch. As far as I can see, these methods are only used in C++ by JPI.

zhengyu, can you confirm this from JPI side?

> do you mind if we mark these UNDER_REVIEW so at least the sun java team can
> try building against these headers and make sure we don't need any final
> tweaking?

I think JPI team will be the best one to answer this question.

zhengyu, can you add comments on this? Thanks.

> do we have a definitive list of interfaces that sun needs?
> I'm reluctant to freeze say nsIFileUtilities nsIFileUtilities
> if it is something that mozilla's OJI impl. uses internally.

The interface list is provided from JPI team. I checked the source code of JPI,
all these interfaces are used by JPI code, that's why we need freeze these
interfaces.

zhengyu, can you confirm that whether all these interfaces are needed to be
frozen?

Comment 21

[Louie Zhao]

Zhengyu can't access bugzilla. Below is the comments from zhengyu.

---------------------------------------------------------------------------
> I have changed (S)GetAuthenticationInfo to (s)getAuthenticationInfo in this
> patch. As far as I can see, these methods are only used in C++ by JPI.
> zhengyu, can you confirm this from JPI side?

This change will break our build once we pull latest Mozilla headers.

> do you mind if we mark these UNDER_REVIEW so at least the sun java team can
> try building against these headers and make sure we don't need any final
> tweaking?

> I think JPI team will be the best one to answer this question.

> zhengyu, can you add comments on this? Thanks.

As our current policy, plug-in can not use any un-frozen interface. It has been
so many time I had to in/out the changes into different versions of plug-in code
base, due to the interface changes that affect release that carries the fix. I
would rather not to do it again this time, and would rather wait the interfaces
to be frozen and committed to a release.

Comment 22

[Darin Fisher]

Louie,

> I have changed (S)GetAuthenticationInfo to (s)getAuthenticationInfo

this will have no impact on the C++ code.  therefore, this should not break any
C++ code using the interface.

>As our current policy, plug-in can not use any un-frozen interface.

I really don't understand this comment.  None of OJI has been marked as FROZEN,
and yet JPI currently talks to OJI.  In fact, it has done so for ages.

It seems to me that there is no need to worry about this interface being marked
frozen or not.  We have all agreed that it will not change.

Comment 23

[James.Melvin]

>> I really don't understand this comment.  None of OJI has been marked as FROZEN,
>> and yet JPI currently talks to OJI.  In fact, it has done so for ages.

True.  JPI talks to OJI which is not frozen.  Due to the business climate under
which OJI was developed, Sun did not originally require OJI interfaces to be
frozen.  In retrospect, this was a serious mistake.  Sun has invested literally
man-years chasing down changes in the interface on the browser side.  We'd like
to prevent breakage-on-delivery and history repeating itself.

Now that the browser is co-bundled with Solaris, contracted interfaces are an
absolute requirement.  Therefore, Sun would like to request that the
super-review process for this new interface continue to completion so we can
move forward with the Java side of the implementation.  We are forward porting
the current Java 1.4.1++ implementation to the latest 1.4.2x update release
anticipating that further super-review comments and adjustments will be residual
rather than wholesale in nature.  When FROZEN, we can move quickly to integrate
the changes into the next available Java release and satisfy business
escalations by some of our key customers and partners.

My team can be available for a concall to discuss details and answer questions
especially if that will help in the review process.

Respectfully,

Jim Melvin
Engineering Manager
Java Plug-in
Sun Microsystems

Comment 24

[James.Melvin]

Any traction on the super-review?  Have we been derailed by the transition to
the Mozilla Foundation?  Also, will there be a stable Mozilla Community release
beyond 1.4?  The roadmap only mentions riskier changes and instability beyond 1.4.

Comment 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We don't freeze interfaces anymore.