Closed
Bug 1225189
Opened 9 years ago
Closed 9 years ago
Temporarily disable Hello for e10s windows
Categories
(Hello (Loop) :: Client, defect, P1)
Hello (Loop)
Client
Tracking
(e10s+, firefox45 fixed)
People
(Reporter: u549168, Assigned: mancas)
References
Details
(Whiteboard: [web sharing])
Attachments
(4 files, 7 obsolete files)
94.72 KB,
image/png
|
Details | |
33.16 KB,
image/png
|
sevaan
:
ui-review+
|
Details |
1.10 KB,
application/zip
|
Details | |
21.42 KB,
patch
|
mikedeboer
:
review+
|
Details | Diff | Splinter Review |
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)
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)
Updated•9 years ago
|
Rank: 16
Comment 2•9 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)
Comment 3•9 years ago
|
||
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•9 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)
Updated•9 years ago
|
Assignee: nobody → b.pmm
Comment 5•9 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." 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?
Updated•9 years ago
|
tracking-e10s:
--- → +
Comment 6•9 years ago
|
||
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•9 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)
Comment 8•9 years ago
|
||
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.)
Comment 9•9 years ago
|
||
(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.
Comment 10•9 years ago
|
||
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•9 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)
Comment 12•9 years ago
|
||
(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 14•9 years ago
|
||
The title gets it backwards; it should be "Firefox Hello does not work in a multi-process window"
Comment 15•9 years ago
|
||
Gotcha, you're right. I'll address this right now and re-upload.
Comment 17•9 years ago
|
||
Visual Spec.
Comment 18•9 years ago
|
||
Assets.
Comment 19•9 years ago
|
||
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•9 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.
Comment 21•9 years ago
|
||
Yeah, let's see a full-panel option and then we can decide.
Assignee | ||
Comment 22•9 years ago
|
||
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
Updated•9 years ago
|
Attachment #8689612 -
Flags: ui-review+
Updated•9 years ago
|
Assignee: b.pmm → b.mcb
Assignee | ||
Comment 25•9 years ago
|
||
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)
Comment 26•9 years ago
|
||
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 27•9 years ago
|
||
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•9 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•9 years ago
|
Flags: needinfo?(mdeboer)
Comment 29•9 years ago
|
||
(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•9 years ago
|
||
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•9 years ago
|
||
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)
Updated•9 years ago
|
Status: NEW → ASSIGNED
Comment 32•9 years ago
|
||
Comment on attachment 8690911 [details] [diff] [review] Disable Hello for e10s windows. Waiting for mochitest update.
Attachment #8690911 -
Flags: review?(mdeboer)
Assignee | ||
Comment 33•9 years ago
|
||
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 34•9 years ago
|
||
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•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
Attachment #8689578 -
Attachment is obsolete: true
Comment 35•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/433f2e9c7239
Keywords: checkin-needed
Comment 36•9 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/433f2e9c7239
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 37•9 years ago
|
||
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.
Description
•