Closed Bug 1072417 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Content Processes, defect)

x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35
Tracking Status
e10s + ---

People

(Reporter: jimm, Assigned: jimm)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 4 obsolete files)

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.
Attached patch wip (obsolete) — Splinter Review
Assignee: nobody → jmathies
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.
Attached patch wip (obsolete) — Splinter Review
Attachment #8494676 - Attachment is obsolete: true
https://tbpl.mozilla.org/?tree=Try&rev=4f9c0981c4fb

still have some customize ui test failures, which are tricky.
Attached patch remote browser.tabs.remote pref (obsolete) — Splinter Review
Attachment #8496569 - Attachment is obsolete: true
Attachment #8496940 - Flags: review?(felipc) → review+
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
(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
er, 'crossed!'
https://hg.mozilla.org/mozilla-central/rev/3baccb75a7b2
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Blocks: 1074914
Blocks: 1075710
Attachment #8496940 - Attachment description: remote browser.tabs.remote pref → remove browser.tabs.remote pref
Blocks: 1200547
You need to log in before you can comment on or make changes to this bug.