#26884 closed defect (fixed)
Update preferences.xul to make it work on mobile
Reported by: | igt0 | Owned by: | tbb-team |
---|---|---|---|
Priority: | Very High | Milestone: | |
Component: | Applications/Tor Browser | Version: | |
Severity: | Normal | Keywords: | tbb-torbutton, tbb-mobile, TorBrowserTeam201808 |
Cc: | sysrqb, gk, arthuredelstein | Actual Points: | |
Parent ID: | #26531 | Points: | |
Reviewer: | Sponsor: | Sponsor8 |
Description
TorButton preferences.xul doesn't work on mobile. It is simpler to make it work on mobile than updating the tor-browser-settings extension.
Child Tickets
Attachments (1)
Change History (32)
comment:1 Changed 17 months ago by
Component: | Applications/Torbutton → Applications/Tor Browser |
---|---|
Keywords: | tbb-torbutton TorBrowserTeam201807 added |
Owner: | set to tbb-team |
Parent ID: | → #26531 |
Priority: | Medium → Very High |
comment:2 Changed 17 months ago by
comment:3 follow-up: 9 Changed 17 months ago by
Status: | new → needs_review |
---|
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
Changed 17 months ago by
Attachment: | screenshot_slider.png added |
---|
comment:4 Changed 17 months ago by
Keywords: | TorBrowserTeam201807R added; TorBrowserTeam201807 removed |
---|
comment:5 Changed 17 months ago by
Keywords: | TorBrowserTeam201808R added; TorBrowserTeam201807R removed |
---|
comment:6 follow-up: 7 Changed 17 months ago by
Nice! How can we test that? I.e. how is it integrated into the .apk (build process)?
comment:7 follow-up: 8 Changed 17 months ago by
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.
comment:8 Changed 17 months ago by
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
- export TB_BUILD_WITH_DISTRIBUTION=1 (or set this env variable however you like) so the distribution is included by .mozconfig-android
- 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.
comment:9 follow-ups: 12 13 Changed 16 months ago by
Cc: | arthuredelstein added |
---|---|
Status: | needs_review → needs_revision |
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>
comment:10 Changed 16 months ago by
Also, how did you bypass the addon signiture restriction?
Gecko : 1534486863961 addons.xpi WARN Invalid XPI: signature is required but missing
loadManifest() calls mustSign()
comment:11 Changed 16 months ago by
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.
comment:12 Changed 16 months ago by
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.
comment:13 Changed 16 months ago by
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.
comment:14 follow-up: 15 Changed 16 months ago by
Status: | needs_revision → needs_review |
---|
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
comment:15 Changed 16 months ago by
Keywords: | TorBrowserTeam201808 added; TorBrowserTeam201808R removed |
---|---|
Status: | needs_review → needs_revision |
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.
comment:16 Changed 16 months ago by
Status: | needs_revision → needs_review |
---|
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
comment:17 follow-up: 18 Changed 16 months ago by
Status: | needs_review → needs_revision |
---|
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.
comment:18 Changed 16 months ago by
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.
comment:19 Changed 16 months ago by
Status: | needs_revision → needs_review |
---|
comment:20 follow-up: 21 Changed 16 months ago by
Status: | needs_review → needs_revision |
---|
Looks good, one final nit: could you remove the comma in torOn: torbutton_tor_check_ok(),
? We only have one item in the object.
comment:21 Changed 16 months ago by
Status: | needs_revision → needs_review |
---|
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.
comment:22 follow-up: 23 Changed 16 months ago by
Status: | needs_review → needs_information |
---|
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?
comment:23 follow-up: 25 Changed 16 months ago by
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 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.
comment:24 Changed 16 months ago by
Status: | needs_information → needs_revision |
---|
While looking at #27221 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.
comment:25 Changed 16 months ago by
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 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.
comment:26 Changed 16 months ago by
Status: | needs_revision → needs_review |
---|
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
comment:27 Changed 16 months ago by
Resolution: | → fixed |
---|---|
Status: | needs_review → closed |
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.
comment:28 follow-up: 29 Changed 16 months ago by
Resolution: | fixed |
---|---|
Status: | closed → reopened |
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).
comment:29 Changed 16 months ago by
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
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 functionthis 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
.
comment:30 Changed 16 months ago by
Keywords: | tbb-mobile added |
---|
XUL components are not well supported on mobile, e.g. the slider doesn't render. I am migrating the XUL markup to XHTML.