[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #26401 [Applications/Tor Browser]: Rebase Orfox patches onto Tor Browser 8.0 for TBA
#26401: Rebase Orfox patches onto Tor Browser 8.0 for TBA
-----------------------------------------------+---------------------------
Reporter: sysrqb | Owner: tbb-team
Type: task | Status:
| needs_review
Priority: Very High | Milestone:
Component: Applications/Tor Browser | Version:
Severity: Normal | Resolution:
Keywords: tbb-mobile, TorBrowserTeam201807R | Actual Points:
Parent ID: #26531 | Points:
Reviewer: | Sponsor:
-----------------------------------------------+---------------------------
Comment (by gk):
Replying to [comment:20 sysrqb]:
> Replying to [comment:17 gk]:
[snip]
> > I guess the comments before the *strip options are essentially
enabling stripping? if so, we should be explicit about it an use
`--enable-strip` like on other platforms as well
> >
>
> The defaults are effectively `--disable-strip` and `--enable-install-
strip` but we should test these options and understand what improves and
what fails when we enable/disable them. I'm still not sure.
Well, that makes it so that we build with symbols but then strip them
during the packaging step (that can be confusing alone in case you want to
get debug symbols after packaging up everything). It seems to be cleaner
to me to indicate upfront in the .mozconfig file that we don't have
symbols available by specifying `--enable-strip`.
[snip]
> > commit 69bdd94ecb8e97e4d590dc75c04963b6659bdae0 -- probably okay (why
do we need the duplicated entries we already have in confvars.sh?)
>
> I added those in torbrowser.configure because they look like configure
flags but they are environment variables. They are still in
torbrowser.configure only for documentation purposes. I can delete them if
it's confusing or you think they aren't helpful.
I don't feel that strongly about it. Let's keep the commit as-is if you
want.
[snip]
> >
> > Why is "#ifdef TOR_BROWSER_VERSION" commented out? It seems to me we
don't want to point our users to the aus5 URL. Maybe I am missing
something here.
>
> Hm! Those `//` were only supposed to be visual and not affect the
inclusion. I thought the preprocessor preserves and enforces the ifdef
when it scans the file - but I see this did not happen.
`app.update.url.android` is still set. I'll look into this more.
I don't think that's the case for the .js file. At any rate, I would
follow the style Mozilla has in this file which is plain `#ifdef`'s.
[snip]
> >
> > commit 4b3c94077749e620f8d9055412ab01bf4286b435 -- probably okay (What
is the threat here?)
>
> I think this provides a safe default. When this is enabled, the user is
provided an icon in the awesome bar for scanning a QRCode that
(presumably) contains a URL the user wants to visit. When the icon is
pressed, another app is opened (if it is installed) where a picture is
taken and it is scanned for a QRCode and then decoded. I believe the
scanning/decoding is performed on a remote server. Disabling this by
default protects against accidentally revealing their location to third
parties. The user could tap the icon by mistake and launch the third party
app, and we don't have any control over what that app does or what
information that app sends.
Good point. I was not aware that the QRCode treatment is actually done
remotely, so, yes, let's keep the commit.
[snip]
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/26401#comment:23>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs