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!
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
Hi!
Thank you for the review. I addressed your comments, attaching a modified version of the proposal.
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.
You're right, for Android it should be android/chrome/content/browser.js, I fixed that.
In the current state I think torbutton.js is needed for mobile: it has logic that is common for mobile and desktop (loading about:tor content script, initializing SecurityPrefs, etc.) and also specific logic for mobile (e.g. setupPreferencesForMobile function). I guess it's currently overlaid on mobile's chrome://browser/content/browser.xul, so the scripts are loaded there.
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?
I guess I meant logic for opening dialogs for those. In any case, I removed these items. There are many things that torbutton.js does that are missing in that enumeration, and I think it's not so relevant for the proposal.
Alex
On 6/11/19 2:50 PM, Georg Koppen wrote:
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:
"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?
"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/
- 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
tbb-dev mailing list tbb-dev@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tbb-dev
-
Alex Catarineu -
Georg Koppen