js-ctypes bindings for sqlite

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: Yoric, Assigned: yzen)

Tracking

Trunk
mozilla29
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 19 obsolete attachments)

13.41 KB, patch
Details | Diff | Splinter Review
11.79 KB, patch
Details | Diff | Splinter Review
Bind the functions of sqlite that we need so that we can manipulate them from workers.
Assignee: nobody → yura.zenevich
Posted patch 853439 patch v1 (obsolete) — Splinter Review
Hi David, this is just a first WIP to make sure I'm moving in the right direction.

I named the module Asyncstorage and it's located in toolkit/components. Perhaps you had a better name in mind?

I only added 2 methods for now: open and close, and have some basic passing tests for them. I just wanted to see if that's what you had in mind. I realize that these might be too low level tests, but this bug does not go beyond the js-ctypes bindings themselves.

Also, afaik, some things are platform specific, like the use of OS.File.Shared.Type.path which is different in windows from unix (from osfile). Does it mean I'd have to supply a platform specific back file? On the other hand perhaps you've had a different file organization in mind.
Attachment #795842 - Flags: feedback?(dteller)
(In reply to Yura Zenevich [:yzen] from comment #2)
> Created attachment 795842 [details] [diff] [review]
> 853439 patch v1
> 
> Hi David, this is just a first WIP to make sure I'm moving in the right
> direction.
> 
> I named the module Asyncstorage and it's located in toolkit/components.
> Perhaps you had a better name in mind?

Directory toolkit/components looks good to me. As for the name, we have many kinds of async storage, so just "Sqlite.js" should be good, shouldn't it?

> I only added 2 methods for now: open and close, and have some basic passing
> tests for them. I just wanted to see if that's what you had in mind. I
> realize that these might be too low level tests, but this bug does not go
> beyond the js-ctypes bindings themselves.

I'll take a look.

> Also, afaik, some things are platform specific, like the use of
> OS.File.Shared.Type.path which is different in windows from unix (from
> osfile). Does it mean I'd have to supply a platform specific back file? On
> the other hand perhaps you've had a different file organization in mind.

I seem to remember that sqlite always uses char* for paths, doesn't it? If so, just use char* instead of OS.File.Shared.Type.path.
Comment on attachment 795842 [details] [diff] [review]
853439 patch v1

Review of attachment 795842 [details] [diff] [review]:
-----------------------------------------------------------------

Good start, thanks for this first snapshot.

::: toolkit/components/asyncstorage/asyncstorage.jsm
@@ +14,5 @@
> +    'resource://gre/modules/workers/require.js',
> +    'resource://gre/modules/asyncstorage/asyncstorage_allthreads.jsm',
> +    'resource://gre/modules/asyncstorage/asyncstorage_back.jsm'
> +  );
> +}

I'd prefer, if possible, if you wrote your code to make use of require().
Also, if this file is meant to be worker thread only, please don't call it .jsm.

::: toolkit/components/asyncstorage/modules/asyncstorage_allthreads.jsm
@@ +12,5 @@
> +
> +  OSFileSharedAll = {};
> +  Components.utils.import(
> +    'resource://gre/modules/osfile/osfile_shared_allthreads.jsm',
> +    OSFileSharedAll);

For the moment, we have all we need on the main thread.
The interesting part is to have bindings for the worker thread, so you can get rid of any code designed for the main thread.

@@ +16,5 @@
> +    OSFileSharedAll);
> +} else {
> +  OSFileSharedAll = require(
> +    'resource://gre/modules/osfile/osfile_shared_allthreads.jsm');
> +}

|OSFileSharedAll| is perhaps an overlong name, I'll let you find something shorter.

@@ +24,5 @@
> +  if (exports.Asyncstorage) {
> +    // Avoid double inclusion
> +    return;
> +  }
> +  exports.Asyncstorage = {};

If your code is designed for require(), you can get rid of this |function(exports)| and the "avoid double inclusion" hacks.

@@ +26,5 @@
> +    return;
> +  }
> +  exports.Asyncstorage = {};
> +
> +  let OS = OSFileSharedAll.OS;

Please don't rely upon that |OSFileSharedAll.OS|, it is on its way out.

@@ +42,5 @@
> +  // Define declareFFI
> +  let declareFFI = OS.Shared.declareFFI.bind(null, libsqlite3);
> +  exports.Asyncstorage.declareFFI = declareFFI;
> +
> + })(this);

Generally speaking, this file is not very useful.

::: toolkit/components/asyncstorage/modules/asyncstorage_back.jsm
@@ +42,5 @@
> +
> +
> +    asyncstorageBack.open = declareFFI('sqlite3_open', ctypes.default_abi,
> +                                       /*return*/ Type.int,
> +                                       /*path*/ Types.path,

For the path, just use a Types.string here.

@@ +43,5 @@
> +
> +    asyncstorageBack.open = declareFFI('sqlite3_open', ctypes.default_abi,
> +                                       /*return*/ Type.int,
> +                                       /*path*/ Types.path,
> +                                       /*SQLite db handle*/ Types.sqlite3.out_ptr);

Side-note: in the future, we may need to move to sqlite3_open_v2. However, for the moment, sqlite3_open is perfectly fine.

::: toolkit/components/asyncstorage/tests/worker_test_asyncstorage_back.js
@@ +11,5 @@
> +
> +function test_init() {
> +  info('Starting test_init');
> +  importScripts('resource://gre/modules/asyncstorage.jsm');
> +  isnot(typeof Asyncstorage, 'undefined', 'Asyncstorage is not undefined.');

This will need to be rewritten with |require|.
Attachment #795842 - Flags: feedback?(dteller) → feedback+
Posted patch 853439 patch v2 (obsolete) — Splinter Review
It's ALIVE :)

Seems like tests are passing and the most basic sql operations are working. Let me know if there's anything that needs to be fixed/cleaned up, otherwise I'll carry on with adding more bindings :).
Attachment #795842 - Attachment is obsolete: true
Attachment #797084 - Flags: feedback?(dteller)
Comment on attachment 797084 [details] [diff] [review]
853439 patch v2

Review of attachment 797084 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good :)
Our objective, eventually, is to mirror the features of Sqlite.jsm in a worker, so I guess that the next thing to do is take a look at what you need to implement these features.

