#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
_______________________________________________ tor-bugs mailing list tor-bugs@lists.torproject.org https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs