Opened 6 years ago

Closed 8 weeks ago

#10760 closed defect (fixed)

Integrate TorButton to TorBrowser core to prevent users from disabling it

Reported by: Rezonansowy Owned by: tbb-team
Priority: High Milestone:
Component: Applications/Tor Browser Version:
Severity: Normal Keywords: AffectsTails, tbb-parity, ux-team, tbb-9.0-must-alpha, GeorgKoppen201908, TorBrowserTeam201910, tbb-no-uplift
Cc: boklm, igt0, intrigeri, anonym Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

I mean integration like this with pdf.js addon, which was simply integrated to Firefox core.

Child Tickets

TicketStatusOwnerSummaryComponent
#24653closedtbb-teamApply security slider improvements made on desktop back to mobileApplications/Tor Browser
#25013closedtbb-teamMove TorButton code to the tor browser repositoryApplications/Tor Browser
#25856closedtbb-teamRemove XUL overlays from TorbuttonApplications/Tor Browser
#27511closedtbb-teamAdd New identity button to toolbarApplications/Tor Browser
#28561closedpospeselrMigrate custom 'About Tor Browser' dialog from torbutton to tor-browser brandingApplications/Tor Browser

Change History (93)

comment:1 Changed 6 years ago by Rezonansowy

Component: - Select a componentTor bundles/installation
Owner: set to erinn

comment:2 Changed 6 years ago by cypherpunks

Some users use the the "core" as a privacy-enhanced Firefox. I use the FF ESR Portable executable launcher to facilitate this.

comment:3 Changed 6 years ago by Rezonansowy

Summary: Iintegrate TorButton and TorLauncher to TorBrowser core to prevent users from disabling themIntegrate TorButton and TorLauncher to TorBrowser core to prevent users from disabling them

comment:4 in reply to:  2 Changed 6 years ago by Rezonansowy

Replying to cypherpunks:

Some users use the the "core" as a privacy-enhanced Firefox. I use the FF ESR Portable executable launcher to facilitate this.

I meant that core is the place where are located integrated addons, which you can't disable in, for example - pdf.js addon.
Allowing users to disable something which even has no description (e.g. TorLauncher) would allow them to completely turn off their privacy by browsing through Tor.

comment:5 Changed 5 years ago by erinn

Keywords: needs-triage added

comment:6 Changed 5 years ago by Rezonansowy

Priority: normalmajor

I think this must be fixed soon, it really makes a big risk to many less experienced users.

comment:7 Changed 5 years ago by cypherpunks

Component: Tor bundles/installationTor Browser
Owner: changed from erinn to tbb-team

comment:8 Changed 3 years ago by nord-stream

Severity: Normal

We can use the directories <app_dir>/browser/extensions or <app_dir>/browser/features to enforce the proper installation of the add-ons.

comment:9 Changed 3 years ago by cypherpunks

Integrate TorButton and TorLauncher to TorBrowser core to prevent users from disabling them

Fuck off. I use TB without Tor to get access to I2P. Sometimes I use TB with plugins disabled when I'm offline.

Last edited 3 years ago by cypherpunks (previous) (diff)

comment:10 Changed 3 years ago by yawning

Cc: yawning added

comment:11 Changed 3 years ago by boklm

Cc: boklm added

comment:12 Changed 21 months ago by gk

Parent ID: #24855

comment:13 Changed 11 months ago by gk

Cc: igt0 added
Keywords: TorBrowserTeam201901 added; needs-triage removed
Summary: Integrate TorButton and TorLauncher to TorBrowser core to prevent users from disabling themIntegrate TorButton to TorBrowser core to prevent users from disabling them

That's the ticket for Torbutton now. The one for Tor Launcher is #28044.

comment:14 Changed 11 months ago by intrigeri

Cc: intrigeri added

comment:15 Changed 11 months ago by intrigeri

Keywords: AffectsTails added

FTR, Tails' "Unsafe Browser" is basically Tor Browser, with Torbutton disabled and a scary homepage. It would be nice if there were still a hidden way for us to disable Torbutton for that browser profile. Otherwise, we'll have to ship binaries for another browser, which will make our ISO and upgrade delta significantly bigger. In order to plan Tails work on this topic, I need to know whether it'll still be possible to disable Torbutton somehow, and if not, I would be very grateful to learn what's the timeline is here, e.g. whether the plan is to ship this change in Tor Browser 8.5. Thanks in advance! Please let me know if you need additional info from me to answer my questions :)

comment:16 in reply to:  15 ; Changed 11 months ago by gk

Replying to intrigeri:

FTR, Tails' "Unsafe Browser" is basically Tor Browser, with Torbutton disabled and a scary homepage. It would be nice if there were still a hidden way for us to disable Torbutton for that browser profile.

I see. I'll keep that in mind and I guess we could make a hidden pref available or something.

Otherwise, we'll have to ship binaries for another browser, which will make our ISO and upgrade delta significantly bigger. In order to plan Tails work on this topic, I need to know whether it'll still be possible to disable Torbutton somehow, and if not, I would be very grateful to learn what's the timeline is here, e.g. whether the plan is to ship this change in Tor Browser 8.5. Thanks in advance! Please let me know if you need additional info from me to answer my questions :)

We didn't plan to have the option of disabling Torbutton. But we can try to make that happen. I doubt we'll have this ready for 8.5. I mean we probably could but I think we should have some more time to test this, in particular as there are downstream projects that might be affected by this. But this should land in alphas definitely way before we start the esr68 transition. So, my current plan is to have this in Tor Browser 9.0a1, which should get out end of March/begin of April.

FWIW: you might be interested in #28044, too.

comment:17 in reply to:  16 ; Changed 11 months ago by intrigeri

Replying to gk:

Replying to intrigeri:

FTR, Tails' "Unsafe Browser" is basically Tor Browser, with Torbutton disabled and a scary homepage. It would be nice if there were still a hidden way for us to disable Torbutton for that browser profile.

I see. I'll keep that in mind and I guess we could make a hidden pref available or something.

Great :)

But this should land in alphas definitely way before we start the esr68 transition. So, my current plan is to have this in Tor Browser 9.0a1, which should get out end of March/begin of April.

Thanks, now tracking this on https://redmine.tails.boum.org/code/issues/16357.

FWIW: you might be interested in #28044, too.

Yep, this one is on our radar (we discussed it in the roadmap thread :)

comment:18 in reply to:  17 ; Changed 11 months ago by gk

Replying to intrigeri:

Replying to gk:

Replying to intrigeri:

FTR, Tails' "Unsafe Browser" is basically Tor Browser, with Torbutton disabled and a scary homepage. It would be nice if there were still a hidden way for us to disable Torbutton for that browser profile.

I see. I'll keep that in mind and I guess we could make a hidden pref available or something.

