Closed Bug 461841 Opened 16 years ago Closed 15 years ago

javascript engine fixes for windows mobile

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: dougt, Assigned: jimb)

References

Details

Attachments

(3 files, 6 obsolete files)

We reduce our dependency on many functions in the windows mobile shunt.  Because of this reduction, there are some slight modifications we need in the js code.
Attached patch patch v.1 (obsolete) — Splinter Review
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #346108 - Flags: review?
Attachment #344976 - Attachment is obsolete: true
Attachment #346108 - Flags: review? → review?(crowder)
Comment on attachment 346108 [details] [diff] [review]
patch v.2

Again, I think we want something different than ifdef WINCE for these sorts of things.  Can't we autodetect header files which the features being cut out here depend on, or something similar?
Hoping for thoughts from either Jim Blandy or Ted (or both) about getting these preprocessor guards right, wrt autoconf
Comment on attachment 346108 [details] [diff] [review]
patch v.2

I think this needs to be freshened up a bit (at the very least, the INT_PTR stuff is handled elsewhere (bug 464178, maybe?)
Attachment #346108 - Flags: review?(crowder) → review-
Some of this could be handled with some AC_CHECK_HEADERS and AC_CHECK_FUNCS calls in js/src/configure.in:

AC_CHECK_HEADERS([io.h])
AC_CHECK_FUNCS([isatty])

and then in the code:

#ifdef HAVE_IO_H
...
#endif

#ifdef HAVE_ISATTY
  isatty(foo);
#endif
JimB:  Any chance you wanna give it a whirl?  :)
Jason Orendorff has promised to file a bug we can work together on to make the <stdint.h>-style types available on all platforms.  Once that's filed, I'll make this bug depend on that, and that'll get rid of the intptr_t / uintptr_t lossage.
Depends on: 465640
Assignee: general → jim
any update here?
There was some motion on the prerequisite bug today, so I think we're moving forward, and this should be done correctly soon.
Attachment 346108 [details] [diff] patches prmjtime.cpp to dyke out the definitions of NowCalibrate, and most of the body of PRMJ_Now.  Which functions, specifically, are unavailable on WinCE?  Those are what I'd like to test for in the configure script.
[submitted to Try server; just posting for comments.]

This should cover the issues addressed by patch.v2 in bug 461841 in
js/src/xpconnect/shell/xpcshell.cpp.  It doesn't deal with the
js/src/prmjtime.cpp issues.

Check for the io.h header, and the setbuf and isatty library
functions.  Use the autoconf-#defined macros to decide what to
#include and use in js/src/xpconnect/shell/xpcshell.cpp.
Comment on attachment 354205 [details] [diff] [review]
Bug 461841: Use autoconf to check for functions and headers missing on WinCE.

bsmedberg kindly points out that we can't do AC_TRY_COMPILE checks for Windows features.  Back to the drawing board.
Attachment #354205 - Attachment is obsolete: true
Jim, any progress?
Not since comment #13.  I'll pick this up tomorrow.
Every piece of the body of js/src/prmjtime.cpp's PRMJ_Now function was
in a preprocessor conditional --- it was three entirely independent
implementations shuffled together.  Unshuffling them prepares the way
for adding a new WinCE definition.
Attachment #355833 - Flags: review?(crowder)
Perform the appropriate configure-time tests, and hard-code the
answers for targets that don't support autoconf-style tests.  Check
for the io.h header, and the setbuf and isatty library functions.

In js/src/xpconnect/shell/xpcshell.cpp, use configure-#defined
preprocessor symbols to decide what to #include and use.  The
top-level configure script defines the preprocessor symbols used here.

In js/src/prmjtime.cpp, use them to select the appropriate method for
retrieving fine-grained time information for Windows and WinCE.  The
js/src/configure script defines the preprocessor symbols used here.

(This should cover the issues addressed by patch.v2 in bug 461841,
except for the stdint issue.)
Attachment #346108 - Attachment is obsolete: true
Attachment #355834 - Flags: review?
These two patches, applied after attachment 355830 [details] [diff] [review] on bug 465640, should, I believe, address everything patch v2 above is meant to.  If some WinCE folks could try it out, that would be awesome.
Attachment #355834 - Flags: review? → review?(crowder)
Comment on attachment 355833 [details] [diff] [review]
Bug 461841: Unshuffle system-specific definitions of PRMJ_Now.

Much better, thanks.
Attachment #355833 - Flags: review?(crowder) → review+
Comment on attachment 355834 [details] [diff] [review]
Bug 461841: Use configure-defined macros in #ifdefs for WinCE in js/src.

+        AC_DEFINE(HAVE_IO_H)
+        AC_DEFINE(HAVE_SETBUF)
+        AC_DEFINE(HAVE_ISATTY)

Shouldn't we be generically testing for these, rather than setting them for WinCE always?

+AC_CHECK_HEADERS(io.h)
Here's an io.h check -- redundant with the define above?

+AC_CHECK_FUNCS(random strerror lchown fchmod snprintf statvfs memmove rint stat64 lstat64 truncate64 statvfs64 setbuf isatty)
Redundant with the other defines above?


+        AC_DEFINE(HAVE_SYSTEMTIMETOFILETIME)
+        AC_DEFINE(JS_CRTDEFS_H_HAS_INTPTR_T)
+        ;;
+    *)
+        AC_DEFINE(HAVE_SYSTEMTIMETOFILETIME)
+        AC_DEFINE(HAVE_GETSYSTEMTIMEASFILETIME)

AC_CHECK_FUNCS instead?

