Closed Bug 1361080 (dt-addon-uishim) Opened 7 years ago Closed 7 years ago

[meta][devtools-addon] Create a UI shim to install/bootstrap devtools addon

Categories

(DevTools :: General, enhancement, P3)

Product:
DevTools
Component:
General
Type:
enhancement

Tracking

(firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: jdescottes, Assigned: jdescottes)

References

(Blocks 1 open bug)

Details

(Keywords: meta)

Attachments

(6 files, 1 obsolete file)

Bug 1361080 - display devtools onboarding screen if devtools.enabled if false;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1361080 - add unit test for aboutdevtools page;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1361080 - Add "Enable DevTools..." menu item if devtools.enabled is false;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1361080 - update webextension tests to directly use devtools loader;
59 bytes, text/x-review-board-request
rpl
: review+
Details
Bug 1361080 - add aboutdevtools component;
59 bytes, text/x-review-board-request
ochameau
: review+
Details
Bug 1361080 - filter out hidden devtools menu items in customizableui test;
59 bytes, text/x-review-board-request
jaws
: review+
Details 
As devtools are moving outside of mc, we need to modify the entry points for devtools in Firefox. Instead of simply starting devtools, the buttons, shortcuts, menu items that start devtools today will now need to start the installation of the addon.

The UX for this installation process is still under development. The initial discussion is at https://docs.google.com/document/d/1ku6Jn12XdTFNno0phhLjlBD2H43k5NYswALYnA_UVBA/edit .
Alias: dt-addon-uishim
Blocks: 1361081
Blocks: 1361146
Comment on attachment 8864846 [details]
Bug 1361080 - Introduce tab page to install DevTools add-on.

This is not ready for review.
Attachment #8864846 - Flags: review?(jdescottes)
Try build for all OSes, with DevTools being stripped and a very minimal shim introducing:
* a menu entry in developer menu
* react to most common key shortcuts

Also, it automatically install the addon if the profile preferences say that you opened the toolbox once in the past.
It also auto-install if you pass any devtools argument as command line.
Rebased on top of bug 1363533 in order to be shipped only if MOZ_DEVTOOLS is set to "addon".
That, to allow landing such patch on mozilla-central without polluting existing Firefox with shim modules that will only be useful once we switch to the Add-on release mechanism.
Comment on attachment 8864846 [details]
Bug 1361080 - Introduce tab page to install DevTools add-on.

Just re-rebased on top of lastest version of bug 1363533 (MOZ_DEVTOOLS=addon)

This patch contains a couple of TODO:
* Stop disabling addon-signature, but we may not have correctly signed add-on until quite some time. The workaround is to say, always toggle the pref manually before...
* Stop disabling non-multiprocess pref, should be removable once bug 1363417 reaches devtools repo.
* Move showInstallDialog implementation to its own module/JSM to lazily load this code which is meant to grow. But may be we could introduce this new JSM whenever we improve the dialog?
* the pref controlling the addon url. right now I track whatever is the latest build out of master branch. It appears to not be that stable these day with me still working hard on automation. May be I should wait for me looking into pushing files to archive.mozilla.org. It can be a manual step so that the xpi is safely available?

Otherwise what do you think about the main pieces of this patch?
* menu injection
* key shortcuts
* the possibly hardcoded strings for key shortuts, or just use one international locale?
* we may have to keep some localization in devtools, which may be hard/confusing, may be we should define and pull strings from /browser?
* command line handling
* automatic installation based on telemetry pref (we already used that pref in some other project to detect devtools usage)
* the addon installation itself

If you think I should land something smaller sooner than later, do not hesitate to designate stuff to move to followups.
Attachment #8864846 - Flags: feedback?(jdescottes)
Depends on: 1363533
Comment on attachment 8864846 [details]
Bug 1361080 - Introduce tab page to install DevTools add-on.

https://reviewboard.mozilla.org/r/136542/#review143064

Sorry for the delay, looks like a good start!

I think the part that is the least clear for me is the command line handler. I've made several comments related to that, but my overall thought about this is that if we can't execute the cmd line flags transparently after installing the addon, then the install should be obvious, with the prompt. If it looks like handling cmd line arguments makes this more complicated, I think it would be a good candidate to be handled in a follow up.

