Update preferences.xul to make it work on mobile

TorButton preferences.xul doesn't work on mobile. It is simpler to make it work on mobile than updating the tor-browser-settings extension.

added TorBrowserTeam201808 component::applications/tor browser owner::tbb-team parent::26531 priority::very high resolution::fixed severity::normal sponsor::8 status::closed tbb-mobile tbb-torbutton type::defect labels

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.

Trac:
Status: needs_revision to needs_review

Looks good, one final nit: could you remove the comma in torOn: torbutton_tor_check_ok(),? We only have one item in the object.

Trac:
Status: needs_review to needs_revision

Updated code:

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

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

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

Replying to gk:

Looks good, one final nit: could you remove the comma in torOn: torbutton_tor_check_ok(),? We only have one item in the object.

Trac:
Status: needs_revision to needs_review

Alright, I set the slider to "safest" and went to the test on ip-check.info and still, it shows that JavaScript is enabled. Can anyone confirm this result?

The second question: What is the planned way to actually deliver this feature to users given that we won't send them to about:config? It seems there is a patch missing for that?

Trac:
Status: needs_review to needs_information

Replying to gk:

Alright, I set the slider to "safest" and went to the test on ip-check.info and still, it shows that JavaScript is enabled. Can anyone confirm this result?

I tested with the patch for #27220 (moved) and still got the same results. Here is the bundle I used:

https://people.torproject.org/~gk/testbuilds/fennec-60.1.0.en-US.android-arm.apk https://people.torproject.org/~gk/testbuilds/fennec-60.1.0.en-US.android-arm.apk.asc

The second question: What is the planned way to actually deliver this feature to users given that we won't send them to about:config? It seems there is a patch missing for that?

FWIW, that's #27220 (moved).

While looking at #27221 (moved) I copied an .xpi with this fix over an older Torbutton version and the browser on desktop is not showing the about:tor page anymore. It's complaining about the torbutton logger not available yet (undefined reference error). Thus, we need a better fix here.

Trac:
Status: needs_information to needs_revision

I opened the apk and it doesn't have the noscript extension in the distribution directory. It is why you are seeing JS enabled.

Replying to gk:

Replying to gk:

Alright, I set the slider to "safest" and went to the test on ip-check.info and still, it shows that JavaScript is enabled. Can anyone confirm this result?

I tested with the patch for #27220 (moved) and still got the same results. Here is the bundle I used:

https://people.torproject.org/~gk/testbuilds/fennec-60.1.0.en-US.android-arm.apk https://people.torproject.org/~gk/testbuilds/fennec-60.1.0.en-US.android-arm.apk.asc

The second question: What is the planned way to actually deliver this feature to users given that we won't send them to about:config? It seems there is a patch missing for that?

FWIW, that's #27220 (moved).

Arthur noticed we don't need the torbutton_get_general_useragent_locale because the getLocale does the exactly same thing and it is simpler. So I removed torbutton_get_general_useragent_locale.

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

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

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

Trac:
Status: needs_revision to needs_review

Okay, this looks good now, thanks! (And, indeed, the missing Noscript was the cause of the problem I encountered).

I merged your branch to master (commit 86adebdcf40095b8ac449129f6c6458c03d7a747) and tagged 2.0.3 we can use for the alpha.

Trac:
Status: needs_review to closed
Resolution: N/A to fixed

Damn, I should have tested it again:

JavaScript error: chrome://torbutton/content/torbutton.js, line 2251: TypeError: show_torbrowser_manual is not a function

this happens on a desktop Tor Browser (8.0a10 Linux).

Trac:
Resolution: fixed to N/A
Status: closed to reopened

Replying to gk:

Damn, I should have tested it again: {{{ JavaScript error: chrome://torbutton/content/torbutton.js, line 2251: TypeError: show_torbrowser_manual is not a function }}} this happens on a desktop Tor Browser (8.0a10 Linux).

Okay, I think what happened was me testing the commit before bumping the Torbutton version and then later, after tagging, I saw the error. Blowing the startupCache away manually "solves" this problem. igt0 verified that an actual version bump has the same effect. Thus, we are good here it seems and can proceed with 2.0.3.

Trac:
Status: reopened to closed
Resolution: N/A to fixed

Trac:
Keywords: N/A deleted, tbb-mobile added

Sponsor8 in August 2018.

Trac:
Sponsor: N/A to Sponsor8

closed

moved to tpo/applications/tor-browser#26884 (closed)