68933 - Bad implementation of IsJavaEnabled and JavaEnabled

Status: VERIFIED FIXED

(Core Graveyard :: Java: OJI, defect)

Description

[Mike Kaply [:mkaply]]

When 61474 was fixed, the function JavaEnabled was created. This was unnecessary 
since IsJavaEnabled already existed.

Checkout:
http://lxr.mozilla.org/seamonkey/source/modules/oji/src/nsJVMManager.cpp#147
and
http://lxr.mozilla.org/seamonkey/source/modules/oji/src/nsJVMManager.cpp#155

This is the same function implemented twice.

The code needs to be cleaned up so that there is one function that does this, 
not two.

Comment 1

[edburns]

Hi Xiaobin,

Can you please respond to the reporter with your reasoning for your fix to 61474?

Comment 2

[Xiaobin Lu]

 Reporter:
    The reason I added a new method to nsIJVMManager.idl is that the exsisting 
one "IsJavaEnabled" method does need return a reference to the callee, but we 
need the return value instead of reference because in Javascript there is no 
way to use a reference. 
    "JavaEnabled" method I added is to return a boolean value so that I can use 
it in Javascript. Of course, we can modify the IsJavaEnabled method to let it 
return a vlaue, however, I am afraid it will break some other code which may 
use this function to return a reference. 

Comment 3

[Xiaobin Lu]

Michael:
    What do you think about this?

Comment 4

[Mike Kaply [:mkaply]]

Sorry, I had thoughts and forgot to write them down.

I think we need to create one implementation here and fix the people that call 
it.

It looks really strange to have two functions that have exactly the same code 
next to each other.

I know it had to be fixed quickly, but I'd like to keep this around for the 
cleanup of this code.

Which API do you think is the "correct" name - IsJavaEnabled or JavaEnabled?

I'll look at putting this one on my plate to cleanup.

Comment 5

[Xiaobin Lu]

 Use IsJavaEnabled is OK! Thanks very much!

Comment 6

[Xiaobin Lu]

Because this bug is related to the bug 56124 I am now fixing, I will take this 
over.

Comment 7

[Xiaobin Lu]

Attachment: JavaEnabled is removed.

Comment 8

[Xiaobin Lu]

  Johnny made all this become possible, he changed navigator.javaEnalbed() 
implementaion. Currently, only java is enabled and installed, the javaEnabled() 
return true.See detail in bug 56124.
Michael:
   Could you review the patch? Thanks!

Comment 9

[Xiaobin Lu]

Attachment: Tab is changed to 2

Comment 10

[Xiaobin Lu]

Ben:
    Could you review this bug for me? Thanks very much!

Xiaobin

Comment 11

[Xiaobin Lu]

Sorry, Ben. I mean review the last patch for me.

Comment 12

[Mike Kaply [:mkaply]]

Why did you choose to add a function to PluginArray rather than something like 
this:

#include "nsIPluginHost.h";

static NS_DEFINE_CID(kRDFXMLDataSourceCID, NS_RDFXMLDATASOURCE_CID);

nsCOMPtr<nsIPluginHost> pluginHost (do_GetService(kPluginManagerCID, &rv));

if (NS_SUCCEEDED(rv))
   if (pluginHost->IsPluginEnabledForType("application/x-java-vm") == NS_OK)
      *aReturn = PR_TRUE
   else
      *aReturn = PR_FALSE;

Was it to avoid the mulithreaded assertion?

Comment 13

[Xiaobin Lu]

 Please see bug 56124 for the reason. I think either way is OK. I prefer to 
leave it there.
 Thanks for asking anyway!

Comment 14

[edburns]

Do you feel like this bug should be closed?

Comment 15

[Xiaobin Lu]

   I am asking for "sr=" and "a=" for the patch.

Comment 16

[Mike Shaver (:shaver emeritus)]

- my comments in 56124 about making mPluginHost an nsCOMPtr and using 
  do_GetService in GetPluginHost still hold.
- while you're changing the IDL, could you:

= Change the case of methods to begin with lowercase, as per the IDL dominant
    style.

= Change single-value accessors to be attributes, such as:

     readonly attribute boolean isJavaEnabled; /* javaIsEnabled? */