::: devtools/shim/devtools-startup.js:119
(Diff revision 3)
> +
> +    // If the add-on is already installed, its own command line component is
> +    // going to take over. Register into Firefox UI and implement the command
> +    // line arguments
> +    if (isInstalled) {
> +      dump("Add-on already installed\n");

Instead of bailing out here, we could directly call the devtools addon startup code. Of course this means to no longer register an additional cmd line handler in the addon and only go through the one in mc.

::: devtools/shim/devtools-startup.js:126
(Diff revision 3)
> +    }
> +
> +    dump("Auto install enabled: " + this.autoInstallEnabled + "\n");
> +    dump("Has any devtools command line argument: " + this.hasAnyDevToolsFlag(cmdLine) + "\n");
> +    dump("Has devtools ever been opened: " + this.hasDevToolsEverBeenOpened + "\n");
> +    if (this.autoInstallEnabled && (this.hasAnyDevToolsFlag(cmdLine) || this.hasDevToolsEverBeenOpened)) {

The conditions for auto installing should be:
- auto-install pref is on
- devtools has already been opened
- devtools was not uninstalled by the user

The last bullet point is important, we need to make sure that the addon doesn't reinstall itself after the user has removed the addon and restarts firefox.

Maybe this is already driven from the addon shutdown method, by touching either the auto-install or the opened.version pref, but somehow I feel we'll need a third pref to check that state? Unless there is something in the addon manager that can give us this information?

::: devtools/shim/devtools-startup.js:128
(Diff revision 3)
> +    dump("Auto install enabled: " + this.autoInstallEnabled + "\n");
> +    dump("Has any devtools command line argument: " + this.hasAnyDevToolsFlag(cmdLine) + "\n");
> +    dump("Has devtools ever been opened: " + this.hasDevToolsEverBeenOpened + "\n");
> +    if (this.autoInstallEnabled && (this.hasAnyDevToolsFlag(cmdLine) || this.hasDevToolsEverBeenOpened)) {
> +      dump("Install DevTools\n");
> +      this.install();

It's good to install devtools when a commandline arg relying on devtools has been passed. In case an install is triggered, will the devtools command line handler kick in immediately and interpret the command line arguments provided here?

For instance, if someone does 
"firefox --jsdebugger" on a profile where the addon is not installed, will the debugger open after the installation of devtools?

::: devtools/shim/devtools-startup.js:131
(Diff revision 3)
> +    if (this.autoInstallEnabled && (this.hasAnyDevToolsFlag(cmdLine) || this.hasDevToolsEverBeenOpened)) {
> +      dump("Install DevTools\n");
> +      this.install();
> +    } else {
> +      Services.obs.addObserver(this, "browser-delayed-startup-finished");
> +      Services.obs.addObserver(this, "browser-inspect-node");

So the plan here is to modify nsContextMenu.js::inspectNode to fire this event rather than directly calling devtools. We can probably land this early.

::: devtools/shim/devtools-startup.js:166
(Diff revision 3)
> +      let menu = doc.getElementById("DevToolsInstallMenu");
> +      if (menu) {
> +        menu.remove();
> +      }
> +    }
> +    CustomizableUI.removeWidgetFromArea("developer-button");

What is the plan for adding the button from the addon? Will we use a different button id? Right now the addon also adds an item with the same id.

I assumed we would reuse developer button, but update the command.

::: devtools/shim/devtools-startup.js:325
(Diff revision 3)
> +  async showInstallDialog(reason, options) {
> +    dump("Show shim UI!\n");
> +    // TODO: Move that code to its own jsm, to lazily load this code which is going to grow
> +    // and is not needed on Firefox startup.
> +    let title = "Do you want to open Developer Tools?";
> +    let message = "It looks like you want to use Developer Tools,\nDo you confirm this?\n";

Regarding localization, I think we definitely need to keep a .properties inside of mozilla central. The shim UI will need to be localized for sure, the about:debugging landing page too. 

The goal is to make sure that people that shouldn't use devtools don't install them accidentally, so that's maybe the most import piece of devtools to localize ever :) 

Regarding the shortcuts, I would be in favor of localizing them too, but I don't feel too strongly about it. I mostly think that if we have non-localized shortcuts somewhere, this will be considered as a bad practice by l10n teams, and we would have to repeatedly justify why we are doing that.

Whether they are localized or not, the main issue is that they should remain synchronized with the ones defined by the addon. Maybe I am missing another issue linked with having a localization file here?

::: devtools/shim/devtools-startup.js:344
(Diff revision 3)
> +      }
> +    }
> +  },
> +
> +  /* eslint-disable max-len */
> +  helpInfo: "  --jsconsole        Open the Browser Console.\n" +

Won't we get the helpInfo twice once the devtools addon is installed?

::: devtools/shim/devtools-startup.manifest:1
(Diff revision 3)
> +component {b8b0da42-30d3-11e7-9cf6-000c29036c20} devtools-startup.js

