[tor-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:
  GeorgKoppen201907, TorBrowserTeam201907,       |
  tbb-9.0-must-nightly                           |
Parent ID:                                       |         Points:
 Reviewer:                                       |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by gk):

 Replying to [comment:67 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`.

 Removing `example A` etc. makes me a bit nervous. It got introduced in
 #15086 back then. I am not sure whether that part has been essential on
 solving the RTL issues but we should double-check that we don't have weird
 regressions, in particular as all the other code surrounding the `<li>`
 elements, like styling is left untouched.

 [snip]

 > > 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?

 Yes, regarding your first and second question. I think there is no need
 for a translation repo patch. Just do the patch in Torbutton and it will
 propagate once someone commits the changes to `master`. The patch could be
 in #24653 which could be on top of the general #10760 patch for review. We
 can squash that one in a later rebasing then if we think that's useful.
 (If you go that route please make #24653 a child bug of this ticket so we
 don't lose track here)

 [snip]

 > > 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.

 Fair enough and, no, I don't have specific cases in mind right now.

 > > 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`?).

 I see. Let's go with what we have for now then.

 We are close here. :)

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