Trac:
Keywords: N/A deleted, tbb-torbutton, TorBrowserTeam201807 added
Parent: N/A to #26531 (moved)
Priority: Medium to Very High
Owner: N/A to tbb-team
Component: Applications/Torbutton to Applications/Tor Browser

XUL components are not well supported on mobile, e.g. the slider doesn't render. I am migrating the XUL markup to XHTML.

XUL doesn't work well on mobile, so I implemented the mobile preferences in XHTML.

Initially, I tried to make the code reusable across the mobile and desktop, however I was losing lot of time trying to make the Desktop version work. So this patchset has just the mobile implementation.

You can see the patches here: https://github.com/igortoliveira/torbutton/commits/26884

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils https://github.com/igortoliveira/torbutton/commit/07382c5ee23470bbc08a785c2b349fdb06010696

Bug 26884 - Part 2: Create mobile security slider https://github.com/igortoliveira/torbutton/commit/6dfbf8bc311208a14f146f80fb285dcd5efc3f45

Bug 26884 - Part 3: Remove optionsURL from install.rdf https://github.com/igortoliveira/torbutton/commit/973b138dd24cd193b990c43c8c9eaa221a8d55b4

Trac:
Status: new to needs_review

Trac:

Trac:
Keywords: TorBrowserTeam201807 deleted, TorBrowserTeam201807R added

Trac:
Keywords: TorBrowserTeam201807R deleted, TorBrowserTeam201808R added

Nice! How can we test that? I.e. how is it integrated into the .apk (build process)?

Replying to gk:

Nice! How can we test that? I.e. how is it integrated into the .apk (build process)?

It is not integrated in the build process. You need to copy the xpi to the distribution directory(mobile/android/torbrowser/assets/distribution/extensions).

Besides that you will need to update the extension preferences info: extensions.enabledAddons: Update the torbutton version to 2.0.1

extensions.legacy.enabled: It needs to be true

I wonder if we can integrate it in the work made by sisbell.

Replying to igt0:

Replying to gk:

Nice! How can we test that? I.e. how is it integrated into the .apk (build process)?

It is not integrated in the build process. You need to copy the xpi to the distribution directory(mobile/android/torbrowser/assets/distribution/extensions).

For this

1. export TB_BUILD_WITH_DISTRIBUTION=1 (or set this env variable however you like) so the distribution is included by .mozconfig-android
2. create mobile/android/torbrowser/assets/distribution/extensions, as igt0 said, if that directory structure doesn't already exist, and copy the xpi into the extensions directory.

Replying to igt0:

XUL doesn't work well on mobile, so I implemented the mobile preferences in XHTML.

Initially, I tried to make the code reusable across the mobile and desktop, however I was losing lot of time trying to make the Desktop version work. So this patchset has just the mobile implementation.

The original XUL implementation still works on Desktop, correct? Only mobile uses XHTML?

You can see the patches here: https://github.com/igortoliveira/torbutton/commits/26884

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils https://github.com/igortoliveira/torbutton/commit/07382c5ee23470bbc08a785c2b349fdb06010696

Seems okay - but Arthur, maybe you want to skim through one?

---

Bug 26884 - Part 2: Create mobile security slider https://github.com/igortoliveira/torbutton/commit/6dfbf8bc311208a14f146f80fb285dcd5efc3f45

Missing (optional) semi-colon - but all the other lines end with a semi-colon:

+// Set the desired slider value and update UI.
+function torbutton_set_slider(sliderValue) {
+  state.slider = sliderValue

(preferences.js is missing a semicolon on this line, too)

---

Unnecessary semi-colon after the closing curly bracket of a few functions. It seems like those may be copied from preferences.js.

---

descNames and linkNames are reversed in preferences-mobile.js (compared to preferences.js). Can you add a comment about this?

---

SECURITY_PREFERENFES_URI should be _PREFERENCES_?

---

Bug 26884 - Part 3: Remove optionsURL from install.rdf https://github.com/igortoliveira/torbutton/commit/973b138dd24cd193b990c43c8c9eaa221a8d55b4

Seems okay.

Do we need to add fennec as a new target application in install.rdf?

+        <!-- fennec -->
+        <em:targetApplication>
+            <Description>
+                <em:id>{aa3c5121-dab2-40e2-81ca-7ea25febc110}</em:id>
+                <em:minVersion>60.0</em:minVersion>
+                <em:maxVersion>10000.0</em:maxVersion>
+            </Description>
+        </em:targetApplication>

Trac:
Status: needs_review to needs_revision
Cc: sysrqb, gk to sysrqb, gk, arthuredelstein

Also, how did you bypass the addon signiture restriction? Gecko : 1534486863961 addons.xpi WARN Invalid XPI: signature is required but missing

loadManifest() calls mustSign()

Oh, right. Okay, i got this working by toggling xpinstall.signatures.required. Then I forced installation by going to file:///data/data/org.torproject.torbrowser/distribution/extensions/ and tapping on torbutton@torproject.org.xpi. After restarting the app, torbutton runs and I see "Security Settings" in the menu. igt0, nice job.

When I open the Security Settings, I see the slider. However, tapping the actual label "Standard" and "Safest" result in the position jumping to the other location (tapping on "Standard" causes the location indicator to jump to "Safest", and vice versa). I think the noscript settings are reversed, as well.

Replying to sysrqb:

Replying to igt0:

XUL doesn't work well on mobile, so I implemented the mobile preferences in XHTML.

Initially, I tried to make the code reusable across the mobile and desktop, however I was losing lot of time trying to make the Desktop version work. So this patchset has just the mobile implementation.

The original XUL implementation still works on Desktop, correct? Only mobile uses XHTML?

You can see the patches here: https://github.com/igortoliveira/torbutton/commits/26884

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils https://github.com/igortoliveira/torbutton/commit/07382c5ee23470bbc08a785c2b349fdb06010696

Seems okay - but Arthur, maybe you want to skim through one?

I have some nits: a) Please keep the comment which was available above torbutton_show_torbrowser_manual(). b)

