Closed Bug 445391 Opened 12 years ago Closed 12 years ago

Re-enable OJI for Firefox 3.1

Categories

(Firefox Build System :: General, defect)

defect
Not set

Tracking

(Not tracked)

VERIFIED FIXED
mozilla3.1b1

People

(Reporter: jst, Assigned: jst)

References

Details

(Keywords: regression)

Attachments

(1 file, 3 obsolete files)

Given that there's unlikely to be a NPRuntime enabled Java plugin available for the Mac in time for our release of Firefox 3.1 we need to re-enable OJI for 3.1. Once 3.1 is out the door and a Java plugin that works w/o OJI exists for the Mac too we can disable it again and finally remove it from the tree, but for now we don't really have a choice other than to re-enable OJI.
Flags: blocking-firefox3.1+
Attached patch Re-enable OJI by default. (obsolete) — Splinter Review
This re-enables OJI :( The bulk of this change is to the JS engine due to its header files no longer compiling cleanly on windows when included from C code. Apparently inline is not recognized as a keyword in C when using devstudio (9), so this replaces two static inline functions with macros (yay!) and moves one into a .cpp file as it wasn't used anywhere else. I'm all ears if there's a better way to do this...
Attachment #331246 - Flags: superreview?(brendan)
Attachment #331246 - Flags: review?
Comment on attachment 331246 [details] [diff] [review]
Re-enable OJI by default.

Igor, could you do the review here? Thanks,

/be
Attachment #331246 - Flags: superreview?(brendan)
Attachment #331246 - Flags: review?(igor)
Attachment #331246 - Flags: review?
Comment on attachment 331246 [details] [diff] [review]
Re-enable OJI by default.

>diff -r eb6abd26a711 configure.in
>--- a/configure.in	Thu Jul 24 15:11:08 2008 -0700
>+++ b/configure.in	Thu Jul 24 18:07:11 2008 -0700
>@@ -4359,7 +4359,7 @@
> MOZ_NO_XPCOM_OBSOLETE=
> MOZ_NO_FAST_LOAD=
> MOZ_MEDIA=
>-MOZ_OJI=
>+MOZ_OJI=1
> MOZ_PERMISSIONS=1
> MOZ_PLACES=
> MOZ_PLAINTEXT_EDITOR_ONLY=
>@@ -5162,10 +5162,10 @@
> dnl ========================================================
> dnl = Open JVM Interface (OJI) support
> dnl ========================================================
>-MOZ_ARG_ENABLE_BOOL(oji,
>-[  --enable-oji           Enable Open JVM Integration support],
>-    MOZ_OJI=1,
>-    MOZ_OJI=)
>+MOZ_ARG_DISABLE_BOOL(oji,
>+[  --disable-oji           Disable Open JVM Integration support],
>+    MOZ_OJI=,
>+    MOZ_OJI=1)
> if test -n "$MOZ_OJI"; then
>     AC_DEFINE(OJI)
> fi
>diff -r eb6abd26a711 js/src/jsfun.cpp
>--- a/js/src/jsfun.cpp	Thu Jul 24 15:11:08 2008 -0700
>+++ b/js/src/jsfun.cpp	Thu Jul 24 18:07:11 2008 -0700
>@@ -2247,7 +2247,7 @@
> 
>     js_ReportValueError3(cx, error,
>                          (fp && fp->regs &&
>-                          StackBase(fp) <= vp && vp < fp->regs->sp)
>+                          STACK_BASE(fp) <= vp && vp < fp->regs->sp)
>                          ? vp - fp->regs->sp
>                          : (flags & JSV2F_SEARCH_STACK)
>                          ? JSDVG_SEARCH_STACK
>diff -r eb6abd26a711 js/src/jsinterp.cpp
>--- a/js/src/jsinterp.cpp	Thu Jul 24 15:11:08 2008 -0700
>+++ b/js/src/jsinterp.cpp	Thu Jul 24 18:07:11 2008 -0700
>@@ -81,6 +81,18 @@
> 
> /* jsinvoke_cpp___ indicates inclusion from jsinvoke.cpp. */
> #if !JS_LONE_INTERPRET ^ defined jsinvoke_cpp___
>+
>+static inline uintN
>+GlobalVarCount(JSStackFrame *fp)

This is a wrong place for GlobalVarCount. This section on Linux and MAC does not compile in the same compilation unit as js_Interpret that uses the function. So this function should also be turned into a macro.
Attachment #331246 - Flags: review?(igor)
(In reply to comment #1)
> Apparently inline is not recognized as a keyword in C when using devstudio (9).

Alternatively we can define something like js_static_inline that under C++ and C99 compilers will be defined as 

  #define js_static_inline static inline 

but for MSVC in C mode as

  #define js_static_inline static __inline

Note also that SpiderMonkey defines JS_INLINE, but this is to ensure that one absolutely wants to inline the function. This is not necessary with the inline functions declared in the header since in a debug build it is nice to have them as functions to be able to step through them in the debugger.
This version uses __inline when MSVC is in C mode and __inline__ for GCC to support code that compiles with -ansi with GCC. The patch also allows to redefine JS_INLINE and JS_ALWAYS_INLINE macros for performance testing.

Would it work on Windows?
Attachment #331289 - Flags: review?(jst)
The new version consistently uses defined XXX , not defined(XXX), in the preprocessor conditionals.
Attachment #331289 - Attachment is obsolete: true
Attachment #331292 - Flags: review?(jst)
Attachment #331289 - Flags: review?(jst)
Comment on attachment 331292 [details] [diff] [review]
using __inline with MSVC in C  mode v1b

Works wonderfully, awesome!.

Bsmedberg, wanna glance over the configure.in changes here?
Attachment #331292 - Flags: superreview?(benjamin)
Attachment #331292 - Flags: review?(jst)
Attachment #331292 - Flags: review+
Attachment #331292 - Flags: superreview?(benjamin) → superreview+
The previous version of the patch has a redundant static specifier in JS_ALWAYS_INLINE definition with GCC. This version fixes this.
Attachment #331246 - Attachment is obsolete: true
Attachment #331292 - Attachment is obsolete: true
Attachment #331529 - Flags: review?(crowder)
Comment on attachment 331529 [details] [diff] [review]
using __inline with MSVC in C mode v2

As I mentioned in another bug:  I like the idea of separating ALWAYS_INLINE and INLINE, so this patch makes me very happy.
Attachment #331529 - Flags: review?(crowder) → review+
landed - http://hg.mozilla.org/index.cgi/mozilla-central/rev/2d91c01ea27b
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
This was a regression with the patch on bug 397080. Running the testcase from bug 444881 I cannot the any remaining issues with the Java Embedding Plugin 0.9.6.4 (MRJ Plugin version 1.0-JEP-0.9.6.4).

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1a2pre) Gecko/20080811020618 Minefield/3.1a2pre ID:20080811020618
Blocks: 397080
Status: RESOLVED → VERIFIED
Keywords: regression
Duplicate of this bug: 444881
Target Milestone: --- → Firefox 3.1b1
Component: Build Config → General
Product: Firefox → Firefox Build System
Target Milestone: Firefox 3.1b1 → mozilla3.1b1
You need to log in before you can comment on or make changes to this bug.