Great :)

FWIW: integration of the various Torbutton pieces directly into the browser (without having an extension-like thing to disable anymore) might complicate the pref plan but it should still be possible. One thing that would be good to know is whether _all_ Torbutton features should be disabled or just, say, the Tor proxy related ones (I could easily see why the unsecure browser in Tails could have the external helper app warning dialog enabled (which is covered by an own pref anyway) or similar non-proxy features).

comment:19 Changed 10 months ago by gk

Keywords: GeorgKoppen201901 added

comment:20 Changed 10 months ago by gk

Keywords: GeorgKoppen201902 added; GeorgKoppen201901 removed

Moving my tickets to Feb

comment:21 Changed 10 months ago by gk

Keywords: TorBrowserTeam201902 added; TorBrowserTeam201901 removed

Moving tickets to February.

comment:22 in reply to:  18 Changed 10 months ago by intrigeri

Replying to gk:

One thing that would be good to know is whether _all_ Torbutton features should be disabled or just, say, the Tor proxy related ones (I could easily see why the unsecure browser in Tails could have the external helper app warning dialog enabled (which is covered by an own pref anyway) or similar non-proxy features).

Sorry for the delay! I think it's fine for us to have only the proxy features disabled, if this makes things easier for you.

comment:23 Changed 9 months ago by gk

Keywords: TorBrowserTeam201903 added; TorBrowserTeam201902 removed

Moving my tickets to March.

comment:24 Changed 9 months ago by gk

Keywords: GeorgKoppen201903 added; GeorgKoppen201902 removed

Now for my keyword.

comment:25 Changed 9 months ago by gk

Keywords: tbb-parity added
Summary: Integrate TorButton to TorBrowser core to prevent users from disabling themIntegrate TorButton to TorBrowser core to prevent users from disabling it

comment:26 Changed 8 months ago by gk

Keywords: TorBrowserTeam201904 added; TorBrowserTeam201903 removed

Moving tickets to April.

comment:27 Changed 8 months ago by gk

Keywords: GeorgKoppen201904 added; GeorgKoppen201903 removed

Moving my tickets for April

comment:28 Changed 7 months ago by gk

Okay, I made some more notes meanwhile:

torbutton-extensions.xul:
-------------------------
due to #10280 + #15984, can go during esr68 trans, brand.dtd contains locale
strings for that one

aboutDialog.xul:
----------------
modifications for about dialog, contains css in skin/aboutDialog.png; got
included in #5698 and we have #28561 for migration

big tasks:
----------
1) setting up localization mechanisms for translating torbutton strings in
tor-browser, maybe solve together with android strings translation

2) getting rid of our overlays and include that code directly in the browser
where it is needed (and get rid of the unnecessary things)

questions:
----------
mcs/brade: what are homePage* items in brand.properties still used for?
double-check dxr for usage

comment:29 Changed 7 months ago by acat

Status: newneeds_information

Are there any issues of using in desktop the same toolkit/torproject/torbutton used already in Android (#25013)?

Here https://github.com/acatarineu/tor-browser/commit/10760 I took the patch from #28044 (https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug28044-01&id=79f8ea9c98c69eb594e7d2aace9199b3bceb22cb) and did the same for torbutton, seems to work fine too. Note that tor-launcher patch uses the path browser/extensions/tor-launcher, while current Android and also this one uses toolkit/torproject/torbutton. I realized that in this case the final code goes to /chrome/torbutton inside omni.ja instead of browser/omni.ja as tor-launcher. Is one path preferred to the other?

There is this GetOverrideStringBundle function in https://gitweb.torproject.org/tor-browser.git/tree/toolkit/xre/nsAppRunner.cpp?h=tor-browser-60.6.1esr-8.5-1&id=178fcbbe24f543a15b165bdc680a5083247e87a3#n1823 that would need to be fixed, since the XPI would not be there. But I wonder if this code is still needed since general.useragent.locale pref is not there anymore (was moved to intl.locale.requested).

If this ticket is in the context of ESR68, then of course there is more work to do. But if it's just Integrate TorButton to TorBrowser core to prevent users from disabling it I think this patch might be good enough (together with a change in tor-browser-build, will do if this approach is fine). Perhaps we could track porting to ESR68, refactoring common parts with tor-launcher, etc. in separate tickets. Or make this ticket include all those, and keep working on this one.

Also, not sure if the idea is to abandon the torbutton git repository in favour of continuing development in tor-browser repo. According to https://gitweb.torproject.org/tor-browser-spec.git/tree/proposals/102-integrate-tor-launcher-into-tor-browser.txt the plan for tor-launcher is to keep the repo, would this also be fine for torbutton?

comment:30 in reply to:  29 ; Changed 7 months ago by gk

Status: needs_informationnew

Replying to acat:

Are there any issues of using in desktop the same toolkit/torproject/torbutton used already in Android (#25013)?

Not really issues, see below.

Here https://github.com/acatarineu/tor-browser/commit/10760 I took the patch from #28044 (https://gitweb.torproject.org/user/brade/tor-browser.git/commit/?h=bug28044-01&id=79f8ea9c98c69eb594e7d2aace9199b3bceb22cb) and did the same for torbutton, seems to work fine too. Note that tor-launcher patch uses the path browser/extensions/tor-launcher, while current Android and also this one uses toolkit/torproject/torbutton. I realized that in this case the final code goes to /chrome/torbutton inside omni.ja instead of browser/omni.ja as tor-launcher. Is one path preferred to the other?

I think the reason Tor Launcher is in the browser dir is that we don't have a Tor Launcher on Android (and won't have one), thus it is desktop-specific. While that's not the case for Torbutton (even though we don't use the tor-browser version for desktop yet).

There is this GetOverrideStringBundle function in https://gitweb.torproject.org/tor-browser.git/tree/toolkit/xre/nsAppRunner.cpp?h=tor-browser-60.6.1esr-8.5-1&id=178fcbbe24f543a15b165bdc680a5083247e87a3#n1823 that would need to be fixed, since the XPI would not be there. But I wonder if this code is still needed since general.useragent.locale pref is not there anymore (was moved to intl.locale.requested).

Good question and I don't know the answer to it right now...

If this ticket is in the context of ESR68, then of course there is more work to do. But if it's just Integrate TorButton to TorBrowser core to prevent users from disabling it I think this patch might be good enough (together with a change in tor-browser-build, will do if this approach is fine). Perhaps we could track porting to ESR68, refactoring common parts with tor-launcher, etc. in separate tickets. Or make this ticket include all those, and keep working on this one.

Also, not sure if the idea is to abandon the torbutton git repository in favour of continuing development in tor-browser repo. According to https://gitweb.torproject.org/tor-browser-spec.git/tree/proposals/102-integrate-tor-launcher-into-tor-browser.txt the plan for tor-launcher is to keep the repo, would this also be fine for torbutton?