= Change methods with a void return and an out parameter to return the value
  directly:

     void IsAllPermissionGranted(in string lf, in string lcn, in string rootf,
                                 in string rcn, out boolean isGranted);

  becomes:

     boolean allPermissionGranted(in string lf, in string lcn, in string rootf,
                                  in string rcn);

  (Note: changing a trailing-out to a return involves no C++ changes.  Also, I
   think the latter name is better, to give you
   
     if (gJVM.allPermissionGranted(...))

   but I won't hold up the SR on a naming nit like that.)

Remember to update the (apparently few) JS uses to match the new case!

Other than that, it looks good.  With those changes, sr=shaver.

Comment 17

[Xiaobin Lu]

Hi, Mike:
     Honestly speaking, I do really agree what you pointed here. However when I 
first time want to do these changes, I found much more things than I expected. 
Because the java plugin side imports the nsJVMManager.cpp file, if I change the 
interface file, I need to do some change in the implementation file which will 
probably break the java plugin side. Obviously that is not we want to see. 
Anyway, thanks in advance for your super review and approval. Really appreciate 
for your help on this.

Comment 18

[Mike Shaver (:shaver emeritus)]

I don't follow: your diff is _already_ changing the interface file
(nsIJVMManager.idl), no?

Anyway, fixing the case in IDL and converting trailing-out to return will not
cause any changes in the C++ interface headers, so they're safe regardless.

Comment 19

[Xiaobin Lu]

Mike:
    I mean if I change the single trribute accssor to an attribute. The method 
name gnerated by the idl file will be changed, something like GetJavaEnabled(). 
That maybe not we wanted, right? Of course, I can change the case and trailing 
out except do this change. Is that OK?
 
    

Comment 20

[Xiaobin Lu]

Attachment: The third iteration, IDL file was changed

Comment 21

[Xiaobin Lu]

Mike:
   Would you like to give me a super review for the patch of 56124 & 68933? 
Thanks in advance!

Xiaobin

Comment 22

[Mike Shaver (:shaver emeritus)]

(Is this patch supposed to address both bugs?  I don't see a new patch in 56124.)

I don't understand how it's OK to remove JavaEnabled but it's not OK to change
IsJavaEnabled to GetJavaEnabled in the C++ code.  Can you explain to me exactly
what kind of breakage you'll see with the latter that you won't see with the former?

Also, I'm still interesting in having mPlugins be an nsCOMPtr, or at least
knowing why it can't be.

The IDL changes so far look good, though your indentation is a little weird: are
you using tabs?

Comment 23

[Xiaobin Lu]

  JavaEnabled function is added by me(please see detail in bug 64174) to 
resolve the IsJavaEnabled is not well written(it does not return a vlaue, 
instead it use parameter to return a vlaue). I know it is not good to add a 
method to an well-done interface. So this is why this bug is called "Bad 
implementation of JavaEnabled". Right now I just remove that function because I 
can use the function which bug 56124 provides. So you are right, this patch 
also solves the problem of 56124.
   I will solve the idl indentation problem and the nsCOMPtr problem.
   Thanks for asking and discussing with me!

Comment 24

[Xiaobin Lu]

   The reason I don't make mPlugins as a nsCOMPtr is I assume:
    if nsCOMPtr<A>, A must be an interface. I think that will make sense. 
   The reason I don't make mPluginHost as a nsCOMPtr is nsIPluginHosts is 
defined as a structure,not an interface. 
    Please point my mistakes if I made. 

Comment 25

[Xiaobin Lu]

Attachment: IDL file indentation was corrected.

Comment 26

[Xiaobin Lu]

Mike:
   Would you give me a sr to the patch? Thanks in advance!

Comment 27

[Mike Shaver (:shaver emeritus)]

I have it on moderately-good authority that you can use nsCOMPtrs on
non-interface classes, as long as they inherit from nsISupports, which
PluginArrayImpl does.  Copying scc for confirmation of that.

Sorry to harp, but I still haven't heard an explanation of why we can't fix the
rest of the interface glitches when it's safe to break the source-and-binary
interface to remove IsJavaEnabled.  I might just be dense, but this seems like a
(rare) opportunity to fix this stuff up (we're usually told that we can't touch
it, because something off in the nether regions of the closed-source Java Plugin
will break) while we're in here changing things anyway.

Comment 28

[Brendan Eich [:brendan]]

You can use an nsCOMPtr with a concrete type if the class inherits singly from
nsISupports, but scc may favor the alternative of holding the strong ref by an
nsCOMPtr<nsIFoo> type (where nsIFoo may be nsISupports, if the concrete class
inherits no narrower interface), and keeping a weak ref to the concrete class
instance in tandem.

/be

Comment 29

[Xiaobin Lu]

Hi, Mike:
   Thank you for correcting me about the nsCOMPtr thing. I will change that to 
nsCOMPtr.
  
   Regard to the interface idl, I found that the java plugin side 
used "IsJavaEnabled" function which refers to the browser side "IsJavaEnabled". 
If I removed that function,I think it will definitely cause problems. I don't 
want to take such risk. Please correct me if I am wrong here.

   Thanks to Brendan for nsCOMPtr!

Xiaobin Lu

   

Comment 30

[Xiaobin Lu]

I tried to convert the mPlugins into using nsCOMPtr. It turns out the ambiguity problem. Looking into the PluginArrayImpl class, it seems that it inherits two interfaces which both inherits from nsISupports. Is that a problem? 

Johnny:
   How do you think about this?

Comment 31

[Xiaobin Lu]

Attachment: IDL was changed and mPluginHost use nsCOMPtr

Comment 32

[Johnny Stenback (:jst)]

I yes, PluginArrayImpl does (currently, this will soon change) implement more
than one interface so I don't think it's possible to make mPlugins be an
nsCOMPtr, and from looking at the code where mPlugins is used we only do
refcounting in a very small number of places so I don't see a good reason to
make it an nsCOMPtr. Sure, it would be nice if we could, but I don't think we
can easily do that, and I don't think there's a strong argument for doing it in
this case either. I say leave it as is. If the refcounting got more complex than
trivial here I'd change my mind.

Comment 33

[Xiaobin Lu]

Mike:
    I have posted the new patch. The new patch contains changes of:
1. Remove the isJavaEnabled, change all the place using IsJavaEnabled to 
GetIsJavaEnabled;
2. Change the idl function name from upcase to lowercase;
3. mPluginHost of PluginHostImpl has been changed to use nsCOMPtr;
4. Patch of bug 56124.
  Please give the patch a sr= and maybe a=. Thanks very much!

Ed:
   Would you please review this patch for me? Because the IsJavaEnabled method 
is removed from IDL file and it has been changed to use an attribute, I would 
like you to review the patch. Thank you in advance!

Comment 34

[Xiaobin Lu]

Thanks, Johnny!

Comment 35

[timeless]

GetIsJavaEnabled sounds really awkward. could we use GetJavaEnabled?

Comment 36

[Mike Shaver (:shaver emeritus)]

I agree with timeless about the naming.  If you change the attribute to be:

  readonly attribute boolean javaEnabled;

sr=shaver (no need to repost patch).

Thanks for your patience, and sorry it took so long.

Comment 37

[Xiaobin Lu]

  Shaver, Thank you! I will rename it and check it into the trunk.

Comment 38

[Xiaobin Lu]

  I will have another around of test and if it is ok, it will be checked into 
the trunk.

Comment 39

[Xiaobin Lu]

*** Bug 56124 has been marked as a duplicate of this bug. ***

Comment 40

[edburns]

Let me know when you want me to review your changes.

Ed

Comment 41

[Xiaobin Lu]

Attachment: Some naming mistakes were corrected

Comment 42

[Xiaobin Lu]

Ed:
   Thanks for asking. I defintely need your review. Basically this patch has 
two parts. The first one is for 56124. Johnny changed the implemtation of 
JavaEnabled to make it more reasonable. After the patch, JavaEnabled method 
returns true only when the preference is enabled and java is installed in your 
system. 
   The second part is to remove the JavaEnabled(0 method from nsIJVMManager.idl 
method. Also by request of Shaver, I also corrected the naming of the methods 
definition. The significant change is to change the IsJavaEnabled method to a 
sigle attribute. That's it! 
   Thanks in advance!

Comment 43

[edburns]

Are you sure that all the files in your diff are current?

nsPluginArray.cpp?

Why is this file modified in this diff?

Other than that, r=edburns.

Ed

Comment 44

[Xiaobin Lu]

Thanks Ed! I will get the latest diff and check it into the trunk.

Comment 45

[Xiaobin Lu]

Ed:
   The reason why the nsPluginArray.cpp is modified is after getting the 
pluginArray, we need to use it to get pluginHost which has a method 
called "IsPluginEnabledForType(const char*)". So we need to add a method to let 
the pluginarray have such capability to call that method.
  This change is due to the modification of navigator.javaEnabled() 
implementation. Please see details from bug 56124.
   Thanks for asking! 

Comment 46

[edburns]

Ok have you gotten review from [e-mail] and super review?

Comment 47

[Xiaobin Lu]

Ed:
   I got the super review and approve from Mike Shaver and I will check this 
into trunk today.

Comment 48

[Xiaobin Lu]

Patch checked in!

Comment 49

[Peter "jag" Annema]

I thought the r= and sr= needed to be two separate people?
Some of your checkins make it seem like the code was r=/sr= shaver:
http://bonsai.mozilla.org/cvsquery.cgi?module=MozillaTinderboxAll&branch=HEAD&cvsroot=/cvsroot&date=explicit&mindate=986422000&maxdate=986423700&who=Xiaobin.Lu%25eng.Sun.com

You added a #include for nsRDFCID.h, which is completely unnecessary.

I've just removed that #include because it was breaking senna (it keeps track of
inter-module dependencies which weren't updated with the new dependency on rdf).

Comment 50

[Xiaobin Lu]

Thanks. Peter! I will do it in the right way next time.

Comment 51

[John Unruh]

Verified.