Closed Bug 1225189 Opened 9 years ago Closed 9 years ago

Temporarily disable Hello for e10s windows

Categories

(Hello (Loop) :: Client, defect, P1)

defect

Tracking

(e10s+, firefox45 fixed)

RESOLVED FIXED
mozilla45
Iteration:
45.2 - Nov 30
Tracking Status
e10s + ---
firefox45 --- fixed

People

(Reporter: u549168, Assigned: mancas)

References

Details

(Whiteboard: [web sharing])

Attachments

(4 files, 7 obsolete files)

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)
Blocks: 1214215
Iteration: --- → 45.2 - Nov 30
Priority: -- → P1
Whiteboard: [web sharing]
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
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)
(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?
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)
(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.)
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)
Attached image e10s Error Panel (obsolete) —
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"
Gotcha, you're right. I'll address this right now and re-upload.
Attached image e10s Error Panel (obsolete) —
Screen updated.
Attachment #8689093 - Attachment is obsolete: true
Attached image e10sError Visual Spec
Visual Spec.
Attached file e10sError_Assets.zip (obsolete) —
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?
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.
Attached patch WIP (obsolete) — Splinter Review
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
Attached image e10s Error Panel
Panel updated.
Attachment #8689106 - Attachment is obsolete: true
Attachment #8689612 - Flags: ui-review+
Assignee: b.pmm → b.mcb
Attached file e10sError_Assets.zip
Updated assets.
Attachment #8689108 - Attachment is obsolete: true
Attached patch Disable Hello for e10s windows. (obsolete) — Splinter Review
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+
(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.
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)
Attached patch Disable Hello for e10s windows. (obsolete) — Splinter Review
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)
Attached patch Disable Hello for e10s windows. (obsolete) — Splinter Review
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)
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+
Keywords: checkin-needed
Attachment #8689578 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/433f2e9c7239
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
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
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: