Closed
Bug 763848
Opened 12 years ago
Closed 12 years ago
Make it easier to link to libxul with js-ctypes
Categories
(Core :: js-ctypes, defect)
Core
js-ctypes
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: Yoric, Assigned: Yoric)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 8 obsolete files)
4.88 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
8.37 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
In some cases, the simplest way of exporting new C functions for use with js-ctypes is to add these functions to libxul and link to libxul with js-ctypes. Unfortunately, for some reason, linking to libxul with js-ctypes is non-trivial/not possible on some platforms – right now, I am thinking of MacOS X from a Chrome Worker. I suggest making the process simple for end-users. I believe that we could tweak the current API so that calling |ctypes.open()| opens libxul.
Assignee | ||
Comment 1•12 years ago
|
||
I suspect that the easiest would be to publish the location of libxul in OS.Constants.
Assignee | ||
Comment 2•12 years ago
|
||
Assignee: nobody → dteller
Attachment #635708 -
Flags: feedback?(landry)
Assignee | ||
Comment 3•12 years ago
|
||
Assignee | ||
Comment 4•12 years ago
|
||
Same one, with a trivial typo fixed and a few additional comments. Gaston, if you have some time, could you check whether this works on *BSD?
Attachment #635708 -
Attachment is obsolete: true
Attachment #635708 -
Flags: feedback?(landry)
Attachment #635723 -
Flags: review?(khuey)
Attachment #635723 -
Flags: feedback?(landry)
Assignee | ||
Updated•12 years ago
|
Attachment #635709 -
Flags: review?(khuey)
Comment 5•12 years ago
|
||
You're going to hate me.. but your diff fails to compile with : /home/landry/src/mozilla-central/dom/system/OSFileConstants.cpp:363: error: invalid conversion from 'const char*' to 'PRUnichar' /home/landry/src/mozilla-central/dom/system/OSFileConstants.cpp:363: error: initializing argument 1 of 'void nsAString_internal::Append( PRUnichar)' /home/landry/src/mozilla-central/dom/system/OSFileConstants.cpp:365: error: invalid conversion from 'const char*' to 'PRUnichar' /home/landry/src/mozilla-central/dom/system/OSFileConstants.cpp:365: error: initializing argument 1 of 'void nsAString_internal::Append( PRUnichar)' Trying with DLL_PREFIX/SUFFIX wrapped without NS_LITERAL_STRING...
Comment 6•12 years ago
|
||
err, within, of course. + xulName.Append(NS_LITERAL_STRING(DLL_PREFIX)); + xulName.Append(NS_LITERAL_STRING("xul")); + xulName.Append(NS_LITERAL_STRING(DLL_SUFFIX)); builds here, will do the actual testing once it finishes
Comment 7•12 years ago
|
||
1 INFO TEST-START | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul 2 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | test_constants.xul: Starting test 3 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | test_constants.xul: Chrome worker created 4 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | test_constants.xul: Test in progress 5 INFO TEST-PASS | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | test_xul: opened libxul successfully 6 INFO TEST-END | chrome://mochitests/content/chrome/dom/system/tests/mochi/test_constants.xul | finished in 282ms 7 INFO TEST-START | Shutdown 8 INFO Passed: 4 So with NS_LITERAL_STRING(), all good for me
Updated•12 years ago
|
Attachment #635723 -
Flags: feedback?(landry) → feedback+
Assignee | ||
Comment 8•12 years ago
|
||
Thanks for the BSD test, gaston. You are, of course, right about the NS_LITERAL_STRING. I had forgotten to update the patch after testing it under Windows.
Attachment #635723 -
Attachment is obsolete: true
Attachment #635723 -
Flags: review?(khuey)
Attachment #636063 -
Flags: review?(khuey)
Comment on attachment 636063 [details] [diff] [review] Exposing path to libxul in OS.Constants Review of attachment 636063 [details] [diff] [review]: ----------------------------------------------------------------- NS_GetSpecialDirectory is not thread-safe.
Attachment #636063 -
Flags: review?(khuey) → review-
Comment 10•12 years ago
|
||
Comment on attachment 636063 [details] [diff] [review] Exposing path to libxul in OS.Constants Review of attachment 636063 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/OSFileConstants.cpp @@ +412,5 @@ > + // On other platforms, libxul is a library "xul" with regular > + // library prefix/suffix > + xulName.Append(NS_LITERAL_STRING(DLL_PREFIX)); > + xulName.Append(NS_LITERAL_STRING("xul")); > + xulName.Append(NS_LITERAL_STRING(DLL_SUFFIX)); You probably want xulName.Append(NS_LITERAL_STRING(DLL_PREFIX "xul" DLL_SUFFIX));
Assignee | ||
Comment 11•12 years ago
|
||
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #9) > Comment on attachment 636063 [details] [diff] [review] > Exposing path to libxul in OS.Constants > > Review of attachment 636063 [details] [diff] [review]: > ----------------------------------------------------------------- > > NS_GetSpecialDirectory is not thread-safe. Great :/ Ok, so plan B is the following: - extract path to libxpcom during initialization on the main thread; - re-publish this information as a global string; - use that global string during initialization of OS.Constants.Sys; - cry a river for all the other constants I will want to expose to OS.File.
OS: Mac OS X → All
Assignee | ||
Comment 12•12 years ago
|
||
Attaching an updated version. As suggested by khuey, it extracts the path name from the main thread, during initialization of the ChromeWorker service, and uses this information once chrome workers are effectively created.
Attachment #636063 -
Attachment is obsolete: true
Attachment #637120 -
Flags: review?(khuey)
Comment on attachment 637120 [details] [diff] [review] Exposing path to libxul in OS.Constants Review of attachment 637120 [details] [diff] [review]: ----------------------------------------------------------------- I'm not going to r- it this time, but I would like to see it again before giving r+. ::: dom/system/OSFileConstants.cpp @@ +48,5 @@ > + > +/** > + * The name of the directory holding all the libraries (libxpcom, libnss, etc.) > + */ > +nsString gLibDirectory; Place these global variables in an anonymous namespace please. @@ +65,5 @@ > + gInitialized = true; > + > + // Initialize gLibDirectory > + nsCOMPtr<nsIFile> xpcomLib; > + nsresult rv = NS_GetSpecialDirectory("XpcomLib", getter_AddRefs(xpcomLib)); Please fix your indentation in this function. @@ +67,5 @@ > + // Initialize gLibDirectory > + nsCOMPtr<nsIFile> xpcomLib; > + nsresult rv = NS_GetSpecialDirectory("XpcomLib", getter_AddRefs(xpcomLib)); > + if (!NS_SUCCEEDED(rv) || !xpcomLib) { > + return false; Do not return false from a function that has an nsresult retval. @@ +73,5 @@ > + > + nsCOMPtr<nsIFile> libDir; > + rv = xpcomLib->GetParent(getter_AddRefs(libDir)); > + if (!NS_SUCCEEDED(rv)) { > + return false; Do not return false from a function that has an nsresult retval. @@ +114,5 @@ > #define S_IRUSR 0400 > #define S_IRWXU 0700 > #endif // !defined(S_IRGRP) > > + Unnecessary newline addition. @@ +428,5 @@ > +#endif // defined(XP_MACOSX) > + > + JSString* strPathToLibXUL = JS_NewUCStringCopyZ(cx, xulPath.get()); > + jsval valXul = STRING_TO_JSVAL(strPathToLibXUL); > + JS_SetProperty(cx, objSys, "libxulpath", &valXul); Why don't you care about the return value here. ::: dom/workers/RuntimeService.cpp @@ +931,5 @@ > mSystemCharset); > } > > + rv = InitOSFileConstants(); > + if(!NS_SUCCEEDED(rv)) { Nit: if (
Attachment #637120 -
Flags: review?(khuey)
Assignee | ||
Comment 14•12 years ago
|
||
Thanks for the quick review. Applied your changes.
Attachment #637120 -
Attachment is obsolete: true
Attachment #637143 -
Flags: review?(khuey)
Comment on attachment 637143 [details] [diff] [review] Exposing path to libxul in OS.Constants Review of attachment 637143 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/system/OSFileConstants.cpp @@ +46,5 @@ > + > +/** > + * The name of the directory holding all the libraries (libxpcom, libnss, etc.) > + */ > +nsString gLibDirectory; This isn't an anonymous namespace. Please put them in a "namespace { };" block so that if anyone does "extern gInitialized" in the future it won't latch on to this. ::: dom/workers/RuntimeService.cpp @@ +931,5 @@ > mSystemCharset); > } > > + rv = InitOSFileConstants(); > + if (!NS_SUCCEEDED(rv)) { if (NS_FAILED(rv))
Attachment #637143 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 16•12 years ago
|
||
Thanks, I just learnt something.
Attachment #637143 -
Attachment is obsolete: true
Attachment #637146 -
Flags: review+
Comment on attachment 635709 [details] [diff] [review] Companion testsuite Review of attachment 635709 [details] [diff] [review]: ----------------------------------------------------------------- I didn't review the actual test, just the build files. ::: dom/system/tests/Makefile.in @@ +4,5 @@ > +VPATH = @srcdir@ > +relativesrcdir = dom/system/tests > + > +MODULE = test_domsystem > +DIRS = mochi You don't need an entire subdirectory for mochitests. Move the contents of ./mochi/ to this directory. ::: dom/system/tests/mochi/Makefile.in @@ +7,5 @@ > +MODULE = domsystem > +_CHROME_TEST_FILES = \ > + test_constants.xul \ > + worker_constants.js \ > + $(NULL) No tabs necessary outside of rules. @@ +9,5 @@ > + test_constants.xul \ > + worker_constants.js \ > + $(NULL) > + > + include $(topsrcdir)/config/rules.mk Nit: extra space?
Attachment #635709 -
Flags: review?(khuey) → review+
Assignee | ||
Comment 18•12 years ago
|
||
Fixed, thanks - and thanks for the quick reviews.
Attachment #635709 -
Attachment is obsolete: true
Attachment #637157 -
Flags: review+
Assignee | ||
Comment 19•12 years ago
|
||
Same patch, except we heap-allocate the string instead of static-allocating it, to avoid any leak.
Attachment #637146 -
Attachment is obsolete: true
Attachment #637183 -
Flags: review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Updated•12 years ago
|
Attachment #637183 -
Attachment is obsolete: true
Comment 21•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b4f69c478fa https://hg.mozilla.org/integration/mozilla-inbound/rev/5d9924488dd2
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6b4f69c478fa https://hg.mozilla.org/mozilla-central/rev/5d9924488dd2
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•