Closed
Bug 461841
Opened 16 years ago
Closed 15 years ago
javascript engine fixes for windows mobile
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: dougt, Assigned: jimb)
References
Details
Attachments
(3 files, 6 obsolete files)
2.87 KB,
patch
|
crowderbt
:
review+
|
Details | Diff | Splinter Review |
6.89 KB,
patch
|
crowderbt
:
review+
|
Details | Diff | Splinter Review |
739 bytes,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•16 years ago
|
||
Reporter | ||
Comment 2•16 years ago
|
||
Attachment #346108 -
Flags: review?
Reporter | ||
Updated•16 years ago
|
Attachment #344976 -
Attachment is obsolete: true
Reporter | ||
Updated•16 years ago
|
Attachment #346108 -
Flags: review? → review?(crowder)
Comment 3•16 years ago
|
||
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?
Comment 4•16 years ago
|
||
Hoping for thoughts from either Jim Blandy or Ted (or both) about getting these preprocessor guards right, wrt autoconf
Comment 5•16 years ago
|
||
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-
Assignee | ||
Comment 6•16 years ago
|
||
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
Comment 7•16 years ago
|
||
JimB: Any chance you wanna give it a whirl? :)
Assignee | ||
Comment 8•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Assignee: general → jim
Reporter | ||
Comment 9•16 years ago
|
||
any update here?
Comment 10•16 years ago
|
||
There was some motion on the prerequisite bug today, so I think we're moving forward, and this should be done correctly soon.
Assignee | ||
Comment 11•16 years ago
|
||
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.
Assignee | ||
Comment 12•16 years ago
|
||
[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.
Assignee | ||
Comment 13•16 years ago
|
||
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
Comment 14•15 years ago
|
||
Jim, any progress?
Assignee | ||
Comment 15•15 years ago
|
||
Not since comment #13. I'll pick this up tomorrow.
Assignee | ||
Comment 16•15 years ago
|
||
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)
Assignee | ||
Comment 17•15 years ago
|
||
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?
Assignee | ||
Comment 18•15 years ago
|
||
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.
Assignee | ||
Updated•15 years ago
|
Attachment #355834 -
Flags: review? → review?(crowder)
Comment 19•15 years ago
|
||
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 20•15 years ago
|
||
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?
Assignee | ||
Updated•15 years ago
|
Attachment #355834 -
Flags: review?(ted.mielczarek)
Assignee | ||
Comment 21•15 years ago
|
||
[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.
Assignee | ||
Comment 22•15 years ago
|
||
[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)
Assignee | ||
Comment 23•15 years ago
|
||
[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)
Updated•15 years ago
|
Attachment #356205 -
Flags: review?(crowder) → review+
Comment 24•15 years ago
|
||
Comment on attachment 356205 [details] [diff] [review] Bug 461841: Unshuffle system-specific definitions of PRMJ_Now. Should the last def just be #else? Whatever.
Updated•15 years ago
|
Attachment #356206 -
Flags: review?(crowder) → review+
Assignee | ||
Comment 25•15 years ago
|
||
(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.
Assignee | ||
Updated•15 years ago
|
Whiteboard: checkin-needed
Comment 26•15 years ago
|
||
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.
Assignee | ||
Comment 27•15 years ago
|
||
True; done.
Comment 28•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #356975 -
Flags: review?(crowder) → review+
Comment 29•15 years ago
|
||
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
Comment 30•15 years ago
|
||
Ugh, sorry for missing that.
Assignee | ||
Comment 31•15 years ago
|
||
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.
Assignee | ||
Comment 32•15 years ago
|
||
Landed in M-C: http://hg.mozilla.org/mozilla-central/rev/7cf6db4b75c0
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Whiteboard: checkin-needed
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.
Assignee | ||
Comment 35•15 years ago
|
||
Attachment #358006 -
Flags: review?(roc)
Attachment #358006 -
Flags: review?(roc) → review+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [needs landing]
Updated•15 years ago
|
Flags: in-testsuite-
Flags: in-litmus-
Updated•15 years ago
|
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]
Updated•15 years ago
|
Target Milestone: --- → mozilla1.9.2a1
Version: unspecified → Trunk
Comment 36•15 years ago
|
||
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]
Updated•15 years ago
|
Keywords: checkin-needed
Whiteboard: [needs landing]
Reporter | ||
Comment 37•15 years ago
|
||
clearing 1.9.1 flag, requesting blocking fennec.
tracking-fennec: --- → ?
Flags: wanted1.9.1?
Updated•10 years ago
|
tracking-fennec: ? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•