1257242 - Split the ::BrowserTabsRemoteAutostart() function into two parts, to allow for the blocking policies to be checked independently from the prefs checks

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

In order to get statistically significant data for the e10s cohorts, we need to first check if there are any e10s blocking conditions for a user (e.g., add-ons, accessibility), and then split the remaining users 50%/50% and setting the "browser.tabs.remote.autostart" pref to true to the test group.

However, the function that does the blocking checks is the same that checks prefs, and it doesn't allow the former to happen indenpendently of the latter.

This leads us to need to use workarounds and duplicate the logic. But I want to fix this the proper way while there is still time. The fix is simple, just split the function into two.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1257242 - Split the ::BrowserTabsRemoteAutostart() function into two parts, to allow for the blocking policies to be checked independently from the prefs checks. r=jimm

The behavior of ::BrowserTabsRemoteAutostart() shouldn't change, meaning that every result remains the same. The new getter can be accessed by JS through Services.appinfo.multiprocessBlockPolicy

Review commit: https://reviewboard.mozilla.org/r/40499/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40499/

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: MozReview Request: Bug 1257242 - Improve RTL checks to actually check for RTL instead of using a hardcoded list. r=jimm

This does the much more correct check instead of relying on the list proposed in bug 1234673.

Review commit: https://reviewboard.mozilla.org/r/40501/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/40501/

Comment 3

[:Felipe Gomes (needinfo for replies!)]

The diff looks kinda messy but it's basically code moves

Comment 4

[Jim Mathies [:jimm]]

Comment on attachment 8731330 [details]
MozReview Request: Bug 1257242 - Improve RTL checks to actually check for RTL instead of using a hardcoded list. r=jimm

https://reviewboard.mozilla.org/r/40501/#review37027

Comment 5

[Jim Mathies [:jimm]]

Comment on attachment 8731329 [details]
MozReview Request: Bug 1257242 - Split the ::BrowserTabsRemoteAutostart() function into two parts, to allow for the blocking policies to be checked independently from the prefs checks. r=jimm

https://reviewboard.mozilla.org/r/40499/#review37025

::: toolkit/xre/nsAppRunner.h:126
(Diff revision 1)
>  void SetupErrorHandling(const char* progname);
>  
> +/**
> + * A numeric value indicating whether multiprocess might be blocked.
> + * Possible values can be found at nsAppRunner.cpp. A value of 0
> + * represents not blocking.  

nit - white space

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0278f6e0475c
https://hg.mozilla.org/integration/mozilla-inbound/rev/1de5e65c1fff

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0278f6e0475c
https://hg.mozilla.org/mozilla-central/rev/1de5e65c1fff

Comment 8

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8731329 [details]
MozReview Request: Bug 1257242 - Split the ::BrowserTabsRemoteAutostart() function into two parts, to allow for the blocking policies to be checked independently from the prefs checks. r=jimm

Approval Request Comment
[Feature/regressing bug #]: Code needed to properly tag "disqualified" cohorts in the system add-on, as requested in bug 1251259 comment 13. This is really important to get clear data out of the test/control groups.
[User impact if declined]: needed to test e10s on beta46 as planned
[Describe test coverage new/current, TreeHerder]: landed on central, QA'ed by Softvision
[Risks and why]: the risk is contained in it not working properly and thus not yielding the results for this experiment phase. But that's what we want to test.
[String/UUID change made/needed]: none

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8731330 [details]
MozReview Request: Bug 1257242 - Improve RTL checks to actually check for RTL instead of using a hardcoded list. r=jimm

Approval Request Comment
[Feature/regressing bug #]: a straightforward improvement that actually simplifies things in this code and improves correctness. Since I was touch this code in the other patch I decided to do it here.
[User impact if declined]: Not strictly necessary but very simple and unlikely to be harmful. It would be better to have this uplifted so that all branches are using the same code.
[Describe test coverage new/current, TreeHerder]: landed on central, QA'ed by softvision
[Risks and why]: restricted to the "block e10s for RTL locales" feature
[String/UUID change made/needed]: none

Comment 10

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8731330 [details]
MozReview Request: Bug 1257242 - Improve RTL checks to actually check for RTL instead of using a hardcoded list. r=jimm

This should land in time for beta 4 build today.

Comment 11

[:Felipe Gomes (needinfo for replies!)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/cd896a92fe69
http://hg.mozilla.org/releases/mozilla-aurora/rev/04a77624807a

http://hg.mozilla.org/releases/mozilla-beta/rev/62e7918cebc5
http://hg.mozilla.org/releases/mozilla-beta/rev/10325f06d055

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Felipe, has SV already done the verification? I don't see any QA footprint on the bug and therefore wondering. Thanks!

Comment 13

[:Felipe Gomes (needinfo for replies!)]

Yeah, it was done on a spreadsheet (https://docs.google.com/spreadsheets/d/1odM2iPc3yYQLT-LeNI3NBGV_8xS5GBxFpfAhHC0QH9o/edit#gid=280492042), as it involved a couple of different bugs together (tracked at bug 1257265)

SV sent an e-mail to release-drivers saying that the QA was green and all testcases passed.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to :Felipe Gomes (needinfo me!) from comment #13)
> Yeah, it was done on a spreadsheet
> (https://docs.google.com/spreadsheets/d/1odM2iPc3yYQLT-
> LeNI3NBGV_8xS5GBxFpfAhHC0QH9o/edit#gid=280492042), as it involved a couple
> of different bugs together (tracked at bug 1257265)
> 
> SV sent an e-mail to release-drivers saying that the QA was green and all
> testcases passed.

Awesome! Thank you for sharing that link.