::: toolkit/components/sqlite/sqlite_back.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */

I suggest "use strict";

@@ +1,4 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +

Some documentation about this file might be useful.

@@ +4,5 @@
> +
> +importScripts('resource://gre/modules/workers/require.js');
> +
> +let OS = require(
> +  'resource://gre/modules/osfile/osfile_shared_allthreads.jsm').OS;

Please don't rely on this object |OS|, it will disappear.
Just do |let SharedAll = require(...)| and use the symbols in |SharedAll|.

@@ +21,5 @@
> +// Define declareFFI
> +let declareFFI = OS.Shared.declareFFI.bind(null, libsqlite3);
> +
> +Sqlite.back = {};
> +let sqliteBack = Sqlite.back;

Just export all the symbols you wish to export, without putting them in an object |back|.

@@ +29,5 @@
> +let Type = OS.Shared.Type;
> +
> +/**
> + * Opaque Structure |sqlite3|
> + * |sqlite3| is equivalent to a void*

To avoid any confusion, I would call this |sqlite_ptr|.

@@ +31,5 @@
> +/**
> + * Opaque Structure |sqlite3|
> + * |sqlite3| is equivalent to a void*
> + */
> +Types.sqlite3 = Type.voidptr_t.withName('sqlite3_ptr');

You can just used SharedAll.Type, this will lower the likelihood of errors between Type and Types.
Alternatively, you can define |let Types = Object.create(SharedAll.Type)| and ensure that all useful types are in the same object.

@@ +35,5 @@
> +Types.sqlite3 = Type.voidptr_t.withName('sqlite3_ptr');
> +
> +/**
> + * |sqlite3_stmt| an instance of an object representing a single SQL statement.
> + * |sqlite3_stmt| is equivalent to a void*

Same here.

@@ +42,5 @@
> +
> +/**
> + * Shortcut for |char*|.
> + */
> +Types.charptr_t = new Type('char*', ctypes.char.ptr);

I believe that there is already a SharedAll.Type.string.

::: toolkit/components/sqlite/tests/worker_test_sqlite_back.js
@@ +28,5 @@
> +    'chrome/toolkit/components/sqlite/tests/test.db', dbPtr);
> +  is(result, 0, 'open_close: opening succeeded');
> +
> +  if (callback) {
> +    callback(db);

You should probably put this in a try/finally to ensure that you always close  the db.

@@ +50,5 @@
> +  result = Sqlite.back.step(sqlStmt);
> +  is(result, stepResult, 'query: step is done');
> +
> +  if (callback) {
> +    callback(sqlStmt);

Same here.
Attachment #797084 - Flags: feedback?(dteller) → feedback+
Posted patch 853439 patch v3 (obsolete) — Splinter Review
Attachment #797084 - Attachment is obsolete: true
Attachment #799272 - Flags: review?(dteller)
Posted patch 853439 tests v1 (obsolete) — Splinter Review
Attachment #799274 - Flags: review?(dteller)
Comment on attachment 799272 [details] [diff] [review]
853439 patch v3

Review of attachment 799272 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good. r+ once we have finished dealing with tests.
We need to consider a landing strategy for this. I'll discuss this with vladan.

::: toolkit/components/sqlite/sqlite_back.js
@@ +30,5 @@
> +
> +// Define declareFFI.
> +let declareFFI = SharedAll.declareFFI.bind(null, libsqlite3);
> +
> +let Types = Object.create(SharedAll.Type);

Nit: I'm trying to remove all instances of "Types" (plural) and only keep "Type" (singular). Could you do the same?

@@ +49,5 @@
> +/**
> + * |sqlite3_destructor_ptr| a constant defining a special destructor behaviour.
> + * |sqlite3_destructor_ptr| is equivalent to a void*.
> + * SQLITE_STATIC      0
> + * SQLITE_TRANSIENT   -1

Mmmh.... possibly, but since that doesn't really match the documentation of sqlite3_bind_stuff, you'll need more comments here.

@@ +65,5 @@
> + */
> +Types.sqlite3_int64 = Types.int64_t.withName('sqlite3_int64');
> +
> +exports.Type = Types;
> +

Have you checked whether we actually use all these functions in our code?
Attachment #799272 - Flags: review?(dteller) → review+
Comment on attachment 799274 [details] [diff] [review]
853439 tests v1

Review of attachment 799274 [details] [diff] [review]:
-----------------------------------------------------------------

That starts to look quite good.
As mentioned, we'll need to discuss landing strategy with vladan. We'll probably also want super-review from Mossop.

::: toolkit/components/sqlite/tests/worker_test_sqlite_back.js
@@ +26,5 @@
> +  Sqlite = require('resource://gre/modules/sqlite/sqlite_back.js');
> +  isnot(typeof Sqlite, 'undefined', 'Sqlite is defined.');
> +}
> +
> +function open_close(callback, open, ...openArgs) {

We generally put callbacks at the end, so I'd suggest
 function open_close(open, openArgs /*array*/, callback = null)
plus a little doc.

Also, it's a matter of taste, but I would call that function |withDB|.

@@ +48,5 @@
> +    is(result, 0, 'open_close: closing succeeded');
> +  }
> +}
> +
> +function query(db, sql, stepResult, bind, callback) {

Documentation needed.
Also, I would call this |withQuery|.

@@ +77,5 @@
> +  } catch (ex) {
> +    ok(false, 'The following error occured: ' + ex.message);
> +  } finally {
> +    // Destroy a prepared statement object.
> +    result = Sqlite.finalize(sqlStmt);

Good :)
I've had to fix emergency segfaults due to users of sqlite forgetting to call finalize.

@@ +116,5 @@
> +  is(typeof Sqlite.prepare_v2, 'function', 'Sqlite.prepare_v2 is a function');
> +  is(typeof Sqlite.step, 'function', 'Sqlite.step is a function');
> +  is(typeof Sqlite.finalize, 'function', 'Sqlite.finalize is a function');
> +
> +  open_close(createTableOnOpen, 'open');

It would be good to remove the table after having created it.

