Introduce a new pref for Loop e10s work

VERIFIED FIXED in Firefox 46

Status

defect
P1
normal
Rank:
10
VERIFIED FIXED
4 years ago
3 years ago

People

(Reporter: mikedeboer, Assigned: mikedeboer)

Tracking

unspecified
mozilla47
Points:
13
Dependency tree / graph
Bug Flags:
firefox-backlog +

Firefox Tracking Flags

(e10s-, firefox45 affected, firefox46 verified, firefox47 verified)

Details

(Whiteboard: [e10s][best merged after bug 1239828])

Attachments

(5 attachments, 21 obsolete attachments)

1.21 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
40 bytes, text/x-github-pull-request
standard8
: review+
Details | Review
48.99 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
26.81 KB, patch
mikedeboer
: review+
Details | Diff | Splinter Review
40 bytes, text/x-github-pull-request
standard8
: review+
Details | Review
Since the work on enabling e10s for Loop will be spread out across several bugs, we'll need to put this functionality behind a flag.

This means that the `remote=true` flag on the browser element will be dependent on this pref's value.

Suggested name: `loop.remote.enabled = false` (`false` by default).
Flags: qe-verify+
Flags: firefox-backlog+
tracking-e10s: --- → -
Rank: 31
Priority: -- → P3
Whiteboard: [e10s][Fx42 at latest]
Rank: 31 → 32
Whiteboard: [e10s][Fx42 at latest] → [e10s][Fx43 at latest]
Rank: 32 → 20
Priority: P3 → P2
Assignee: nobody → mdeboer
Whiteboard: [e10s][Fx43 at latest] → [e10s][Fx44]
Rank: 20 → 17
Whiteboard: [e10s][Fx44] → [web sharing][e10s][Fx44]
Points: 1 → 5
Iteration: --- → 45.2 - Nov 30
Hi Shane! I'd really like to hear your opinion on this WIP patch. It makes a chat window document run in the content process (so the browser becomes a remote browser).
What do you think of the messageManager parts here? It'll work for non-e10s chat documents too.
Attachment #8689072 - Flags: feedback?(mixedpuppy)
Comment on attachment 8689072 [details] [diff] [review]
WIP: run Loop Conversation in the content process

Generally looks fine.

Can we skip Social: on standard events (e.g. DOMWindowClose instead of Social:DOMWindowClose)?

I don't see mm being used to handle DOMContentLoaded anywhere else, are you sure that it necessary?

Should stuff like Social:GetAllWebrtcStats be Loop:GetAllWebrtcStats?  If it is not in the shared js, I'd go for Loop:.

If you wanted to, you could replace use of socialFrameShow/Hide with the standard visibility API.
Attachment #8689072 - Flags: feedback?(mixedpuppy) → feedback+
Status: NEW → ASSIGNED
Points: 5 → 8
(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Comment on attachment 8689072 [details] [diff] [review]
> WIP: run Loop Conversation in the content process
> 
> Generally looks fine.
> 
> Can we skip Social: on standard events (e.g. DOMWindowClose instead of
> Social:DOMWindowClose)?
> 
> I don't see mm being used to handle DOMContentLoaded anywhere else, are you
> sure that it necessary?
> 
> Should stuff like Social:GetAllWebrtcStats be Loop:GetAllWebrtcStats?  If it
> is not in the shared js, I'd go for Loop:.
> 
> If you wanted to, you could replace use of socialFrameShow/Hide with the
> standard visibility API.

Thanks for advice! And see you in Orlando soon ;-)
I just had a look at this patch with respect to the system add-on (bug 1186172), as I was worried about some build flags being necessary.

According to https://developer.mozilla.org/en-US/Firefox/Multiprocess_Firefox/Which_URIs_load_where there's various flags which affect the behaviour of where things load. Currently those checks are only done in tabbrowser.xml.