Won't this be conflicting with our existing devtools-startup.js file? At least when I try to build locally with devtools still in mc, the build fails because of that. Maybe this isn't an issue if one of the components is registered via an addon?
(In reply to Julian Descottes [:jdescottes] from comment #11)
> Comment on attachment 8864846 [details]
> Bug 1361080 - Introduce shim for managing upcoming DevTools addon.
> ::: devtools/shim/devtools-startup.js:119
> (Diff revision 3)
> > +
> > +    // If the add-on is already installed, its own command line component is
> > +    // going to take over. Register into Firefox UI and implement the command
> > +    // line arguments
> > +    if (isInstalled) {
> > +      dump("Add-on already installed\n");
> 
> Instead of bailing out here, we could directly call the devtools addon
> startup code. Of course this means to no longer register an additional cmd
> line handler in the addon and only go through the one in mc.

Yes. It sounds great. The only issue is that we don't want to enable this devtools-startup.js for current nightly.
Because it contains code that is useless until the addon is released and will slow down firefox startup for no good reason.

I don't see how to do that without copying existing code:
http://searchfox.org/mozilla-central/source/devtools/client/devtools-startup.js#27
This "handle" function, to some module (in order to be able to use it from here).
But, we don't want to load the SDK loader on Firefox startup just for that.
"handle" function is run early during firefox startup, we don't want to load devtools loader just for checking if a command line is available.

So, the only proposal I have is to copy it to some new module, and use it from the new command line handler (devtools/shim/devtools-startup.js).


> The conditions for auto installing should be:
> - auto-install pref is on
> - devtools has already been opened
> - devtools was not uninstalled by the user
> 
> The last bullet point is important, we need to make sure that the addon
> doesn't reinstall itself after the user has removed the addon and restarts
> firefox.

Good catch, yes I have to do something about uninstall/disable.
Instead of introducing a new pref, shouldn't we toggle autoInstall pref off on disable/uninstall?
Do you have some more gut feeling why we would need another pref?
Otherwise, yes, the addon manager could tell us if the addon is around but disabled.
But I don't think it can tell us it was around but uninstalled.


> ::: devtools/shim/devtools-startup.js:128
> (Diff revision 3)
> > +    dump("Auto install enabled: " + this.autoInstallEnabled + "\n");
> > +    dump("Has any devtools command line argument: " + this.hasAnyDevToolsFlag(cmdLine) + "\n");
> > +    dump("Has devtools ever been opened: " + this.hasDevToolsEverBeenOpened + "\n");
> > +    if (this.autoInstallEnabled && (this.hasAnyDevToolsFlag(cmdLine) || this.hasDevToolsEverBeenOpened)) {
> > +      dump("Install DevTools\n");
> > +      this.install();
> 
> It's good to install devtools when a commandline arg relying on devtools has
> been passed. In case an install is triggered, will the devtools command line
> handler kick in immediately and interpret the command line arguments
> provided here?

With the current patch, but I meant to, like key shortcuts...
I just forgot doing it. I related to your first comment. I'll do it.
Nevertheless, installing the add-on takes some time, we may have to at least show up something.
Dumps on stdout sounds not enough to understand what is going on?

> ::: devtools/shim/devtools-startup.js:131
> (Diff revision 3)
> > +    if (this.autoInstallEnabled && (this.hasAnyDevToolsFlag(cmdLine) || this.hasDevToolsEverBeenOpened)) {
> > +      dump("Install DevTools\n");
> > +      this.install();
> > +    } else {
> > +      Services.obs.addObserver(this, "browser-delayed-startup-finished");
> > +      Services.obs.addObserver(this, "browser-inspect-node");
> 
> So the plan here is to modify nsContextMenu.js::inspectNode to fire this
> event rather than directly calling devtools. We can probably land this early.

Yes. I think it will simplify nsContextMenu and would be a way to drop devtools dependency from it.

> 
> ::: devtools/shim/devtools-startup.js:166
> (Diff revision 3)
> > +      let menu = doc.getElementById("DevToolsInstallMenu");
> > +      if (menu) {
> > +        menu.remove();
> > +      }
> > +    }
> > +    CustomizableUI.removeWidgetFromArea("developer-button");
> 
> What is the plan for adding the button from the addon? Will we use a
> different button id? Right now the addon also adds an item with the same id.
> 
> I assumed we would reuse developer button, but update the command.

I sounds easier to remove it and recreate it from scratch, like everething else.
Do you think removing/recreating it will introduce some regressions?

> 
> ::: devtools/shim/devtools-startup.js:325
> (Diff revision 3)
> > +  async showInstallDialog(reason, options) {
> > +    dump("Show shim UI!\n");
> > +    // TODO: Move that code to its own jsm, to lazily load this code which is going to grow
> > +    // and is not needed on Firefox startup.
> > +    let title = "Do you want to open Developer Tools?";
> > +    let message = "It looks like you want to use Developer Tools,\nDo you confirm this?\n";
> 
> Regarding localization, I think we definitely need to keep a .properties
> inside of mozilla central. The shim UI will need to be localized for sure,
> the about:debugging landing page too. 

Ok, I initiated a discussion with Flod to keep some locales in /devtools after the move to github.

> Whether they are localized or not, the main issue is that they should remain
> synchronized with the ones defined by the addon. Maybe I am missing another
> issue linked with having a localization file here?

I see multiple options:
* define these key shortcut only in m-c, do not stop listening for them on addon install and instead call some addon function to tell a key shortcut has been pressed. That wouldn't allow us modifying these key shortcuts from the addon and would have to follow firefox release cycles. I think it is the saniest option, but probably hard to implement because of how complex per-tool shortcut is implemented (it is very generic).
* define them in both, with the risk of getting out of sync. But I'm not sure it will be worth setting up a process to cover that.


> > +  helpInfo: "  --jsconsole        Open the Browser Console.\n" +
> 
> Won't we get the helpInfo twice once the devtools addon is installed?
> 
> > +component {b8b0da42-30d3-11e7-9cf6-000c29036c20} devtools-startup.js
> 
> Won't this be conflicting with our existing devtools-startup.js file? At
> least when I try to build locally with devtools still in mc, the build fails
> because of that. Maybe this isn't an issue if one of the components is
> registered via an addon?

I didn't rebased over last version of MOZ_DEVTOOLS=addon, the new devtools-startup.js is meant to be loaded only if MOZ_DEVTOOLS=addon is set, so ther the other one no longer exists.
(In reply to Alexandre Poirot [:ochameau] from comment #12)
> (In reply to Julian Descottes [:jdescottes] from comment #11)
> > Comment on attachment 8864846 [details]
> > Bug 1361080 - Introduce shim for managing upcoming DevTools addon.
> > ::: devtools/shim/devtools-startup.js:119
> > (Diff revision 3)
> > > +
> > > +    // If the add-on is already installed, its own command line component is
> > > +    // going to take over. Register into Firefox UI and implement the command
> > > +    // line arguments
> > > +    if (isInstalled) {
> > > +      dump("Add-on already installed\n");
> > 
> > Instead of bailing out here, we could directly call the devtools addon
> > startup code. Of course this means to no longer register an additional cmd
> > line handler in the addon and only go through the one in mc.
> 
> Yes. It sounds great. The only issue is that we don't want to enable this
> devtools-startup.js for current nightly.
> Because it contains code that is useless until the addon is released and
> will slow down firefox startup for no good reason.
> 
> I don't see how to do that without copying existing code:
> http://searchfox.org/mozilla-central/source/devtools/client/devtools-startup.
> js#27
> This "handle" function, to some module (in order to be able to use it from
> here).
> But, we don't want to load the SDK loader on Firefox startup just for that.
> "handle" function is run early during firefox startup, we don't want to load
> devtools loader just for checking if a command line is available.
> 
> So, the only proposal I have is to copy it to some new module, and use it
> from the new command line handler (devtools/shim/devtools-startup.js).
> 

I assumed we would need a way to call the addon startup programmatically for the use case where the addon needs to be installed on startup. "So why not reuse it here", but it it's easier to just register two separate command line handlers, I am fine with that.

> 
> > The conditions for auto installing should be:
> > - auto-install pref is on
> > - devtools has already been opened
> > - devtools was not uninstalled by the user
> > 
> > The last bullet point is important, we need to make sure that the addon
> > doesn't reinstall itself after the user has removed the addon and restarts
> > firefox.
> 
> Good catch, yes I have to do something about uninstall/disable.
> Instead of introducing a new pref, shouldn't we toggle autoInstall pref off
> on disable/uninstall?
> Do you have some more gut feeling why we would need another pref?

I'm not really sure how auto-install will be used, in which cases it will be set etc... so I might be worrying for nothing.
Let's say we flip auto-install to false when the user uninstalls the addon. Later on if the user decides to manually install the addon, should we flip back the pref to true? So my fear was "what if this was on a profile which was initially configured with auto-install set to false". Not sure in which situation we can need an auto-install after the user manually installed the addon though. 


> > ::: devtools/shim/devtools-startup.js:128
> > (Diff revision 3)
> > > +    dump("Auto install enabled: " + this.autoInstallEnabled + "\n");
> > > +    dump("Has any devtools command line argument: " + this.hasAnyDevToolsFlag(cmdLine) + "\n");
> > > +    dump("Has devtools ever been opened: " + this.hasDevToolsEverBeenOpened + "\n");
> > > +    if (this.autoInstallEnabled && (this.hasAnyDevToolsFlag(cmdLine) || this.hasDevToolsEverBeenOpened)) {
> > > +      dump("Install DevTools\n");
> > > +      this.install();
> > 
> > It's good to install devtools when a commandline arg relying on devtools has
> > been passed. In case an install is triggered, will the devtools command line
> > handler kick in immediately and interpret the command line arguments
> > provided here?
> 
> With the current patch, but I meant to, like key shortcuts...
> I just forgot doing it. I related to your first comment. I'll do it.
> Nevertheless, installing the add-on takes some time, we may have to at least
> show up something.
> Dumps on stdout sounds not enough to understand what is going on?
> 

I think dumps are more than enough for our first iteration. What about "--wait-for-jsdebugger", I'm not sure this one is compatible with installing devtools at the same time (just need to mention it in the helptext maybe ... 99.99% of the users who will need it will have DevTools installed I think).

> > ::: devtools/shim/devtools-startup.js:131
> > (Diff revision 3)
> > > +    if (this.autoInstallEnabled && (this.hasAnyDevToolsFlag(cmdLine) || this.hasDevToolsEverBeenOpened)) {
> > > +      dump("Install DevTools\n");
> > > +      this.install();
> > > +    } else {
> > > +      Services.obs.addObserver(this, "browser-delayed-startup-finished");
> > > +      Services.obs.addObserver(this, "browser-inspect-node");
> > 
> > So the plan here is to modify nsContextMenu.js::inspectNode to fire this
> > event rather than directly calling devtools. We can probably land this early.
> 
> Yes. I think it will simplify nsContextMenu and would be a way to drop
> devtools dependency from it.
> 
> > 
> > ::: devtools/shim/devtools-startup.js:166
> > (Diff revision 3)
> > > +      let menu = doc.getElementById("DevToolsInstallMenu");
> > > +      if (menu) {
> > > +        menu.remove();
> > > +      }
> > > +    }
> > > +    CustomizableUI.removeWidgetFromArea("developer-button");
> > 
> > What is the plan for adding the button from the addon? Will we use a
> > different button id? Right now the addon also adds an item with the same id.
> > 
> > I assumed we would reuse developer button, but update the command.
> 
> I sounds easier to remove it and recreate it from scratch, like everething
> else.
> Do you think removing/recreating it will introduce some regressions?
> 

I was not sure this code would run "before" the addon had registered its button. If we are guaranteed of the sequence of events here, then this is fine.
 
> > 
> > ::: devtools/shim/devtools-startup.js:325
> > (Diff revision 3)
> > > +  async showInstallDialog(reason, options) {
> > > +    dump("Show shim UI!\n");
> > > +    // TODO: Move that code to its own jsm, to lazily load this code which is going to grow
> > > +    // and is not needed on Firefox startup.
> > > +    let title = "Do you want to open Developer Tools?";
> > > +    let message = "It looks like you want to use Developer Tools,\nDo you confirm this?\n";
> > 
> > Regarding localization, I think we definitely need to keep a .properties
> > inside of mozilla central. The shim UI will need to be localized for sure,
> > the about:debugging landing page too. 
> 
> Ok, I initiated a discussion with Flod to keep some locales in /devtools
> after the move to github.
> 
> > Whether they are localized or not, the main issue is that they should remain
> > synchronized with the ones defined by the addon. Maybe I am missing another
> > issue linked with having a localization file here?
> 
> I see multiple options:
> * define these key shortcut only in m-c, do not stop listening for them on
> addon install and instead call some addon function to tell a key shortcut
> has been pressed. That wouldn't allow us modifying these key shortcuts from
> the addon and would have to follow firefox release cycles. I think it is the
> saniest option, but probably hard to implement because of how complex
> per-tool shortcut is implemented (it is very generic).
> * define them in both, with the risk of getting out of sync. But I'm not
> sure it will be worth setting up a process to cover that.
> 
> 

I think we just need a heavy dose of comments around those shortcuts both in mc and on GitHub. I don't think they're likely to ever change. Let's start with the second approach, and see if we can implement you first option later on.

> > > +  helpInfo: "  --jsconsole        Open the Browser Console.\n" +
> > 
> > Won't we get the helpInfo twice once the devtools addon is installed?
> > 
> > > +component {b8b0da42-30d3-11e7-9cf6-000c29036c20} devtools-startup.js
> > 
> > Won't this be conflicting with our existing devtools-startup.js file? At
> > least when I try to build locally with devtools still in mc, the build fails
> > because of that. Maybe this isn't an issue if one of the components is
> > registered via an addon?
> 
> I didn't rebased over last version of MOZ_DEVTOOLS=addon, the new
> devtools-startup.js is meant to be loaded only if MOZ_DEVTOOLS=addon is set,
> so ther the other one no longer exists.

I understand how this allows Firefox to build, but I am just curious if defining "devtools-startup.js" as a component in the addon will not trigger the same kind of issue when installing the addon? I probably just need to actually try it :)
Blocks: 1367428
Comment on attachment 8864846 [details]
Bug 1361080 - Introduce tab page to install DevTools add-on.

Here is a new version of the addon, with better install url to archive.mozilla.org and signed add-on.
The path on archive.mozilla.org is still temporary. I just want to wait before asking for another access to a better path, to see if we end up hosting the addon that way.

I ended up putting a locale in /browser for the shortcut with helpful comments.
It may be meaningful to also move the code to /browser...
Attachment #8864846 - Flags: feedback?(jdescottes)
I forgot to "git add" some files, here is a new patch with everything!
Blocks: 1372520
Depends on: 1378397
I just moved previous patch to bug 1378397 in order to focus on the in-tab UI in this bug.
I attached a patch here to introduce this UI from victoria's designs.
New patch rebased against the new, unified, lazy, efficient devtools-startup.js module.
No longer blocks: 1378863
Rebased again, but we should revamp this patch to not depend on add-on installation and instead display the UI shim on first open.
(In reply to Alexandre Poirot [:ochameau] from comment #50)
> Rebased again, but we should revamp this patch to not depend on add-on
> installation and instead display the UI shim on first open.

Taking the bug and will do so.
Assignee: nobody → jdescottes
Status: NEW → ASSIGNED
The current patch attached is a very rough rebase on top of Bug 1382377. 

Try (incl osx builds) should be at https://treeherder.mozilla.org/#/jobs?repo=try&revision=43e9566a30b7998d2eee5da9ec14da3cb7425f32

Displaying the installation screen is no longer linked to the addon, but only based on the the preference devtools.enabled
If you want to try the patch, keep in mind that this screen is only supposed to be displayed once. If you want to retry the screen after displaying it a first time you have to :
- go to about:config
- flip devtools.enabled to false
- restart firefox
I have a question related to about:debugging. Alex, do you know if there is a proper way to redirect a navigation from about:page1 to about:page2.

With the current patch attached here for about:debugging, I silently replace the content of about:debugging by about:devtools, but the URL remains about:debugging in the URL bar. Is there a way to do that properly in the newChannel() method of the component? 

Otherwise I can navigate from the client code of about:debugging. I would like to avoid initializing DevTools in case devtools.enabled is false, so I'll probably check that first before loading the DevTools loader etc...
Flags: needinfo?(poirot.alex)
I see two options:
* loadInfo.resultPrincipalURI = uri;
  This would make `uri` be the one displayed in the urlbar.
  TBH, I don't know exactly what are the other consequences of using this.

* uri = Services.io.newURI("data:text/html,<meta http-equiv=\"refresh\" content=\"0; URL=about:devtools\" />");

There is more proper ways to handle that at the protocol handler:
  http://searchfox.org/mozilla-central/source/netwerk/protocol/about/nsAboutProtocolHandler.cpp#108
  but nsIAboutModule interface doesn't allow controling what about: protocol handler (which implements the more generic nsIProtocolHandler interface) returns in the newURI method. Here we would have returned about:devtools URI and that would be the one being displayed/loaded.
Flags: needinfo?(poirot.alex)
Comment on attachment 8902430 [details]
Bug 1361080 - add unit test for aboutdevtools page;

Alex: can I get your overall feedback about the way I switch the system menu & photon menu entries between a fake "Developer Tools ..." item to the real menu item?

I know I will probably need a review from someone else since it touches non-devtools UI but I just want to check if the approach seems reasonable to you.
Attachment #8902430 - Flags: feedback?(poirot.alex)
Comment on attachment 8902431 [details]
Bug 1361080 - Add "Enable DevTools..." menu item if devtools.enabled is false;

(ditto)
Attachment #8902431 - Flags: feedback?(poirot.alex)
Comment on attachment 8902430 [details]
Bug 1361080 - add unit test for aboutdevtools page;

There is some missing files:
devtools/shim/aboutdevtools/aboutdevtools.{js,css}
devtools/shim/aboutdevtools/otter.png
devtools/shim/aboutdevtools/moz.build
devtools/shim/locales/en-US/aboutdevtools.dtd

Some people care about view-source, to some it is a very special feature, an iconic and unique feature that defines the web. It partly explain why it is in toolkit and not in devtools.
With this patch, you are hiding it from Tools menu, whereas the key shortcut and context menu entry will still be visible.
May be that's more a comment for Victoria/Harald than you?

Otherwise it looks good, it may make the second patch less important.
Attachment #8902430 - Flags: feedback?(poirot.alex) → feedback+
Attachment #8902431 - Flags: feedback?(poirot.alex) → feedback+
Attachment #8864846 - Attachment is obsolete: true
Comment on attachment 8901805 [details]
Bug 1361080 - display devtools onboarding screen if devtools.enabled if false;

https://reviewboard.mozilla.org/r/173254/#review191406

::: devtools/shim/DevToolsShim.jsm
(Diff revision 7)
> -   * Lazy getter for the `gDevTools` instance. Should only be called when users interacts
> -   * with DevTools as it will force loading them.
> -   *
> -   * @return {DevTools} a devtools instance (from client/framework/devtools)
> -   */
> -  get gDevTools() {

I decided to simplify a bit the DevTools shim and only make it care about devtools being initialized or not. 

If devtools are not initialized for any reason, the shim just asks devtools startup to initialize devtools. If devtools are not enabled, this will fail and force the caller to fallback in the same way as if devtools were not "installed".

So rather than handling installed/enabled/initialized, I would like to try and just use initialized. In the future, if some code paths really need to take a different action depending on the reason why DevTools are not available, we can always adjust. 

I still kept the isInstalled method as a public API, but maybe this should go too since it doesn't have any consumer.
Comment on attachment 8915113 [details]
Bug 1361080 - add aboutdevtools component;

https://reviewboard.mozilla.org/r/186376/#review191434

Looks good to me but did you pinged victoria about this?
She should at least give a f+ this and then followup on it.

::: devtools/shim/aboutdevtools/aboutdevtools-registration.js:7
(Diff revision 1)
> + * 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";
> +
> +// Register the about:devtools URL, that is open whenever user attempt to open

s/open/opened/?
s/attempt/attempts/?

::: devtools/shim/aboutdevtools/aboutdevtools-registration.js:9
(Diff revision 1)
> +
> +"use strict";
> +
> +// Register the about:devtools URL, that is open whenever user attempt to open
> +// DevTools for the first time.
> +// This page prompt for DevTools add-on installation only if user confirm his

s/prompt/prompts/?
s/confirm/confirms/?

::: devtools/shim/aboutdevtools/aboutdevtools.css:18
(Diff revision 1)
> +  width: 100%;
> +}
> +
> +.left-pane {
> +  width: 360px;
> +  background-image: url(chrome://devtools-shim/content/aboutdevtools/images/otter.png);

Would relative url work? Is it broken due to about:devtools magic url?

::: devtools/shim/aboutdevtools/aboutdevtools.js:37
(Diff revision 1)
> +function onInstallButtonClick() {
> +  Services.prefs.setBoolPref("devtools.enabled", true);
> +  // If the install page was opened form AboutDebugging, navigate back to it.
> +  if (reason == "AboutDebugging") {
> +    window.location = "about:debugging";
> +  }

If we don't do that (resume original action) for other commands, may be we should not do that for about:debugging either?
Attachment #8915113 - Flags: review?(poirot.alex) → review+
Comment on attachment 8901805 [details]
Bug 1361080 - display devtools onboarding screen if devtools.enabled if false;

https://reviewboard.mozilla.org/r/173254/#review191424

The 'Inspector Element' context menu is missing. It still forces enabling the tools without any question.
You can do that in another patch in this bug or a followup.

Do you plan to followup on about:debugging? The current redirect looks bad.

::: devtools/shim/DevToolsShim.jsm:181
(Diff revision 7)
>  
> -    return this.gDevTools.inspectNode(tab, selectors);
> +    this.listeners = [];
>    },
>  
>    /**
> -   * Initialize DevTools via the devtools-startup command line handler component.
> +   * Should be called if a shim method should attempt to initialize devtools.

nit: I think second "should" should be removed.

This comment should say it will only initialize them, and return true, if the enabling pref is set.

This method confuses me, may be because of its name, but I can't think of anything better. As it is private to this file, it isn't a big deal.

::: devtools/shim/DevToolsShim.jsm:215
(Diff revision 7)
>    "openBrowserConsole",
>  ];
>  
>  for (let method of webExtensionsMethods) {
>    this.DevToolsShim[method] = function () {
> -    return this.gDevTools[method].apply(this.gDevTools, arguments);
> +    let devtoolsReady = this._maybeInitializeDevTools();

Do we expect to open devtools install panel when hitting this codepath?
If I remember correctly, this should not be called unless devtools are already enabled. If that's the case, it should be good.

::: devtools/shim/aboutdebugging-registration.js:39
(Diff revision 7)
>        loadInfo
>      );
> -    chan.owner = Services.scriptSecurityManager.getSystemPrincipal();
> -    return chan;
> +
> +    channel.owner = Services.scriptSecurityManager.getSystemPrincipal();
> +
> +    return channel;

I imagine you could undo this unecessary changes?
Attachment #8901805 - Flags: review?(poirot.alex) → review+
Comment on attachment 8902431 [details]
Bug 1361080 - Add "Enable DevTools..." menu item if devtools.enabled is false;

https://reviewboard.mozilla.org/r/174016/#review191438

Was there a real conclusion/decision about getting rid of "View Source" menu entry when devtools are disabled?
I'm still anxious about hidding of it.

You may want to r? a /browser/ peer.

::: devtools/shim/devtools-startup.js:358
(Diff revision 6)
>      }, { once: true });
>    },
>  
>    /**
> +   * Specific menu items in the system & hamburger menu should lead to the DevTools
> +   * installation page. Attach the associated event listeners for the provided window.

This comment is unclear to me.
What about phrasing it like this:
"Attach listeners to menu items in the system & hamburger menu that lead to the DevTools installation page."

::: devtools/shim/devtools-startup.js:374
(Diff revision 6)
> +    });
> +  },
> +
> +  /**
> +   * Update the visibility of the DevTools top-level menu items in both the system and
> +   * the hamburger menu for the provided. If DevTools are disabled:

s/provided/provided window/

::: devtools/shim/devtools-startup.js:379
(Diff revision 6)
> +   * the hamburger menu for the provided. If DevTools are disabled:
> +   * - the real menu items (opening the detailed DevTools submenu) should be hidden
> +   * - the "enable devtools" menu items should be displayed
> +   * (and vice-versa if DevTools are enabled).
> +   */
> +  updateDevToolsMenuItems(window) {

nit: you could save some characters in the next few lines by destructuring:
updateDevToolsMenuItems({document})
Comment on attachment 8902430 [details]
Bug 1361080 - add unit test for aboutdevtools page;

https://reviewboard.mozilla.org/r/174014/#review191444

::: devtools/shim/aboutdevtools/test/browser_aboutdevtools_enables_devtools.js:11
(Diff revision 6)
> +
> +/* eslint-env browser */
> +
> +add_task(async function () {
> +  // Using SpecialPowers pushPref makes the test hang after shutting down.
> +  Services.prefs.setBoolPref("devtools.enabled", false);

You should figure this out or manually restore the previous value at test end.

May be BrowserTestUtils.removeTab() can help? (I have no idea what is going on)
(In reply to Alexandre Poirot [:ochameau] from comment #85)
> Comment on attachment 8915113 [details]
> Bug 1361080 - add aboutdevtools component;
> 
> https://reviewboard.mozilla.org/r/186376/#review191434
> 
> Looks good to me but did you pinged victoria about this?
> She should at least give a f+ this and then followup on it.
> 

I sent an email to Victoria earlier to describe the current differences between my implementation and the initial mockups. The intent is to land a basic version here and to polish + add the missing features in follow ups.

Victoria: can you share your feedback here and say if you agree to proceed with this approach?
Flags: needinfo?(victoria)
Since we are mentioning follow ups, I plan to log the following items:
- add form to subscribe to devtools mailing list
- add footer to download Firefox Dev Edition
- devtools onboarding page UI & UX polish
- initialize devtools.enabled depending on existing preferences in profile
- measure click-through rate to enable a shield study on the onboarding page
- default devtools.enabled to false on Release and Beta channels
(In reply to Alexandre Poirot [:ochameau] from comment #85)
> Comment on attachment 8915113 [details]
> Bug 1361080 - add aboutdevtools component;
> ::: devtools/shim/aboutdevtools/aboutdevtools.js:37
> (Diff revision 1)
> > +function onInstallButtonClick() {
> > +  Services.prefs.setBoolPref("devtools.enabled", true);
> > +  // If the install page was opened form AboutDebugging, navigate back to it.
> > +  if (reason == "AboutDebugging") {
> > +    window.location = "about:debugging";
> > +  }
> 
> If we don't do that (resume original action) for other commands, may be we
> should not do that for about:debugging either?

That's a very good point and it also disturbed me to implement it this way. Maybe tops we could have a button on the confirmation page "take me back to about:debugging" or something like that. But navigating immediately feels weird.
(In reply to Alexandre Poirot [:ochameau] from comment #86)
> Comment on attachment 8901805 [details]
> Bug 1361080 - display devtools onboarding screen if devtools.enabled if
> false;
> 
> https://reviewboard.mozilla.org/r/173254/#review191424
> 
> The 'Inspector Element' context menu is missing. It still forces enabling
> the tools without any question.
> You can do that in another patch in this bug or a followup.
> 
> Do you plan to followup on about:debugging? The current redirect looks bad.
> 

Yes I think it deserves a follow-up. I tried the suggestions you provided but could not get them to work. The only thing I managed to was to load "aboutdevtools.xhtml" instead of "aboutdebugging.xhtml". I have two issues with that: first it's a silent redirect, second I can't pass a ?reason=AboutDebugging parameter when loading the page. 

I was thinking at some point that we could keep the about:debugging tab opened, showing "Sorry, it looks like DevTools are not enabled" or something like that, and then open about:devtools in a _new_ tab as we do for all the other entry points.
  
> 
> ::: devtools/shim/DevToolsShim.jsm:215
> (Diff revision 7)
> >    "openBrowserConsole",
> >  ];
> >  
> >  for (let method of webExtensionsMethods) {
> >    this.DevToolsShim[method] = function () {
> > -    return this.gDevTools[method].apply(this.gDevTools, arguments);
> > +    let devtoolsReady = this._maybeInitializeDevTools();
> 
> Do we expect to open devtools install panel when hitting this codepath?
> If I remember correctly, this should not be called unless devtools are
> already enabled. If that's the case, it should be good.
> 

Normally yes, all the webextensions methods should only be called after DevTools have already been opened. Now that's just the theory. At some point I considered only displaying the onboarding page if a "reason" was provided. Do you think that could be a good thing?
(In reply to Alexandre Poirot [:ochameau] from comment #86)
> Comment on attachment 8901805 [details]
> Bug 1361080 - display devtools onboarding screen if devtools.enabled if
> false;
> 
> https://reviewboard.mozilla.org/r/173254/#review191424
> 
> The 'Inspector Element' context menu is missing. It still forces enabling
> the tools without any question.
> You can do that in another patch in this bug or a followup.
> 

Ah, "Inspect Element" should be implemented and should trigger the onboarding tab. 
At least it does on my machine. Can you verify?
Flags: needinfo?(poirot.alex)
(In reply to Julian Descottes [:jdescottes] from comment #93)
> Ah, "Inspect Element" should be implemented and should trigger the
> onboarding tab. 
> At least it does on my machine. Can you verify?

Ah, yes, it works, but not always. I think I was in a special case when testing it.

Here is at least one STR to reproduce:
* have devtools.enabled set to true
* trigger devtools via any entry
* devtools open, now close them
* toggle the pref to false
* now, try opening tools via inspect element context menu
  => tools will open again, instead of the onboarding

I probably did something like that while testing.
It isn't critical as the STR is special, I thought it wasn't working at all.
Flags: needinfo?(poirot.alex)
Comment on attachment 8902436 [details]
Bug 1361080 - update webextension tests to directly use devtools loader;

https://reviewboard.mozilla.org/r/174024/#review193280

r=me (on green and happy try :-))
Attachment #8902436 - Flags: review?(lgreco) → review+
Sounds like this first patch has not landed yet, and that it would be helpful to land a rough patch first and then work out minor UX issues in the next patch? Also, sounds like this UI would only be visible if you go into prefs in Nightly and disable DevTools? 

If so, this is fine by me, except if it affects View Source. Re: View Source, I would say it's important to leave its entry points unchanged for now. (Not much discussion was had over it, but I listed pros and cons here: https://docs.google.com/document/d/1ku6Jn12XdTFNno0phhLjlBD2H43k5NYswALYnA_UVBA/edit#heading=h.1g73hyys6u6g It seems risky to remove any entry points without some kind of replacement.)

One odd thing I noticed in the Shim screenshots is the first paragraph seems to have different line height than the second paragraph - maybe this is an easy fix?
Flags: needinfo?(victoria)
Thanks for the feedback!

(In reply to Victoria Wang [:victoria] from comment #96)
> Sounds like this first patch has not landed yet, and that it would be
> helpful to land a rough patch first and then work out minor UX issues in the
> next patch? Also, sounds like this UI would only be visible if you go into
> prefs in Nightly and disable DevTools? 

Yes correct.

> If so, this is fine by me, except if it affects View Source. Re: View
> Source, I would say it's important to leave its entry points unchanged for
> now. (Not much discussion was had over it, but I listed pros and cons here:
> https://docs.google.com/document/d/
> 1ku6Jn12XdTFNno0phhLjlBD2H43k5NYswALYnA_UVBA/edit#heading=h.1g73hyys6u6g It
> seems risky to remove any entry points without some kind of replacement.)
> 

To be clear, it only affects "view source" if you disabled devtools in about:config. And the "view source" entry in the context menu is not affected. But "view source" can be also found in the system menu (Tools>WebDeveloper) and the hamburger menu (Web Developer submenu). These are the two entry points which are no longer available if DevTools are disabled.

> One odd thing I noticed in the Shim screenshots is the first paragraph seems
> to have different line height than the second paragraph - maybe this is an
> easy fix?

Good catch I'll fix that before landing.
(In reply to Alexandre Poirot [:ochameau] from comment #88)
> Comment on attachment 8902430 [details]
> Bug 1361080 - add unit test for aboutdevtools page;
> 
> https://reviewboard.mozilla.org/r/174014/#review191444
> 
> :::
> devtools/shim/aboutdevtools/test/browser_aboutdevtools_enables_devtools.js:11
> (Diff revision 6)
> > +
> > +/* eslint-env browser */
> > +
> > +add_task(async function () {
> > +  // Using SpecialPowers pushPref makes the test hang after shutting down.
> > +  Services.prefs.setBoolPref("devtools.enabled", false);
> 
> You should figure this out or manually restore the previous value at test
> end.
> 
> May be BrowserTestUtils.removeTab() can help? (I have no idea what is going
> on)

The bug comes from using pushpref _and_ updating the pref during the test. It looks like pushpref expects the pref to have the same value at the beginning and end of the test.
Thanks Julian!

> To be clear, it only affects "view source" if you disabled devtools in
> about:config. And the "view source" entry in the context menu is not
> affected. But "view source" can be also found in the system menu
> (Tools>WebDeveloper) and the hamburger menu (Web Developer submenu). These
> are the two entry points which are no longer available if DevTools are
> disabled.

Just to clarify: Does this mean that for all new installs of Firefox (58) with no DevTools installed yet, all the View Source entry points will still be there? It's only if someone manually disables DevTools in about:config that we'd lose those two View Source entry points?
(In reply to Victoria Wang [:victoria] from comment #104)
> Thanks Julian!
> 
> > To be clear, it only affects "view source" if you disabled devtools in
> > about:config. And the "view source" entry in the context menu is not
> > affected. But "view source" can be also found in the system menu
> > (Tools>WebDeveloper) and the hamburger menu (Web Developer submenu). These
> > are the two entry points which are no longer available if DevTools are
> > disabled.
> 
> Just to clarify: Does this mean that for all new installs of Firefox (58)
> with no DevTools installed yet, all the View Source entry points will still
> be there? It's only if someone manually disables DevTools in about:config
> that we'd lose those two View Source entry points?

Sorry I was unclear. 

You made a similar point about the UI only being visible if DevTools were manually disabled:
> Also, sounds like this UI would only be visible if you go into prefs in Nightly and disable DevTools? 

Same for view source, since DevTools will still be enabled for everybody, no one will see an impact. We still need to decide what we do about it before we actually "disable" DevTools for new profiles. It's just that we don't have to find all the answers before landing this bug.

In any case I'm working on another version of the patch that will preserve all the view source entry points for now.
Alex, the two patches should be ready for review. The one related to menu items now preserves view source and no longer impacts browser/. The testing one is now using pushPref (I would like to add more tests in follow ups).
Flags: needinfo?(poirot.alex)
(In reply to Julian Descottes [:jdescottes] from comment #91)
> (In reply to Alexandre Poirot [:ochameau] from comment #85)
> > If we don't do that (resume original action) for other commands, may be we
> > should not do that for about:debugging either?
> 
> That's a very good point and it also disturbed me to implement it this way.
> Maybe tops we could have a button on the confirmation page "take me back to
> about:debugging" or something like that. But navigating immediately feels
> weird.

Note that this still happens. I would have thought we wouldn't do anything now, and later do something (redirect, button, ...).

Comment 94 still happens, do you plan to followup on this? (it isn't very important but would be nice to be fixed)
Flags: needinfo?(poirot.alex)
Comment on attachment 8902430 [details]
Bug 1361080 - add unit test for aboutdevtools page;

https://reviewboard.mozilla.org/r/174014/#review193530

Thanks for figuring this out.
This is surprising we are not getting hit by this pushPrefEnv issue more often...
Attachment #8902430 - Flags: review?(poirot.alex) → review+
Comment on attachment 8902431 [details]
Bug 1361080 - Add "Enable DevTools..." menu item if devtools.enabled is false;

https://reviewboard.mozilla.org/r/174016/#review193536

Thanks, I tested the patch queue against, it looks good to me.
Attachment #8902431 - Flags: review?(poirot.alex) → review+
Thanks for the reviews!

(In reply to Alexandre Poirot [:ochameau] from comment #112)
> (In reply to Julian Descottes [:jdescottes] from comment #91)
> > (In reply to Alexandre Poirot [:ochameau] from comment #85)
> > > If we don't do that (resume original action) for other commands, may be we
> > > should not do that for about:debugging either?
> > 
> > That's a very good point and it also disturbed me to implement it this way.
> > Maybe tops we could have a button on the confirmation page "take me back to
> > about:debugging" or something like that. But navigating immediately feels
> > weird.
> 
> Note that this still happens. I would have thought we wouldn't do anything
> now, and later do something (redirect, button, ...).
> 

I will remove the redirect for now.

> Comment 94 still happens, do you plan to followup on this? (it isn't very
> important but would be nice to be fixed)

Let's have a follow up for the "disabling devtools" scenario.
Comment on attachment 8915113 [details]
Bug 1361080 - add aboutdevtools component;

https://reviewboard.mozilla.org/r/186376/#review191434

> Would relative url work? Is it broken due to about:devtools magic url?

I works actually, thanks for catching that.
Added a new changeset to fix a testfailure spotted on try. New try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c2baa6760f6707160d85d84998615ec9b26483dd
(In reply to Julian Descottes [:jdescottes] from comment #105)

Thanks for the clarification! I'll be working on a list of further UI polish items.
Comment on attachment 8917392 [details]
Bug 1361080 - filter out hidden devtools menu items in customizableui test;

https://reviewboard.mozilla.org/r/188392/#review194196
Attachment #8917392 - Flags: review?(jaws) → review+
Pushed by jdescottes@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d2365ea174e9
add aboutdevtools component;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/986fad4df322
display devtools onboarding screen if devtools.enabled if false;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/8cddec5ae4ee
Add "Enable DevTools..." menu item if devtools.enabled is false;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/e53ec162daed
update webextension tests to directly use devtools loader;r=rpl
https://hg.mozilla.org/integration/autoland/rev/63a2497c280c
add unit test for aboutdevtools page;r=ochameau
https://hg.mozilla.org/integration/autoland/rev/a5a45ed8b400
filter out hidden devtools menu items in customizableui test;r=jaws
Depends on: 1408330
Depends on: 1408331
Depends on: 1408334
Depends on: 1408337
Depends on: 1408339
Depends on: 1408340
Comment on attachment 8915113 [details]
Bug 1361080 - add aboutdevtools component;

https://reviewboard.mozilla.org/r/186376/#review194422

::: devtools/shim/locales/en-US/aboutdevtools.dtd:1
(Diff revision 4)
> +<!ENTITY  aboutDevtools.headTitle "About Developer Tools">

File is missing a license header.

::: devtools/shim/locales/en-US/aboutdevtools.dtd:20
(Diff revision 4)
> +          "As of Firefox 58, Developer Tools are disabled by default to give you more control over your browser.">
> +
> +<!ENTITY  aboutDevtools.enable.learnMoreLink "Learn more about DevTools">
> +<!ENTITY  aboutDevtools.enable.installButton "Enable Developer Tools">
> +<!ENTITY  aboutDevtools.welcome.title "Welcome to Firefox Developer Tools!">
> +<!ENTITY  aboutDevtools.welcome.message "You’ve successfully enabled DevTools! To get started explore the Web Developer menu or open the tools with ##INSPECTOR_SHORTCUT##.">

Please add a localization comment when you do something so strange in strings (i.e. do not localize '##INSPECTOR_SHORTCUT##').
Depends on: 1408368
(In reply to Francesco Lodolo [:flod] from comment #134)
> Comment on attachment 8915113 [details]
> Bug 1361080 - add aboutdevtools component;
> 
> https://reviewboard.mozilla.org/r/186376/#review194422
> 
> ::: devtools/shim/locales/en-US/aboutdevtools.dtd:1
> (Diff revision 4)
> > +<!ENTITY  aboutDevtools.headTitle "About Developer Tools">
> 
> File is missing a license header.
> 
> ::: devtools/shim/locales/en-US/aboutdevtools.dtd:20
> (Diff revision 4)
> > +          "As of Firefox 58, Developer Tools are disabled by default to give you more control over your browser.">
> > +
> > +<!ENTITY  aboutDevtools.enable.learnMoreLink "Learn more about DevTools">
> > +<!ENTITY  aboutDevtools.enable.installButton "Enable Developer Tools">
> > +<!ENTITY  aboutDevtools.welcome.title "Welcome to Firefox Developer Tools!">
> > +<!ENTITY  aboutDevtools.welcome.message "You’ve successfully enabled DevTools! To get started explore the Web Developer menu or open the tools with ##INSPECTOR_SHORTCUT##.">
> 
> Please add a localization comment when you do something so strange in
> strings (i.e. do not localize '##INSPECTOR_SHORTCUT##').

As discussed, let's move aboutdevtools.dtd out of the localized folders for now. startup.properties can stay (unless you spotted something wrong with this file too).
(In reply to Julian Descottes [:jdescottes] from comment #135)
> As discussed, let's move aboutdevtools.dtd out of the localized folders for
> now. startup.properties can stay (unless you spotted something wrong with
> this file too).

Nope, that looks good
Depends on: 1408369
No longer depends on: 1378397
No longer depends on: 1408369
No longer depends on: 1408334
No longer depends on: 1408340
No longer depends on: 1408330, 1408339
No longer depends on: 1408331
No longer depends on: 1408337
Depends on: 1413246
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: