The "request English language web pages"-prompt is not working in 7.0a3
Using a non-en-US locale does not show the "request English language web pages"-prompt anymore with the switch to 7.0a3. I have not looked closer but I guess the progress listener is broken due to e10s.
Activity
Here's a patch for review: https://github.com/arthuredelstein/torbutton/commit/21999
Trac:
Keywords: TorBrowserTeam201706 deleted, TorBrowserTeam201706R added
Status: new to needs_review
Replying to arthuredelstein:
Here's a patch for review: https://github.com/arthuredelstein/torbutton/commit/21999
The code looks okay but Kathy and I have a couple of questions:
- Why is the delay before showing the prompt only imposed for about:tor?
- The old code only showed the prompt when the user accessed an HTTP page. The new code will show it at startup on about:tor, which seems like a less friendly "first run" experience (mostly because the prompt will cover up a lot of the about:tor content). Are we okay with this change in behavior?
Replying to mcs:
Replying to arthuredelstein:
Here's a patch for review: https://github.com/arthuredelstein/torbutton/commit/21999
The code looks okay but Kathy and I have a couple of questions:
- Why is the delay before showing the prompt only imposed for about:tor?
- The old code only showed the prompt when the user accessed an HTTP page. The new code will show it at startup on about:tor, which seems like a less friendly "first run" experience (mostly because the prompt will cover up a lot of the about:tor content). Are we okay with this change in behavior?
No. I think we should try to get it going the way it was implemented in #18019 (moved). Or at least the patch should result in a similar experience.
Trac:
Status: needs_review to needs_revisionMakes sense to me. Here's a different patch that I think preserves the behavior from before. One problem is that with e10s, the nsIWebProgressListener no longer delays the request. But "http-on-modify-request" observers correctly delays the request until their observe() function returns, so I am using one of those instead.
The only drawback I see with this approach is that sometimes Firefox makes a background http request (such as checking for updates) and I'm not sure whether or not we want to show the prompt when that happens, and what the criteria should be for ignoring such requests.
https://github.com/arthuredelstein/torbutton/commit/21999+2
Trac:
Status: needs_revision to needs_review-
Replying to arthuredelstein:
Makes sense to me. Here's a different patch that I think preserves the behavior from before. One problem is that with e10s, the nsIWebProgressListener no longer delays the request. But "http-on-modify-request" observers correctly delays the request until their observe() function returns, so I am using one of those instead.
Sounds like a good idea. While testing this for a while I found two possible outcomes both of them being suboptimal:
-
The dialog does not get shown at all. This happens when the Torbutton update check is the first request captured by the observer. Subsequent surfing to different websites does not bring the dialog up either. Not sure why that is happening.
-
I get the dialog shown with the
about:torpage (triggered by the updater check). This is pretty confusing as it boils down to a modal dialog during the start-up process, a thing we should avoid if possible.
I think what we could do is to use the
setTimeout()call which we need for the homepage case anyway and apply it generally. There should not be any automatic requests on start-up besides the update checks for extensions and the browser itself and the Torbutton version check. As those happen often on start-up and there is no need to show localized web content in a response basically exempting those requests seems fine to me.The only drawback I see with this approach is that sometimes Firefox makes a background http request (such as checking for updates) and I'm not sure whether or not we want to show the prompt when that happens, and what the criteria should be for ignoring such requests.
The prompt is shown once. And it is not really important for background update requests/checks as the response content displayed in the browser (if it is displayed at all) is not dependent on the
Accept-Languageheader. So it seems fine to me we can ignore this kind of requests.Apart from the suggestion above there are some nits:
// add a web progress listener that will show a "request English language // web pages?" prompt the first time an http or https page is opened.But we have no web progress listener anymore.
+function torbutton_http_connection_observed(aRequest, aData) { + // If we are loading an HTTP page, show the + // "request English language web pages?" prompt.Indentation.
Trac:
Keywords: TorBrowserTeam201706R deleted, TorBrowserTeam201706 added
Status: needs_review to needs_revision -
-
#22659 (moved) is related to that feature.
Replying to gk:
Thanks for the review and testing. Here is a revised branch, with the same patch revised to trigger the prompt only when a request is launched from a content "load context". I tested this new version with update requests and various about: pages and the prompt was not triggered.
I also noticed that some homepage URLs were not being properly detected by function
torbutton_is_homepage_url(aURI). So I added a second patch to this new branch to try to take care of that problem.https://github.com/arthuredelstein/torbutton/commits/21999+3
Trac:
Keywords: TorBrowserTeam201707 deleted, TorBrowserTeam201707R added
Status: needs_revision to needs_review