This is a thing we wanted to do for a while now. So, it has not been really ESR68-related. However, I felt we could start integrating the remaining Torbutton pieces directly into the browser while we are at it and while we need to make substantial changes to our UI anyway (given that there are no overlays anymore in esr68). In that sense it *is* an esr69 item and we should make progress as much as we can now so we have less to fix later on.

Ideally we would not even have Torbutton as an extension and separate repo anymore at the end of the process, probably "just" some JS modules somewhere in toolkit and the UI in the respective files per platform. (Hence the proposal idea as this is not just making a git submodule anymore).
We should get rid of the onion button in the toolbar, for instance, and expose the new identity feature as a button instead directly (the update check in the menu can go, I think, and the other item is a tor launcher overlay which we need to think about what to do in the still open tor launcher bug).

I am happy to take a step-wise approach here obviously. The obvious choice would be 1) get it into tor-browser ala #28044, 2) make the things compatible with esr68 3) get rid of it as an own extension, however, I hoped we can avoid doing the full 2) and then 3) by doing part (or the whole!) 3) while doing minimal 2). :)

comment:31 in reply to:  30 Changed 7 months ago by mcs

Replying to gk:

I think the reason Tor Launcher is in the browser dir is that we don't have a Tor Launcher on Android (and won't have one), thus it is desktop-specific. While that's not the case for Torbutton (even though we don't use the tor-browser version for desktop yet).

I don't remember all of the reasons, but one reason we put the Tor Launcher code under browser/extensions is because we were following the example of other built-in "extensions" such as pdfjs.

There is this GetOverrideStringBundle function in https://gitweb.torproject.org/tor-browser.git/tree/toolkit/xre/nsAppRunner.cpp?h=tor-browser-60.6.1esr-8.5-1&id=178fcbbe24f543a15b165bdc680a5083247e87a3#n1823 that would need to be fixed, since the XPI would not be there. But I wonder if this code is still needed since general.useragent.locale pref is not there anymore (was moved to intl.locale.requested).

Good question and I don't know the answer to it right now...

I think it is a bug that that code still uses general.useragent.locale. When we integrate Torbutton or its code into the tor-browser repo, we need to decide what to do for localization. The code in nsAppRunner.cpp can probably be simplified if we can assume that Torbutton's locale files are always present.

comment:32 Changed 7 months ago by gk

Keywords: TorBrowserTeam201905 added; TorBrowserTeam201904 removed

Moving tickets to May

comment:33 Changed 7 months ago by gk

Keywords: GeorgKoppen201905 added; GeorgKoppen201904 removed

Move my tickets.

comment:34 in reply to:  30 Changed 7 months ago by acat

Replying to gk:

I am happy to take a step-wise approach here obviously. The obvious choice would be 1) get it into tor-browser ala #28044, 2) make the things compatible with esr68 3) get rid of it as an own extension, however, I hoped we can avoid doing the full 2) and then 3) by doing part (or the whole!) 3) while doing minimal 2). :)

Yes, I think it makes sense to do this directly for ESR68, removing things that will not be needed anymore. I can take a deeper look at the code and update the proposal draft in https://lists.torproject.org/pipermail/tbb-dev/2018-January/000723.html.

comment:35 Changed 7 months ago by acat

Status: newneeds_revision

I made a draft proposal here: https://storm.torproject.org/shared/epDa1tjvv4tocwoK_PW_YijaZm5gbYeKjcriE28jSvY. I tried not to do too many changes for the migration and left refactoring work for torbutton.js and other chrome/content scripts out of the proposal. If you think this is needed, I guess the proposal could be extended to include those too.

I have tested the approach by partially integrating torbutton into firefox central according to the document. Working:

  • Circuit display and circuit isolation.
  • New identity/new circuit in burger menu (new identity with some suspicious error messages on "restart").
  • NoScript integration for security levels.
  • about:tor

Still some things missing, like About Tor Browser dialog, torbutton-extensions.xul and others. Can finish it if the proposal is ok.

comment:36 in reply to:  35 Changed 7 months ago by mcs

Replying to acat:

I made a draft proposal here: https://storm.torproject.org/shared/epDa1tjvv4tocwoK_PW_YijaZm5gbYeKjcriE28jSvY.

It is exciting to see this moving forward. A few comments on the proposal:

  • In the section 3.2 title, please replace Tor Launcher with Torbutton.
  • (related to comment:29 and following) Although the Torbutton code in its current form may not be around for too much longer, it seems strange to me that the Tor Launcher and Torbutton code won't be located near each other in the source tree or in the final package. I think other people will be surprised too. Should we relocate the Torbutton or Tor Launcher code, or not worry about this? I will leave the final decision to Georg.
  • Can you explain what the issues are r.e. new identity (mentioned near the end of section 3.5.3)? Probably that deserves its own ticket eventually.
  • For localization (section 4) we should think about how to support language packs. With Tor Launcher, we chose to continue to package all of the locales so that installing a language pack would continue to work. If we are only going to ship one locale, then we need to also create a plan for shipping modified language packs (to include the Torbutton strings).
  • Regarding ideas like moving to Fluent or enforcing Mozilla's coding style, we should have separate tickets for those things (especially since doing either at the same time as other work will make reviewing the integration changes much more difficult).

comment:37 Changed 7 months ago by mcs

One more thought: does Tails need a way to disable Torbutton functionality (as they require for Tor Launcher)?

comment:38 Changed 7 months ago by acat

Thanks for the review, I sent a proposal with changes addressing your comments to tbb-dev. Leaving torbutton path as /toolkit/torproject/torbutton, since according to Georg this is more suitable for code that is used in both Android and Desktop, unlike Tor Launcher. I also created tickets for the tasks you mentioned and made it clear that we should do them after this proposals work.

comment:39 in reply to:  37 ; Changed 7 months ago by intrigeri

Replying to mcs:

One more thought: does Tails need a way to disable Torbutton functionality (as they require for Tor Launcher)?

No, we don't disable Torbutton :)

comment:40 in reply to:  39 ; Changed 7 months ago by mcs

Replying to intrigeri:

Replying to mcs:

One more thought: does Tails need a way to disable Torbutton functionality (as they require for Tor Launcher)?

No, we don't disable Torbutton :)

Right. I guess I was thinking of the setup mode (where Tor Launcher is currently used as a standalone XUL app). In that case, if you use Tor Browser, you may not want the Torbutton code doing anything... but maybe it does not matter.

comment:41 in reply to:  40 ; Changed 7 months ago by intrigeri

Replying to mcs:

Replying to intrigeri:

Replying to mcs:

