Closed Bug 1266415 Opened 7 years ago Closed 7 years ago

about:debugging should display a warning if service workers are disabled

Categories

(DevTools :: about:debugging, defect, P2)

48 Branch
defect

Tracking

(firefox48 affected, firefox49 fixed)

RESOLVED FIXED
Firefox 49
Tracking Status
firefox48 --- affected
firefox49 --- fixed

People

(Reporter: jdescottes, Assigned: gasolin, Mentored)

Details

(Keywords: dev-doc-complete, Whiteboard: [good taipei bug])

Attachments

(2 files)

Service workers can be disabled by several means:
- set the preference 'dom.serviceWorkers.enabled' to false
- navigate in private browsing mode
- set the history preference to "Never remember history" (see bug 1266409)

Users landing on about:debugging to get information about service workers should have enough information to know why service workers might not be working on their browser.

Full private browsing mode can be detected checking the preference "browser.privatebrowsing.autostart"
(In reply to Julian Descottes [:jdescottes] from comment #0)
> - navigate in private browsing mode
> - set the history preference to "Never remember history" (see bug 1266409)

These two items are somewhat related.
"Never remember history" enables the private browsing to all windows.
It can be detected as browser.privatebrowsing.autostart will be true.

The regular private browsing mode, where you just open one tab is going to be harder to explain.
about:debugging may be in a non-private browsing window, while another window is and loads a website that the developer want to debug. 
But it sounds like a edge case. Checking that about:debugging is living in a private window may be enough.

Note that we will most likely face the same issue on Fennec. It would be great to also support it nicely via remote protocol calls instead of only checking (prefs directly|window private browsing mode).
(In reply to Julian Descottes [:jdescottes] from comment #0)
> Users landing on about:debugging to get information about service workers
> should have enough information to know why service workers might not be
> working on their browser.

You're right, very good idea! Would you be interested in addressing or mentoring this bug?
Flags: needinfo?(jdescottes)
Priority: -- → P2
(In reply to Jan Keromnes [:janx] from comment #2)
> (In reply to Julian Descottes [:jdescottes] from comment #0)
> > Users landing on about:debugging to get information about service workers
> > should have enough information to know why service workers might not be
> > working on their browser.
> 
> You're right, very good idea! Would you be interested in addressing or
> mentoring this bug?

Sure. I'd happily take the bug, but I still have Bug 1153292 to finish and this should be my priority. Glad to mentor here though.

First of all, let's list the "outage" reasons we want to detect :
- the about:debugging window is in private browsing mode
- the browser is in full private browsing mode or is set to "Never remember history" : browser.privatebrowsing.autostart is true
- service workers are disabled by configuration: dom.serviceWorkers.enabled is false

For the actual UI implementation, I think we could just display a warning message (instead of the "Nothing yet" message displayed when there are no service workers available). It would be nice to have a different message depending on the outage reason.

If service workers cannot be used, I also think we should not list any previously registered service worker, as it might be confusing => only the warning message should be displayed.

Alex: I'm not sure how to address this issue with remote protocol calls. I guess for preferences we can use the preferences actor, and I suppose the TabActor can be extended to check if a given tab is in private browsing mode? Could you add some hints on how to deal with this?
Mentor: jdescottes
Flags: needinfo?(jdescottes) → needinfo?(poirot.alex)
Attached image sw-warning-1266415.png
Helen, what do you think about displaying a warning message here? (see attachment)
Flags: needinfo?(hholmes)
(In reply to Julian Descottes [:jdescottes] from comment #4)
> Created attachment 8745365 [details]
> sw-warning-1266415.png
> 
> Helen, what do you think about displaying a warning message here? (see
> attachment)

Makes sense to me!
Flags: needinfo?(hholmes)
(In reply to Julian Descottes [:jdescottes] from comment #3)
> Alex: I'm not sure how to address this issue with remote protocol calls. I
> guess for preferences we can use the preferences actor, and I suppose the
> TabActor can be extended to check if a given tab is in private browsing
> mode? Could you add some hints on how to deal with this?

Yes, I was thinking about the preferences actor. Here is an usage of it:
http://mxr.mozilla.org/mozilla-central/source/devtools/client/framework/toolbox.js#1627

I don't think we need to change anything on the TabActor?
Flags: needinfo?(poirot.alex)
Whiteboard: [good taipei bug]
I''d like to fix it.

What's the suggest link for (more info) UI? MDN?

https://developer.mozilla.org/en-US/docs/Web/API/Service_Worker_API#Service_worker_concepts_and_usage
Assignee: nobody → gasolin
Flags: needinfo?(jdescottes)
Thanks for picking this up Fred! 

Let's use the link you provided for now.
I think we will add a dedicated section in https://developer.mozilla.org/en-US/docs/Tools/about%3Adebugging , but that can easily be changed before landing the patch.

As I said in the first comment, we have two preferences we can check:
- browser.privatebrowsing.autostart
- dom.serviceWorkers.enabled
We should try to retrieve them using a PreferenceFront (you can have a look at the example from comment 6). Let me know if you need help with this.

I am not sure how to check a private browsing window. It has no impact on the preferences, and is normally checked using PrivateBrowsingUtils.jsm. If the goal is to make this remote friendly we need to be able to query the debuggee about this.
Alex: That's the use case for which I thought we should modify the TabActor? Am I missing something?
Flags: needinfo?(jdescottes) → needinfo?(poirot.alex)
Comment on attachment 8754637 [details]
MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes

WIP patch that render a rich text warning message when any of 3 private browsing conditions is met. Since its my very first patch for devtools, ask feedback to better fit with devtools' coding style.

Haven't deal with remote debugging yet, will need some research to figure it out.
Attachment #8754637 - Flags: feedback?(jdescottes)
Comment on attachment 8754637 [details]
MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54124/diff/1-2/
Attachment #8754637 - Flags: feedback?(jdescottes)
Comment on attachment 8754637 [details]
MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes

add f? flag back after update the patch
Attachment #8754637 - Flags: feedback?(jdescottes)
https://reviewboard.mozilla.org/r/54124/#review50904

This looks very good, thanks for the patch! We will deal with the remote-friendliness in a follow up.
The coding style looks good, not much to say about this.
I have a few nits and suggestions below, and we should also add a test here.

::: devtools/client/aboutdebugging/components/target-list.js:31
(Diff revision 2)
>      targets = targets.map(target => {
>        return targetClass({ client, target, debugDisabled });
>      });
>  
> -    return dom.div({ id: this.props.id, className: "targets" },
> -      dom.h2(null, this.props.name),
> +    let section = "";
> +    if (this.props.isServiceWorkerDisabled) {

The target-list component is generic so it would be best to move the creation of the service-worker specific message out of there.

One suggestion: For now the target-list accepts a "targets" property to build its content. We could extend this by having an "error" property. If the error is defined the target list displays the error, otherwise it can display the targets (or "nothing" if targets is empty).

This means the workers/panel.js component would now be responsible for checking the service worker state and potentially building an error component.

::: devtools/client/aboutdebugging/components/target-list.js:36
(Diff revision 2)
> -      dom.h2(null, this.props.name),
> -      targets.length > 0 ?
> -        dom.ul({ className: "target-list" }, targets) :
> -        dom.p(null, Strings.GetStringFromName("nothing"))
> +    if (this.props.isServiceWorkerDisabled) {
> +      section = dom.p(null, dom.img(
> +        { className: "service-worker-disabled-alert" }),
> +        Strings.GetStringFromName("disabledInPrivateBrowsingMode"),
> +        " (",
> +        dom.a({ href:

nit: The more info link that we use for addons debugging has _target = blank. Both should behave in the same way.

::: devtools/client/locales/en-US/aboutdebugging.properties:15
(Diff revision 2)
>  unregister = unregister
>  
>  addons = Add-ons
>  addonDebugging.label = Enable add-on debugging
>  addonDebugging.tooltip = Turning this on will allow you to debug add-ons and various other parts of the browser chrome
>  addonDebugging.moreInfo = more info

nit: Now that addons.moreInfo is used both for addons and workers panels let's remove the "addons" suffix.

::: devtools/client/locales/en-US/aboutdebugging.properties:29
(Diff revision 2)
>  otherWorkers = Other Workers
>  
>  tabs = Tabs
>  
>  nothing = Nothing yet.
> -
> +disabledInPrivateBrowsingMode = Service workers are disabled when using Private browsing mode

nit: The message hints at service workers being disabled in private browsing mode, but they can also simply be disabled by configuration, or when the user has the "never remember history" setting on (which is mapped to the "permanent private browsing" mode, but this is not obvious for the end user). The warning message should be open ended.

(same goes for the property name)
Comment on attachment 8754637 [details]
MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes

Great start for the patch, have a look at my comments above.

Let's add a test on top of this!

We should simply have three test cases, one for each of the conditions we are validating.
You can look at browser_service_workers.js to see how to get the test to open about:debugging on the service-workers panel.

For permanent browsing and disabled service workers, you can push temporary preferences using SpecialPowers.pushPrefEnv (we use this in several about-debugging tests).

Looking at tests such as devtools/client/styleeditor/test/browser_styleeditor_private_perwindowpb.js it looks like you can open a private browser window by using OpenBrowserWindow({ private: false }). To be able to use this private window with our current test helpers, you will probably have to modify openAboutDebugging to accept a custom window object. 

Give it a try, but if you're blocked just let me know, I will be happy to help.
Attachment #8754637 - Flags: feedback?(jdescottes) → feedback+
> > -
> > +disabledInPrivateBrowsingMode = Service workers are disabled when using Private browsing mode
> 
> nit: The message hints at service workers being disabled in private browsing
> mode, but they can also simply be disabled by configuration, or when the
> user has the "never remember history" setting on (which is mapped to the
> "permanent private browsing" mode, but this is not obvious for the end
> user). The warning message should be open ended.
> 
> (same goes for the property name)


Mate, could you give some suggestion about wording or forward it to proper person?

The word will be shown in field like comment 4, and might need cover these warnings

1. Service workers are disabled when in Private browsing mode
2. Service workers are disabled when browser with Private window
3. User explicitly disabled Service workers

My poor suggestion is:
You are in Private Browsing Mode or Service workers pref is disabled.
Flags: needinfo?(matej)
> > +    if (this.props.isServiceWorkerDisabled) {
> 
> The target-list component is generic so it would be best to move the
> creation of the service-worker specific message out of there.
> 
> One suggestion: For now the target-list accepts a "targets" property to
> build its content. We could extend this by having an "error" property. If
> the error is defined the target list displays the error, otherwise it can
> display the targets (or "nothing" if targets is empty).
> 
> This means the workers/panel.js component would now be responsible for
> checking the service worker state and potentially building an error
> component.

I'm not familiar with React though so it might be a stupid idea. How about create a `Warning` component at same level as TargetList, and switch the component by a boolean flag? Then we don't have to pass anything to TargetList. ex:

dom.div({ id: "workers", className: "inverted-icons" },
  isServiceWorkerDisabled? Warning({...}) : TargetList({...}),
  TargetList({...}),
  TargetList({...})
)
Flags: needinfo?(jdescottes)
(In reply to Fred Lin [:gasolin] from comment #16)
> > > +    if (this.props.isServiceWorkerDisabled) {
> > 
> > The target-list component is generic so it would be best to move the
> > creation of the service-worker specific message out of there.
> > 
> > One suggestion: For now the target-list accepts a "targets" property to
> > build its content. We could extend this by having an "error" property. If
> > the error is defined the target list displays the error, otherwise it can
> > display the targets (or "nothing" if targets is empty).
> > 
> > This means the workers/panel.js component would now be responsible for
> > checking the service worker state and potentially building an error
> > component.
> 
> I'm not familiar with React though so it might be a stupid idea. How about
> create a `Warning` component at same level as TargetList, and switch the
> component by a boolean flag? Then we don't have to pass anything to
> TargetList. ex:
> 
> dom.div({ id: "workers", className: "inverted-icons" },
>   isServiceWorkerDisabled? Warning({...}) : TargetList({...}),
>   TargetList({...}),
>   TargetList({...})
> )

That could work, but keep in mind we still want to display the message below the header (h2) of the TargetList. So if we add another component it will have to duplicate part of the TargetList implementation.
That's why I proposed passing a component to TargetList. Maybe you had something in mind to easily reuse the header from TargetList? 

Other topic: regarding the warning message, I just realized you probably took it from the mockup I provided. So sorry about that! Here is another proposal "Your browser configuration is not compatible with Service Workers".

(clearing ni? for Alex, we will deal with this in a follow up bug)
Flags: needinfo?(poirot.alex)
Flags: needinfo?(jdescottes)
Comment on attachment 8754637 [details]
MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54124/diff/2-3/
Attachment #8754637 - Flags: feedback+
WIP patch with tests and separate warning message into warning component.


The test works fine but while close all test set, private content window test got "Found an unexpected browser window at the end of test run" error.

56 INFO TEST-UNEXPECTED-FAIL | devtools/client/aboutdebugging/test/browser_service_workers.js | Found an unexpected browser window at the end of test run


Will find out the reason and wait for the proper string before set review.
Comment on attachment 8754637 [details]
MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54124/diff/3-4/
Attachment #8754637 - Attachment description: MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; → MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes
Attachment #8754637 - Flags: review?(jdescottes)
Comment on attachment 8754637 [details]
MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54124/diff/4-5/
fixed the test by close private window in head.js. 
Please help review the patch, thanks.
Comment on attachment 8754637 [details]
MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes

https://reviewboard.mozilla.org/r/54124/#review51724

The new tests are really good, thanks a lot! 
I still would like another round of review for the "warning" component creation, but we are really close to landing.

::: devtools/client/aboutdebugging/components/target-list.js:18
(Diff revisions 2 - 4)
>  
>  const LocaleCompare = (a, b) => {
>    return a.name.toLowerCase().localeCompare(b.name.toLowerCase());
>  };
>  
> +const Warning = createFactory(require("./workers/warning"));

The warning component is service-worker specific and should not be created by the target-list component.

My proposal was to create this component in the workers panel and pass it as an "error" property to the target list.

::: devtools/client/aboutdebugging/components/target-list.js:24
(Diff revisions 2 - 4)
> +
>  module.exports = createClass({
>    displayName: "TargetList",
>  
>    render() {
>      let { client, debugDisabled, targetClass, targets, sort } = this.props;

To be consistent, you can create a variable "error" here by using destructuring: 

let { client, debugDisabled, error, sort, targetClass, targets} = this.props;

::: devtools/client/aboutdebugging/components/workers/moz.build:9
(Diff revision 4)
>  
>  DevToolsModules(
>      'panel.js',
>      'service-worker-target.js',
>      'target.js',
> +    'warning.js'

Add a trailing comma here. This will make it easier to read future diffs.

::: devtools/client/aboutdebugging/components/workers/warning.js:5
(Diff revision 4)
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +"use strict";

Not sure we really need a dedicated file for this component as it is pretty simple and only used once? It could be created on the fly in the workers panel. 

If you decide to keep it, it should be renamed to indicate it is only about service-workers. service-worker-warning.js for instance.

::: devtools/client/aboutdebugging/components/workers/warning.js:14
(Diff revision 4)
> +const Services = require("Services");
> +
> +const Strings = Services.strings.createBundle(
> +  "chrome://devtools/locale/aboutdebugging.properties");
> +
> +const MORE_INFO_URL = "https://developer.mozilla.org/en-US/docs/Tools/" +

Long string constants are allowed to go over 80 chars, feel free to unwrap.

::: devtools/client/aboutdebugging/test/browser_service_workers.js:79
(Diff revision 4)
>  
>    yield removeTab(swTab);
>    yield closeAboutDebugging(tab);
>  });
> +
> +add_task(function* test_show_warning_when_service_worker_disabled() {

Create a new separate test, dedicated to testing the service worker warning message. Your 3 new test tasks should be moved there.

The new test file should have a comment on top explaining what it is testing. This is not done on all the aboutdebugging test files, but we are pretty consistent about it in other devtools modules. See devtools/client/aboutdebugging/test/browser_service_workers_push.js

For consistency, please use anonymous functions (add comments if needed).

::: devtools/client/aboutdebugging/test/browser_service_workers.js:80
(Diff revision 4)
>    yield removeTab(swTab);
>    yield closeAboutDebugging(tab);
>  });
> +
> +add_task(function* test_show_warning_when_service_worker_disabled() {
> +  yield new Promise(done => {

Add an info() log here saying what is done/tested.

::: devtools/client/aboutdebugging/test/browser_service_workers.js:96
(Diff revision 4)
> +
> +  yield closeAboutDebugging(tab);
> +});
> +
> +add_task(function* test_show_warning_when_in_private_browsing_mode() {
> +  yield new Promise(done => {

Add an info() log here saying what is done/tested.

::: devtools/client/aboutdebugging/test/browser_service_workers.js:124
(Diff revision 4)
> +  ok(img, "warning message is rendered");
> +
> +  yield closeAboutDebugging(tab, win);
> +});
> +
> +function waitForDelayedStartupFinished(win) {

Move this helper method to head.js and add some JS Doc

::: devtools/client/aboutdebugging/test/head.js:109
(Diff revision 4)
>        info("Tab removed and finished closing.");
>        done();
>      }, false);
>  
>      targetBrowser.removeTab(tab);
> +    if (win) {

The test created the additional window, I think it should close it as well. This will avoid this if statement and will make the helper more flexible.
Attachment #8754637 - Flags: review?(jdescottes)
Comment on attachment 8754637 [details]
MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54124/diff/5-6/
Attachment #8754637 - Flags: review?(jdescottes)
https://reviewboard.mozilla.org/r/54124/#review51724

> The test created the additional window, I think it should close it as well. This will avoid this if statement and will make the helper more flexible.

Thanks for fast reiew!

I've all address issues, please kindly review it again. 

For this case, do `targetWindow.close` makes normal test cases pending.
It might because we trigger tests with `addTab`, but this will close the whole window.
Comment on attachment 8754637 [details]
MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54124/diff/6-7/
Comment on attachment 8754637 [details]
MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes

https://reviewboard.mozilla.org/r/54124/#review52128

Looks good! A few nits and comments. 
I'd like to take a last look after my points have been addressed, so keeping my review flag.

::: devtools/client/aboutdebugging/test/head.js:331
(Diff revisions 5 - 7)
>      });
>    });
>  }
> +
> +/**
> + * Wait for create a new window, ususally used with create private browsing window.

nits: 

- "Waits for the creation of... "
- s/ususally/usually
- s/returns/Returns

::: devtools/client/aboutdebugging/test/head.js:333
(Diff revisions 5 - 7)
>  }
> +
> +/**
> + * Wait for create a new window, ususally used with create private browsing window.
> + * returns a promise that will resolve when the window is successfully created.
> + * @param {win} window

nit: Should be "{Window} win"

::: devtools/client/aboutdebugging/aboutdebugging.css:115
(Diff revision 7)
>    background-color: #f3b0b0;
>    padding: 5px 10px;
>    margin: 5px 4px 5px 0px;
>  }
>  
> +.service-worker-disabled-alert,

nit: it would be nice to be consistent with the addons-install-error message here.

Let's add a "service-worker-disabled" class on your dom.p container, and use the classname "warning" for the icon.

::: devtools/client/aboutdebugging/aboutdebugging.css:126
(Diff revision 7)
>    width: 13px;
>    height: 12px;
>  }
>  
>  @media (min-resolution: 1.1dppx) {
>    .addons-install-error .warning {

Please add the selector for your warning icon here as well.

(this rule is used to get a different image for retina displays, otherwise the icon looks blurry)

::: devtools/client/aboutdebugging/components/workers/panel.js:24
(Diff revision 7)
>    "chrome://devtools/locale/aboutdebugging.properties");
>  
>  const WorkerIcon = "chrome://devtools/skin/images/debugging-workers.svg";
> +const MORE_INFO_URL = "https://developer.mozilla.org/en-US/docs/Tools/about%3Adebugging";
> +
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");

We should import PrivateBrowsingUtils.jsm using the devtools loader. 

loader.lazyImporter(this, "PrivateBrowsingUtils",
  "resource://gre/modules/PrivateBrowsingUtils.jsm");
  
This way we no longer need Cu and no longer need XPCOMUtils.

We should move up this import between the "use strict"; and the first require.

::: devtools/client/aboutdebugging/components/workers/panel.js:117
(Diff revision 7)
> +    let isPrivateBrowsingMode = PrivateBrowsingUtils.permanentPrivateBrowsing;
> +    let isServiceWorkerDisabled = !Services.prefs
> +                                    .getBoolPref("dom.serviceWorkers.enabled");
> +    let errorMsg = isWindowPrivate || isPrivateBrowsingMode ||
> +           isServiceWorkerDisabled ?
> +      dom.p(null, dom.img({ className: "service-worker-disabled-alert" }),

See my comment in aboutdebugging.css regarding classnames.

Also the "icon" here should not be an IMG because the image is set using background-image in CSS. 

For the addons error message a DIV is used, please do the same here.

::: devtools/client/aboutdebugging/components/workers/panel.js:121
(Diff revision 7)
> +           isServiceWorkerDisabled ?
> +      dom.p(null, dom.img({ className: "service-worker-disabled-alert" }),
> +        Strings.GetStringFromName("configurationIsNotCompatible"),
> +        " (",
> +        dom.a({ href: MORE_INFO_URL, target: "_blank" },
> +        Strings.GetStringFromName("moreInfo")),

nit: please indent with 2 additional spaces. "moreInfo" is a TextNode in dom.a, here it looks like it is a child of the dom.p.

::: devtools/client/aboutdebugging/components/workers/panel.js:143
(Diff revision 7)
>          id: "service-workers",
>          name: Strings.GetStringFromName("serviceWorkers"),
>          sort: true,
>          targetClass: ServiceWorkerTarget,
> -        targets: workers.service
> +        targets: workers.service,
> +        error: errorMsg

nit: the props seem alphabetically sorted: move error between client and id.

::: devtools/client/aboutdebugging/test/browser_service_workers_not_compatible.js:5
(Diff revision 7)
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */
> +
> +/* eslint-disable mozilla/no-cpows-in-tests */
> +/* global sendAsyncMessage */

This is not needed.

::: devtools/client/aboutdebugging/test/browser_service_workers_not_compatible.js:10
(Diff revision 7)
> +/* global sendAsyncMessage */
> +
> +"use strict";
> +
> +// Test that Service Worker section should show warning message in
> +// about:debugging if any of folowing conditions is met:

nit: s/folowing/following

::: devtools/client/aboutdebugging/test/browser_service_workers_not_compatible.js:49
(Diff revision 7)
> +  ok(img, "warning message is rendered");
> +
> +  yield closeAboutDebugging(tab);
> +});
> +
> +add_task(function* test_show_warning_when_window_is_private() {

Please use anonymous functions (and otherwise we only use camel case in the devtools).

::: devtools/client/aboutdebugging/test/browser_service_workers_not_compatible.js:59
(Diff revision 7)
> +  let { tab, document } = yield openAboutDebugging("workers", win);
> +  // Check that the warning img appears in the UI
> +  let img = document.querySelector(".service-worker-disabled-alert");
> +  ok(img, "warning message is rendered");
> +
> +  yield closeAboutDebugging(tab, win);

Please add :

win.close();

after "yield closeAboutDebugging(tab, win);" and completely remove "if (win) { win.close() }" from the removeTab helper in head.js.

I tested this locally and it works fine.
Attachment #8754637 - Flags: review?(jdescottes)
https://reviewboard.mozilla.org/r/54124/#review52140

::: devtools/client/aboutdebugging/test/head.js:335
(Diff revision 7)
> +/**
> + * Wait for create a new window, ususally used with create private browsing window.
> + * returns a promise that will resolve when the window is successfully created.
> + * @param {win} window
> + */
> +function waitForDelayedStartupFinished(win) {

One more thing: we have several ESLint failures for this patch. 

If you did not setup ESLint yet, you can have a look at https://wiki.mozilla.org/DevTools/CodingStandards

Alternativaly, ESLint is run on treeherder: https://treeherder.mozilla.org/#/jobs?repo=try&revision=442104673376&selectedJob=21484672

You will see a LOT of unrelated warnings in this ESLint job, but if you scroll down, you will see Errors related to this patch. For the "no unused vars" issue, look at the top of this file you will see that the exported globals are all listed in a comment. Regarding the 80 char limit, it was "just" bumped to 90 yesterday, so some of the errors reported in this build are no longer valid.
https://reviewboard.mozilla.org/r/54124/#review52128

> Please add the selector for your warning icon here as well.
> 
> (this rule is used to get a different image for retina displays, otherwise the icon looks blurry)

nice catch!

> We should import PrivateBrowsingUtils.jsm using the devtools loader. 
> 
> loader.lazyImporter(this, "PrivateBrowsingUtils",
>   "resource://gre/modules/PrivateBrowsingUtils.jsm");
>   
> This way we no longer need Cu and no longer need XPCOMUtils.
> 
> We should move up this import between the "use strict"; and the first require.

Good to know that!

> Please add :
> 
> win.close();
> 
> after "yield closeAboutDebugging(tab, win);" and completely remove "if (win) { win.close() }" from the removeTab helper in head.js.
> 
> I tested this locally and it works fine.

Yes that will work as well
Comment on attachment 8754637 [details]
MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54124/diff/7-8/
Attachment #8754637 - Flags: review?(jdescottes)
Comment on attachment 8754637 [details]
MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes

https://reviewboard.mozilla.org/r/54124/#review52386

Great work!
Thank you for your patience and addressing all my nits :)

Try looks good, you just need to rebase before landing, the current patch does not apply on fx-team (small conflict in head.js).
Attachment #8754637 - Flags: review?(jdescottes) → review+
Adding dev-doc-needed keyword: we need a small section on the MDN about:debugging page to explain when the service workers might be disabled.
Keywords: dev-doc-needed
Started a new section on about:debugging MDN's page: https://developer.mozilla.org/en-US/docs/Tools/about:debugging#Service_workers_disabled . Needs review!
Comment on attachment 8754637 [details]
MozReview Request: Bug 1266415 - display a warning on about:debugging if service workers are disabled; r?jdescottes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54124/diff/8-9/
rebased, thanks!
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e0354aa795ba
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 49
Clearing my flag since this in resolved.
Flags: needinfo?(matej)
(In reply to Julian Descottes [:jdescottes] from comment #33)
> Started a new section on about:debugging MDN's page:
> https://developer.mozilla.org/en-US/docs/Tools/about:
> debugging#Service_workers_disabled . Needs review!

Thanks Julian! I've given the text an edit, added a screenshot, and changed the title to "Service workers not compatible" so that it fits better with the text used in the UI.

https://developer.mozilla.org/en-US/docs/Tools/about:debugging#Service_workers_not_compatible

I also added a note into the Fx 49 developer release notes:

https://developer.mozilla.org/en-US/Firefox/Releases/49#Developer_Tools

Let me know if that looks ok to you.

One quick query — in the UI you provide a "more info" link that links to the about:debugging page; wouldn't it be better if it linked directly to the document fragment of that section of the page, so the user can find the information more easily?
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.