Child Tickets

Change History (25)

comment:1 Changed 4 months ago by igt0

Cc: igt0 added
Parent ID: #26531

comment:2 Changed 3 months ago by igt0

Status: newneeds_review

Bug 27111 - Update about:tor desktop version to work on mobile
https://github.com/igortoliveira/torbutton/commit/6ccd196ed2cf54d14fcb661cfdff0c59626191cd

comment:3 Changed 3 months ago by gk

Keywords: tbb-mobile TorBrowserTeam201808R added

comment:4 Changed 3 months ago by gk

Keywords: TorBrowserTeam201809R added; TorBrowserTeam201808R removed

Moving review tickets to September

comment:5 Changed 3 months ago by sysrqb

Keywords: TBA-a2 added
Parent ID: #26531

Moving to second-alpha TBA keyword.

comment:6 Changed 2 months ago by gk

Keywords: TorBrowserTeam201810R added; TorBrowserTeam201809R removed

Moving review tickets to October

comment:7 Changed 8 weeks ago by sysrqb

Status: needs_reviewneeds_revision

Partially unrelated, but ideally, when the new tb-manual is available for TBA, then we'll probably need to adjust the URL construction within src/chrome/content/aboutTor/aboutTor-content.js:onLocaleChange(). (I guess this is similar to option 1 in #20739). We'll need a new mockup for this, too.

---

In src/chrome/content/torbutton.js:torbutton_init():

+    if (torbutton_is_mobile()) {
+        torbutton_abouttor_message_handler.updateAllOpenPages();
+    }
+

Can you add a comment about why this is needed only on mobile?


We should set the homepage as about:tor, too. Fennec doesn't respect browser.startup.homepage, so we must hardcode it :/

The easiest option may be modifying mobile/android/base/java/org/mozilla/gecko/AboutPages.java:HOME. Unfortunately, this won't work well until torbutton is integrated and there's no longer the need of restarting the app before torbutton fully installs. Another option is adding in a check for browser.startup.homepage, and that overrides AboutPages:HOME.


This patch doesn't provide the same view as the mockup with the virtual keyboard opened (right-side of https://trac.torproject.org/projects/tor/attachment/ticket/25696/24309%20-%20TBA%20-%20Welcome%20Page.png). This'll require a tor-browser patch, as well, because Fennec automatically opens the ActivityStream HomePager every time the URL bar is focused.


Overall, I don't see any problems with this torbutton patch. This should be reviewed by someone else (in particular someone with more knowledge of this code) - making sure this doesn't break anything on desktop. I'm setting to needs_revision because I think explaining why about:tor requires an explicit signal on mobile during init() is helpful for reviewing this.

comment:8 Changed 7 weeks ago by gk

Keywords: TorBrowserTeam201810 added; TorBrowserTeam201810R removed

comment:9 Changed 7 weeks ago by pili

Sponsor: Sponsor8

comment:10 Changed 5 weeks ago by gk

Keywords: TorBrowserTeam201811 added; TorBrowserTeam201810 removed

Moving our tickets to November.

comment:11 Changed 5 weeks ago by gk

Priority: MediumHigh

comment:12 Changed 5 weeks ago by gk

Priority: HighVery High

comment:13 in reply to:  7 Changed 4 weeks ago by igt0

Status: needs_revisionneeds_review

I updated the patch:

https://github.com/igortoliveira/torbutton/commit/f5953aec52e8f9662c45cfff60cfe0b775ba43cb

and I replied the comments bellow:

Replying to sysrqb:

Partially unrelated, but ideally, when the new tb-manual is available for TBA, then we'll probably need to adjust the URL construction within src/chrome/content/aboutTor/aboutTor-content.js:onLocaleChange(). (I guess this is similar to option 1 in #20739). We'll need a new mockup for this, too.

Oh nice catch. I will talk with Antonela about it.

---

In src/chrome/content/torbutton.js:torbutton_init():

+    if (torbutton_is_mobile()) {
+        torbutton_abouttor_message_handler.updateAllOpenPages();
+    }
+

Can you add a comment about why this is needed only on mobile?

Left over. I removed it.


We should set the homepage as about:tor, too. Fennec doesn't respect browser.startup.homepage, so we must hardcode it :/

The easiest option may be modifying mobile/android/base/java/org/mozilla/gecko/AboutPages.java:HOME. Unfortunately, this won't work well until torbutton is integrated and there's no longer the need of restarting the app before torbutton fully installs. Another option is adding in a check for browser.startup.homepage, and that overrides AboutPages:HOME.

You are right. It is why I have a second patch for this bug where I add the following file to tor browser:

mobile/android/torbrowser/assets/distribution/preferences.json

{
  "ApplicationPreferences": {
    "newtab.load_homepage": true
  },
  "AndroidPreferences": {
    "homepage": "about:tor",
    "startpane_enabled_after_57": true,
    "startpane_enabled": true
  }
}


This patch doesn't provide the same view as the mockup with the virtual keyboard opened (right-side of https://trac.torproject.org/projects/tor/attachment/ticket/25696/24309%20-%20TBA%20-%20Welcome%20Page.png). This'll require a tor-browser patch, as well, because Fennec automatically opens the ActivityStream HomePager every time the URL bar is focused.

Yes, In a follow up patch, I will remove the following method call from the BrowserApp.java

showHomePagerWithAnimator(panelId, null, animator);

Overall, I don't see any problems with this torbutton patch. This should be reviewed by someone else (in particular someone with more knowledge of this code) - making sure this doesn't break anything on desktop. I'm setting to needs_revision because I think explaining why about:tor requires an explicit signal on mobile during init() is helpful for reviewing this.

comment:15 Changed 4 weeks ago by gk

Status: needs_reviewneeds_revision

I'll do a proper code review tomorrow with hopefully more brain available. However, here is some feedback. Strings are clipped on a Samsung Galaxy S5 mini and a bit misaligned if I have the browser open vertically (see: attached screenshot). Horizontally it looks pretty good.

Git is complaining about a new blank line at the end of `aboutTor.css. We should remove it.

More to come tomorrow...

Changed 4 weeks ago by gk

comment:16 Changed 4 weeks ago by gk

What is body[mobile] .torstatus-version, doing given that there is no class="torstatus-version" as far as I can see? And why do we have a second <div id="torstatus-version"/> now?

comment:17 in reply to:  16 Changed 4 weeks ago by igt0

Status: needs_revisionneeds_review

It is a left over from a rebase conflict. I updated the patch removing the inner torstatus-version and changing the style:

https://github.com/igortoliveira/torbutton/commit/08eff9958ff9035290501180bdeef2787f3e0971

Replying to gk:

What is body[mobile] .torstatus-version, doing given that there is no class="torstatus-version" as far as I can see? And why do we have a second <div id="torstatus-version"/> now?

comment:18 Changed 4 weeks ago by gk

Status: needs_reviewneeds_revision

I imagine we are doing torbutton_abouttor_message_handler.updateAllOpenPages(); directly after loadFrameScript() because we need take mobile/non-mobile state into account now? Could you add a comment above the updateAllOpenPages() explaining why it is needed?

There is still the empty line at the end of aboutTor.css that got added.

One thing I noticed while testing: we have about:tor now but it is not started every time Tor Browser for Android starts contrary to the desktop behavior. I think it would be good to have this behavior on mobile as well and be it just to see the donation banner more than once.

comment:19 in reply to:  18 Changed 3 weeks ago by igt0

I updated the code:

https://github.com/igortoliveira/torbutton/commit/4320dfc7c36eea5f486f38d391316a2f6695ce8b

Replying to gk:

I imagine we are doing torbutton_abouttor_message_handler.updateAllOpenPages(); directly after loadFrameScript() because we need take mobile/non-mobile state into account now? Could you add a comment above the updateAllOpenPages() explaining why it is needed?

Done

There is still the empty line at the end of aboutTor.css that got added.

Done

One thing I noticed while testing: we have about:tor now but it is not started every time Tor Browser for Android starts contrary to the desktop behavior. I think it would be good to have this behavior on mobile as well and be it just to see the donation banner more than once.

In fact, this is scary. Because all the privacy configuration we have for desktop are not working on mobile. I opened a ticket about it:

https://trac.torproject.org/projects/tor/ticket/28507

comment:20 Changed 3 weeks ago by gk

Keywords: TorBrowserTeam201811R added; TorBrowserTeam201811 removed

comment:21 Changed 3 weeks ago by gk

Status: needs_revisionneeds_review

comment:22 Changed 3 weeks ago by igt0

I updated the code:

https://github.com/igortoliveira/torbutton/commit/dcc568c0ba6e3a0c478d79c9e1e51a9dd311e68a

We don't need to explicitly call updateAllOpenPages.

comment:23 Changed 3 weeks ago by gk

Resolution: fixed
Status: needs_reviewclosed

The patches look good to me. I applied the Torbutton one as commit 61deb44d0c07eb1ec1d71eac2f0a7aeca87ea054 to the master branch and commit c0c87e1a9a6573ac480e4b46d232a4b451f6475c is now on tor-browser-60.3.0esr-8.5-1. I pushed a submodule update commit as well (commit 9a300eedd12c23e1ce48484816df00f4c1a77033).

Note: See TracTickets for help on using tickets.