I guess I was thinking of the setup mode (where Tor Launcher is currently used as a standalone XUL app). In that case, if you use Tor Browser, you may not want the Torbutton code doing anything...

Good catch! Indeed, Tor Launcher as we currently run it has no Torbutton extension available. I don't know what would happen if it had. Should I try, or would the results of testing the standalone XUL app be moot anyway?

comment:42 in reply to:  41 ; Changed 7 months ago by mcs

Replying to intrigeri:

Good catch! Indeed, Tor Launcher as we currently run it has no Torbutton extension available. I don't know what would happen if it had. Should I try, or would the results of testing the standalone XUL app be moot anyway?

Probably it would not hurt to try. Do you know if the standalone XUL app feature will still be available in Firefox 68? We talked about this before but I cannot remember our conclusion....

comment:43 in reply to:  42 ; Changed 7 months ago by intrigeri

Replying to mcs:

Replying to intrigeri:

Good catch! Indeed, Tor Launcher as we currently run it has no Torbutton extension available. I don't know what would happen if it had. Should I try, or would the results of testing the standalone XUL app be moot anyway?

Probably it would not hurt to try.

I'm afraid I don't know where I should put the Torbutton extension in the tree resulting from unpacking tor-launcher@…. Any idea?

Do you know if the standalone XUL app feature will still be available in Firefox 68? We talked about this before but I cannot remember our conclusion....

There wasn't really a conclusion back then, but I did some more testing and reported on that thread: https://lists.torproject.org/pipermail/tbb-dev/2019-May/000993.html

comment:44 in reply to:  41 Changed 7 months ago by anonym

Replying to intrigeri:

Replying to mcs:

I guess I was thinking of the setup mode (where Tor Launcher is currently used as a standalone XUL app). In that case, if you use Tor Browser, you may not want the Torbutton code doing anything...

Good catch! Indeed, Tor Launcher as we currently run it has no Torbutton extension available. I don't know what would happen if it had.

First, I don't see why this should be any different than the "vanilla" (i.e. non-Tails) Tor Browser, where Torbutton and Tor Launcher are enabled at the same time.

Second, Tails has a third use case for Tor Browser: the Unsafe Browser. It has direct Internet access (so users can deal with captive portals to get Tor working) and aims to look like a normal Firefox, so I guess we'd still want a way to disable Torbutton for it. But if we cannot for some reason that's actually not so bad: since we any way have all the Tor Browser patches applied it probably distinguishes itself quite a bit from a "normal" Firefox already, so adding Torbutton (but with disabled proxying) might not make things much worse.

comment:45 in reply to:  43 Changed 7 months ago by anonym

Replying to intrigeri:

Replying to mcs:

Replying to intrigeri:

Good catch! Indeed, Tor Launcher as we currently run it has no Torbutton extension available. I don't know what would happen if it had. Should I try, or would the results of testing the standalone XUL app be moot anyway?

Probably it would not hurt to try.

I'm afraid I don't know where I should put the Torbutton extension in the tree resulting from unpacking tor-launcher@…. Any idea?

It should be extracted into the profile's extensions/torbutton@torproject.org folder. I tested this (by making Tails' /usr/local/bin/tor-browser script extracting the xpi into the right place) and after Tor Launcher exited I could see in its prefs.js that the extensions.torbutton.startup pref was set, which I interpret as Torbutton successfully loading. And I could connect to the Tor network fine (both with and without bridges), so there seems to be no problem.

comment:46 Changed 7 months ago by anonym

Cc: anonym added

comment:47 Changed 7 months ago by antonela

Keywords: ux-team added

comment:48 Changed 6 months ago by acat

