Temporarily disable Hello for e10s windows

RESOLVED FIXED in Firefox 45

Status

Hello (Loop)
Client
P1
normal
Rank:
16
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: u549168, Assigned: mancas)

Tracking

unspecified
mozilla45
Points:
---

Firefox Tracking Flags

(e10s+, firefox45 fixed)

Details

(Whiteboard: [web sharing])

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

3 years ago
Until e10s patches land the new user journey (which launches directly into tab sharing) won't work with e10s.  As a temporary measure, when Hello launches in an e10s window it should display an error message instead of the normal Start Conversation panel.

(Once the e10s patches are landed we would revert this behavior)
(Reporter)

Updated

3 years ago
Blocks: 1214215
Iteration: --- → 45.2 - Nov 30
Priority: -- → P1
Whiteboard: [web sharing]
(Reporter)

Comment 1

3 years ago
We need a design for the panel when Hello is in a disabled state.

This page has an image of how the e10s pref looks (to help people turn off e10s if they want to use Hello): https://wiki.mozilla.org/Electrolysis#Enabling_and_Disabling_Electrolysis
Flags: needinfo?(sfranks)
Flags: needinfo?(b.pmm)
Rank: 16

Comment 2

3 years ago
When may this issue occur? I mean, in which scenarios? We need to know that because we have to provide a message that gives the user some kind of clue of what's happening.
Flags: needinfo?(b.pmm)
Pau, users either have e10s (multi-process) on or off. If it's on, Hello isn't going to work, so we need a panel that communicates this and offers the user some instructions on how to disable it. We'll need some strings first before design work can begin. Matej, do you mind giving your input? Suggestion:

Title: "Firefox Hello does not work with Electrolysis enabled."

Text: "To disable Electrolysis so you can use Firefox Hello, open Preferences and uncheck the "Enable multi-process" checkbox and then restart your browser."

Questions: Is Electrolysis too obscure a term for most users to understand?
Flags: needinfo?(sfranks) → needinfo?(matej)

Comment 4

3 years ago
(In reply to Sevaan Franks [:sevaan] from comment #3)
> Pau, users either have e10s (multi-process) on or off. If it's on, Hello
> isn't going to work, so we need a panel that communicates this and offers
> the user some instructions on how to disable it. We'll need some strings
> first before design work can begin. Matej, do you mind giving your input?
> Suggestion:
> 
> Title: "Firefox Hello does not work with Electrolysis enabled."
> 
> Text: "To disable Electrolysis so you can use Firefox Hello, open
> Preferences and uncheck the "Enable multi-process" checkbox and then restart
> your browser."
> 
> Questions: Is Electrolysis too obscure a term for most users to understand?

That's my question as well, and unfortunately not one I can answer. The copy looks pretty good, but I think we could shorten it a bit:

Text: "To disable Electrolysis and use Firefox Hello, open Preferences, uncheck "Enable multi-process" and then restart Firefox."
Flags: needinfo?(matej)
Assignee: nobody → b.pmm
(In reply to Sevaan Franks [:sevaan] from comment #3)
> Pau, users either have e10s (multi-process) on or off. If it's on, Hello
> isn't going to work, so we need a panel that communicates this and offers
> the user some instructions on how to disable it. We'll need some strings
> first before design work can begin. Matej, do you mind giving your input?
> Suggestion:
> 
> Title: "Firefox Hello does not work with Electrolysis enabled."
> 
> Text: "To disable Electrolysis so you can use Firefox Hello, open
> Preferences and uncheck the "Enable multi-process" checkbox and then restart
> your browser."
Please do not encourage people to disable e10s. This has permanent effects on our testing audience and you seem to intend for this to be a temporary fix.
 
> Questions: Is Electrolysis too obscure a term for most users to understand?
tracking-e10s: --- → +
Thanks Brad, will revise.

When users have e10s on, there is an option to launch a non-e10s window (thanks for pointing that out :dmose). If we launch the window from the Hello panel, we could say:

"Firefox Hello requires a non-e10s window to work" with a button that says "Launch Non-e10s Window"

Matej, running the above by you. I think referring to it as e10s is better than Elecrolysis as that's how it's referred to elsewhere in Firefox.

Ideally the new window would open with the Hello panel also open. Can someone confirm if this possible, and would Hello then work fine in the new window?
Flags: needinfo?(matej)

Comment 7

3 years ago
(In reply to Sevaan Franks [:sevaan] from comment #6)
> Thanks Brad, will revise.
> 
> When users have e10s on, there is an option to launch a non-e10s window
> (thanks for pointing that out :dmose). If we launch the window from the
> Hello panel, we could say:
> 
> "Firefox Hello requires a non-e10s window to work" with a button that says
> "Launch Non-e10s Window"
> 
> Matej, running the above by you. I think referring to it as e10s is better
> than Elecrolysis as that's how it's referred to elsewhere in Firefox.

Yup, that looks good to me. Thanks
Flags: needinfo?(matej)
I confirmed tab sharing works when you open a non-e10s window in Nightly (this option is not available in Aurora, but this error message shouldn't reach Aurora).  Given that this is temporary I'd rather avoid any complications in the implementation, including opening the window automatically or starting Hello automatically.

Can we just say "Temporarily to use Hello you must go to <strong>File > New Non-e10s Window</strong> and re-activate Hello" ?

(The reason we are implementing this error message is so we can land Bug 1214215 ASAP, as that bug is essential to the new product experience.  If we wait until the e10s work is done then we won't have the time to verify the new product experience before uplift.)
(In reply to Ian Bicking (:ianb) from comment #8)
> I confirmed tab sharing works when you open a non-e10s window in Nightly
> (this option is not available in Aurora, but this error message shouldn't
> reach Aurora).  Given that this is temporary I'd rather avoid any
> complications in the implementation, including opening the window
> automatically or starting Hello automatically.
> 
> Can we just say "Temporarily to use Hello you must go to <strong>File > New
> Non-e10s Window</strong> and re-activate Hello" ?
> 
> (The reason we are implementing this error message is so we can land Bug
> 1214215 ASAP, as that bug is essential to the new product experience.  If we
> wait until the e10s work is done then we won't have the time to verify the
> new product experience before uplift.)

Is the button to launch the window much added complication? Just for the sake of convenience. If it's not super-easy, then it's okay to forget it and I am totally fine with that approach, Ian.
Launching the window can't be done from content, so it involves sending messages and touching more parts of the code, so yes, it's not super-easy.  (Also any work we do here has to be reverted once e10s support lands, so we want to keep it simple.)

Comment 11

3 years ago
Just a suggestion for the copy, to me, either e10s and Electrolysis seem really obscure terms so why don't we just say "Multi-process" instead? This is what appears in the preferences panel and I think it's a more friendly term.

Title: "Firefox Hello requires multi-process window to work"

Text: "to use Hello you must go to <strong>File > New Non-e10s Window</strong> and re-activate Hello"

Thoughts?
Flags: needinfo?(sfranks)
Flags: needinfo?(matej)
(In reply to Pau Masiá [:Pau] from comment #11)
> Just a suggestion for the copy, to me, either e10s and Electrolysis seem
> really obscure terms so why don't we just say "Multi-process" instead? This
> is what appears in the preferences panel and I think it's a more friendly
> term.

I would tweak this as follows:

Title: "Firefox Hello requires a multi-process window to work"

Text: "Go to <strong>File > New Non-e10s Window</strong> and re-activate Hello"
Flags: needinfo?(matej)

Comment 13

3 years ago
Created attachment 8689093 [details]
e10s Error Panel

Here's the UX proposal for this error.
Flags: needinfo?(sfranks)
The title gets it backwards; it should be "Firefox Hello does not work in a multi-process window"

Comment 15

3 years ago
Gotcha, you're right. I'll address this right now and re-upload.

Comment 16

3 years ago
Created attachment 8689106 [details]
e10s Error Panel

Screen updated.
Attachment #8689093 - Attachment is obsolete: true

Comment 17

3 years ago
Created attachment 8689107 [details]
e10sError Visual Spec

Visual Spec.

Comment 18

3 years ago
Created attachment 8689108 [details]
e10sError_Assets.zip

Assets.
Wouldn't this be better as a full-panel (like the Get Started) rather than a note at the top of a panel? A user may still try to click on the rooms they have...why even give them that option?

Comment 20

3 years ago
Yes, I thought users might need to be able to edit their rooms but I don't really have a strong position on that issue. I feel a full-panel can work as well.

I'll take care of it tomorrow, though.
Yeah, let's see a full-panel option and then we can decide.
(Assignee)

Comment 22

3 years ago
Created attachment 8689578 [details] [diff] [review]
WIP

This patch allows the user to open a non e10s window by just clicking a button. So UX team can make a design with a button instead of using a large string to explain the user how to open the window, hello needs to work

Comment 23

3 years ago
Created attachment 8689612 [details]
e10s Error Panel

Panel updated.
Attachment #8689106 - Attachment is obsolete: true
Attachment #8689612 - Flags: ui-review+

Updated

3 years ago
Assignee: b.pmm → b.mcb

Comment 24

3 years ago
Created attachment 8689615 [details]
e10sError_Assets.zip

Updated assets.
Attachment #8689108 - Attachment is obsolete: true
(Assignee)

Comment 25

3 years ago
Created attachment 8690718 [details] [diff] [review]
Disable Hello for e10s windows.

Patch ready! Mike, test are failing because we need to stub, in the mochitests, this method: |IsMultiProcessEnabled| of the LoopAPI.

Could you tell me how I can stub it? I'm not sure about how to do that in a mochitest.

Thanks!
Attachment #8690718 - Flags: feedback?(mdeboer)
Given we should be landing e10s support before the next merges, I wouldn't bother with the loop.properties additions, but hard code them.

If it looks like e10s isn't going to make the merge, then we can land the strings.
Comment on attachment 8690718 [details] [diff] [review]
Disable Hello for e10s windows.

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

Good start!

::: browser/components/loop/content/js/panel.jsx
@@ +865,5 @@
>  
>    /**
> +   * E10s not supported view
> +   */
> +  var NotSupportedBrowser = React.createClass({

This names look inappropriate.

@@ +871,5 @@
> +      onClick: React.PropTypes.func.isRequired
> +    },
> +
> +    getDefaultProps: function() {
> +      return {};

Please remove this function.

@@ +920,5 @@
>          fxAEnabled: true,
>          hasEncryptionKey: false,
>          userProfile: null,
> +        gettingStartedSeen: true,
> +        e10sEnabled: false

Can you make this prop name match the LoopAPI call: multiProcessEnabled ?

@@ +1020,5 @@
>        e.preventDefault();
>      },
>  
> +    launchNonE10sWindow: function(e) {
> +      console.info("yuhu");

Forgot to add the loop.request call here...

@@ +1029,5 @@
>  
> +      if (this.state.e10sEnabled) {
> +        return (
> +          <NotSupportedBrowser
> +              onClick={this.launchNonE10sWindow} />

nit: too much indentation.

::: browser/components/loop/modules/MozLoopAPI.jsm
@@ +596,5 @@
>      win.gBrowser.selectedBrowser.messageManager.sendAsyncMessage("PageMetadata:GetPageData");
>    },
>  
>    /**
> +   * Opens a non e10s window

The idea of adding handlers to this file is that they should be inserted in alphabetical order.

@@ +605,5 @@
> +   * @param {Function} reply   Callback function, invoked with the result of this
> +   *                           message handler. The result will be sent back to
> +   *                           the senders' channel.
> +   */
> +  OpenNone10sWindow: function(message, reply) {

'OpenNonE10sWindow' - so 'non' will not be read as 'none'.

@@ +608,5 @@
> +   */
> +  OpenNone10sWindow: function(message, reply) {
> +    let win = Services.wm.getMostRecentWindow("navigator:browser");
> +    let url = message.data[0] ? message.data[0] : "about:home";
> +    win.openDialog("chrome://browser/content/", "_blank", "chrome,all,dialog=no,non-remote", url);

This should be: `OpenBrowserWindow({ remote: false });` (no need to supply a URL - we'll add that functionality in another bug when needed.)

@@ +624,5 @@
> +   */
> +  IsMultiProcessEnabled: function(message, reply) {
> +    let win = Services.wm.getMostRecentWindow("navigator:browser");
> +    let browser = win && win.gBrowser.selectedBrowser;
> +    reply(browser.getAttribute("remote") == "true");

...so this needs to be `!!(browser && browser.getAttribute("remote") == "true")`

::: browser/components/loop/test/desktop-local/panel_test.js
@@ +73,5 @@
>        NotifyUITour: sinon.stub(),
>        OpenURL: sinon.stub(),
>        GetSelectedTabMetadata: sinon.stub().returns({}),
> +      GetUserProfile: function() { return null; },
> +      IsMultiProcessEnabled: sinon.stub()

Hmm, can you sort these alphabetically again? Thanks!
Attachment #8690718 - Flags: feedback?(mdeboer) → feedback+
(Assignee)

Comment 28

3 years ago
(In reply to Mike de Boer [:mikedeboer] from comment #27)
> Comment on attachment 8690718 [details] [diff] [review]
> Disable Hello for e10s windows.
> 
> Review of attachment 8690718 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +608,5 @@
> > +   */
> > +  OpenNone10sWindow: function(message, reply) {
> > +    let win = Services.wm.getMostRecentWindow("navigator:browser");
> > +    let url = message.data[0] ? message.data[0] : "about:home";
> > +    win.openDialog("chrome://browser/content/", "_blank", "chrome,all,dialog=no,non-remote", url);
> 
> This should be: `OpenBrowserWindow({ remote: false });` (no need to supply a
> URL - we'll add that functionality in another bug when needed.)

The idea was to open the non e10s window with the url of the current selected tab so the user could start browsing that url without an effort.
(Assignee)

Updated

3 years ago
Flags: needinfo?(mdeboer)
(In reply to Manuel Casas Barrado [:mancas] from comment #28)
> The idea was to open the non e10s window with the url of the current
> selected tab so the user could start browsing that url without an effort.

Well, in that case the correct method would be `win.openUILinkIn(url, "window");`
Flags: needinfo?(mdeboer)
(Assignee)

Comment 30

3 years ago
Created attachment 8690874 [details] [diff] [review]
Disable Hello for e10s windows.

Using |win.openDialog| instead of |win.openUILinkIn| beacuse we can't open a non e10s window
Attachment #8690718 - Attachment is obsolete: true
Attachment #8690874 - Flags: review?(mdeboer)
(Assignee)

Comment 31

3 years ago
Created attachment 8690911 [details] [diff] [review]
Disable Hello for e10s windows.

Unit test added to check if the view is rendered when e10s is enabled.
Attachment #8690874 - Attachment is obsolete: true
Attachment #8690874 - Flags: review?(mdeboer)
Attachment #8690911 - Flags: review?(mdeboer)
Status: NEW → ASSIGNED
Comment on attachment 8690911 [details] [diff] [review]
Disable Hello for e10s windows.

Waiting for mochitest update.
Attachment #8690911 - Flags: review?(mdeboer)
(Assignee)

Comment 33

3 years ago
Created attachment 8691389 [details] [diff] [review]
Disable Hello for e10s windows.

Hey Mike, all the tests are passing now =)

Btw, thanks for the patch
Attachment #8690911 - Attachment is obsolete: true
Attachment #8691389 - Flags: review?(mdeboer)
Comment on attachment 8691389 [details] [diff] [review]
Disable Hello for e10s windows.

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

Alright, this is looking a-ok to me! Thanks!
Attachment #8691389 - Flags: review?(mdeboer) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Attachment #8689578 - Attachment is obsolete: true

Comment 36

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/433f2e9c7239
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Updating the summary to reflect that fact that this is meant to be temporary.
Summary: Disable Hello for e10s windows → Temporarily disable Hello for e10s windows

Updated

3 years ago
Duplicate of this bug: 1208402
You need to log in before you can comment on or make changes to this bug.