795161 - dom/system/gonk/systemlibs.js is not importable

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Gonk, defect)

Description

[Hubert Figuiere [:hub]]

Can't import dom/system/gonk/systemlibs.js using Components.Utils.import().

Attaching patch.

Comment 1

[Hubert Figuiere [:hub]]

Attachment: Bug 795161 - Make systemlibs.js importable.

Comment 2

[Philipp von Weitershausen [:philikon]]

Comment on attachment 665692 [details] [diff] [review]
Bug 795161 - Make systemlibs.js importable.

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

The shell.js part seems unrelated to this patch. Also libcutils isn't used in shell.js so far. Maybe you accidentally included this in part in the patch?

::: dom/system/gonk/systemlibs.js
@@ +16,5 @@
> +if (!this.ctypes) {
> +  Components.utils.import("resource://gre/modules/ctypes.jsm");
> +}
> +
> +const EXPORTED_SYMBOLS = [ "libcutils", "libnetutils", "netHelpers" ];

I just had a thought. If this gets included inside a worker using importScripts() along with other modules that have similar consts, we're going to get an error because of two conflicting const definitions.

Here's what I suggest:

  let EXPORTED_SYMBOLS;
  if (!this.ctypes) {
    // We're likely being loaded as a JSM.
    EXPORTED_SYMBOLS = ["libcutils", "libnetutils", "netHelpers"];
    Components.utils.import("resource://gre/modules/ctypes.jsm");
  }

Comment 3

[Hubert Figuiere [:hub]]

(In reply to Philipp von Weitershausen [:philikon] from comment #2)
> Comment on attachment 665692 [details] [diff] [review]
> Bug 795161 - Make systemlibs.js importable.
> 
> Review of attachment 665692 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The shell.js part seems unrelated to this patch. Also libcutils isn't used
> in shell.js so far. Maybe you accidentally included this in part in the
> patch?

Oops. this wasn't supposed to be slid in. Will provide a new patch.

> 
> ::: dom/system/gonk/systemlibs.js
> @@ +16,5 @@
> > +if (!this.ctypes) {
> > +  Components.utils.import("resource://gre/modules/ctypes.jsm");
> > +}
> > +
> > +const EXPORTED_SYMBOLS = [ "libcutils", "libnetutils", "netHelpers" ];
> 
> I just had a thought. If this gets included inside a worker using
> importScripts() along with other modules that have similar consts, we're
> going to get an error because of two conflicting const definitions.
> 
> Here's what I suggest:
> 
>   let EXPORTED_SYMBOLS;
>   if (!this.ctypes) {
>     // We're likely being loaded as a JSM.
>     EXPORTED_SYMBOLS = ["libcutils", "libnetutils", "netHelpers"];
>     Components.utils.import("resource://gre/modules/ctypes.jsm");
>   }

Sounds fair. I'll try that approach.

Comment 4

[Hubert Figuiere [:hub]]

Attachment: Bug 795161 - Make systemlibs.js importable.

removed the irrelevant changes and adopted philikon suggest approach for EXPORTED_SYMBOLS.

Comment 5

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4cf6ad64c3f9

Comment 6

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4cf6ad64c3f9