We need to go back to using xpidl.exe for generating the Java interface files, because: (1) it provides a stand-alone util for generating Java interface file(s) directly from the IDL file; (2) xpidl can generate files with the original IDL comments and param names intact (this really helps out when using an IDE such as Eclipse for writing Java files); and (3) it integrates better with the build system.

This patch merges some of the changes found in extensions/java/xpcom/tools/xpidl into the main xpidl (whose Java support was broken anyway). The biggest change was to generate a separate Java interface file for each interface, since some IDL files contain multiple scriptable interfaces. Should this be handled in xpidl, or should the IDL files be fixed to only include on interface? This patched version of xpidl still cannot be used due to several issues: (1) Some frozen interfaces depend on non-frozen interfaces (bug 324026). (2) Some interfaces contain forward declarations of non-scriptable interfaces (bug 324035). There is no good way to handle this in xpidl (that's why I original created GenerateJavaInterfaces, which uses nsIInterfaceInfoManager, which has the correct information). One solution is to always output "nsISupports" for the interfaces that xpidl doesn't know about; but this leads to far too many paramter types incorrectly declared as "nsISupports". This patch always outputs the correct name; but as explained in the bug, the resulting Java interface files won't compile since some interfaces are missing.

Perhaps, to solve the issue of forward-declared non-IDL classes you could invent a [notidl] interface marker for the forward-declared class. When the java output encountered that interface, it could declare the Java version with nsISupports (or not declare it at all). Seems to me that it would not be necessary to expose the [notidl] marker in the XPT file, which is good because we don't have many flag bits left.

Comment on attachment 218066 [details] [diff] [review] xpidl patch timeless: can you go over this? I'll sr if you bless it with r.

Comment on attachment 218066 [details] [diff] [review] xpidl patch offhand, strcat (really the whole section) worries me because coverity will complain eventually. >+ /* >+ * Each Java interface must be in its own file. >+ */ >+ p = strrchr(state->filename, '/'); >+ if (p) { >+ strncpy(outname, state->filename, p + 1 - state->filename); >+ outname[p + 1 - state->filename] = '\0'; wrong ^^ indent >+ } >+ strcat(outname, interface_name); >+ strcat(outname, ".java"); >+ >+ state->file = fopen(outname, "w"); bad whitespace ^^ >+ if (!state->file) { >+ perror("error opening output file"); >+ return FALSE; >+ } >+ * Parse uuid and then output resulting nsID to string, to validate Parse uuid and output the resulting nsID as a string (?) >+ * uuid and normalize resulting .h files. I don't understand the bit about validating the uuid and normalizing the resulting .h files. especially, aren't you generating .java files? i could be confused. >+ * Write interface constants for IID >+ */ >+ if (iid) { >+ /* String NS_ISUPPORTS_IID = "{00000000-0000-0000-c000-000000000046}" */ Assuming this is the template, i believe it's missing a ';'. >+ write_indent(state->file); >+ fputs("String ", state->file); >+ write_classname_iid_define(state->file, interface_name); >+ fputs(" =\n", state->file); >+ write_indent(state->file); >+ write_indent(state->file); >+ fprintf(state->file, "\"{%s}\";\n\n", iid_parsed); >+#if 1 >+ /* >+ * In JavaXPCOM, we handle weak references internally; no need >+ * for the |nsIWeakReference| interface. So just return >+ * |nsISupports|. >+ */ >+ if (strcmp(className, "nsIWeakReference") == 0) { >+ fputs("nsISupports", state->file); >+ } else { >+ fprintf(state->file, "%s", className); >+ } >+#else >+ /* XXX How do we want to handle this? If it's an IDLN_INTERFACE, >+ * then we can just output the name of the class, since the IDL >+ * files exist for those classes. However, if it's an >+ * IDLN_FORWARD_DCL, some of those interfaces are not defined in >+ * IDL files, so we get an error when trying to compile the java >+ * files. So, for now, we just output them as the base iface >+ * (nsISupports). >+ */ Except, the comment is in an unreachable code block. which worries me. Could you make the compiler spit out a dummy .java file (date stamp: beginning of time) which it recognizes as being empty for each such creature. Such that when the compiler finds a real .idl file it would gladly overwrite this one, but it could otherwise use it? (some magic for ant or whatever would be needed to favor giving such a generated .class file a beginning of time date i suppose) >+ if (IDL_NODE_TYPE(param) == IDLN_PARAM_DCL && >+ IDL_tree_property_get(IDL_PARAM_DCL(param).simple_declarator, >+ "iid_is")) why is this indented two spaces past IDL_PARAM_DCL ? >+ { >+ state->tree = param; >+ goto handle_iid_is; >+/* fputs("nsISupports", state->file); */ I've become amazingly unfond of /* around code lines */ that used to be code and aren't meant as example code. (i prefer #if ...) Coverity will complain that this break is unreachable: >+ break; At certain points in this block you use strncmp, but not consistently. assuming there's a reason, i think it merits a comment. >+ if (strcmp(user_type, "PRInt8") == 0) { >+ fputs("byte", state->file); >+ } else if (strcmp(user_type, "PRInt16") == 0 || >+ strcmp(user_type, "PRUint8") == 0) { >+ fputs("short", state->file); >+ } else if (strcmp(user_type, "PRInt32") == 0 || >+ strcmp(user_type, "int") == 0 || >+ strcmp(user_type, "PRUint16") == 0) { >+ fputs("int", state->file); >+ } else if (strcmp(user_type, "PRInt64") == 0 || >+ strcmp(user_type, "PRUint32") == 0) { >+ fputs("long", state->file); >+ } else if (strcmp(user_type, "PRUint64") == 0) { >+ fputs("double", state->file); >+ } else if (strcmp(user_type, "PRBool") == 0) { >+ fputs("boolean", state->file); >+ } else if (strncmp(user_type, "char", 4) == 0 || >+ strncmp(user_type, "const char", 10) == 0 || >+ strncmp(user_type, "unsigned char", 13) == 0) { >+ if (IDL_tree_property_get(type, "ptr")) { >+ fputs("byte[]", state->file); >+ } else { >+ fputs("char", state->file); >+ } >+ } else if (strcmp(user_type, "nsIID") == 0) { >+ fputs("String", state->file); >+ } else if (strcmp(user_type, "nsString") == 0 || >+ strcmp(user_type, "nsAString") == 0 || >+ strcmp(user_type, "nsACString") == 0) { >+ fputs("String", state->file); >+ } else { >+ fputs("long", state->file); >+ } > >+ char *method_name = >+ g_strdup_printf("%c%s", >+ tolower(IDL_IDENT(method->ident).str[0]), >+ IDL_IDENT(method->ident).str + 1); i'd imagine this can theoretically fail :). >+#if 1 I wish you'd do #define SUCKY_HACK_WE_ARE_USING #ifdef SUCKY_HACK_WE_ARE_USING instead of #if 1. it'd give a hint about what's going on, instead of making me read to the #else clause (the define would be descriptive of what we're hacking around and doesn't need to include SUCKY or HACK in its name...) >+ if (method_notxpcom) >+ return TRUE; >+ if (method_noscript && !print_noscript_method(state)) >+ return TRUE; >+#else >+ /* do not write nonscriptable methods */ >+ if (method_notxpcom || method_noscript) { >+ return TRUE; >+ } >+#endif >+ /* XXX >+ Disable this for now. How do we specify exceptions? again, i'd prefer #if or better #ifdef RECORD_EXCEPTION_RECORD or something, where people can see that we don't do that. You can try talking to jst and dbradley about exceptions. might as well include biesi and bz (and myself) in such conversations. DOM kinda has them, but i think in a not particularly useful manner. > if (method->raises_expr) { > IDL_tree iter = method->raises_expr; > IDL_tree dataNode = IDL_LIST(iter).data; > > fputs(" throws ", state->file); > fputs(IDL_IDENT(dataNode).str, state->file); >@@ -561,42 +796,114 @@ method_declaration(TreeState *state) >+ } */ >+ * Whoops, it's some other kind of number >+ */ trailing whitespace, please strip. Note to self: next time i'm going to use the diff diff viewer in bugzilla, so, I hope that the next patch is compatible with it ;-) My comments are mostly generic, some suggestions are definitely very optional and don't need to be addressed in this patch (esp the dummy .java files), but I believe most of them are very reasonable. i'm very sorry for the delay. I need to work poking my request queue into my routine.

> Except, the comment is in an unreachable code block. which worries me. Could > you make the compiler spit out a dummy .java file (date stamp: beginning of > time) which it recognizes as being empty for each such creature. Such that when > the compiler finds a real .idl file it would gladly overwrite this one, but it > could otherwise use it? (some magic for ant or whatever would be needed to > favor giving such a generated .class file a beginning of time date i suppose) Let me see if I understand what you are suggesting. When xpidl comes to a type that is IDLN_FORWARD_DCL, it doesn't know whether the interface is defined in an IDL or not. So if a .java file doesn't exist for that interface, then xpidl should write out a dummy .java file that declares the interface, but with no methods or fields. Later, if an IDL does exist for that interface, the dummy gets over-written by the actual Java interface file. For example, nsIDOMNode might get first written as a dummy, and later over-written with full interface declaration. But nsIPresShell would only have a dummy file. Is this what you are saying? And this brings up the question of what does it mean for a method to take a non-IDL interface parameter? What would a Java dev do with a method that takes nsIPresShell? Now that I think of it, should these methods really be [noscript]?

(In reply to comment #4) > Could you make the compiler spit out a dummy .java file? There is another problem with this approach. xpidl cannot output the IID for these 'dummy' classes, since they are not defined by IDL files. So nsIPresShell.java would look like this: public interface nsIPresShell extends nsISupports { } No IID field. That means that queryInteface() won't work on the Java side for this method. Darin said in bug 338876: "the interface itself can still be used from script... script can call QueryInterface. we shouldn't prevent script from passing around interfaces as variables just because they don't have any scriptable methods." But this only works if the interface itself is scriptable (even if all its methods are [noscript]).

(In reply to comment #4) > >+ * Parse uuid and then output resulting nsID to string, to validate > Parse uuid and output the resulting nsID as a string (?) > >+ * uuid and normalize resulting .h files. > I don't understand the bit about validating the uuid and normalizing the > resulting .h files. especially, aren't you generating .java files? i could be > confused. This is code that I took from xpidl_header.c, but never changed the comment. I'll fix the comment. > You can try talking to jst and dbradley about exceptions. might as well include > biesi and bz (and myself) in such conversations. DOM kinda has them, but i > think in a not particularly useful manner. Opened bug 340015 for handling DOM exceptions. New patch on the way.

Addresses timeless' comments. Also, added workaround for bug 324035: basically, I just hardcoded the names of the non-IDL interfaces, and when xpidl encounters them as types, it writes out "nsISupports" instead.

Comment on attachment 224122 [details] [diff] [review] patch v1.1 :), sorry for the delay. only :(, you dropped strncmp in favor of strcmp, i was hoping you'd go the other way.

Build patch to use xpidl.exe to generate Java interfaces, rather than the GenerateJavaInterfaces.exe program.

Comment on attachment 226544 [details] [diff] [review] build patch >Index: config/config.mk >+# Java macros >+JAVA_DIST_DIR = $(DIST)/java Per discussion on IRC, this should be $(DEPTH)/_javagen/default if XPI_NAME isn't set, and $(DEPTH)/_javagen/$(XPI_NAME) if XPI_NAME is set. >Index: config/rules.mk >+ifdef MOZ_JAVAXPCOM >+ifneq ($(XPIDLSRCS)$(SDK_XPIDLSRCS),) >+$(JAVA_DIST_DIR):: >+ @if test ! -d $@; then echo Creating $@; rm -rf $@; $(NSINSTALL) -D $@; else true; fi I don't think you need the test ! -d bits here because the rule is for the directory itself. Just "$(NSINSTALL) -D $@" should do the trick. >+# generate .java files into $(XPIDL_GEN_DIR) >+JAVA_GEN_DIR = $(XPIDL_GEN_DIR)/org/mozilla/xpcom >+$(JAVA_GEN_DIR): $(XPIDL_GEN_DIR)/.done >+ @if test ! -d $@; then echo Creating $@; rm -rf $@; $(NSINSTALL) -D $@; else true; fi ditto here

This is the patch I'll check in. It addresses bsmedberg's comments, as well as cleaning up rules.mk a little more. As for installing the generated Java interfaces depending on $(XPI_NAME): only one file wasn't installed in 'default', and that was xulrunner/examples/simple/components/public/nsISimpleTest.idl. Benjamin, do you see this feature being more useful in the future? At the moment, this patch only creates a jar file for the interfaces in $(DEPTH)/_javagen/default, but none of the other dirs. That case will be easier to handle when with the patch from bug 328901.

> xulrunner/examples/simple/components/public/nsISimpleTest.idl. Benjamin, do > you see this feature being more useful in the future? absoutely. As we add tools to the SDK we'll probably have additional interfaces that aren't part of XR proper.

bsmedberg, I'm trying to come up with some Makefile rules to create the Java interface libraries for the modules with XPI_NAME set. It's pretty ugly, but this is what I came up with: ----------------------------- IFACE_MODULES = $(filter-out default, $(notdir $(wildcard $(JAVA_DIST_DIR)/*))) IFACE_MODULES_JARS = $(addsuffix Interfaces.jar, \ $(shell echo $(IFACE_MODULES) | perl -pe "s/\b([a-z])/\u\1/g")) IFACE_MODULES_SRC_JARS = $(IFACE_MODULES_JARS:.jar=-src.jar) iface_modules:: $(SDK_LIB_DIR) $(EXIT_ON_ERROR) \ for d in $(IFACE_MODULES); do \ $(NSINSTALL) -D _javamodules/$$d; \ find $(JAVA_DIST_DIR)/$$d -name "*.java" > _javamodules/$$d/java.files; \ $(CYGWIN_WRAPPER) $(JAVAC) $(JAVAC_FLAGS) \ -classpath $(IFACES_JAR)$(CLASSPATH_SEP)_javamodules/$$d \ -d _javamodules/$$d -sourcepath $$d @_javamodules/$$d/java.files; \ $(JAR) cf `echo $$d | perl -pe "s/\b([a-z])/\u\1/g"`Interfaces.jar \ -C _javamodules/$$d .; \ $(JAR) cf `echo $$d | perl -pe "s/\b([a-z])/\u\1/g"`Interfaces-src.jar \ -C $(JAVA_DIST_DIR)/$$d .; \ if test -z $(NO_DIST_INSTALL); then \ $(INSTALL) $(IFLAGS2) `echo $$d | perl -pe "s/\b([a-z])/\u\1/g"`Interfaces.jar $(SDK_LIB_DIR); \ $(INSTALL) $(IFLAGS2) `echo $$d | perl -pe "s/\b([a-z])/\u\1/g"`Interfaces-src.jar $(SDK_LIB_DIR); \ fi; \ done ------------------------ The hard part is that I have to create rules that depend on unknown names. Can you think of a better way to do this?

(In reply to comment #14) On IRC, bsmedberg suggested doing this as part of the XPI_PKGNAME packaging step (http://lxr.mozilla.org/mozilla/source/config/rules.mk#1599). One problem with this approach is that when the build gets to the XPI_PKGNAME step, MozillaInterfaces.jar hasn't been built yet (and the XPI module interfaces will depend on the interfaces in MozillaInterfaces.jar). The best way to make this work would be to build MozillaInterfaces.jar incrementally (if this is even possible).

Comment on attachment 224122 [details] [diff] [review] patch v1.1 Benjamin, do you feel OK with super-reviewing this patch, or should I ask brendan or darin?

Comment on attachment 224122 [details] [diff] [review] patch v1.1 In several places you #include "limits.h" or "windef.h"... please use <limits.h>

Checked in to trunk. ->FIXED

Asking for 1.8.1 approval. XULRunner only. Although patch touches xpidl files, it is for adding proper Java support to xpidl (which is only used by XULRunner builds).

Comment on attachment 236821 [details] [diff] [review] combined patch for 1.8.1 branch a=schrep for drives for XULRunner-only build patch.

Checked in to MOZILLA_1_8_BRANCH (along with bug 350489 and bug 351194).