@@ +119,5 @@
> +
> +  open_close(createTableOnOpen, 'open');
> +}
> +
> +function onStep(sqlStmt) {

Nit: documentation needed.

@@ +216,5 @@
> +    result = Sqlite.bind_double(sqlStmt, 3, 1.2);
> +    is(result, 0, 'onBind: bind_double succeeded');
> +
> +    // Destructor.
> +    let destructor = Sqlite.Type.sqlite3_destructor_ptr.implementation(-1);

We might want to move these magic constants -1 and 0 to OSFileConstants.cpp.

@@ +235,5 @@
> +  function onOpen(db) {
> +    createTableOnOpen(db);
> +    query(db, 'INSERT INTO TEST VALUES (?, ?, ?, ?, ?, ?);', 101,
> +      onBind);
> +    query(db, 'SELECT * FROM TEST;', 100, null, onStep);

These arguments 100 and 101 are unclear to me.
Attachment #799274 - Flags: review?(dteller) → feedback+
Mossop, we need to discuss a landing strategy for this.
Flags: needinfo?(dtownsend+bugmail)
Vladan, we need to discuss a landing strategy for this.
Flags: needinfo?(vdjeric)
(In reply to David Rajchenbach Teller [:Yoric] from comment #9)
> @@ +65,5 @@
> > + */
> > +Types.sqlite3_int64 = Types.int64_t.withName('sqlite3_int64');
> > +
> > +exports.Type = Types;
> > +
> 
> Have you checked whether we actually use all these functions in our code?

As far as I know yes, unless I missed something.
(In reply to David Rajchenbach Teller [:Yoric] from comment #11)
> Mossop, we need to discuss a landing strategy for this.

I'm not sure what you're asking for here, is there something about this code that makes landing it harder than checking it in?
Flags: needinfo?(dtownsend+bugmail)
(In reply to Dave Townsend (:Mossop) from comment #14)
> (In reply to David Rajchenbach Teller [:Yoric] from comment #11)
> > Mossop, we need to discuss a landing strategy for this.
> 
> I'm not sure what you're asking for here, is there something about this code
> that makes landing it harder than checking it in?

Well, it's the beginning of a new toolkit feature (mozStorage for chrome workers), so I figured you would want to be in the loop.
Posted patch 853439 patch v4 (obsolete) — Splinter Review
Carrying over the r+ from Yoric.
Attachment #799272 - Attachment is obsolete: true
Posted patch 853439 tests v2 (obsolete) — Splinter Review
Attachment #799274 - Attachment is obsolete: true
Attachment #800118 - Flags: review?(dteller)
Comment on attachment 800118 [details] [diff] [review]
853439 tests v2

Review of attachment 800118 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like a few trivial changes, but I like it a lot.

::: toolkit/components/sqlite/tests/worker_test_sqlite_back.js
@@ +7,5 @@
> +let Sqlite;
> +
> +const SQLITE_OK = 0;     /* Successful result */
> +const SQLITE_ROW = 100;  /* sqlite3_step() has another row ready */
> +const SQLITE_DONE = 101; /* sqlite3_step() has finished executing */

You should define these in Sqlite.Constants.

@@ +39,5 @@
> + * @param  {Function} callback = null An optional callback to be run after the
> + *                                    database is opened but before it is
> + *                                    closed.
> + */
> +function withDB(open, openArgs = [] /*Array*/, callback = null) {

Sorry for the confusion, you don't need to write /*Array*/ :)

@@ +140,5 @@
> +  is(typeof Sqlite.step, 'function', 'Sqlite.step is a function');
> +  is(typeof Sqlite.finalize, 'function', 'Sqlite.finalize is a function');
> +
> +  withDB('open', [], createTableOnOpen);
> +}

I believe I was unclear: you should clean up the database at the beginning of the test and at the end of the test.
Attachment #800118 - Flags: review?(dteller) → review+
Comment on attachment 800117 [details] [diff] [review]
853439 patch v4

(In reply to David Rajchenbach Teller [:Yoric] from comment #15)
> (In reply to Dave Townsend (:Mossop) from comment #14)
> > (In reply to David Rajchenbach Teller [:Yoric] from comment #11)
> > > Mossop, we need to discuss a landing strategy for this.
> > 
> > I'm not sure what you're asking for here, is there something about this code
> > that makes landing it harder than checking it in?
> 
> Well, it's the beginning of a new toolkit feature (mozStorage for chrome
> workers), so I figured you would want to be in the loop.

Ok, probably warrants an sr pass then
Attachment #800117 - Flags: superreview?(dtownsend+bugmail)
Posted patch 853439 patch v5 (obsolete) — Splinter Review
Carrying over the r+ from Yoric
Attachment #800117 - Attachment is obsolete: true
Attachment #800117 - Flags: superreview?(dtownsend+bugmail)
Posted patch 853439 tests v3 (obsolete) — Splinter Review
Fixed nits. Carrying over the r+ from Yoric
Attachment #800118 - Attachment is obsolete: true
Comment on attachment 800158 [details] [diff] [review]
853439 patch v5

As this is a new feature I'm going to do an SR pass on it.
Attachment #800158 - Flags: superreview?(dtownsend+bugmail)
Mossop, for clarification - this is just the first step towards having a Sqlite.jsm equivalent for workers. We still need to build the actual API.
Flags: needinfo?(vdjeric)
Comment on attachment 800158 [details] [diff] [review]
853439 patch v5

Review of attachment 800158 [details] [diff] [review]:
-----------------------------------------------------------------

sqlite_back seems an odd name for the module. Maybe sqlite_internal or sqlite_bindings would be better?
Attachment #800158 - Flags: superreview?(dtownsend+bugmail) → superreview+
Should I rename and/or anything else i should do?
Flags: needinfo?(dteller)
Let's call this sqlite_internal.
Flags: needinfo?(dteller)
Posted patch 853439 patch v6 (obsolete) — Splinter Review
Carrying over r+ from Yoric and sr+ from Mossop (renamed sqlite_back into sqlite_internal)
Attachment #800158 - Attachment is obsolete: true
Posted patch 853439 tests v4 (obsolete) — Splinter Review
Carrying over r+ from Yoric (renamed sqlite_back into sqlite_internal).
Attachment #800159 - Attachment is obsolete: true
That last Try Result doesn't look too good.
Posted patch 853439 tests v5 (obsolete) — Splinter Review
Added better worker error reporting (message, fileName, lineno).
Attachment #812431 - Attachment is obsolete: true
Sent to try https://tbpl.mozilla.org/?tree=Try&rev=8f47536ef816 to get a better handle on the errors.
Have you found out how to open the library under Windows?
Posted patch 853439 patch v7 (obsolete) — Splinter Review
Attachment #812430 - Attachment is obsolete: true
Attachment #8342851 - Flags: review?(dteller)
Posted patch 853439 tests v6 (obsolete) — Splinter Review
https://tbpl.mozilla.org/?tree=Try&rev=2fa71c774ae3
Attachment #828087 - Attachment is obsolete: true
Attachment #8342853 - Flags: review?(dteller)
Comment on attachment 8342851 [details] [diff] [review]
853439 patch v7

Review of attachment 8342851 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with the change below.
I'd remove "[PATCH 1/2]" from the commit message, though.
Asking gps for a moz.build review.

::: toolkit/components/sqlite/sqlite_internal.js
@@ +243,5 @@
> +               /*value*/      Type.voidptr_t,
> +               /*nBytes*/     Type.int,
> +               /*destructor*/ Type.sqlite3_destructor_ptr);
> +
> +module.exports = {

I'd rather use |module.exports = Sqlite3|, unless there is a good reason not to.
Attachment #8342851 - Flags: review?(gps)
Attachment #8342851 - Flags: review?(dteller)
Attachment #8342851 - Flags: review+
Comment on attachment 8342853 [details] [diff] [review]
853439 tests v6

Review of attachment 8342853 [details] [diff] [review]:
-----------------------------------------------------------------

Asking gps for a moz.build & al. review.

::: toolkit/components/sqlite/tests/xpcshell/data/worker_sqlite_internal.js
@@ +67,5 @@
> +    if (callback) {
> +      callback(db);
> +    }
> +  } catch (ex) {
> +    do_check_true(false);

You probably want to rethrow errors here.

@@ +113,5 @@
> +    if (callback) {
> +      callback(sqlStmt);
> +    }
> +  } catch (ex) {
> +    do_check_true(false);

You probably want to rethrow errors here.

@@ +160,5 @@
> +/**
> + * Read column values after evaluating the SQL SELECT statement.
> + * @param  {sqlite3_stmt_ptr} sqlStmt A pointer to the SQL statement.
> + */
> +function onStep(sqlStmt) {

Nit: Given that this is a global function, could you find a more precise name than |onStep|?
Attachment #8342853 - Flags: review?(gps)
Attachment #8342853 - Flags: review?(dteller)
Attachment #8342853 - Flags: review+
Hi David,

I tried changing to module.exports = Sqlite3; and I got this error:
Unexpected exception Error: Error: property "open" is non-configurable and can't be deleted at resource://gre/modules/osfile/osfile_shared_allthreads.jsm:964 ... (open is the first method that we access in test afaik). It works fine if I add each function individually though(e.g. module.exports.open = Sqlite3.open;).
Flags: needinfo?(dteller)
My bad, you're right, the defineLazyFFI doesn't work on a frozen object.
You could make it happen as follows:

for (let k of Object.keys(Sqlite3)) {
  Object.defineProperty(exports, k, {
    get: function() {
      return Sqlite3[k];
    }
  });
}
Flags: needinfo?(dteller)
(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #41)
> My bad, you're right, the defineLazyFFI doesn't work on a frozen object.
> You could make it happen as follows:
> 
> for (let k of Object.keys(Sqlite3)) {
>   Object.defineProperty(exports, k, {
>     get: function() {
>       return Sqlite3[k];
>     }
>   });
> }

As far as I understand declareLazyFFI defines a non-enumerable property (defaulted to false).
David, please see my comment above (forgot to add need info :) )
Flags: needinfo?(dteller)
Ah, right.
Then forget about it :)
Flags: needinfo?(dteller)
Posted patch 853439 patch v8 (obsolete) — Splinter Review
Mostly carrying over the Yoric's r+. David, I just wanted to make sure that the latest way I export things is good with you.
Attachment #8342851 - Attachment is obsolete: true
Attachment #8342851 - Flags: review?(gps)
Attachment #8347761 - Flags: review?(gps)
Attachment #8347761 - Flags: review?(dteller)
Posted patch 853439 tests v7 (obsolete) — Splinter Review
Carrying over the r+ from Yoric.
Attachment #8342853 - Attachment is obsolete: true
Attachment #8342853 - Flags: review?(gps)
Attachment #8347762 - Flags: review?(gps)
Comment on attachment 8347761 [details] [diff] [review]
853439 patch v8

Review of attachment 8347761 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/sqlite/sqlite_internal.js
@@ +118,5 @@
> +declareLazyFFI(Sqlite3, 'open', lib, 'sqlite3_open', null,
> +               /*return*/    Type.int,
> +               /*path*/      Type.char.in_ptr,
> +               /*db handle*/ Type.sqlite3_ptr.out_ptr);
> +module.exports.open = Sqlite3.open;

No, that defeats the purpose of the "lazy" part of declareLazyFFI.
Your previous version was better.

Let's wait for bug 951202 to (possibly) simplify your code in some followup bug.
Attachment #8347761 - Flags: review?(dteller)
Comment on attachment 8347761 [details] [diff] [review]
853439 patch v8

Review of attachment 8347761 [details] [diff] [review]:
-----------------------------------------------------------------

I guess this review covers just the build bits, since I don't really know what's going on with all the FFI magic.

::: toolkit/components/sqlite/moz.build
@@ +3,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +TEST_DIRS += ['tests']

Where is the tests/moz.build file? This patch by itself would fail the build.

::: toolkit/components/sqlite/sqlite_internal.js
@@ +1,2 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,

Nit: You are using the old license template. The new version has "file," on the 3rd line.

@@ +11,5 @@
> + * - defines sqlite3 API functions;
> + * - defines the necessary sqlite3 types.
> + */
> +
> +'use strict';

I thought we used double quotes in our style?

@@ +26,5 @@
> +} else if (SharedAll.Constants.Win) {
> +  path = ctypes.libraryName('nss3');
> +} else {
> +  path = SharedAll.Constants.Path.libxul;
> +}

I'm not crazy about build details leaking into the source like this. Seems like something we should integrate into the source code via preprocessing. But since we have test coverage and it would require Makefile.in hackery currently, I think it can be a followup. Please file a bug in Core :: Build Config and drop a comment in the source.

@@ +33,5 @@
> +try {
> +  lib = ctypes.open(path);
> +} catch (ex) {
> +  throw new Error('Could not open system library: ' + ex.message);
> +}

What's wrong with the initial exception?
Attachment #8347761 - Flags: review?(gps) → review+
Comment on attachment 8347762 [details] [diff] [review]
853439 tests v7

Review of attachment 8347762 [details] [diff] [review]:
-----------------------------------------------------------------

r+ conditional on eliminating toolkit/components/sqlite/tests/moz.build.

::: toolkit/components/sqlite/tests/moz.build
@@ +3,5 @@
> +# This Source Code Form is subject to the terms of the Mozilla Public
> +# License, v. 2.0. If a copy of the MPL was not distributed with this
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +XPCSHELL_TESTS_MANIFESTS += ['xpcshell/xpcshell.ini']

Oh, there's the tests/moz.build file!

Since there is nothing in this file other than declaring XPCSHELL_TESTS_MANIFESTS, just move that line up to toolkit/components/sqlite/moz.build, prefix the path with 'tests/' and nuke this file and references to it.

Reducing the number of moz.build/Makefile.in files in the tree cuts down on the time it takes to traverse the build system which cuts down on build times. IMO it also makes things easier to manage and grok since build config is spread out across N-1 files.
Attachment #8347762 - Flags: review?(gps) → review+
(In reply to Gregory Szorc [:gps] from comment #49)
> @@ +26,5 @@
> > +} else if (SharedAll.Constants.Win) {
> > +  path = ctypes.libraryName('nss3');
> > +} else {
> > +  path = SharedAll.Constants.Path.libxul;
> > +}
> 
> I'm not crazy about build details leaking into the source like this. Seems
> like something we should integrate into the source code via preprocessing.
> But since we have test coverage and it would require Makefile.in hackery
> currently, I think it can be a followup. Please file a bug in Core :: Build
> Config and drop a comment in the source.

It's a fairly common idiom in Mozilla JS code to dynamically platform detect.  In Thunderbird we took to using idioms like "(Services.appinfo.OS == 'Darwin')".  (Before that it was '("nsILocalFileMac" in Ci)', and it appears Services.appinfo is getting a lot of use in Firefox too (http://mxr.mozilla.org/mozilla-central/search?string=Services.appinfo).  There is also of course a whole lot of older JS code that lives in jar.mn that gets preprocessed up the wazoo, but...

The cost of pre-processing has historically involved breaking text editors with some smarts in them/ external linting/static analysis tools/etc.  So ideally in the master plan, please take that into account, limiting JS to something syntactically valid like:

- just doing @substitutions@ inside strings (checking that it's actually in a string with naive-ish string detecting)

- having the preprocessor directives live in something JS understands as a comment, like "//#if"

- I think it's actually too powerful to use in its raw form, but using something like the mozilla hygienic macro project https://github.com/mozilla/sweet.js is a way to do something that can be used to help enforce valid control-flow when doing preprocessing.  Specifically, rather than having snippets of code that may or may not end up in the code, resulting in smart tools potentially seeing illegal combinations of things, the conditionalized code could be forced into blocks.

However, my favorite is to completely avoid preprocessing the JS and just bulk up the support in nsIXULAppInfo (https://developer.mozilla.org/en-US/docs/Using_nsIXULAppInfo) or something similar.  Far better to just have a single JSM that gets pre-processed gunk from the build system/configure.in embedded into it like the fact that we're using system NSS or system SQLite.  I expect this would be easier for new contributors to understand too, since then there is no magic preprocessing in the code, but the magic lives at an explicit dependency path that is/can be documented on MDN and that follows resolution rules.  (And if they just mxr the jsm or whatever, that JSM can include comments in it that explain what is getting filled in, etc.)
Build peeps: Please read last few comments w.r.t. including library names in JS and preprocessing JS. Stuff to keep in mind going forward.

Andrew: Your comments regarding existing preprocessing sucking, using sweet.js, and merging things into Services.appinfo won't fall on deaf ears. Chances are that most things are the way they are for hysterical raisins. I'm not a huge fan of the existing JS preprocessing either (because it typically breaks my editor). We're doing stuff to the build system to make plugging in different processors easier. Come up with a proposal for doing it better, file a bug, and we'll see what we can do.
These days, we cannot rely upon nsIXULAppInfo or Services.jsm, as we want our code to work on workers. However, as far as js-ctypes is concerned, we are trying to move this kind of information to |OS.Constants|. This is a native JS object build from pre-processing information.

I believe that this should not get in the way of the current bug. If somebody disagrees, we could quickly add |OS.Constants.Path.libSqlite|.
(In reply to David Rajchenbach Teller [:Yoric] (away 20 Dec to Jan 4th - please use "needinfo?") from comment #53)
> I believe that this should not get in the way of the current bug. If
> somebody disagrees, we could quickly add |OS.Constants.Path.libSqlite|.

I suggested a followup in my review. We're just having a creative conversation about suckitude. Sorry for derailing.
Filed this specific bit as Bug 951838.
Note bug 862317 should be fixed instead of doing this:

@@ +26,5 @@
> +} else if (SharedAll.Constants.Win) {
> +  path = ctypes.libraryName('nss3');
> +} else {
> +  path = SharedAll.Constants.Path.libxul;
> +}
Depends on: 951838
Posted patch 853439 patch v9 (obsolete) — Splinter Review
Depends on OS.Constants.Path.libSqlite3 availability. Addressed gps' comments.
Attachment #8347761 - Attachment is obsolete: true
Attachment #8349861 - Flags: review?(dteller)
Posted patch 853439 tests v8 (obsolete) — Splinter Review
Carrying over r+ from gps and yoric with comments addressed.
Attachment #8349861 - Attachment is obsolete: true
Attachment #8349861 - Flags: review?(dteller)
Attachment #8349863 - Flags: review+
Posted patch 853439 patch v9 (obsolete) — Splinter Review
Oops, made the wrong patch obsolete.
Attachment #8347762 - Attachment is obsolete: true
Attachment #8349865 - Flags: review?(dteller)
Comment on attachment 8349863 [details] [diff] [review]
853439 tests v8

># HG changeset patch
># User Yura Zenevich <yura.zenevich@gmail.com>
>
>Bug 853439 - Adding tests for js-ctypes bindings for sqlite. r=yoric, gps
>
>---
> .../sqlite/tests/xpcshell/data/chrome.manifest     |   1 +
> .../tests/xpcshell/data/worker_sqlite_internal.js  | 279 +++++++++++++++++++++
> .../tests/xpcshell/data/worker_sqlite_shared.js    |  39 +++
> .../sqlite/tests/xpcshell/test_sqlite_internal.js  |  43 ++++
> .../components/sqlite/tests/xpcshell/xpcshell.ini  |   9 +
> 5 files changed, 371 insertions(+)
> create mode 100644 toolkit/components/sqlite/tests/xpcshell/data/chrome.manifest
> create mode 100644 toolkit/components/sqlite/tests/xpcshell/data/worker_sqlite_internal.js
> create mode 100644 toolkit/components/sqlite/tests/xpcshell/data/worker_sqlite_shared.js
> create mode 100644 toolkit/components/sqlite/tests/xpcshell/test_sqlite_internal.js
> create mode 100644 toolkit/components/sqlite/tests/xpcshell/xpcshell.ini
>
>diff --git a/toolkit/components/sqlite/tests/xpcshell/data/chrome.manifest b/toolkit/components/sqlite/tests/xpcshell/data/chrome.manifest
>new file mode 100644
>index 0000000..92b9cf6
>--- /dev/null
>+++ b/toolkit/components/sqlite/tests/xpcshell/data/chrome.manifest
>@@ -0,0 +1 @@
>+content test_sqlite_internal ./
>diff --git a/toolkit/components/sqlite/tests/xpcshell/data/worker_sqlite_internal.js b/toolkit/components/sqlite/tests/xpcshell/data/worker_sqlite_internal.js
>new file mode 100644
>index 0000000..cb0876e
>--- /dev/null
>+++ b/toolkit/components/sqlite/tests/xpcshell/data/worker_sqlite_internal.js
>@@ -0,0 +1,279 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+importScripts("worker_sqlite_shared.js",
>+  "resource://gre/modules/workers/require.js");
>+
>+self.onmessage = function onmessage(msg) {
>+  try {
>+    run_test();
>+  } catch (ex) {
>+    let {message, moduleStack, moduleName, lineNumber} = ex;
>+    let error = new Error(message, moduleName, lineNumber);
>+    error.stack = moduleStack;
>+    dump("Uncaught error: " + error + "\n");
>+    dump("Full stack: " + moduleStack + "\n");
>+    throw error;
>+  }
>+};
>+
>+let Sqlite;
>+
>+let SQLITE_OK;   /* Successful result */
>+let SQLITE_ROW;  /* sqlite3_step() has another row ready */
>+let SQLITE_DONE; /* sqlite3_step() has finished executing */
>+
>+function test_init() {
>+  do_print("Starting test_init");
>+  // Sqlite should be loaded.
>+  Sqlite = require("resource://gre/modules/sqlite/sqlite_internal.js");
>+  do_check_neq(typeof Sqlite, "undefined");
>+  do_check_neq(typeof Sqlite.Constants, "undefined");
>+  SQLITE_OK = Sqlite.Constants.SQLITE_OK;
>+  SQLITE_ROW = Sqlite.Constants.SQLITE_ROW;
>+  SQLITE_DONE = Sqlite.Constants.SQLITE_DONE;
>+}
>+
>+/**
>+ * Clean up the database.
>+ * @param  {sqlite3_ptr} db A pointer to the database.
>+ */
>+function cleanupDB(db) {
>+  withQuery(db, "DROP TABLE IF EXISTS TEST;", SQLITE_DONE);
>+}
>+
>+/**
>+ * Open and close sqlite3 database.
>+ * @param  {String}   open            A name of the sqlite3 open function to be
>+ *                                    used.
>+ * @param  {Array}    openArgs = []   Optional arguments to open function.
>+ * @param  {Function} callback = null An optional callback to be run after the
>+ *                                    database is opened but before it is
>+ *                                    closed.
>+ */
>+function withDB(open, openArgs = [], callback = null) {
>+  let db = Sqlite.Type.sqlite3_ptr.implementation();
>+  let dbPtr = db.address();
>+
>+  // Open database.
>+  let result = Sqlite[open].apply(Sqlite, ["data/test.db", dbPtr].concat(
>+    openArgs));
>+  do_check_eq(result, SQLITE_OK);
>+
>+  // Drop the test table if it already exists.
>+  cleanupDB(db);
>+
>+  try {
>+    if (callback) {
>+      callback(db);
>+    }
>+  } catch (ex) {
>+    do_check_true(false);
>+    throw ex;
>+  } finally {
>+    // Drop the test table if it still exists.
>+    cleanupDB(db);
>+    // Close data base.
>+    result = Sqlite.close(db);
>+    do_check_eq(result, SQLITE_OK);
>+  }
>+}
>+
>+/**
>+ * Execute an SQL query using sqlite3 API.
>+ * @param  {sqlite3_ptr} db         A pointer to the database.
>+ * @param  {String}      sql        A SQL query string.
>+ * @param  {Number}      stepResult Expected result code after evaluating the
>+ *                                  SQL statement.
>+ * @param  {Function}    bind       An optional callback with SQL binding steps.
>+ * @param  {Function}    callback   An optional callback that runs after the SQL
>+ *                                  query completes.
>+ */
>+function withQuery(db, sql, stepResult, bind, callback) {
>+  // Create an instance of a single SQL statement.
>+  let sqlStmt = Sqlite.Type.sqlite3_stmt_ptr.implementation();
>+  let sqlStmtPtr = sqlStmt.address();
>+
>+  // Unused portion of an SQL query.
>+  let unused = Sqlite.Type.cstring.implementation();
>+  let unusedPtr = unused.address();
>+
>+  // Compile an SQL statement.
>+  let result = Sqlite.prepare_v2(db, sql, sql.length, sqlStmtPtr, unusedPtr);
>+  do_check_eq(result, SQLITE_OK);
>+
>+  try {
>+    if (bind) {
>+      bind(sqlStmt);
>+    }
>+
>+    // Evaluate an SQL statement.
>+    result = Sqlite.step(sqlStmt);
>+    do_check_eq(result, stepResult);
>+
>+    if (callback) {
>+      callback(sqlStmt);
>+    }
>+  } catch (ex) {
>+    do_check_true(false);
>+    throw ex;
>+  } finally {
>+    // Destroy a prepared statement object.
>+    result = Sqlite.finalize(sqlStmt);
>+    do_check_eq(result, SQLITE_OK);
>+  }
>+}
>+
>+function test_open_close() {
>+  do_print("Starting test_open_close");
>+  do_check_eq(typeof Sqlite.open, "function");
>+  do_check_eq(typeof Sqlite.close, "function");
>+
>+  withDB("open");
>+}
>+
>+function test_open_v2_close() {
>+  do_print("Starting test_open_v2_close");
>+  do_check_eq(typeof Sqlite.open_v2, "function");
>+
>+  withDB("open_v2", [0x02, null]);
>+}
>+
>+function createTableOnOpen(db) {
>+  withQuery(db, "CREATE TABLE TEST(" +
>+              "ID INT PRIMARY KEY NOT NULL," +
>+              "FIELD1 INT," +
>+              "FIELD2 REAL," +
>+              "FIELD3 TEXT," +
>+              "FIELD4 TEXT," +
>+              "FIELD5 BLOB" +
>+            ");", SQLITE_DONE);
>+}
>+
>+function test_create_table() {
>+  do_print("Starting test_create_table");
>+  do_check_eq(typeof Sqlite.prepare_v2, "function");
>+  do_check_eq(typeof Sqlite.step, "function");
>+  do_check_eq(typeof Sqlite.finalize, "function");
>+
>+  withDB("open", [], createTableOnOpen);
>+}
>+
>+/**
>+ * Read column values after evaluating the SQL SELECT statement.
>+ * @param  {sqlite3_stmt_ptr} sqlStmt A pointer to the SQL statement.
>+ */
>+function onSqlite3Step(sqlStmt) {
>+  // Get an int value from a query result from the ID (column 0).
>+  let field = Sqlite.column_int(sqlStmt, 0);
>+  do_check_eq(field, 3);
>+
>+  // Get an int value from a query result from the column 1.
>+  field = Sqlite.column_int(sqlStmt, 1);
>+  do_check_eq(field, 2);
>+  // Get an int64 value from a query result from the column 1.
>+  field = Sqlite.column_int64(sqlStmt, 1);
>+  do_check_eq(field, 2);
>+
>+  // Get a double value from a query result from the column 2.
>+  field = Sqlite.column_double(sqlStmt, 2);
>+  do_check_eq(field, 1.2);
>+
>+  // Get a number of bytes of the value in the column 3.
>+  let bytes = Sqlite.column_bytes(sqlStmt, 3);
>+  do_check_eq(bytes, 4);
>+  // Get a text(cstring) value from a query result from the column 3.
>+  field = Sqlite.column_text(sqlStmt, 3);
>+  do_check_eq(field.readString(), "DATA");
>+
>+  // Get a number of bytes of the UTF-16 value in the column 4.
>+  bytes = Sqlite.column_bytes16(sqlStmt, 4);
>+  do_check_eq(bytes, 8);
>+  // Get a text16(wstring) value from a query result from the column 4.
>+  field = Sqlite.column_text16(sqlStmt, 4);
>+  do_check_eq(field.readString(), "TADA");
>+
>+  // Get a blob value from a query result from the column 5.
>+  field = Sqlite.column_blob(sqlStmt, 5);
>+  do_check_eq(ctypes.cast(field,
>+    Sqlite.Type.cstring.implementation).readString(), "BLOB");
>+}
>+
>+function test_insert_select() {
>+  do_print("Starting test_insert_select");
>+  do_check_eq(typeof Sqlite.column_int, "function");
>+  do_check_eq(typeof Sqlite.column_int64, "function");
>+  do_check_eq(typeof Sqlite.column_double, "function");
>+  do_check_eq(typeof Sqlite.column_bytes, "function");
>+  do_check_eq(typeof Sqlite.column_text, "function");
>+  do_check_eq(typeof Sqlite.column_text16, "function");
>+  do_check_eq(typeof Sqlite.column_blob, "function");
>+
>+  function onOpen(db) {
>+    createTableOnOpen(db);
>+    withQuery(db,
>+      "INSERT INTO TEST VALUES (3, 2, 1.2, \"DATA\", \"TADA\", \"BLOB\");",
>+      SQLITE_DONE);
>+    withQuery(db, "SELECT * FROM TEST;", SQLITE_ROW, null, onSqlite3Step);
>+  }
>+
>+  withDB("open", [], onOpen);
>+}
>+
>+function test_insert_bind_select() {
>+  do_print("Starting test_insert_bind_select");
>+  do_check_eq(typeof Sqlite.bind_int, "function");
>+  do_check_eq(typeof Sqlite.bind_int64, "function");
>+  do_check_eq(typeof Sqlite.bind_double, "function");
>+  do_check_eq(typeof Sqlite.bind_text, "function");
>+  do_check_eq(typeof Sqlite.bind_text16, "function");
>+  do_check_eq(typeof Sqlite.bind_blob, "function");
>+
>+  function onBind(sqlStmt) {
>+    // Bind an int value to the ID (column 0).
>+    let result = Sqlite.bind_int(sqlStmt, 1, 3);
>+    do_check_eq(result, SQLITE_OK);
>+
>+    // Bind an int64 value to the FIELD1 (column 1).
>+    result = Sqlite.bind_int64(sqlStmt, 2, 2);
>+    do_check_eq(result, SQLITE_OK);
>+
>+    // Bind a double value to the FIELD2 (column 2).
>+    result = Sqlite.bind_double(sqlStmt, 3, 1.2);
>+    do_check_eq(result, SQLITE_OK);
>+
>+    // Destructor.
>+    let destructor = Sqlite.Constants.SQLITE_TRANSIENT;
>+    // Bind a text value to the FIELD3 (column 3).
>+    result = Sqlite.bind_text(sqlStmt, 4, "DATA", 4, destructor);
>+    do_check_eq(result, SQLITE_OK);
>+
>+    // Bind a text16 value to the FIELD4 (column 4).
>+    result = Sqlite.bind_text16(sqlStmt, 5, "TADA", 8, destructor);
>+    do_check_eq(result, SQLITE_OK);
>+
>+    // Bind a blob value to the FIELD5 (column 5).
>+    result = Sqlite.bind_blob(sqlStmt, 6, ctypes.char.array()("BLOB"), 4,
>+      destructor);
>+    do_check_eq(result, SQLITE_OK);
>+  }
>+
>+  function onOpen(db) {
>+    createTableOnOpen(db);
>+    withQuery(db, "INSERT INTO TEST VALUES (?, ?, ?, ?, ?, ?);", SQLITE_DONE,
>+      onBind);
>+    withQuery(db, "SELECT * FROM TEST;", SQLITE_ROW, null, onSqlite3Step);
>+  }
>+
>+  withDB("open", [], onOpen);
>+}
>+
>+function run_test() {
>+  test_init();
>+  test_open_close();
>+  test_open_v2_close();
>+  test_create_table();
>+  test_insert_select();
>+  test_insert_bind_select();
>+  do_test_complete();
>+}
>diff --git a/toolkit/components/sqlite/tests/xpcshell/data/worker_sqlite_shared.js b/toolkit/components/sqlite/tests/xpcshell/data/worker_sqlite_shared.js
>new file mode 100644
>index 0000000..54351a0
>--- /dev/null
>+++ b/toolkit/components/sqlite/tests/xpcshell/data/worker_sqlite_shared.js
>@@ -0,0 +1,39 @@
>+/* Any copyright is dedicated to the Public Domain.
>+ * http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+function send(message) {
>+  self.postMessage(message);
>+}
>+
>+function do_test_complete() {
>+  send({kind: "do_test_complete", args: []});
>+}
>+
>+function do_check_true(x) {
>+  send({kind: "do_check_true", args: [!!x]});
>+  if (x) {
>+    dump("TEST-PASS: " + x + "\n");
>+  } else {
>+    throw new Error("do_check_true failed");
>+  }
>+}
>+
>+function do_check_eq(a, b) {
>+  let result = a == b;
>+  send({kind: "do_check_true", args: [result]});
>+  if (!result) {
>+    throw new Error("do_check_eq failed " + a + " != " + b);
>+  }
>+}
>+
>+function do_check_neq(a, b) {
>+ let result = a != b;
>+ send({kind: "do_check_true", args: [result]});
>+ if (!result) {
>+   throw new Error("do_check_neq failed " + a + " == " + b);
>+ }
>+}
>+
>+function do_print(x) {
>+  dump("TEST-INFO: " + x + "\n");
>+}
>diff --git a/toolkit/components/sqlite/tests/xpcshell/test_sqlite_internal.js b/toolkit/components/sqlite/tests/xpcshell/test_sqlite_internal.js
>new file mode 100644
>index 0000000..c78aa6f
>--- /dev/null
>+++ b/toolkit/components/sqlite/tests/xpcshell/test_sqlite_internal.js
>@@ -0,0 +1,43 @@
>+/* Any copyright is dedicated to the Public Domain.
>+   http://creativecommons.org/publicdomain/zero/1.0/ */
>+
>+Components.utils.import("resource://gre/modules/Promise.jsm");
>+
>+let WORKER_SOURCE_URI =
>+  "chrome://test_sqlite_internal/content/worker_sqlite_internal.js";
>+do_load_manifest("data/chrome.manifest");
>+
>+function run_test() {
>+  run_next_test();
>+}
>+
>+add_task(function() {
>+  let worker = new ChromeWorker(WORKER_SOURCE_URI);
>+  let deferred = Promise.defer();
>+  worker.onmessage = function(event) {
>+    let data = event.data;
>+    switch (data.kind) {
>+      case "do_check_true":
>+        try {
>+          do_check_true(data.args[0]);
>+        } catch (ex) {
>+          // Ignore errors
>+        }
>+        break;
>+      case "do_test_complete":
>+        deferred.resolve();
>+        worker.terminate();
>+        break;
>+      case "do_print":
>+        do_print(data.args[0]);
>+        break;
>+    }
>+  };
>+  worker.onerror = function(event) {
>+    let error = new Error(event.message, event.filename, event.lineno);
>+    worker.terminate();
>+    deferred.reject(error);
>+  };
>+  worker.postMessage("START");
>+  return deferred.promise;
>+});
>diff --git a/toolkit/components/sqlite/tests/xpcshell/xpcshell.ini b/toolkit/components/sqlite/tests/xpcshell/xpcshell.ini
>new file mode 100644
>index 0000000..b83610f
>--- /dev/null
>+++ b/toolkit/components/sqlite/tests/xpcshell/xpcshell.ini
>@@ -0,0 +1,9 @@
>+[DEFAULT]
>+head =
>+tail =
>+support-files =
>+  data/worker_sqlite_shared.js
>+  data/worker_sqlite_internal.js
>+  data/chrome.manifest
>+
>+[test_sqlite_internal.js]
>\ No newline at end of file
>-- 
>1.8.4.1
>
Attachment #8349863 - Flags: review+
Posted patch 853439 patch v9 (obsolete) — Splinter Review
Removed the [PATCH 1/2] bit from the commit message.
Attachment #8349865 - Attachment is obsolete: true
Attachment #8349865 - Flags: review?(dteller)
Attachment #8349869 - Flags: review?(dteller)
Removed the [PATCH 2/2] bit from the commit message.
Attachment #8349863 - Attachment is obsolete: true
(In reply to Mike Hommey [:glandium] from comment #56)
> Note bug 862317 should be fixed instead of doing this:
> 
> @@ +26,5 @@
> > +} else if (SharedAll.Constants.Win) {
> > +  path = ctypes.libraryName('nss3');
> > +} else {
> > +  path = SharedAll.Constants.Path.libxul;
> > +}

Now if only the developer needinfo?-ed on bug 862317 could provide the required information, that would be great :)
Comment on attachment 8349869 [details] [diff] [review]
853439 patch v9

Review of attachment 8349869 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Note that we didn't ask you to make the "nss" change in this bug, this could have waited for a followup bug.
Attachment #8349869 - Flags: review?(dteller) → review+
Carrying over the r+ from Yoric. Put back the platform conditionals.
Attachment #8349869 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b92c42667e59
https://hg.mozilla.org/mozilla-central/rev/2bb13ecf0297
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.