I don't think we need to add the checks here, but I think it might be an idea to update AboutRedirector to specify URI_CAN_LOAD_IN_CHILD or URI_MUST_LOAD_IN_CHILD in case someone is looking through later and gets confused.

It might also be worth a comment or two in that file, to say where the remote browser is currently specified.
Iteration: 45.2 - Nov 30 → ---
Rank: 17 → 22
also I commented out the 'don't use e10s' stuff
Assignee: mdeboer → rjesup
Attachment #8694251 - Attachment is obsolete: true
The patch in bug 1226200 will let this patch be landable, it appears
Assignee: rjesup → mdeboer
Iteration: --- → 46.1 - Dec 28
Attachment #8689072 - Attachment is obsolete: true
Attachment #8694250 - Attachment is obsolete: true
Attachment #8701499 - Flags: review?(standard8)
Attachment #8701503 - Flags: feedback?(standard8)
Comment on attachment 8701499 [details] [diff] [review]
Patch 1: introduce a pref to switch e10s support for Loop/ Hello on or off

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

Looks good. r=Standard8
Attachment #8701499 - Flags: review?(standard8) → review+
Comment on attachment 8701503 [details] [diff] [review]
Patch 2: make chat windows run in the content process

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

I haven't tested it, but the general ideas here look fine.