+#elif defined (HAVE_SYSTEMTIMETOFILETIME)
+JSInt64
+PRMJ_Now(void)
Is it possible that this could cause us to have duplicate PRMJ_Nows on some Win32 platforms?
Attachment #355834 - Flags: review?(ted.mielczarek)
[Discussed more on IRC, but I might as well get this into the bug...]

As I understand it, we can't do AC_CHECK_HEADERS tests on Windows.  The clause that those AC_DEFINEs are added to sets SKIP_COMPILER_CHECKS and SKIP_LIBRARY_CHECKS.  So, if we can't do inspection, then we need to hard-code the answers.  But either way, it should be configure's job to answer those questions.  Thus, the hard-code AC_DEFINEs handle Windows, and the AC_CHECK_HEADERS calls handle other platforms.

> +#elif defined (HAVE_SYSTEMTIMETOFILETIME)
> +JSInt64
> +PRMJ_Now(void)
> Is it possible that this could cause us to have duplicate PRMJ_Nows on some
> Win32 platforms?

You're right, that really should be an 'else-if' chain.
[revised to use an else chain]

Every piece of the body of js/src/prmjtime.cpp's PRMJ_Now function was
in a preprocessor conditional --- it was three entirely independent
implementations shuffled together.  Unshuffling them prepares the way
for adding a new WinCE definition.
Attachment #355833 - Attachment is obsolete: true
Attachment #356205 - Flags: review?(crowder)
[adapted to apply on top of latest bug 465640 patch and the attachment just updated on this bug]

Perform the appropriate configure-time tests, and hard-code the
answers for targets that don't support autoconf-style tests.  Check
for the io.h header, and the setbuf and isatty library functions.

In js/src/xpconnect/shell/xpcshell.cpp, use configure-#defined
preprocessor symbols to decide what to #include and use.  The
top-level configure script defines the preprocessor symbols used here.

In js/src/prmjtime.cpp, use them to select the appropriate method for
retrieving fine-grained time information for Windows and WinCE.  The
js/src/configure script defines the preprocessor symbols used here.

(This should cover the issues addressed by patch.v2 in bug 461841,
except for the stdint issue.)
Attachment #355834 - Attachment is obsolete: true
Attachment #356206 - Flags: review?(crowder)
Attachment #355834 - Flags: review?(ted.mielczarek)
Attachment #355834 - Flags: review?(crowder)
Attachment #356205 - Flags: review?(crowder) → review+
Comment on attachment 356205 [details] [diff] [review]
Bug 461841: Unshuffle system-specific definitions of PRMJ_Now.

Should the last def just be #else?  Whatever.
Attachment #356206 - Flags: review?(crowder) → review+
(In reply to comment #24)
> Should the last def just be #else?  Whatever.

I don't think so --- if none of the #if conditions succeed, we want a link failure, not the selection of the last definition.
Whiteboard: checkin-needed
A #else #error "PRMJ_Now needs to be implemented!" would be a nicer touch than a compiler error that doesn't explain what's wrong.
True; done.
This patch did not cleanly apply in WinCE build as of 14-Jan-09.  

This attachment does essentially the same thing, but applies cleanly on source pulled from mozilla-central.

r=crowder from previous version of attachment.  Crowder, can you just eyeball this again to make sure I did not make a fat-fingered mistake?
Attachment #356206 - Attachment is obsolete: true
Attachment #356975 - Flags: review?(crowder)
Attachment #356975 - Flags: review?(crowder) → review+
Comment on attachment 356975 [details] [diff] [review]
v.4 updated patch for configure-defined macros in #ifdefs for WinCE in js/src
[Checkin: Comment 32]

>+#ifdef HAVE_SYSTEMTIMETOFILETIME
>+JSInt64
>+   PRMJ_Now(void)

Prevailing style puts the function name at the start of the line. Three space and other fibonacci indentation is nowhere in Mozilla prevailing style :-P.

/be
Ugh, sorry for missing that.
A question about attachment 356975 [details] [diff] [review]:

This patch is supposed to make the big honkin' definition for full Windows ("I just want to know WHAT TIME IT IS!!!  Ahem.") conditional on HAVE_GETSYSTEMTIMEASFILETIME, and then the brief WinCE definition conditional on HAVE_SYSTEMTIMETOFILETIME.  Both of those are #defined on Windows, so I think this will select the wrong definition of the function on Windows.
Landed in M-C: http://hg.mozilla.org/mozilla-central/rev/7cf6db4b75c0
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: checkin-needed
we want this for 1.9.1 for mobile.
Flags: wanted1.9.1?
IMHO it's a mistake for xpcshell.cpp to treat files in interactive mode by default when HAVE_ISATTY is not defined. Because of bug 474236, such files are almost certain to not compile, and anyway the shell will spew prompts. It should default to non-interactive mode.
Depends on: 474236
Keywords: checkin-needed
Whiteboard: [needs landing]
Flags: in-testsuite-
Flags: in-litmus-
Attachment #356975 - Attachment description: v.4 updated patch for configure-defined macros in #ifdefs for WinCE in js/src → v.4 updated patch for configure-defined macros in #ifdefs for WinCE in js/src [Checkin: Comment 32]
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Comment on attachment 358006 [details] [diff] [review]
Bug 461841: xpcshell: default to non-interactive when isatty is missing.
[Checkin: Comment 36]


http://hg.mozilla.org/mozilla-central/rev/22446b1a041c
Attachment #358006 - Attachment description: Bug 461841: xpcshell: default to non-interactive when isatty is missing. → Bug 461841: xpcshell: default to non-interactive when isatty is missing. [Checkin: Comment 36]
Keywords: checkin-needed
Whiteboard: [needs landing]
clearing 1.9.1 flag, requesting blocking fennec.
tracking-fennec: --- → ?
Flags: wanted1.9.1?
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.