Last Comment Bug 763848 - Make it easier to link to libxul with js-ctypes
: Make it easier to link to libxul with js-ctypes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: js-ctypes (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla16
Assigned To: David Teller [:Yoric] (please use "needinfo")
:
Mentors:
Depends on: 771087
Blocks: 552551 771928
  Show dependency treegraph
 
Reported: 2012-06-12 00:29 PDT by David Teller [:Yoric] (please use "needinfo")
Modified: 2012-07-08 12:20 PDT (History)
5 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Exposing path to libxul in OS.Constants (2.17 KB, patch)
2012-06-22 05:44 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Companion testsuite (5.33 KB, patch)
2012-06-22 05:45 PDT, David Teller [:Yoric] (please use "needinfo")
khuey: review+
Details | Diff | Splinter Review
Exposing path to libxul in OS.Constants (2.92 KB, patch)
2012-06-22 06:22 PDT, David Teller [:Yoric] (please use "needinfo")
landry: feedback+
Details | Diff | Splinter Review
Exposing path to libxul in OS.Constants (2.96 KB, patch)
2012-06-23 04:49 PDT, David Teller [:Yoric] (please use "needinfo")
khuey: review-
Details | Diff | Splinter Review
Exposing path to libxul in OS.Constants (7.54 KB, patch)
2012-06-27 07:48 PDT, David Teller [:Yoric] (please use "needinfo")
no flags Details | Diff | Splinter Review
Exposing path to libxul in OS.Constants (7.11 KB, patch)
2012-06-27 08:50 PDT, David Teller [:Yoric] (please use "needinfo")
khuey: review+
Details | Diff | Splinter Review
Exposing path to libxul in OS.Constants (7.10 KB, patch)
2012-06-27 08:59 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
Companion testsuite (4.88 KB, patch)
2012-06-27 09:23 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
Exposing path to libxul in OS.Constants (7.45 KB, patch)
2012-06-27 10:47 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review
Exposing path to libxul in OS.Constants (8.37 KB, patch)
2012-06-28 14:19 PDT, David Teller [:Yoric] (please use "needinfo")
dteller: review+
Details | Diff | Splinter Review

Description David Teller [:Yoric] (please use "needinfo") 2012-06-12 00:29:43 PDT
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.
Comment 1 David Teller [:Yoric] (please use "needinfo") 2012-06-14 03:30:51 PDT
I suspect that the easiest would be to publish the location of libxul in OS.Constants.
Comment 2 David Teller [:Yoric] (please use "needinfo") 2012-06-22 05:44:48 PDT
Created attachment 635708 [details] [diff] [review]
Exposing path to libxul in OS.Constants
Comment 3 David Teller [:Yoric] (please use "needinfo") 2012-06-22 05:45:10 PDT
Created attachment 635709 [details] [diff] [review]
Companion testsuite
Comment 4 David Teller [:Yoric] (please use "needinfo") 2012-06-22 06:22:33 PDT
Created attachment 635723 [details] [diff] [review]
Exposing path to libxul in OS.Constants

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?
Comment 5 Landry Breuil (:gaston) 2012-06-22 10:08:52 PDT
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 Landry Breuil (:gaston) 2012-06-22 10:11:30 PDT
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 Landry Breuil (:gaston) 2012-06-22 13:20:47 PDT
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
Comment 8 David Teller [:Yoric] (please use "needinfo") 2012-06-23 04:49:29 PDT
Created attachment 636063 [details] [diff] [review]
Exposing path to libxul in OS.Constants

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.
Comment 9 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-06-25 07:47:09 PDT
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.
Comment 10 Mike Hommey [:glandium] 2012-06-25 09:01:36 PDT
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));
Comment 11 David Teller [:Yoric] (please use "needinfo") 2012-06-26 06:10:56 PDT
(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.
Comment 12 David Teller [:Yoric] (please use "needinfo") 2012-06-27 07:48:05 PDT
Created attachment 637120 [details] [diff] [review]
Exposing path to libxul in OS.Constants

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.
Comment 13 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-06-27 08:03:50 PDT
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 (
Comment 14 David Teller [:Yoric] (please use "needinfo") 2012-06-27 08:50:21 PDT
Created attachment 637143 [details] [diff] [review]
Exposing path to libxul in OS.Constants

Thanks for the quick review. Applied your changes.
Comment 15 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-06-27 08:53:41 PDT
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))
Comment 16 David Teller [:Yoric] (please use "needinfo") 2012-06-27 08:59:02 PDT
Created attachment 637146 [details] [diff] [review]
Exposing path to libxul in OS.Constants

Thanks, I just learnt something.
Comment 17 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2012-06-27 09:02:01 PDT
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?
Comment 18 David Teller [:Yoric] (please use "needinfo") 2012-06-27 09:23:55 PDT
Created attachment 637157 [details] [diff] [review]
Companion testsuite

Fixed, thanks - and thanks for the quick reviews.
Comment 19 David Teller [:Yoric] (please use "needinfo") 2012-06-27 10:47:02 PDT
Created attachment 637183 [details] [diff] [review]
Exposing path to libxul in OS.Constants

Same patch, except we heap-allocate the string instead of static-allocating it, to avoid any leak.
Comment 20 David Teller [:Yoric] (please use "needinfo") 2012-06-28 14:19:41 PDT
Created attachment 637670 [details] [diff] [review]
Exposing path to libxul in OS.Constants

+ merging

Note You need to log in before you can comment on or make changes to this bug.