[tbb-bugs] #26401 [Applications/Tor Browser]: Rebase Orfox patches onto Tor Browser 8.0 for TBA

[Tor Bug Tracker & Wiki]

#26401: Rebase Orfox patches onto Tor Browser 8.0 for TBA
-----------------------------------------------+---------------------------
 Reporter:  sysrqb                             |          Owner:  tbb-team
     Type:  task                               |         Status:
                                               |  needs_review
 Priority:  Very High                          |      Milestone:
Component:  Applications/Tor Browser           |        Version:
 Severity:  Normal                             |     Resolution:
 Keywords:  tbb-mobile, TorBrowserTeam201807R  |  Actual Points:
Parent ID:  #26531                             |         Points:
 Reviewer:                                     |        Sponsor:
-----------------------------------------------+---------------------------

Comment (by gk):

 I started my review based on `tor-browser-60.1.0esr-8.0-1+26401` which is
 what I stuck to. Here comes the first batch of comments:

 commit 970c95dff0b30eaf0597e43c442b375fe8a68da0 -- not okay

 "We exclude the all testStumbler*.java files" (one of "the" and "all"
 should be
 enough :) )

 Maybe use '' instead of "" as all exclude rules are using the former

 commit 2f7b87b25f2b2648f2f72addae1bcf7283bbf7da  -- okay
 commit 66ecd900c106de3bb84ba9f5aa1cdc59ab28cdd7 -- not okay

 .mozconfig-android: are we really still using ndk r10e for esr60; I guess
 the comments before the *strip options are essentially enabling stripping?
 if so, we should be explicit about it an use `--enable-strip` like on
 other platforms as well

 mozconfig.common.override: why do we have all of those changes here?
 Shouldn't the .mozconfig-android file already take care of them? If there
 are some options missing in the former let's add them there. Looking
 closer at that file it seems this is intended for Mozilla's try infra?
 While I think that's worthwhile I think we should have a separate bug for
 that and thinking about a strategy including all the other platforms we
 support as well.

 "When using --disable-crashreporter the symbole file" -> "When using
 --disable-crashreporter the symbols file"

 commit 9d8303c145317d067b130855eb0b17c70c614d76 -- okay (should we file a
 bug in Mozilla's bug tracker for the unused defines?)

 commit 6d7a72d3e95bf483a9256cfe3bf9b3888a363da3 -- okay
 commit ae36b1d20547358c49ee2206af5e28767ea7d48b -- not okay

 Indentation:
 {{{
 +        if (!AppConstants.isTorBrowser()) {
          getApplicationContext().sendBroadcast(
                  new Intent(INTENT_REGISTER_STUMBLER_LISTENER)
          );
 +        } // !isTorBrowser()
 }}}

 commit 69bdd94ecb8e97e4d590dc75c04963b6659bdae0 -- probably okay (why do
 we need the duplicated entries we already have in confvars.sh?)

 commit 97ca08c7c8bc6d58cbeac4838cf2587dc8603050 -- not okay

 I think we can skip the whole UA override thing as it does not play any
 role anymore once we set the resistfingerprinting pref (which is done for
 mobile as well)

 Why is "#ifdef TOR_BROWSER_VERSION" commented out? It seems to me we don't
 want to point our users to the aus5 URL. Maybe I am missing something
 here.

 s/URLS/URLs/

 no need for dom.battery.enabled anymore (see: #22554)

 `network.manage-offline-status` is already set in 000-tor-browser.js
 `geo.enabled` is already set in 000-tor-browser.js

 Do we need `network.tickle-wifi.enabled` given #18799?

 I think we should exclude the VR related pref and fix that in the desktop
 pref
 file instead (#21607).

 It's not clear to me why we have some of the prefs set in mobile but not
 desktop (e.g. the clearOnShutdown ones). I guess we could look over it in
 a new ticket to make sure we really only include android specific prefs in
 the new prefs file?

 commit 25ec90395fc82795e8b130c3d763d4b8893f1114 -- okay
 commit 4d1310b1c43975cfefaa8134f54f1e487db6e431 -- okay
 commit 91a6e589e9c89219eb15c721122a0564d7399b4b -- okay
 commit 4b3c94077749e620f8d9055412ab01bf4286b435 -- probably okay (What is
 the threat here?)
 commit 85b5d47bfa7f2ecc33767e0d5cd4bfd0a0a33460 -- okay
 commit 908c2f542a82df40f5757f0439a054e8067d8da8 -- probably okay (Is the
 additional, empty line intentional?
 {{{
      <uses-permission
 android:name="android.permission.ACCESS_NETWORK_STATE"/>
  #endif
 +
      <uses-permission android:name="android.permission.INTERNET"/>
 }}}
 )
 commit 47c04b0641898dfe2a2f1ca53aee101f41809d15 -- okay
 commit d9370c64b055fdcb9293aa1fc718eab8c2876257 -- okay
 commit 5494b0193552b2833d05bd507264d9796b651cca -- okay
 commit cc199e9fda917acee9435eba791a8d1a69536443 -- okay
 commit ad48b4d561522353c621a5d5166fc31b2517b624 -- okay
 commit 035fbfb88a3c05c9b9776139b4e318ee956a1f6f -- okay
 commit cb7427ebb4805c42e67cb0e93b3f8b8a5b19a365 -- okay
 commit 21f0480aff989a3853188a2ae424152a8e34b72b -- okay
 commit e0bb700e33001643de6d3e58adf8a2c881a61dbc -- okay

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