[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]
Re: [tor-bugs] #30429 [Applications/Tor Browser]: Rebase Tor Browser patches for Firefox ESR 68
#30429: Rebase Tor Browser patches for Firefox ESR 68
-------------------------------------------------+-------------------------
Reporter: gk | Owner: tbb-
| team
Type: task | Status:
| needs_revision
Priority: Very High | Milestone:
Component: Applications/Tor Browser | Version:
Severity: Normal | Resolution:
Keywords: tbb-9.0-must-nightly, | Actual Points:
TorBrowserTeam201908 |
Parent ID: | Points:
Reviewer: | Sponsor:
-------------------------------------------------+-------------------------
Comment (by acat):
Thanks again for the reviews. A new branch addressing your comments and
some small issues I found: https://github.com/acatarineu/tor-
browser/commits/30429+6
---
> d61046e445419d7c990571aace0616d1663d4a4d - not okay. It's confusing to
put the Torbutton related part in this commit. Let's have it in the
respective integration commit instead (the one for #10760 for example).
Thanks, this must have slipped in one of the conflicts while rebasing.
Note I also fixed a bug in each commit I noticed:
`loc.installer.uninstallAddon` instead of `loc.uninstallAddon`.
---
>>>What is the reason to patch GetUser$Directory now while we did not have
done so in the esr60 patch? (I guess we should follow Mozilla here and if
not, please add some comment/explanation for that deviation)
>>Not sure if you mean changing the static calls by the non-static ones.
If so, this is needed because the patch needs to make several
GetUser$Directory non-static, and apparently in esr60 the static functions
were called via object (I would expect C++ to at least warn if you call a
static method via an object?).
>Yes, that's what I mean. We should comment the reason for that change at
least, like explaining why they suddenly need to be non-static, in
particular given that Mozilla does not make that change but we have to
now.
>...
>d03d360bd773c8d8a97ef1f69e565afcc19796e5 - not okay (comment is still
missing)
Ok, added a comment in the declaration of the function which causes all
the static->non-static changes. Please let me know if you were thinking of
something else.
---
>9442dda80f9beb90b90934da2bb1a614d39117a4 - not okay. The test is probably
not working anymore as we don't have static pins for #29811; we should
either drop it or fix #29811 and test that the test is still working then
(or working then again)
I dropped it, i can add a comment to #29811 to add it again when it is
fixed.
---
>062794f7cb6819ec1a8a34f3117f8f6b03eaacf8 - not okay (note :ja-JP-mac ->
ja; so mabye we need to adapt tor-browser-build here?)
>We just need changes to the locales we support, e.g. not be but es-AR.
You find the full list e.g. in tor-browser-build's rbm.conf.
Sorry, I looked at the wrong source I guess, should be fixed.
---
>>This is working fine the first time I open the browser, but
unfortunately for some reason after restarting it the search engine icons
disappear (although they still work). I'll try to find out why, but I
think this should not be a blocker for a nightly. It might also be
possible that it's some issue with my local build, so we can see if it
reproduces.
>How did that go? Any further insight here?
I still did not find the cause, but I tested reverting this patch and the
issue still happens to me with the Firefox default search engines, so it's
either an issue with my local build that only happens to me, or caused by
some other patch. But I think we can see in the nightly.
---
>While "reusing" the existing search engines, did you make sure they
comply with what we shipped so far. For example the ddg.xml we ship in our
esr60 branches makes sure the requests go out via POST and there is no
further information transmitted to DDG, while the one we reuse in esr68
seems at least to have differentiation about how things got searched
(context menu, searchbar etc.). I think we don't want to have those.
Ok, I changed the default ddg to use POST as before, removed the params,
and also changed the icon for the current one, so that it can be
distinguised from the .onion one (I guess that's the reason why they are
different). Also removed these params from the yahoo search, even though I
think they were already in esr60 yahoo.xml. Also changed some search
extensions so that they are not localized, took the strings from the
english version and replaced them in the manifests. Then I removed the
_locales folder for these search extensions, since they were not used
anymore. Removing these is probably not strictly needed, but I did just in
case, to make sure that we have the same search extensions for all shipped
locales.
---
>"pageInfo_EncryptionWithBitsAndProtocol" +
"pageInfo_OnionEncryptionWithBitsAndProtocol" formtatting in security.js
(see previous "hdr =" you have removed)
Done, the second hdr = formatting was fixed by eslint when trying to do
the same as the first one, so I assume it's ok.
{{{
if (isHttpScheme && IsPotentiallyTrustworthyOrigin(innerContentLocation))
{
*aDecision = ACCEPT;
return NS_OK;
}}}
---
>You copied that block but it needs to get moved (the block before the
comment about the CSP directive should go I think)
Oops, nice catch. Fixed.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/30429#comment:26>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs