891300 - [OS.File] Port ospath_* to worker module loader

Status: RESOLVED FIXED

(Toolkit Graveyard :: OS.File, defect)

Description

[David Teller [:Yoric] - still alive but not very active]

Continuing the work started in bug 888479.

Comment 1

[David Teller [:Yoric] - still alive but not very active]

Attachment: Porting OS.Path to require()

Comment 2

[David Teller [:Yoric] - still alive but not very active]

Main changes:
- ospath_{win, unix}_back.jsm are renamed ospath_{win, unix}.jsm;
- on non-main thread, both modules are now meant to be opened with require() rather than importScripts() – this should work with Jetpack's require(), too, untested;
- these two modules now export their functions directly instead of having side-effects upon global |OS|;
- these two modules don't depend on osfile_shared_allthreads.jsm anymore, and can be loaded on any platform;
- a new module ospath.jsm will make it easier to load the correct OS.Path without loading the whole of OS.File.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: Changes to ospath_{win, unix}.jsm, ignoring whitespace

To make it easier on my favorite OS.File reviewer, here's the ospath_*.jsm patch, minus whitespace changes.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 785000 [details] [diff] [review]
Changes to ospath_{win, unix}.jsm, ignoring whitespace

Most useful attachment EVAR.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 784993 [details] [diff] [review]
Porting OS.Path to require()

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

::: toolkit/components/osfile/Makefile.in
@@ +17,5 @@
>  
>  include $(topsrcdir)/config/rules.mk
>  
>  libs::
>  	$(NSINSTALL) $(srcdir)/osfile_shared_allthreads.jsm $(FINAL_TARGET)/modules/osfile

There ought to be some sort of rules.mk magic that would install all of these, shouldn't there?  (For a potential followup, not for this bug.)

::: toolkit/components/osfile/ospath_unix.jsm
@@ +22,3 @@
>  if (typeof Components != "undefined") {
> +  // Global definition of |module|, to keep everybody happy.
> +  // In non-main thread, |moodule| is provided by the module

Cows don't program.  Please fix the moodule.  (Shouldn't this be |exports| anyway?

::: toolkit/components/osfile/ospath_win.jsm
@@ +31,3 @@
>  if (typeof Components != "undefined") {
> +  // Global definition of |module|, to keep everybody happy.
> +  // In non-main thread, |moodule| is provided by the module

Cows don't program on Windows, either.  Same comments from the unix version apply.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=f74f0b4c3893

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Attachment: Porting OS.Path to require(), v2

Same one plus the missing file ospath.jsm.

Comment 8

[David Teller [:Yoric] - still alive but not very active]

Try: https://tbpl.mozilla.org/?tree=Try&rev=b083dd4477f9

Comment 9

[David Teller [:Yoric] - still alive but not very active]

Attachment: Porting OS.Path to require(), v3

Fixing typo
Try: https://tbpl.mozilla.org/?tree=Try&rev=b7f34a0a5560

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Unfortunately, I cannot land this bug atm because of bug 910159.

Comment 11

[David Teller [:Yoric] - still alive but not very active]

Attachment: Porting OS.Path to require(), v4

Comment 12

[David Teller [:Yoric] - still alive but not very active]

Attachment: Porting OS.Path to require(), v5

Unbitrotting.
Let's see if the blockers have been fixed.
Try: https://tbpl.mozilla.org/?tree=Try&rev=434af04f8b2c

Comment 13

[David Teller [:Yoric] - still alive but not very active]

Hey, looks like the blockers _have_ been fixed!
Time to land this.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/f2b7bf005584

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/f2b7bf005584