Closed Bug 795161 Opened 7 years ago Closed 7 years ago

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

Categories

(Core Graveyard :: Widget: Gonk, defect)

x86_64
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla18

People

(Reporter: hub, Assigned: hub)

References

Details

Attachments

(1 file, 1 obsolete file)

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

Attaching patch.
Attachment #665692 - Flags: review?(philipp)
Blocks: 777187
Version: 16 Branch → Trunk
Blocks: 795162
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");
  }
Attachment #665692 - Flags: review?(philipp)
Assignee: nobody → 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.
removed the irrelevant changes and adopted philikon suggest approach for EXPORTED_SYMBOLS.
Attachment #665692 - Attachment is obsolete: true
Attachment #665701 - Flags: review?(philipp)
Attachment #665701 - Flags: review?(philipp) → review+
https://hg.mozilla.org/mozilla-central/rev/4cf6ad64c3f9
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.