24/02/14 16:54, Kathleen Brade wrote:
> On 2/20/14, 4:15 PM, anonym wrote:
>> Any ETA on when my patches can be reviewed? ...
>
> Feedback from Mark and myself for:
> 0001-Support-packaging-as-a-standalone-XUL-application.patch
> ------------------
> For the Makefile:
> A small issue: Mark and I would prefer that files not be copied into
> the "src" tree during packaging. An alternative would be to stage the
> files under "pkg" (I realize that will be more work).
Fixed. Also got rid of `--transform` which apparently is a GNU extension
of tar.
> For application.ini.in:
> Why do you have MinVersion set to 17.0.0?
> Unless for some reason you need MinVersion set to 17, you should make it
> 24.0.0. At this point, we really don't want anyone running with Gecko
> older than 24. We are going to change the minVersion in Tor Launcher's
> install.rdf file to 24.0.0 as well.
I picked the MinVersion from src/install.rdf. Bumped to 24.0.0 now.
> Feedback for:
> 0002-Split-Tor-process-starting-code-from-control-code.patch
> ------------------
> For tl-process.js:
> Mark and I prefer that the line setting mTorProcessStatus to unknown be
> left in _startTor(). Some day we may call that multiple times and we'll
> want it initialized correctly in there. The status is initialized to
> unknown (0) upon creation so you do not need to set it in _controlTor().
Fixed.
> Feedback for:
> 0003-If-TOR_CONFIGURE_ONLY-1-only-configure-Tor.patch
> ------------------
> For tl-process.js:
> Please fix the else to be on its own line (to match the rest of the file).
Fixed.
> For tl-util.jsm:
> It would be nice if "shouldOnlyConfigureTor()" had a corresponding pref
> like shouldStartAndOwnTor() does (other people may find the concept
> useful). Perhaps "extensions.torlauncher.only_configure_tor"?
Added as "extensions.torlauncher.only_configure_tor", like you suggested.
> Feedback for:
> 0004-If-FORCE_NET_CONFIG-is-set-show-network-settings-iff.patch
> ------------------
> Please change "FORCE_NET_CONFIG" to "TOR_FORCE_NET_CONFIG".
Fixed.
Per Mike's request I created tickets for this and attached the new
patches (also rebased against current master), so please have a look at
#11074 and children.
Cheers!
