For playbackRate, we need to use SoundTouch. This bug is about importing the files in the tree.

about:license things may have to be changed.

I need some background here before reviewing. Why does this code need to be in a separate shared library?

Kyle, it is LGPL-licensed. Gerv and the legal folks have determined that it needs to be in a shared library to comply with the license. The discussion happened in bug 764495.

Yuck, ok.

So, to be clear, the shared library this code becomes will be shipped under the LGPL, as opposed to the rest of Mozilla, which is tri-licensed? If that's the case, we need to do a bunch more work here, because we do things put the tri-license in the copyright field on Windows DLLs.

I think you're right. We cannot re-license code that we take from somewhere on the internet, right? Are you referring to an .rc files such as this one: http://mxr.mozilla.org/mozilla-central/source/gfx/angle/src/libEGL/libEGL.rc, to put infos in the DLL ?

Yes. I didn't realize you could override them by providing your own. The default one is generated by http://mxr.mozilla.org/mozilla-central/source/config/version_win.pl#237. So this code will need a custom rc file.

Comment on attachment 648763 [details] [diff] [review] v1 Review of attachment 648763 [details] [diff] [review]: ----------------------------------------------------------------- This needs the custom .rc file that we talked about. Also we need to add soundtouch.dll to dependentlibs.list. Other than that it looks good. ::: browser/installer/windows/nsis/shared.nsh @@ +1030,5 @@ > Push "nspr4.dll" > Push "nssdbm3.dll" > Push "mozsqlite3.dll" > Push "xpcom.dll" > + Push "soundtouch.dll" I'd like Rob Strong to glance at this to make sure that the order doesn't matter here or something weird. Also there are several dlls missing ... ::: layout/media/symbols.def.in @@ +104,5 @@ > speex_resampler_strerror > #endif > +#ifdef MOZ_SOUNDTOUCH > + > +#endif ....

I added the RCFILE and RESFILE variable assignment in the makefile and the .rc file (adapted from the one present in the original soundtouch sources). Somehow, they are not picked up by the build system, and that result in a .dll that has the default information. Troubleshooting that issue, I noticed that the same thing happens with the libEGL{v2,}.dll we have in the tree, that _should_ be copyright Google, but are shown as copyright Mozilla.

(Spotted a typo when checking I had attached the right patch.)

Any news on the .rc problem?

I tried to apply the patch and build with it, but the build fails.

I guess the attached patch on the bug was a bit old or something. This compiles on all platform on try.

That patch works fine for me if I uncomment the relevant lines in the makefile. It's pretty messed up that we have to point RESFILE at a non-existent file to get RCFILE to get picked up by the build though.

Indeed, this compiles fine locally, but not on try. I'll try to install the same VS version to check what's going on.

Disregard the previous comment, it was just me being stupid. I've checked that libsoundtouch is picked up by the dependentlibs.py script, and addressed the other comment. I've also added the license bits that Gerv told me to put in about:license.

Comment on attachment 661824 [details] [diff] [review] Import SoundTouch Library in the tree. r= Robert, could you look at the Windows installer bits in this patch?

(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) [Away until September 18th (MozCamp EU, then vacation)] from comment #9) > Comment on attachment 648763 [details] [diff] [review] > v1 > > Review of attachment 648763 [details] [diff] [review]: > ----------------------------------------------------------------- > > This needs the custom .rc file that we talked about. > > Also we need to add soundtouch.dll to dependentlibs.list. > > Other than that it looks good. > > ::: browser/installer/windows/nsis/shared.nsh > @@ +1030,5 @@ > > Push "nspr4.dll" > > Push "nssdbm3.dll" > > Push "mozsqlite3.dll" > > Push "xpcom.dll" > > + Push "soundtouch.dll" > > I'd like Rob Strong to glance at this to make sure that the order doesn't > matter here or something weird. > > Also there are several dlls missing ... This is done before installing so we can notify the user prior to installing that they will likely need to restart their system if any of these files are in use. It doesn't list them all because it has to make copies of the dll which can take a while for the larger ones. I also don't want to add to the list unless we know it is necessary and I plan on reworking this code anyways.

Comment on attachment 661824 [details] [diff] [review] Import SoundTouch Library in the tree. r= Don't bother with adding the dll to the check in shared.nsh per previous comment

Addressed rstrong comments.

Comment on attachment 663177 [details] [diff] [review] Import SoundTouch Library in the tree. r= r+ for the installer/* changes

Comment on attachment 663177 [details] [diff] [review] Import SoundTouch Library in the tree. r= I just noticed that I cancelled the wrong review. Kyle, can you make sure this is alright? Changes from last patch are mentioned in comment 17.

Backed out because reviewers names were missing from the commit message in https://hg.mozilla.org/integration/mozilla-inbound/rev/a100edfd3ca0, relanded in https://hg.mozilla.org/integration/mozilla-inbound/rev/2df5960779db.

The library is not used by anything at the moment, and we're going to merge to aurora. Are there plans to use this library in FF19 or is that for later? If the former, we should really disable building and shipping it.

(In reply to Mike Hommey [:glandium] from comment #27) > If the former err, latter.

Would disabling it in the file system be sufficient? It would not be compiled nor shipped this way, I guess.

(In reply to Paul ADENOT (:padenot) from comment #29) > Would disabling it in the file system be sufficient? It would not be > compiled nor shipped this way, I guess. You'd need to remove the -lsoundtouch from the libxul linkage command line.

This is green on try. I plan to enable it and land the patch that uses the library right after the uplift.

Comment on attachment 683095 [details] [diff] [review] Disable build and linking of libsoundtouch as it is not used for now. r= Paul: I think you forgot to add the soundtouch lib to removed-files.in so that the file gets removed on update via the installer or the in-app update.

I don't think this matters much for a file that was never shipped in a release or a beta.

(In reply to Phil Ringnalda (:philor) from comment #32) > https://hg.mozilla.org/mozilla-central/rev/e35029dcac2d Note this didn't make it to aurora, we need an uplift.

Comment on attachment 683095 [details] [diff] [review] Disable build and linking of libsoundtouch as it is not used for now. r= [Approval Request Comment] Bug caused by (feature/regressing bug #): Bug 779997 User impact if declined: libsoundtouch would be shipped with aurora and subsequent releases, but not used. This makes the packages bigger in size. Testing completed (on m-c, etc.): Green try. Risk to taking this patch (and alternatives if risky): Not risky, the code was not exercised anyways. It's only a build system change, the removal of a library. String or UUID changes made by this patch: None.

Comment on attachment 683095 [details] [diff] [review] Disable build and linking of libsoundtouch as it is not used for now. r= [Triage Comment] Low risk build system change in support of smaller package sizes. Approving for Aurora 19.

So this is disabled on m-c now and will soon be disabled on aurora too, right?

Ryan, this is not disabled on central, because I'm about to land a big patch that uses this library. This is disabled on aurora because I lacked time to land the patch before the uplift and we did not want to increase the size of the package.

OK, I guess I was confused by comment 32 then.

And backed out since I landed the feature patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/98a1b656277b