1062874 - install head_libmar.js via test manifests, rather than libs:: rules

Status: RESOLVED FIXED

(Toolkit :: Application Update, defect)

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: part 1 - remove preprocessor-requiring bits from head_libmar.js.in

We can determine certain bits at runtime, rather than requiring the file to be
preprocessed.

Comment 2

[Nathan Froyd [:froydnj]]

Attachment: part 2 - don't preprocess head_libmar.js

Now that we don't have to preprocess head_libmar.js, we can install it via the
test manifest.

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8484239 [details] [diff] [review]
part 2 - don't preprocess head_libmar.js

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

Nice, I fixed a few instances of this while doing the Mochitest-manifest work, it's great to get rid of these hacks.

Comment 4

[(not currently active) Ted Mielczarek]

Comment on attachment 8484166 [details] [diff] [review]
part 1 - remove preprocessor-requiring bits from head_libmar.js.in

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

::: modules/libmar/tests/unit/head_libmar.js.in
@@ +4,4 @@
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  
> +const isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes);

I believe nowadays you could just use nsIXULRuntime.OS:
http://mxr.mozilla.org/mozilla-central/source/xpcom/system/nsIXULRuntime.idl#42

Comment 5

[Brian R. Bondy [:bbondy]]

Comment on attachment 8484166 [details] [diff] [review]
part 1 - remove preprocessor-requiring bits from head_libmar.js.in

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

::: modules/libmar/tests/unit/head_libmar.js.in
@@ -1,4 @@
>  /* Any copyright is dedicated to the Public Domain.
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
> -const BIN_SUFFIX = "@BIN_SUFFIX@";

Why don't we use @BIN_SUFFIX@ anymore?  I understand that most OS do not have a bin suffix.

@@ +4,4 @@
>  const Cc = Components.classes;
>  const Ci = Components.interfaces;
>  
> +const isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes);

I guess that would be a check like this http://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/DownloadsCommon.jsm#522 (minus the version part of it)

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Brian R. Bondy [:bbondy] from comment #5)
> Comment on attachment 8484166 [details] [diff] [review]
> part 1 - remove preprocessor-requiring bits from head_libmar.js.in
> 
> Review of attachment 8484166 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: modules/libmar/tests/unit/head_libmar.js.in
> @@ -1,4 @@
> >  /* Any copyright is dedicated to the Public Domain.
> >     http://creativecommons.org/publicdomain/zero/1.0/ */
> >  
> > -const BIN_SUFFIX = "@BIN_SUFFIX@";
> 
> Why don't we use @BIN_SUFFIX@ anymore?  I understand that most OS do not
> have a bin suffix.

Because @BIN_SUFFIX@ requires preprocessing; we might as well just hardcode the only case (Windows) that matters in this file.

> @@ +4,4 @@
> >  const Cc = Components.classes;
> >  const Ci = Components.interfaces;
> >  
> > +const isWindows = ("@mozilla.org/windows-registry-key;1" in Components.classes);
> 
> I guess that would be a check like this
> http://dxr.mozilla.org/mozilla-central/source/browser/components/downloads/
> DownloadsCommon.jsm#522 (minus the version part of it)

Indeed.  I thought using nsIXULRuntime was more complicated than that in xpcshell, but I could use that as well.

Comment 7

[Brian R. Bondy [:bbondy]]

Comment on attachment 8484166 [details] [diff] [review]
part 1 - remove preprocessor-requiring bits from head_libmar.js.in

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

Seems to me like it is trivial processing needed, but since ted looked at this and didn't comment on the @BIN_SUFFIX@ I assume that's ok.
For the other change about isWindows, it's already like that in a bunch of other places so I'm fine with it, but feel free to update to using the recommended method by ted.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e57baba6e459
https://hg.mozilla.org/mozilla-central/rev/636b1a268c33