With respect to strings that are duplicated in torbutton *.properties and *.dtd (see comment https://trac.torproject.org/projects/tor/ticket/30464#comment:1), I first thought that we could maybe start migrating the duplicated properties to Fluent, which should not have the duplication issue since it would all be in *.ftl files. However, it's unclear to me how to easily achieve in Fluent the same "multi-locale" builds that we decided to do for tor-launcher and torbutton integration (all locales are shipped in the build).

I think a more pragmatic approach for now is to remove the duplicated strings from *.properties and have them only in the DTDs. Using DOMParser, we can programatically translate strings in privileged JS in a similar way as we do now for *.properties files (see localizeEntity from https://gitweb.torproject.org/tor-browser.git/tree/testing/marionette/l10n.js?h=tor-browser-60.7.0esr-9.0-1#n46). While this is not ideal, I think it would be a good enough solution until we decide to migrate to Fluent.

comment:49 Changed 6 months ago by cypherpunks

I use TB without Tor to get access to I2P. Sometimes I use TB with plugins disabled when I'm offline.

comment:50 in reply to:  48 Changed 6 months ago by gk

Replying to acat:

With respect to strings that are duplicated in torbutton *.properties and *.dtd (see comment https://trac.torproject.org/projects/tor/ticket/30464#comment:1), I first thought that we could maybe start migrating the duplicated properties to Fluent, which should not have the duplication issue since it would all be in *.ftl files. However, it's unclear to me how to easily achieve in Fluent the same "multi-locale" builds that we decided to do for tor-launcher and torbutton integration (all locales are shipped in the build).

I think a more pragmatic approach for now is to remove the duplicated strings from *.properties and have them only in the DTDs. Using DOMParser, we can programatically translate strings in privileged JS in a similar way as we do now for *.properties files (see localizeEntity from https://gitweb.torproject.org/tor-browser.git/tree/testing/marionette/l10n.js?h=tor-browser-60.7.0esr-9.0-1#n46). While this is not ideal, I think it would be a good enough solution until we decide to migrate to Fluent.

Yes, that's a good idea. Let's go with that one for now.

comment:51 Changed 6 months ago by acat

I replaced the duplicated strings

securityLevel.securityLevel
securityLevel.standard.level
securityLevel.standard.summary
securityLevel.safer.level
securityLevel.safer.summary
securityLevel.safer.description1
securityLevel.safer.description2
securityLevel.safer.description3
securityLevel.safest.level
securityLevel.safest.summary
securityLevel.safest.description1
securityLevel.safest.description2
securityLevel.safest.description3
securityLevel.learnMore

with the corresponding ones from DTD in securityLevel.js (https://github.com/acatarineu/tor-browser/commit/30429+1).

Are these the only duplicated ones? My understanding is that we should remove these from the translations repository and then update the torbutton strings with the script, is this right?

comment:52 Changed 6 months ago by acat

Keywords: TorBrowserTeam201906R added
Status: needs_revisionneeds_review

comment:53 Changed 6 months ago by acat

Status: needs_reviewneeds_information

with the corresponding ones from DTD in securityLevel.js (​https://github.com/acatarineu/tor-browser/commit/30429+1).

Sorry, this was not the right commit implementing this. The duplicated translations removal is https://github.com/acatarineu/tor-browser/commit/6647e87dc1ed59c5b39e8618bf8753d1d8423343, and the rest of the torbutton work in tor-browser is in https://github.com/acatarineu/tor-browser/commit/8f841e5ccd0af5b1f73f5f9c74bb0fd866ba4c33.

The torbutton branch with the changes is https://github.com/acatarineu/torbutton/commits/10760.

Should we do the review of this in #30429 or here? And should we wait for reviewing until the next steps of this torbutton integration are done? (moving current torbutton code to tor-browser, refactoring, etc.)?

comment:54 in reply to:  53 Changed 6 months ago by gk

Status: needs_informationnew

Replying to acat:

[snip]

Should we do the review of this in #30429 or here? And should we wait for reviewing until the next steps of this torbutton integration are done? (moving current torbutton code to tor-browser, refactoring, etc.)?

I think we should do the review here as #30429 is for all the things rebased and the Torbutton patches are not rebased but new. And, no, please move ahead full steam while I try to review things. You could probably start with just removing code not needed anymore before starting tighter integration of the parts we want to keep and exposing functionality in a new way (like the new New Identity button instead of the onion button on the toolbar)

comment:55 Changed 6 months ago by gk

Parent ID: #24855

We have tbb-parity now, unparenting.

comment:56 Changed 6 months ago by gk

Keywords: TorBrowserTeam201906 added; TorBrowserTeam201905 removed

Moving tickets to June

comment:57 Changed 6 months ago by gk

Keywords: GeorgKoppen201906 added; GeorgKoppen201905 removed

Moving my tickets to June

comment:58 in reply to:  53 Changed 6 months ago by gk

Keywords: TorBrowserTeam201906R removed
Status: newneeds_revision

Replying to acat:

with the corresponding ones from DTD in securityLevel.js (​https://github.com/acatarineu/tor-browser/commit/30429+1).

Sorry, this was not the right commit implementing this. The duplicated translations removal is https://github.com/acatarineu/tor-browser/commit/6647e87dc1ed59c5b39e8618bf8753d1d8423343, and the rest of the torbutton work in tor-browser is in https://github.com/acatarineu/tor-browser/commit/8f841e5ccd0af5b1f73f5f9c74bb0fd866ba4c33.

The torbutton branch with the changes is https://github.com/acatarineu/torbutton/commits/10760.

Let's start with the Torbutton changes here:

2a501682217eff74df47f45c60618b71f2a38353 -- please add the dependent changes to other files to this commit as well so it is self-contained as much as possible (there are still mentions of torcookiedialog.xul, popup.xul etc. in other files)

18e7226776301a82bb2c5715739b25f9c2b281ad -- There are several things to fix up/think through, in no particular order:

1) What is the reason for commenting out code in the patch? If we don't need it then we should remove it. Additionally, the torbutton-new-identity part is even disabled only sometimes but it's not obvious why not every time.

2) In torbutton_utils.js there is Services used, but // let { Services }

3) In aboutTor.js (and cookie-jar-selector.js + domain-isolator.js + dragDropFilter.js + external-app-blocker.js + torCheckService.js + torbutton-logger.js + partly tor-control-port.js + utils.js)
we have

const {XPCOMUtils}
const {Services}

while having spaces after and before the curly braces elsewhere in the same changes, please stick to one style.

4) nsIObserverService in cookie-jar-selector.js -> Services.obs

5) + QueryInterface: ChromeUtils.generateQI([Ci.nsISupports, Ci.nsIClassInfo]),

Why suddenly here Ci.nsISupports but a lot of other generateQI() calls don't have it? There is more of this in other components, like domain-isolator.js, dragDropFilter.js, external-app-blocker.js, torCheckService.js, torbutton-logger.js

6) s/mecanism/mechanism/ in noscript-control.js

7) Why do we need to resort to broadCastAsyncMessage and sendAsyncMessage would not be enough?

8) There is an addTab call in menu-overlay.xul that needs to get adjusted

9)

            eventSuppressor = browser.contentWindow.
                QueryInterface(Ci.nsIInterfaceRequestor).
                       getInterface(Ci.nsIDOMWindowUtils);

We should use windowUtils here as well, no? Not sure whether we can just take m_tb_domWindowUtils or actually need the content window version, though.

8) It seems you missed to adapt startup-observer.js and doing all the clean-up there, too?

9) In aboutTor-content.js replace

let brandBundle = Cc["@mozilla.org/intl/stringbundle;1"]
                          .getService(Ci.nsIStringBundleService)

with Services.strings?

10) new Array() in let tabsToRemove = new Array(); in torbutton.js should get changed as well.

11) There are nsICookieManager2 calls in cookie-jar-selector.js which could get replaced by Services.cookies.

Marking this as needs_revision while I look over the other commits.

comment:59 Changed 6 months ago by gk

9cf79bcf222cfb1ca47f2cd6f119ac1f19b2f54d -- looks good (modulo what I said for 2a501682217eff74df47f45c60618b71f2a38353)

02c91349acdb2a493fe0089139ae765b64250a77 -- okay
f7238d222fe4803070f65638cd5bbd4f778a477a -- okay
5426045ea02fb077b1d3c35f5c9f92c2f07facc3 -- okay

comment:60 Changed 6 months ago by acat

Thanks for the review :)

I addressed your comments (and some other small things I found while looking at it) in https://github.com/acatarineu/torbutton/commits/10760+1. I also squashed the previous 9cf79bcf222cfb1ca47f2cd6f119ac1f19b2f54d, ce346d069afa5f1a42dfa13eb8fce2a23cfbf679 and 4dd346e2b81527785d7f8b68a5cff14972dd645c (overlays removal) into the new 0140cfca33be68fedd895672ab7e88f55b023803. The other commits are in the same place, with some changes addressing the comments.

Some of the inconsistencies (but not all) were because initially I had more style fixes (all ESLint ones), but I decided to undo most of them (and keep the important ones) to reduce the number of changes. I think in the process I undid too much (e.g. some of the short-form Services.obs, Services.strings, Services.cookies...).

1) What is the reason for commenting out code in the patch? If we don't need it then we should remove >it. Additionally, the torbutton-new-identity part is even disabled only sometimes but it's not obvious >why not every time.

I'm sorry, I forgot to remove these commented lines. Even though there is one that should not have been commented (torbutton_toggle_plugins). For torbutton-new-identity, I removed all of them, since the UI element is not there anymore.

2) In torbutton_utils.js there is Services used, but let { Services }

