Hi,
Alan wrote (24 Nov 2013 15:05:50 GMT) :
> Code review
> ===========
> - I'm not convinced by the folowing change
> in config/chroot_apt/preferences
> +Package: *
> +Pin: origin mozilla.debian.net
> +Pin-Priority: 990
> If I'm not mistaken, that would pull any package from
> mozilla.debian.net.
If 1. it's newer than the corresponding package in the Debian archive;
and 2. we don't ship the same package in our own APT repository, then
yes.
> If a package appears there for whatever reason,
> it is likely to break the feature in subtle ways. Why not list the
> specific packages we want, as we did before?
New packages appear there (and old packages disappear, which has been
much more problematic for us) when a new Iceweasel is ready, that has
new dependencies. What you suggest forces the person who updates our
Iceweasel packages to also update apt/preferences accordingly.
That's tedious try'n'error work, and I'd rather skip it.
I trust the Mozilla Debian team not to upload unnecessary stuff in
their repository. If such a mistake happened, then I believe Debian
users will detect it before we do, so I'm not worried.
There are two other reasons why I'm not worried. First, we do review
package version changes at ISO testing time, so we'll detect any crazy
change. Second, the time when the content of this repository changes
is a time when we're going to build, upload and test a new Iceweasel
ourselves, so we'll have our eyes focused on potential new problems.
Your attention to detail is appreciated: it forced me to think about
it more thoroughly than what I had previously done.
> - wiki/src/contribute/design/stream_isolation.mdwn: please document why
> the port number doesn't follow the other's logic:
> +* dedicated `SocksPort` for web browser (9151): no stream
> + isolation options
The answer to your question is documented in the right section of this
page already, and would have nothing to do in the implementation
details section. Reviewing doc changes without having the big picture
in mind often gives poor results.
> - you forgot to push 496ad87 to the feature branch.
Right. Pushed.
> Otherwise I'm happy with it.
:)
> Tests
> =====
>> Running getTorbuttonUserAgent should produce the useragent set by the
>> Torbutton version installed and used in Iceweasel.
> Please give me a hint how to run that.
Start a terminal, type "getT", then <TAB> :)
> My conculsion
> =============
> Amazing job!
> Please push your branch, and answer the APT preferences question before
> I merge.
I think we're now good to go. I'm relieved.
Cheers,
--
intrigeri
| GnuPG key @ https://gaffer.ptitcanardnoir.org/intrigeri/intrigeri.asc
| OTR fingerprint @ https://gaffer.ptitcanardnoir.org/intrigeri/otr.asc
