[tbb-dev] proposal for #10760 (Integrate TorButton to TorBrowser core ...)

[Georg Koppen]

Hi!

Alex Catarineu:
> A draft proposal for #10760 (Integrate TorButton to TorBrowser core to
> prevent users from disabling it) is
> attached.
> 
> This should already address some review comments in
> https://trac.torproject.org/projects/tor/ticket/10760#comment:36.
> 
> Feedback appreciated!

I think this mostly looks good, thanks! I try to comment on things from
the proposal, please bear with me as I am quoting from the document
without being able to comment inline as the proposal was attached. So, I
might have got things wrong or my comments might be misleading due to
not enough context provided. In general, I read the proposal from top to
down and comment in that order:

2.

"We refer to Tor Browser proposal 102 for motivation, since it is shared."

Could you add a link to prop 102 at the end of your proposal?

3.

"in tor-browser repository"

in the tor-browser repository

s/torbutton/Torbutton/g if you are talking about the project (like you
did in the overview part)

3.1

"in tor-browser repository"

in the tor-browser repository

"loaded in main process"

loaded in the main process

"Tor Button"

Torbutton

"commiters"

committers

3.3

"code in Android but also in Desktop"

code on Android but also on Desktop

3.4

"of torbutton repository"

of the torbutton repository

3.5.1

"Probably contains code that this not used anymore."

s/this/is/. (and, yes, the feature is not used anymore but there is some
code in New Identity that is still using the clean-up part; we want to
just use `torbutton_clear_cookies()` instead and remove the special
cookie handling in Torbutton)

3.5.2

"These will not work anymore on ESR68"

Overlays will not work anymore in ESR68

"Scripts that are needed both in mobile and desktop can be loaded from
  browser/base/content/browser.js"

Are you sure that the scripts on mobile still work if put there? I had
expected we would need to put code into
android/chrome/content/browser.js for that *if* they would be needed at
all. In particular because torbutton.xul is not be used on mobile, I
think, given that the UI is not in XUL.

"in previous file"

in the previous file

"   4) The security slider
    5) Tor Network Settings
"

It seems to me the former is implemented in security-prefs.js and the
latter in Tor Launcher, or mabye you meant something else?

s/Ticket/ticket/

4. Localization

"Tor button" -> Torbutton

Please update the link to 8.5 based on 60.6.1esr to 9.0 based on 60.7.0esr

"##30505" -> #30505

"been accepted merged" -> been accepted and merged

s/Ticket/ticket/g

Could you update the proposal regarding localization with the changes
for our duplicated entries we currently specify for both desktop and mobile?

Georg

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.torproject.org/pipermail/tbb-dev/attachments/20190611/1ad88d5a/attachment.sig>