Yes, currently the util functions in this file are used as globals from other files. For that, this is loaded via Services.scriptloader in browser.xul and via <script> tag in torbutton/chrome/content/preferences.xhtml. And with that loader there is a problem if you try to redefine the Services variable (since it's already defined in the global scope). I removed the commented Services line (hoping that for mobile, torbutton/chrome/content/preferences.xhtml already has Services in the globals). I also filed #30888 to deal with the torbutton global variables and functions that are "leaked" in global scope of the browser window (this also happens in current browser, see all m_tb_* variables in browser console for example). I think we should try to minimize these, if I'm not wrong I think we only need as globals the functions that need to be called from some menu button (e.g. torbutton_new_circuit, torbutton_new_identity...).

3) In aboutTor.js (and cookie-jar-selector.js + domain-isolator.js + dragDropFilter.js + external-app-blocker.js + torCheckService.js + torbutton-logger.js + partly tor-control-port.js + utils.js)
we have
const {XPCOMUtils}
const {Services}
while having spaces after and before the curly braces elsewhere in the same changes, please stick to >one style.

Ok, I changed everything to the style with spaces.

5) + QueryInterface: ChromeUtils.generateQI([Ci.nsISupports, Ci.nsIClassInfo]),
Why suddenly here Ci.nsISupports but a lot of other generateQI() calls don't have it? There is more of >this in other components, like domain-isolator.js, dragDropFilter.js, external-app-blocker.js, >torCheckService.js, torbutton-logger.js

I removed nsISupports from all GenerateQI, since they seem to be added anyway in MozQueryInterface.cpp ChromeUtils::GenerateQI.

7) Why do we need to resort to broadCastAsyncMessage and sendAsyncMessage would not be enough?

I think I tried Services.cpmm.sendAsyncMessage and for some reason it was not working, although clearly I must have done something wrong as it is working now (as expected). So fixed that too. My understanding is that this will work as long as the webextension runs in content process (as I think it does now in all platforms).

8) There is an addTab call in menu-overlay.xul that needs to get adjusted

Ok, after looking more, I saw addWebTab and addTrustedTab that call addTab with nullprincipal and systemprincipal respectively, and used this accordingly now.

8) It seems you missed to adapt startup-observer.js and doing all the clean-up there, too?

Yes, not really sure why :(

comment:61 Changed 6 months ago by acat

Status: needs_revisionneeds_review

comment:62 Changed 6 months ago by cypherpunks

webextension runs in content process (as I think it does now in all platforms)

It might be run in webextensions process in ff68-esr.

BTW, if you want to get a review from the Tor Browser team, you should set TorBrowserTeam201906R keyword.

comment:63 Changed 6 months ago by acat

Keywords: TorBrowserTeam201906R added; TorBrowserTeam201906 removed

comment:64 in reply to:  59 Changed 6 months ago by gk

Keywords: TorBrowserTeam201906 added; TorBrowserTeam201906R removed
Status: needs_reviewneeds_revision

Replying to gk:

9cf79bcf222cfb1ca47f2cd6f119ac1f19b2f54d -- looks good (modulo what I said for 2a501682217eff74df47f45c60618b71f2a38353)

02c91349acdb2a493fe0089139ae765b64250a77 -- okay
f7238d222fe4803070f65638cd5bbd4f778a477a -- okay
5426045ea02fb077b1d3c35f5c9f92c2f07facc3 -- okay

Here come the remaining bits. The Torbutton patches seem to be okay to me. However, there are some smaller issues with the respective tor-browser patch (commit 8f841e5ccd0af5b1f73f5f9c74bb0fd866ba4c33).

ce346d069afa5f1a42dfa13eb8fce2a23cfbf679

  • aboutDialog.xul -- okay; corresponding part in 8f841e5ccd0af5b1f73f5f9c74bb0fd866ba4c33 -- please put the href entries back on the <label> line so it matches the overall style in aboutDialog.xul; I guess you removed the ifdef because we need a way to include our .dtd file while that xul file is using Fluent? Could we get a comment for that explaining the rationale?
  • menu-items-overlay.xul -- okay; corresponding part in 8f841e5ccd0af5b1f73f5f9c74bb0fd866ba4c33 -- okay; i guess we could squash the patch for bug 26321 (commit 34126c910efab560ff0d6923437c50758f8dc03f) here?
  • menu-overlay.xul -- okay; corresponding part in 8f841e5ccd0af5b1f73f5f9c74bb0fd866ba4c33 -- okay
  • tor-circuit-display.xul -- okay; corresponding part in 8f841e5ccd0af5b1f73f5f9c74bb0fd866ba4c33 -- please put the label attributes on separate lines as done for other <label> elements in identityPanel.inc.xul and the original overlay; the same goes for the <html:button>k
  • torbutton-extension.xul --okay
  • torbutton.xul -- okay; corresponding part in 8f841e5ccd0af5b1f73f5f9c74bb0fd866ba4c33 -- could you fix up the spelling and indentation of the onLoad Hander-part? We don't need the stringbundleset for torbutton.properties anymore? I wonder why that's the case. Additionally, I guess you omitted all the toolbaricon parts as we don't want to expose the onion anymore on the toolbar? If so, then this sounds good to me.

4dd346e2b81527785d7f8b68a5cff14972dd645c -- okay
5b13748cdf2af2db9e829cf76e52287fb2eb2445 -- okay; corresponding part in 8f841e5ccd0af5b1f73f5f9c74bb0fd866ba4c33 -- please add "tor" in components.conf after "telemetry" as the list is in alphabetical order; any reason to not append the about:tor handler to the list in nsAboutRedirector.cpp but putting it between crashparent and crashcontent?

tor-browser 8f841e5ccd0af5b1f73f5f9c74bb0fd866ba4c33: .gitmodules change (and similar ones to get the browser built without having official branches yet) better in an additional commit after all the rebased ones as that is easier to discard and minimizes the risk of forgetting such items.

comment:65 in reply to:  60 Changed 6 months ago by gk

Replying to acat:

Thanks for the review :)

I addressed your comments (and some other small things I found while looking at it) in https://github.com/acatarineu/torbutton/commits/10760+1. I also squashed the previous 9cf79bcf222cfb1ca47f2cd6f119ac1f19b2f54d, ce346d069afa5f1a42dfa13eb8fce2a23cfbf679 and 4dd346e2b81527785d7f8b68a5cff14972dd645c (overlays removal) into the new 0140cfca33be68fedd895672ab7e88f55b023803. The other commits are in the same place, with some changes addressing the comments.

Some of the inconsistencies (but not all) were because initially I had more style fixes (all ESLint ones), but I decided to undo most of them (and keep the important ones) to reduce the number of changes. I think in the process I undid too much (e.g. some of the short-form Services.obs, Services.strings, Services.cookies...).

No worries. The changes in 10760+1 look good to me (with one thing to think about):

