[tbb-bugs] #10760 [Applications/Tor Browser]: Integrate TorButton to TorBrowser core to prevent users from disabling it

[Tor Bug Tracker & Wiki]

#10760: Integrate TorButton to TorBrowser core to prevent users from disabling it
-------------------------------------------------+-------------------------
 Reporter:  Rezonansowy                          |          Owner:  tbb-
                                                 |  team
     Type:  defect                               |         Status:
                                                 |  needs_revision
 Priority:  High                                 |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  AffectsTails, tbb-parity, ux-team,   |  Actual Points:
  TorBrowserTeam201906, GeorgKoppen201906        |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by acat):

 I addressed the comments in a new branch: https://github.com/acatarineu
 /tor-browser/compare/30429+1..30429+2.

 I rebased the previous branch, fixing up the torbutton commit and
 squashing the circuit UI commit. Also realized that I had done the locale
 deduplication changes as a fixup for the torbutton integration commit, but
 I think it should have been for the security UI one. So I moved those
 changes to the security UI commit (Bug 25658). Also included a couple of
 changes not mentioned in the review: removed redundant torbutton in
 `toolkit/moz.build` and removed `Example *` from `identityPanel.inc.xul`.

 Is it fine to take 30429+2 as the next 'active' branch for #30429, or will
 make the ongoing review of the patches more difficult?

 > We don't need the stringbundleset for torbutton.properties anymore?.
 I think we don't, did not see any code in torbutton getting the
 stringbundle element by id.

 > Additionally, I guess you omitted all the toolbaricon parts as we don't
 want to expose the onion anymore on the toolbar? If so, then this sounds
 good to me.
 Yes, that's the case.

 > Where does it remove the duplicated translations in that commit?
 I meant that the duplicated strings from properties are not used anymore
 in that commit (and therefore can be removed). But did not remove them
 from the translation files yet.

 > The general approach looks good to me, nice find. I think we should get
 rid of all getString() calls while we are at it and make sure that
 everything we need is available both for desktop and mobile, hence in the
 .dtd file (we have #24653 for the localization parity part which could be
 solved while redoing this part).
 So, if I understand it correctly, we should remove
 securityLevel.properties and move the non-duplicated ones to
 torbutton.dtd. Should we adapt the keys like:
 `securityLevel.safest.tooltip` -> `torbutton.prefs.sec_safest_tooltip`?
 Should I do a translation repository patch for this and review it here?

 > any reason to not append the about:tor handler to the list in
 nsAboutRedirector.cpp but putting it between crashparent and crashcontent?
 No, changed that.

 > We should think as well more general about a mechanism of avoiding
 duplicated translations as I am not sure whether the hack you found is
 applicable in more than the sec-settings situation.
 Do you have in mind specific cases where it would not work? I think from
 privileged code we will always be able to create a domparser to get the
 translation (actually, currently also from non-privileged web content).
 But in any case, I think Fluent is the way to go for this, a single format
 that will work for programatic and "document" localization without these
 hacks.

 > using a NullPrincipal for new tabs seems indeed to be a good idea; Would
 we get away with that for about:tor as well given that we try hard to give
 it only content privs with the nsIAboutModule
 I did not find a way to open it with nullprincipal. I also tried other
 about:*, and only `about:home`, `about:newtab` and `about:welcome` worked
 for me (maybe it's because they have special handling in
 `AboutRedirector::NewChannel`?).

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/10760#comment:67>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online