Closed Bug 1062874 Opened 7 years ago Closed 7 years ago

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

Categories

(Toolkit :: Application Update, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(2 files)

No description provided.
We can determine certain bits at runtime, rather than requiring the file to be
preprocessed.
Attachment #8484166 - Flags: review?(robert.strong.bugs)
Now that we don't have to preprocess head_libmar.js, we can install it via the
test manifest.
Attachment #8484239 - Flags: review?(ted)
Attachment #8484166 - Flags: review?(robert.strong.bugs) → review?(netzen)
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.
Attachment #8484239 - Flags: review?(ted) → review+
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 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)
(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 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.
Attachment #8484166 - Flags: review?(netzen) → review+
https://hg.mozilla.org/mozilla-central/rev/e57baba6e459
https://hg.mozilla.org/mozilla-central/rev/636b1a268c33
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
You need to log in before you can comment on or make changes to this bug.