::: browser/extensions/loop/bootstrap.js
@@ +136,5 @@
> +            let mm = iframe.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader.messageManager;
> +
> +            mm.sendAsyncMessage("Social:WaitForDocumentVisible");
> +            mm.addMessageListener("Social:DocumentVisible", () => {
> +              if (tabId) {

We could drop the tabId stuff whilst we're here - we no longer do it. I've been meaning to do it at some stage and keep forgetting.

::: dom/base/nsFrameLoader.cpp
@@ +929,5 @@
>    nsCOMPtr<nsIBrowserDOMWindow> browserDOMWindow =
>      mRemoteBrowser->GetBrowserDOMWindow();
>  
> +  if (otherBrowserDOMWindow && browserDOMWindow) {
> +    aOther->mRemoteBrowser->SetBrowserDOMWindow(browserDOMWindow);

Just to note, this will need reviewing by a dom peer. I don't mind reviewing the rest though.

::: toolkit/content/widgets/browser.xml
@@ +1240,5 @@
>              // Rewire the remote listeners
>              this._remoteWebNavigation.swapBrowser(this);
>              aOtherBrowser._remoteWebNavigation.swapBrowser(aOtherBrowser);
>  
> +            if (this._remoteWebProgressManager && aOtherBrowser._remoteWebProgressManager) {

Should we be providing some sort of warning if these aren't the same?
Attachment #8701503 - Flags: feedback?(standard8) → feedback+
Rank: 22 → 10
Priority: P2 → P1
Olli, this change is necessary, because <browser> elements inside a Chat window are not attached to a DOMWindow, so there's nothing to swap in that case.
Attachment #8701503 - Attachment is obsolete: true
Attachment #8705115 - Flags: review?(bugs)
Attachment #8705117 - Flags: review?(standard8)
Attachment #8705117 - Flags: review?(mixedpuppy)
Separated this out so it can be applied to the add-on later as well.
Attachment #8705119 - Flags: review?(standard8)
Comment on attachment 8705115 [details] [diff] [review]
Patch 2.1: allow to swap docShells on remote browsers that are not a child of a DOMWindow

>Bug 1154277: Part 2 - allow to swap docShells on remote browsers that are not a child of a DOMWindow. r?smaug
This is not about DOMWindow, but about BrowserDOMWindow, so fix the commit message.


>-  if (!otherBrowserDOMWindow || !browserDOMWindow) {
>-    return NS_ERROR_NOT_IMPLEMENTED;
Could you leave this if-check but change it to
if (!!otherBrowserDOMWindow != !!browserDOMWindow) {
  return NS_ERROR_NOT_IMPLEMENTED;
}

>+  if (otherBrowserDOMWindow && browserDOMWindow) {
>+    aOther->mRemoteBrowser->SetBrowserDOMWindow(browserDOMWindow);
>+    mRemoteBrowser->SetBrowserDOMWindow(otherBrowserDOMWindow);
>   }
And then you wouldn't actually need this change. We'd just replace null BrowserDOMWindow with null two lines below.
Attachment #8705115 - Flags: review?(bugs) → review+
Carrying over r=smaug. Thanks Olli!
Attachment #8705115 - Attachment is obsolete: true
Attachment #8705191 - Flags: review+
Updated panel height setting to not happen in chrome anymore.
Attachment #8705119 - Attachment is obsolete: true
Attachment #8705119 - Flags: review?(standard8)
Attachment #8706338 - Flags: review?(standard8)
Moved test updated over from patch 4 to here.
Attachment #8705117 - Attachment is obsolete: true
Attachment #8705117 - Flags: review?(standard8)
Attachment #8705117 - Flags: review?(mixedpuppy)
Attachment #8706341 - Flags: review?(standard8)
Attachment #8706341 - Flags: review?(mixedpuppy)
Moved test updates over to part 3.
Attachment #8706338 - Attachment is obsolete: true
Attachment #8706338 - Flags: review?(standard8)
Attachment #8706342 - Flags: review?(standard8)
Comment on attachment 8706341 [details] [diff] [review]
Patch 3.1: support running Social API documents to run in a remote browser

- if loop is an addon, shouldn't loop code go into the addon?  I understand the convenience of putting the loop code here, but it does break the purpose of putting loop into an addon.
- the toolkit looks fine to me, though I wonder why this is necessary for social frames
Attachment #8706341 - Flags: review?(mixedpuppy) → feedback+
(In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> - if loop is an addon, shouldn't loop code go into the addon?  I understand
> the convenience of putting the loop code here, but it does break the purpose
> of putting loop into an addon.
> - the toolkit looks fine to me, though I wonder why this is necessary for
> social frames

Thanks for the feedback!

The reason is quite simple and unfortunate: the 'remote' attribute has to be set in the binding (i.e. not dynamically in the constructor or elsewhere), because otherwise bad things happen and it'll not show we throw at it.
Removing the remote attribute dynamically as soon as we logically can, however, does seem to work.

If it weren't for these 'quirks', I would've put it all inside the add-on... but helas!
Attachment #8706342 - Attachment is obsolete: true
Attachment #8706342 - Flags: review?(standard8)
Attachment #8707444 - Flags: review?(standard8)
Rebased patch. Carrying over r=Standard8.
Attachment #8701499 - Attachment is obsolete: true
Attachment #8707445 - Flags: review+
(In reply to Mike de Boer [:mikedeboer] from comment #21)
> (In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> > - if loop is an addon, shouldn't loop code go into the addon?  I understand
> > the convenience of putting the loop code here, but it does break the purpose
> > of putting loop into an addon.
> > - the toolkit looks fine to me, though I wonder why this is necessary for
> > social frames
> 
> Thanks for the feedback!
> 
> The reason is quite simple and unfortunate: the 'remote' attribute has to be
> set in the binding (i.e. not dynamically in the constructor or elsewhere),
> because otherwise bad things happen and it'll not show we throw at it.
> Removing the remote attribute dynamically as soon as we logically can,
> however, does seem to work.
> 
> If it weren't for these 'quirks', I would've put it all inside the add-on...
> but helas!

I wasn't looking at the remote attr, having remote browsers should be generally helpful.  There is loop specific stuff in social-content.js and PanelFrame.jsm.  A content script should work just fine from an addon, and PanelFrame could be reworked to allow additions (e.g. showPopup grows an attrs attr).
(In reply to Shane Caraveo (:mixedpuppy) from comment #24)
> (In reply to Mike de Boer [:mikedeboer] from comment #21)
> > (In reply to Shane Caraveo (:mixedpuppy) from comment #20)
> > > - if loop is an addon, shouldn't loop code go into the addon?  I understand
> > > the convenience of putting the loop code here, but it does break the purpose
> > > of putting loop into an addon.
> > > - the toolkit looks fine to me, though I wonder why this is necessary for
> > > social frames
> > 
> > Thanks for the feedback!
> > 
> > The reason is quite simple and unfortunate: the 'remote' attribute has to be
> > set in the binding (i.e. not dynamically in the constructor or elsewhere),
> > because otherwise bad things happen and it'll not show we throw at it.
> > Removing the remote attribute dynamically as soon as we logically can,
> > however, does seem to work.
> > 
> > If it weren't for these 'quirks', I would've put it all inside the add-on...
> > but helas!
> 
> I wasn't looking at the remote attr, having remote browsers should be
> generally helpful.  There is loop specific stuff in social-content.js and
> PanelFrame.jsm.  A content script should work just fine from an addon, and
> PanelFrame could be reworked to allow additions (e.g. showPopup grows an
> attrs attr).

Ah ok. Sure, can do. Or I can do that in a follow-up that's less time-critical.
Test fixups.
Attachment #8706341 - Attachment is obsolete: true
Attachment #8706341 - Flags: review?(standard8)
Attachment #8707503 - Flags: review?(standard8)
Test fixes.
Attachment #8707444 - Attachment is obsolete: true
Attachment #8707444 - Flags: review?(standard8)
Attachment #8707508 - Flags: review?(standard8)
> I can do that in a follow-up that's less
> time-critical.

+1
With the latest patches, I'm seeing this when I pop-out the window. It appears to create the window and not finish destroying the old.

NS_ERROR_NOT_IMPLEMENTED at browser.xml line 1209:

         this.QueryInterface(Components.interfaces.nsIFrameLoaderOwner)
              .swapFrameLoaders(aOtherBrowser);
Points: 8 → 13
Comment on attachment 8707503 [details] [diff] [review]
Patch 3.2: support running Social API documents to run in a remote browser

Clearing to wait for follow-ups.
Attachment #8707503 - Flags: review?(standard8)
Depends on: 1241532
Comment on attachment 8712136 [details] [review]
[loop] mikedeboer:e10s > mozilla:master

This PR doesn't work without the updated browser patches. But certainly ready.
Attachment #8712136 - Flags: review?(standard8)
Attachment #8707508 - Attachment is obsolete: true
Attachment #8707512 - Attachment is obsolete: true
Attachment #8712710 - Flags: review?(standard8)
Attachment #8707503 - Attachment is obsolete: true
Attachment #8712711 - Flags: review?(standard8)
Attachment #8712711 - Flags: review?(mixedpuppy)
Comment on attachment 8712711 [details] [diff] [review]
Patch 4: implement test changes to work in e10s mode as well

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

::: browser/base/content/test/chat/browser_focus.js
@@ +11,5 @@
>  function isTabFocused() {
>    let tabb = gBrowser.getBrowserForTab(gBrowser.selectedTab);
>    // focus sucks in tests - our window may have lost focus.
>    let elt = Services.focus.getFocusedElementForWindow(window, false, {});
> +  dump("WHATS THE EL??\nA:" + elt.parentNode.outerHTML + "\nB:" + tabb.parentNode.outerHTML + "\n");

I'll be removing this...
Comment on attachment 8712711 [details] [diff] [review]
Patch 4: implement test changes to work in e10s mode as well

Needs a bug # for the crash comment.  Remove dumps.
Attachment #8712711 - Flags: review?(mixedpuppy) → review+
Whiteboard: [web sharing][e10s][Fx44] → [e10s]
Attachment #8712710 - Attachment is obsolete: true
Attachment #8712710 - Flags: review?(standard8)
Attachment #8713140 - Flags: review?(standard8)
Carrying over r=mixedpuppy - thanks Shane!!
Attachment #8712711 - Attachment is obsolete: true
Attachment #8712711 - Flags: review?(standard8)
Attachment #8713141 - Flags: review?(standard8)
Note-to-try-watchers: browser_mozLoop_sharingListeners.js is failing due to the about:newtab shenanigans. You may ignore this, since the upcoming Loop code uplift should work with this. At least, it's unrelated to the work going on in this bug.
^-- says 'all green' once I disable 'browser/base/content/test/social/browser_social_status.js' (temporarily). That test runs fine for me locally, so needs a bit of investigation beyond this bug.
Comment on attachment 8713140 [details] [diff] [review]
Patch 3.4: support running Social API documents to run in a remote browser

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

r=me once you've addressed the comments, unless that remote attribute issue causes bigger rework.

::: browser/base/content/socialchat.xml
@@ +584,5 @@
>          ]]></body>
>        </method>
>  
>        <method name="openChat">
> +        <parameter name="aOptions"/>

I think you really want to document the possible options here - a couple of lines below, you extract most things, but not the remote. So unless people examine the code, they aren't going to pick up on all the possible options.

@@ +737,5 @@
>              let otherChatbox = otherWin.document.getElementById("chatter");
>              aChatbox.setDecorationAttributes(otherChatbox);
>              aChatbox.swapDocShells(otherChatbox);
> +            if (!aChatbox.content.hasAttribute("remote")) {
> +              otherChatbox.content.removeAttribute("remote");

This feels like the wrong order. If the aChatbox isn't remote, and otherChatbox is, then I can see swapDocShells having issues.

I don't think it'll affect the crash you're seeing at the moment, but I suspect it might cause extra issues for detaching in non-e10s mode, as the remote attribute is default.
Attachment #8713140 - Flags: review?(standard8) → review+
Attachment #8713141 - Flags: review?(standard8) → review+
Comment on attachment 8712136 [details] [review]
[loop] mikedeboer:e10s > mozilla:master

I think this is almost there, I'd just like to have a look at the changes once you've addressed the comments.
Attachment #8712136 - Flags: review?(standard8)
(In reply to Mark Banner (:standard8) from comment #53)
> @@ +737,5 @@
> >              let otherChatbox = otherWin.document.getElementById("chatter");
> >              aChatbox.setDecorationAttributes(otherChatbox);
> >              aChatbox.swapDocShells(otherChatbox);
> > +            if (!aChatbox.content.hasAttribute("remote")) {
> > +              otherChatbox.content.removeAttribute("remote");
> 
> This feels like the wrong order. If the aChatbox isn't remote, and
> otherChatbox is, then I can see swapDocShells having issues.
> 
> I don't think it'll affect the crash you're seeing at the moment, but I
> suspect it might cause extra issues for detaching in non-e10s mode, as the
> remote attribute is default.

Well, I agree that is seems the wrong order, but all this has to do with the fact that we can't set the 'remote' attribute at a later time. Hence I had to put it there in the binding and remove it explicitly as soon as we know that we're not in remote mode.
What I can look into, however, is to defer setting the 'src' until after we're sure that remote is necessary.
Carrying over r=Standard8, mixedpuppy.

So the difference I made here is to have two <xul:browser> elements in the chatbox binding - one remote and one not remote. To make sure that the document isn't loaded twice, I explicitly set the src to 'about:blank' in the other browser instance that won't be used.
This would potentially allow runtime switching between remote and non-remote, but that won't happen in practice.

I also made sure that detaching a chat window works in non-e10s browser windows. Doing this in e10s browsers will crash the content process, currently.

There's another issue with messageManager instance management wrt `swapDocShells` - the messageManager gets detached somehow and RemotePageManager.jsm messages are lost in the void.

At this point there's so much brokenness with e10s on that I'm advising to drop support for detaching a window altogether for now, until things stabilize.
Attachment #8713140 - Attachment is obsolete: true
Attachment #8714806 - Flags: review+
Carrying over r=Standard8.
Attachment #8713141 - Attachment is obsolete: true
Attachment #8714807 - Flags: review+
The try push in comment 61 deems this ready to land. Mark, are we ready to do an add-on push at the same time as pushing this to the tree?
Flags: needinfo?(standard8)
As discussed on irc, I've got an L10n patch in flight (bug 1239828) which will hopefully get r+ overnight. So we'll try and land this tomorrow.
Flags: needinfo?(standard8)
Comment on attachment 8712136 [details] [review]
[loop] mikedeboer:e10s > mozilla:master

r=Standard8 once you've give the XXX in the browser.ini a bug number.
Attachment #8712136 - Flags: review+
Blocks: 1245798
Blocks: 1245799
Blocks: 1245800
Blocks: 1245803
Blocks: 1245805
Blocks: 1245808
Blocks: 1245813
https://hg.mozilla.org/integration/fx-team/rev/30006ad754bf44b906de8fb9ed2b6236caf43406
Bug 1154277: Part 1 - allow to swap docShells on remote browsers that are not a child of a BrowserDOMWindow. r=smaug

https://hg.mozilla.org/integration/fx-team/rev/a4ede73cb8b3f7d87fbd83976406e174749a76a8
Bug 1154277: Part 2 - support running Social API documents to run in a remote browser, i.e. the content process. f=mixedpuppy, r=Standard8

https://hg.mozilla.org/integration/fx-team/rev/da59790ccdad8a053591348b84adeb2d74c25400
Bug 1154277: Part 3 - implement test changes to work in e10s mode as well. r=mixedpuppy,Standard8

https://hg.mozilla.org/integration/fx-team/rev/6371c0aebc73a3b8f049625cc93571120969fc30
Bug 1154277: Part 4 - Update the loop add-on and introduce a pref to switch e10s support for Loop/ Hello on or off. The default for now is off. r=Standard8
https://hg.mozilla.org/integration/fx-team/rev/d2522fbd0f6328c7c2b54975636d951924ec7372
Bug 1154277: follow-up - fix linting error by removing a leftover break statement. rs=me
Attachment #8715872 - Flags: review?(standard8)
Attachment #8715872 - Flags: review?(standard8) → review+
Depends on: 1246511
No longer depends on: 1246511
Comment on attachment 8705191 [details] [diff] [review]
Patch 2.2: allow to swap docShells on remote browsers that are not a child of a BrowserDOMWindow

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: When e10s is enabled by default, Loop will not start. This patch is one of three necessary to get things running.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, tests pass. Chat tests needed to be disabled, but not signal of functionality loss.
[Risks and why]: This patch has minor risk. Other patches would be higher.
[String/UUID change made/needed]: n/a.
Attachment #8705191 - Flags: approval-mozilla-beta?
Attachment #8705191 - Flags: approval-mozilla-aurora?
Comment on attachment 8714806 [details] [diff] [review]
Patch 3.5: support running Social API documents to run in a remote browser

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: When e10s is enabled by default, Loop will not start. This patch is one of three necessary to get things running.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, tests pass. Chat tests needed to be disabled, but not signal of functionality loss.
[Risks and why]: Moderate risk, due to disabled tests. Possible bitrot would pose minimal risk; relatively easy to fix.
[String/UUID change made/needed]: n/a.
Attachment #8714806 - Flags: approval-mozilla-beta?
Attachment #8714806 - Flags: approval-mozilla-aurora?
Comment on attachment 8714807 [details] [diff] [review]
Patch 4.2: implement test changes to work in e10s mode as well

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: When e10s is enabled by default, Loop will not start. This patch is one of three necessary to get things running.
[Describe test coverage new/current, TreeHerder]: Landed on m-c, tests pass. Chat tests needed to be disabled, but not signal of functionality loss.
[Risks and why]: Moderate risk, due to disabled test. Possible bitrot would pose minimal risk; relatively easy to fix.
[String/UUID change made/needed]: n/a.
Attachment #8714807 - Flags: approval-mozilla-beta?
Attachment #8714807 - Flags: approval-mozilla-aurora?
Whiteboard: [e10s] → [e10s][best merged after bug 1239828]
Comment on attachment 8714806 [details] [diff] [review]
Patch 3.5: support running Social API documents to run in a remote browser

Workaround to get Hello as a system addon working when e10s is enabled. 
OK to uplift to aurora.
Attachment #8714806 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8714807 [details] [diff] [review]
Patch 4.2: implement test changes to work in e10s mode as well

Skips chat tests for Hello as system addon with e10s enabled
Attachment #8714807 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8705191 [details] [diff] [review]
Patch 2.2: allow to swap docShells on remote browsers that are not a child of a BrowserDOMWindow

No idea how to assess the risk here. Let's uplift it to aurora and see how it goes.
Attachment #8705191 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8705191 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8714806 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8714807 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
mike, this failed to apply to beta with in part 4:

warning: conflicts while merging browser/extensions/loop/chrome/content/modules/MozLoopService.jsm! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/extensions/loop/chrome/content/preferences/prefs.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use hg resolve and hg graft --continue)
Flags: needinfo?(mdeboer)
Depends on: 1246078
Flags: needinfo?(mdeboer)
E created a new profile, created prefs.js file with 'browser.tabs.remote.autostart';'true', started Firefox 45 beta 6 and I clicked on Hello icon (basically I forced to have e10s enabled by default in fresh new profile). I could create rooms with ease. Note that 'loop.remote.enabled' is not available in beta though.

Also on Nightly if I shift 'browser.tabs.remote.autostart' to 'false' and back to 'true', I will be able to create rooms as well.

Am I testing the right thing here? Any ideas?
Flags: needinfo?(mdeboer)
Blocks: 1250534
(In reply to Bogdan Maris, QA [:bogdan_maris] from comment #84)
> E created a new profile, created prefs.js file with
> 'browser.tabs.remote.autostart';'true', started Firefox 45 beta 6 and I
> clicked on Hello icon (basically I forced to have e10s enabled by default in
> fresh new profile). I could create rooms with ease. Note that
> 'loop.remote.enabled' is not available in beta though.

I don't think this would have worked in beta 6 - I think we backed out everything before that.

> Also on Nightly if I shift 'browser.tabs.remote.autostart' to 'false' and
> back to 'true', I will be able to create rooms as well.

I think its safe to say some of this is still in progress - see bug 1154273 for outstanding issues.

The main thing to test here for now would be that we still work fine without e10s enabled. Once we get towards enabling e10s, we can do a full shakedown of the e10s code.
Flags: needinfo?(mdeboer)
(In reply to Mark Banner (:standard8) from comment #86)
> (In reply to Bogdan Maris, QA [:bogdan_maris] from comment #84)
> > E created a new profile, created prefs.js file with
> > 'browser.tabs.remote.autostart';'true', started Firefox 45 beta 6 and I
> > clicked on Hello icon (basically I forced to have e10s enabled by default in
> > fresh new profile). I could create rooms with ease. Note that
> > 'loop.remote.enabled' is not available in beta though.
> 
> I don't think this would have worked in beta 6 - I think we backed out
> everything before that.
> 
> > Also on Nightly if I shift 'browser.tabs.remote.autostart' to 'false' and
> > back to 'true', I will be able to create rooms as well.
> 
> I think its safe to say some of this is still in progress - see bug 1154273
> for outstanding issues.
> 
> The main thing to test here for now would be that we still work fine without
> e10s enabled. Once we get towards enabling e10s, we can do a full shakedown
> of the e10s code.

Based on this, I think we can safely say that we did test hello without e10s enabled on multiple occasions and we can say that it was verified on 46.
Depends on: 1270943
We recently done a smoketest on Firefox 47 beta 4 across platforms and can safely confirm that Hello works as expected without e10s enabled. I'll go ahead and mark the status of the bug to Verified Fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.