1072417 - Remove the browser.tabs.remote pref and turn everything it controls on by default

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Jim Mathies [:jimm]]

This is causing testing problems since our main options pref for e10s doesn't control this pref. When the pref is set to false weird things break, but e10s looks like it's running properly in the browser.

Comment 1

[Jim Mathies [:jimm]]

http://mxr.mozilla.org/mozilla-central/search?string=browser.tabs.remote
http://mxr.mozilla.org/mozilla-central/search?string=BrowserTabsRemote%28%29
http://mxr.mozilla.org/mozilla-central/search?string=browserTabsRemote
http://mxr.mozilla.org/mozilla-central/search?string=GetBrowserTabsRemote

Comment 2

[Jim Mathies [:jimm]]

Attachment: wip

Comment 3

[Jim Mathies [:jimm]]

Attachment: wip

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=5ce4380c54e3

Comment 4

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8494676 [details] [diff] [review]
wip

Review of attachment 8494676 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/browser.js
@@ -6887,5 @@
> -    if (!remoteTabs) {
> -      newRemoteWindow.hidden = true;
> -      newNonRemoteWindow.hidden = true;
> -      return;
> -    }

This will expose the "New E10s window" menu entries to all branches, whereas they have only been exposed to Nightly users before. Same for CustomizableWidgets

::: dom/ipc/ContentParent.cpp
@@ +3803,5 @@
>  ContentParent::ShouldContinueFromReplyTimeout()
>  {
>      // The only time ContentParent sends blocking messages is for CPOWs, so
>      // timeouts should only ever occur in electrolysis-enabled sessions.
> +    MOZ_ASSERT(BrowserTabsRemoteAutostart());

won't this change break e10s windows that were created through "new e10s window"? (i.e. without autostart). I think the assert should  be removed as it's meant to always be true now with the removal of BrowserTabsRemote

::: layout/forms/nsListControlFrame.cpp
@@ -1802,5 @@
>      mChangesSinceDragStart = change;
>    } else {
>      // NOTE: the combo box is responsible for dropping it down
>      if (mComboboxFrame) {
> -      if (XRE_GetProcessType() == GeckoProcessType_Content && BrowserTabsRemote()) {

This is a trick.. The reason the check is here is to make this behavior exclusive to Firefox, and not to Android/b2g which also share this code but have different implementations for dropdowns which should take the other path.

I don't know yet the best way to handle this.. is there an ifdef for Firefox only in gecko code? but that's usually frowned upon

::: toolkit/components/satchel/nsFormAutoComplete.js
@@ -494,5 @@
>  };
>  
>  
> -let remote = Services.appinfo.browserTabsRemote;
> -if (Services.appinfo.processType == Services.appinfo.PROCESS_TYPE_CONTENT && remote) {

Same reason here as to the one in nsListControlFrame.

Comment 5

[Jim Mathies [:jimm]]

Attachment: wip

Comment 6

[Jim Mathies [:jimm]]

https://tbpl.mozilla.org/?tree=Try&rev=4f9c0981c4fb

still have some customize ui test failures, which are tricky.

Comment 7

[Jim Mathies [:jimm]]

Attachment: remote browser.tabs.remote pref

Comment 8

[Jim Mathies [:jimm]]

Attachment: remove browser.tabs.remote pref

https://tbpl.mozilla.org/?tree=Try&showall=0&rev=77a87b74a81a

Comment 9

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8496940 [details] [diff] [review]
remove browser.tabs.remote pref

Review of attachment 8496940 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/nsWindow.cpp
@@ +809,5 @@
>              gAndroidScreenBounds.width = newScreenWidth;
>              gAndroidScreenBounds.height = newScreenHeight;
>  
> +            if (XRE_GetProcessType() != GeckoProcessType_Default &&
> +                Preferences::GetBool("browser.tabs.remote.desktopbehavior", false)) {

Missing the ! from the logic

Comment 10

[Jim Mathies [:jimm]]

(In reply to :Felipe Gomes from comment #9)
> Comment on attachment 8496940 [details] [diff] [review]
> remote browser.tabs.remote pref
> 
> Review of attachment 8496940 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/android/nsWindow.cpp
> @@ +809,5 @@
> >              gAndroidScreenBounds.width = newScreenWidth;
> >              gAndroidScreenBounds.height = newScreenHeight;
> >  
> > +            if (XRE_GetProcessType() != GeckoProcessType_Default &&
> > +                Preferences::GetBool("browser.tabs.remote.desktopbehavior", false)) {
> 
> Missing the ! from the logic

Thanks. 

Repushed with that update - 

https://tbpl.mozilla.org/?tree=Try&rev=d80e87da23ab

Comment 11

[Jim Mathies [:jimm]]

fingers cross!

https://hg.mozilla.org/integration/mozilla-inbound/rev/3baccb75a7b2

Comment 12

[Jim Mathies [:jimm]]

er, 'crossed!'

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3baccb75a7b2