[tor-bugs] #30237 [Applications/Tor Browser]: Tor Browser: Improve TBB UI of hidden service client authorization

[Tor Bug Tracker & Wiki]

#30237: Tor Browser: Improve TBB UI of hidden service client authorization
-------------------------------------------------+-------------------------
 Reporter:  asn                                  |          Owner:  mcs
     Type:  defect                               |         Status:
                                                 |  needs_information
 Priority:  Medium                               |      Milestone:
Component:  Applications/Tor Browser             |        Version:
 Severity:  Normal                               |     Resolution:
 Keywords:  TorBrowserTeam201911R, network-      |  Actual Points:
  team-roadmap-september                         |
Parent ID:  #30000                               |         Points:
 Reviewer:                                       |        Sponsor:
                                                 |  Sponsor27-must
-------------------------------------------------+-------------------------

Comment (by acat):

 > So, if I had my way, very little apart from init/uninit's and
 module+script includes would need to be added to vanilla Firefox sources
 to get the additional Tor-Browser functionality. That said I don't
 actually know if this pattern is a good idea or not in practice.

 > Do you have any opinions on this acat, sysrqb, gk?

 I think in this case it made sense to move it out to a separate module and
 just have `init` and `uninit`. Probably it always make sense to do this
 when the new code is a reasonably self-contained component.

 Not so sure about having a general rule, I think there are many cases
 where the changes are fairly small and doing includes might be a bit
 "artificial". For example: https://gitweb.torproject.org/tor-
 browser.git/commit/?h=tor-
 browser-68.2.0esr-9.5-1&id=15738bd0be40f8ee3f33b5da7d548c2b9066bce4. Here
 I think it makes more sense to have these two new `toolbarbutton` inline
 rather than putting them on a separate file and include it. For me it's
 more readable this way. Same for the new code in `CustomizableUI.jsm`, you
 can see that there is a `kTorVersion` variable which is analogous to
 Firefox's `kVersion` because it's next to it, and then the migration code
 for Tor Browser is next to the other Firefox's migration code. I just
 think there are cases where it's more readable to not do includes, but
 perhaps what I'm saying is obvious and pospeselr didn't actually mean
 these cases.

 Anyway, I think it's a good idea to try to keep modifications to Firefox
 sources as minimal as possible, and use includes and imports whenever it
 feels "natural" to do so. It's a bit vague, but I guess there will always
 be subjectiveness in code reviews :)

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