// torbutton_show_torbrowser_manual() returns true.

needs to get reworded. c) If there is an error in get_general_useragent_locale() we should have some way of showing it (e.g. by emitting a message to the console), even if the usual logging functionality is not available in the module. I'd like to avoid omitting any kind of notification in such a case.

Bug 26884 - Part 3: Remove optionsURL from install.rdf https://github.com/igortoliveira/torbutton/commit/973b138dd24cd193b990c43c8c9eaa221a8d55b4

Seems okay.

Do we need to add fennec as a new target application in install.rdf? {{{

•    <!-- fennec -->

•    <em:targetApplication>

•        <Description>

•            <em:id>{aa3c5121-dab2-40e2-81ca-7ea25febc110}</em:id>

•            <em:minVersion>60.0</em:minVersion>

•            <em:maxVersion>10000.0</em:maxVersion>

•        </Description>

•    </em:targetApplication>

}}}

I am not sure, because legacy extensions are not supported by default in Fennec and I believe the targetApplication is a way to protect the user of installing it in unsupported versions. So even if the user tries to install just the torbutton, Fennec will not allow it.

Updated code:

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils https://github.com/igortoliveira/torbutton/commit/d69a48bbaa5fbed41e11448ed5800f9f75cfd124

Bug 26884 - Part 2: Create mobile security slider https://github.com/igortoliveira/torbutton/commit/67b54c3d6a767dfa0146967a8e3675d8dee1e8ff

Bug 26884 - Part 3: Remove optionsURL from install.rdf https://github.com/igortoliveira/torbutton/commit/742843fe8fd181cf7c8632779069517814d6110b

Trac:
Status: needs_revision to needs_review

Replying to igt0:

Updated code:

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils https://github.com/igortoliveira/torbutton/commit/d69a48bbaa5fbed41e11448ed5800f9f75cfd124

Let's keep the loglevel to 4 as we had it in the original code. And probably add err to the log string to give some clue about what went wrong? I guess just the "Error while getting locale" + err would do it.

Trac:
Status: needs_review to needs_revision
Keywords: TorBrowserTeam201808R deleted, TorBrowserTeam201808 added

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils https://github.com/igortoliveira/torbutton/commit/056b82ca7e77c1d3848b149a74b644a2ef003519

Bug 26884 - Part 2: Create mobile security slider https://github.com/igortoliveira/torbutton/commit/b6392b49c8eb7f586cbc7dcfb07be991a80f0e2b

Bug 26884 - Part 3: Remove optionsURL from install.rdf https://github.com/igortoliveira/torbutton/commit/8bd10b44346f26e5d8e881fd33e3458e80b9ee91

Trac:
Status: needs_revision to needs_review

Thanks, looks good. We are close here. Could you do a final rebase of your patches against master? There are a bunch of conflicts due to the desktop onboarding/about:tor-changes.

Trac:
Status: needs_review to needs_revision

Rebased code:

Bug 26884 - Part 1: Move show_torbrowser_manual and get_general_useragent_locale to utils https://github.com/igortoliveira/torbutton/commit/9757d18579fd63340cd2877ea0b26da9cb61eb91

Bug 26884 - Part 2: Create mobile security slider https://github.com/igortoliveira/torbutton/commit/9b194a0a6e8ea659d59024796f5df33f4b0c327c

Bug 26884 - Part 3: Remove optionsURL from install.rdf https://github.com/igortoliveira/torbutton/commit/cc90adc868bfaebe298db09155a9412334e1dfa3

Here is the branch: https://github.com/igortoliveira/torbutton/commits/26884-v4

Replying to gk:

Thanks, looks good. We are close here. Could you do a final rebase of your patches against master? There are a bunch of conflicts due to the desktop onboarding/about:tor-changes.