d3b70db2205de2a12e6bf6133818b9b005c7f6c9 -- good
ba1d6f3b0bf32a2a5a1b42466169d348b49b58b4 -- using a NullPrincipal for new tabs seems indeed to be a good idea; Would we get away with that for about:tor as well given that we try hard to give it only content privs with the nsIAboutModule flags?
0140cfca33be68fedd895672ab7e88f55b023803 -- okay
81101babce500fb2e2a6e534f5586afea2f5b247 -- okay
8ae82f990355f28f32e56cdf4a191b3fdf8a874b -- okay
cbcb5da551a459e2c4140409be6d03959e18be62 -- okay
bdbd3ac550bf2efe9e5ae97a656f1f868eff4699 -- okay

comment:66 in reply to:  53 Changed 6 months ago by gk

Replying to acat:

with the corresponding ones from DTD in securityLevel.js (​https://github.com/acatarineu/tor-browser/commit/30429+1).

Sorry, this was not the right commit implementing this. The duplicated translations removal is https://github.com/acatarineu/tor-browser/commit/6647e87dc1ed59c5b39e8618bf8753d1d8423343,

Where does it remove the duplicated translations in that commit? The general approach looks good to me, nice find. I think we should get rid of all getString() calls while we are at it and make sure that everything we need is available both for desktop and mobile, hence in the .dtd file (we have #24653 for the localization parity part which could be solved while redoing this part).

We should think as well more general about a mechanism of avoiding duplicated translations as I am not sure whether the hack you found is applicable in more than the sec-settings situation.

comment:67 Changed 5 months ago by acat

I addressed the comments in a new branch: https://github.com/acatarineu/tor-browser/compare/30429+1..30429+2.

I rebased the previous branch, fixing up the torbutton commit and squashing the circuit UI commit. Also realized that I had done the locale deduplication changes as a fixup for the torbutton integration commit, but
I think it should have been for the security UI one. So I moved those changes to the security UI commit (Bug 25658). Also included a couple of changes not mentioned in the review: removed redundant torbutton in toolkit/moz.build and removed Example * from identityPanel.inc.xul.

Is it fine to take 30429+2 as the next 'active' branch for #30429, or will make the ongoing review of the patches more difficult?

We don't need the stringbundleset for torbutton.properties anymore?.

I think we don't, did not see any code in torbutton getting the stringbundle element by id.

Additionally, I guess you omitted all the toolbaricon parts as we don't want to expose the onion anymore on the toolbar? If so, then this sounds good to me.

Yes, that's the case.

Where does it remove the duplicated translations in that commit?

I meant that the duplicated strings from properties are not used anymore in that commit (and therefore can be removed). But did not remove them from the translation files yet.

The general approach looks good to me, nice find. I think we should get rid of all getString() calls while we are at it and make sure that everything we need is available both for desktop and mobile, hence in the .dtd file (we have #24653 for the localization parity part which could be solved while redoing this part).

So, if I understand it correctly, we should remove securityLevel.properties and move the non-duplicated ones to torbutton.dtd. Should we adapt the keys like: securityLevel.safest.tooltip -> torbutton.prefs.sec_safest_tooltip? Should I do a translation repository patch for this and review it here?

any reason to not append the about:tor handler to the list in nsAboutRedirector.cpp but putting it between crashparent and crashcontent?

No, changed that.

We should think as well more general about a mechanism of avoiding duplicated translations as I am not sure whether the hack you found is applicable in more than the sec-settings situation.

Do you have in mind specific cases where it would not work? I think from privileged code we will always be able to create a domparser to get the translation (actually, currently also from non-privileged web content). But in any case, I think Fluent is the way to go for this, a single format that will work for programatic and "document" localization without these hacks.

using a NullPrincipal for new tabs seems indeed to be a good idea; Would we get away with that for about:tor as well given that we try hard to give it only content privs with the nsIAboutModule

I did not find a way to open it with nullprincipal. I also tried other about:*, and only about:home, about:newtab and about:welcome worked for me (maybe it's because they have special handling in AboutRedirector::NewChannel?).

comment:68 Changed 5 months ago by yawning

Since I can't do so, can someone remove me from the cc of this. Thanks.

comment:69 Changed 5 months ago by gk

Cc: yawning removed

comment:70 in reply to:  67 Changed 5 months ago by gk

Replying to acat:

I addressed the comments in a new branch: https://github.com/acatarineu/tor-browser/compare/30429+1..30429+2.

I rebased the previous branch, fixing up the torbutton commit and squashing the circuit UI commit. Also realized that I had done the locale deduplication changes as a fixup for the torbutton integration commit, but
I think it should have been for the security UI one. So I moved those changes to the security UI commit (Bug 25658). Also included a couple of changes not mentioned in the review: removed redundant torbutton in toolkit/moz.build and removed Example * from identityPanel.inc.xul.

Is it fine to take 30429+2 as the next 'active' branch for #30429, or will make the ongoing review of the patches more difficult?

Works for me. I stick to 30429 for now until I am done with that one for a first round.

comment:71 Changed 5 months ago by gk

Keywords: GeorgKoppen201907 added; GeorgKoppen201906 removed

Moving my tickets to July.

comment:72 Changed 5 months ago by gk

Keywords: TorBrowserTeam201907 added; TorBrowserTeam201906 removed

Moving tickets to July

comment:73 Changed 5 months ago by gk

Keywords: tbb-9.0-must-nightly added

Starting with 9.0 keywords

comment:74 in reply to:  67 Changed 4 months ago by gk

Replying to acat:

I addressed the comments in a new branch: https://github.com/acatarineu/tor-browser/compare/30429+1..30429+2.

I rebased the previous branch, fixing up the torbutton commit and squashing the circuit UI commit. Also realized that I had done the locale deduplication changes as a fixup for the torbutton integration commit, but
I think it should have been for the security UI one. So I moved those changes to the security UI commit (Bug 25658). Also included a couple of changes not mentioned in the review: removed redundant torbutton in toolkit/moz.build and removed Example * from identityPanel.inc.xul.

Removing example A etc. makes me a bit nervous. It got introduced in #15086 back then. I am not sure whether that part has been essential on solving the RTL issues but we should double-check that we don't have weird regressions, in particular as all the other code surrounding the <li> elements, like styling is left untouched.

[snip]

Where does it remove the duplicated translations in that commit?

I meant that the duplicated strings from properties are not used anymore in that commit (and therefore can be removed). But did not remove them from the translation files yet.

The general approach looks good to me, nice find. I think we should get rid of all getString() calls while we are at it and make sure that everything we need is available both for desktop and mobile, hence in the .dtd file (we have #24653 for the localization parity part which could be solved while redoing this part).

So, if I understand it correctly, we should remove securityLevel.properties and move the non-duplicated ones to torbutton.dtd. Should we adapt the keys like: securityLevel.safest.tooltip -> torbutton.prefs.sec_safest_tooltip? Should I do a translation repository patch for this and review it here?

Yes, regarding your first and second question. I think there is no need for a translation repo patch. Just do the patch in Torbutton and it will propagate once someone commits the changes to master. The patch could be in #24653 which could be on top of the general #10760 patch for review. We can squash that one in a later rebasing then if we think that's useful. (If you go that route please make #24653 a child bug of this ticket so we don't lose track here)

[snip]

We should think as well more general about a mechanism of avoiding duplicated translations as I am not sure whether the hack you found is applicable in more than the sec-settings situation.

Do you have in mind specific cases where it would not work? I think from privileged code we will always be able to create a domparser to get the translation (actually, currently also from non-privileged web content). But in any case, I think Fluent is the way to go for this, a single format that will work for programatic and "document" localization without these hacks.

Fair enough and, no, I don't have specific cases in mind right now.

using a NullPrincipal for new tabs seems indeed to be a good idea; Would we get away with that for about:tor as well given that we try hard to give it only content privs with the nsIAboutModule

I did not find a way to open it with nullprincipal. I also tried other about:*, and only about:home, about:newtab and about:welcome worked for me (maybe it's because they have special handling in AboutRedirector::NewChannel?).

I see. Let's go with what we have for now then.

We are close here. :)

comment:75 Changed 4 months ago by acat

Removing example A etc. makes me a bit nervous. It got introduced in #15086 back then. I am not sure whether that part has been essential on solving the RTL issues but we should double-check that we don't have weird regressions, in particular as all the other code surrounding the <li> elements, like styling is left untouched.

I tested with current torbutton extension and tor-browser, removing example A, B... does not break the RTL with farsi.

Yes, regarding your first and second question. I think there is no need for a translation repo patch. Just do the patch in Torbutton and it will propagate once someone commits the changes to master. The patch could be in #24653 which could be on top of the general #10760 patch for review. We can squash that one in a later rebasing then if we think that's useful. (If you go that route please make #24653 a child bug of this ticket so we don't lose track here)

Ok, tracking this in #24653.

mcs mentioned there are asserts when building the browser with debugging enabled, and these are caused by torbutton, because some QueryInterface: ChromeUtils.generateQI have nsISupports in it. I removed these in https://www.github.com/acatarineu/torbutton/commit/10760+1 and verified that the asserts disappear. These changes were already present in #28745, but perhaps it's worth also having these here for the nightly.

Apart from this, is anything else needed here, or we are good?

comment:76 in reply to:  75 Changed 4 months ago by gk

Replying to acat:

Removing example A etc. makes me a bit nervous. It got introduced in #15086 back then. I am not sure whether that part has been essential on solving the RTL issues but we should double-check that we don't have weird regressions, in particular as all the other code surrounding the <li> elements, like styling is left untouched.

I tested with current torbutton extension and tor-browser, removing example A, B... does not break the RTL with farsi.

Yes, regarding your first and second question. I think there is no need for a translation repo patch. Just do the patch in Torbutton and it will propagate once someone commits the changes to master. The patch could be in #24653 which could be on top of the general #10760 patch for review. We can squash that one in a later rebasing then if we think that's useful. (If you go that route please make #24653 a child bug of this ticket so we don't lose track here)

Ok, tracking this in #24653.

mcs mentioned there are asserts when building the browser with debugging enabled, and these are caused by torbutton, because some QueryInterface: ChromeUtils.generateQI have nsISupports in it. I removed these in https://www.github.com/acatarineu/torbutton/commit/10760+1 and verified that the asserts disappear. These changes were already present in #28745, but perhaps it's worth also having these here for the nightly.

Apart from this, is anything else needed here, or we are good?

I think we are good here then, thanks!

comment:77 Changed 4 months ago by gk

Keywords: TorBrowserTeam2019008R added; TorBrowserTeam201907 removed
Status: needs_revisionneeds_review

comment:78 Changed 4 months ago by cypherpunks

Who the uck has deleted my comment?
s/TorBrowserTeam2019008R/TorBrowserTeam201908R

comment:79 Changed 4 months ago by pospeselr

Keywords: TorBrowserTeam201908R added; TorBrowserTeam2019008R removed

comment:80 Changed 4 months ago by gk

Keywords: TorBrowserTeam201908 added

Moving tickets to August, part 1

comment:81 Changed 4 months ago by cypherpunks

gk: vacation?

comment:82 Changed 4 months ago by gk

Keywords: GeorgKoppen201908 added; GeorgKoppen201907 removed

Move my tickets.

comment:83 Changed 4 months ago by acat

Ok, I just realized I made a big/stupid mistake here, I'm sorry :(. At some point I looked at what I thought was the latest branch (10760) and saw that it was missing the Remove nsISupports from ChromeUtils.generateQI changes, so pushed a new 10760+1 which overwrote the actual latest branch (which I had missed), so all the changes addressing the review comments got lost.

Anyway, I recovered the original 10760+1 branch, which is now at https://github.com/acatarineu/torbutton/commits/10760+2. I tried merging with torbutton 2.2.1, and there were some conflicts but just due to some removed files.

comment:84 Changed 3 months ago by gk

Status: needs_reviewneeds_revision

This looks mostly good to me. But it has a ton of merge conflicts with master. I'd like to have a new branch for that.

comment:85 Changed 3 months ago by cypherpunks

Remove TorBrowserTeam201908R ;)

comment:86 Changed 3 months ago by acat

The commit in https://github.com/acatarineu/torbutton/commit/10760+3 should have all the changes and apply cleanly to master.

comment:87 Changed 3 months ago by cypherpunks

Now you ;)

comment:88 in reply to:  86 Changed 3 months ago by gk

Keywords: tbb-9.0-must-alpha added; TorBrowserTeam201908R tbb-9.0-must-nightly removed
Status: needs_revisionnew

Replying to acat:

The commit in https://github.com/acatarineu/torbutton/commit/10760+3 should have all the changes and apply cleanly to master.

Looks good now, thanks! Merged to master with commit 11c41cf294050dbe364f287e35f06da8b437214b. I think we are done here with the actual meat. Let's reuse this ticket as a meta ticket and close off the children from now on.

comment:89 Changed 3 months ago by gk

Keywords: TorBrowserTeam201909 added; TorBrowserTeam201908 removed

Moving must-alpha tickets to September.

comment:90 Changed 2 months ago by pili

Keywords: TorBrowserTeam201910 added

comment:91 Changed 2 months ago by pili

Keywords: TorBrowserTeam201909 removed

comment:92 Changed 2 months ago by sysrqb

Keywords: tbb-no-uplift added

comment:93 Changed 8 weeks ago by gk

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.