1223341 - Add the Firefox Devtools to the SeaMonkey UI

Status: VERIFIED FIXED

(SeaMonkey :: General, defect)

Description

[Philip Chee]

Make the Firefox Devtools show up and work in SeaMonkey

Comment 1

[Philip Chee]

Attachment: Patch 0.1WIP add glue code for the Firefox Devtools.

Comment 2

[Ian Neal]

Nit: can you make sure the case of the accesskey matches the case of the letter in the label
Nit: comments starting with a capital letter should end with a full stop (or other punctuation)

Comment 5

[Philip Chee]

Attachment: Patch v1.0 ready for review

Perquisites:
Bug 1022354 SeaMonkey forces 3rd-party themes to not use defaultFavicon.png but hardcode a bookmarks-item.png
Bug 1223344 Some tweaks to gDevTools.jsm to get Devtools working in SeaMonkey [mozilla-central]
Bug 1223338 Add DevTools client L10N language files to SeaMonkey builds

This bug/patch does not do the following:
* OSX key bindings (because I don't have a Mac!)
* WebIDE
* Responsive Design
* about:debugging
* Integrate with light weight themes (?)
Separate followup bugs will be filed.

Also TODO on the Devtools side:
* replace chrome://browser/skin/ => chrome://communicator/skin/
* replace chrome://browser/skin/tabbrowser/loading.png
-------------------------------------------------------------------
SeaMonkey uses "Tasks:ErrorConsole" so this doesn't affect us. The following lines are just to keep Devtools happy.

> +    // Enable Error Console?
> +    var consoleEnabled = Services.prefs.getBoolPref("devtools.errorconsole.enabled");
> +    toggleCmd("Tools:ErrorConsole", consoleEnabled);
....
> +    <command id="Tools:ErrorConsole" oncommand="toJavaScriptConsole()" disabled="true" hidden="true"/>
....
> +    <broadcaster id="devtoolsMenuBroadcaster_PageSource"
> +                 key="key_viewSource"
> +                 command="View:PageSource"/>
> +    <broadcaster id="devtoolsMenuBroadcaster_ErrorConsole"
> +                 command="Tools:ErrorConsole"/>

> +        <toolbarbutton id="developer-toolbar-closebutton"
> +                       class="close-icon"

The Devtools [class="devtools-closebutton"] looks ugly so I'm using the close icons from our Classic/Modern themes.

> +    this.isRemote = gContextMenuContentData && gContextMenuContentData.isRemote;

We have a fake gContextMenuContentData that just returns false for isRemote. Actually now that we have a content frame script I think we can implement a real gContextMenuContentData...

>  [extensions/{972ce4c6-7e08-4474-a285-3208198ce6fd}] chrome.jar:
>  % override chrome://mozapps/skin/places/defaultFavicon.png chrome://communicator/skin/bookmarks/bookmark-item.png
> +% override chrome://browser/skin/browser.css chrome://communicator/skin/communicator.css

Needs Bug 1022354. Please review?

> +% override chrome://browser/skin/browser.css chrome://communicator/skin/communicator.css

There are just too many places in Devtools that use "chrome://browser/skin/". I'll file a bug to change that but in the mean time this makes the Devtools UI somewhat better. And probably benefits some Firefox centric extensions too.

Comment 6

[Philip Chee]

> There are just too many places in Devtools that use
> "chrome://browser/skin/". I'll file a bug to change that but in the mean
> time this makes the Devtools UI somewhat better. And probably benefits some
> Firefox centric extensions too.
Devtools has replaces all references to "chrome://browser/skin/" with "chrome://global/skin/" so I can revert that part of the patch.

Comment 7

[Philip Chee]

Attachment: Patch v2.0 Added Devtools styles to modern

Comment 8

[[e-mail]]

Comment on attachment 8703186 [details] [diff] [review]
Patch v2.0 Added Devtools styles to modern

Whoa, this is mostly great! It doesn't seem to like private windows though.

>+/* Developer Tools: Error counter */
>+
>+#developer-toolbar-toolbox-button[error-count]:before {
Do you know how I can find all of the various elements so that I can comment on the CSS you've used? Thanks.

>+  <toolbar id="developer-toolbar"
>+           hidden="true"/>
Nit: don't have to wrap if it fits on one line. (Also, not ideal to have this here instead of getting the overlay to insert it correctly, although moving the attributes here would be acceptable.)

>+XPCOMUtils.defineLazyGetter(this, "BrowserToolboxProcess", function() {
You can actually use defineLazyModuleGetter for this. (Do we actually use this one?)

>+XPCOMUtils.defineLazyGetter(this, "DeveloperToolbar", function() {
>+  var tmp = {};
>+  Components.utils.import("resource://devtools/client/shared/DeveloperToolbar.jsm", tmp);
>+  return new tmp.DeveloperToolbar(window, document.getElementById("developer-toolbar"));
>+});
(But not this because it doesn't just return tmp.foo).

>+XPCOMUtils.defineLazyModuleGetter(this, "gDevTools",
>+                                  "resource://devtools/client/framework/gDevTools.jsm");
(Do we use this?)

>+function openEyedropper() {
>+  var eyedropper = new this.Eyedropper(this, { context: "menu",
>+                                               copyOnSelect: true });
>+  eyedropper.open();
>+}
>+
>+Object.defineProperty(this, "Eyedropper", {
(Do we use Eyedropper outside of openEyedropper?)

>+Object.defineProperty(this, "HUDService", {
(Do we use this?)

>-    if (uri && (uri.schemeIs("http") || uri.schemeIs("https")))
>-      menuitem.removeAttribute("disabled");
>-    else
>-      menuitem.setAttribute("disabled", true);
>+    var itemEnabled = uri && (uri.schemeIs("http") || uri.schemeIs("https"));
>+    menuitem.disabled = !itemEnabled;
Why this change?

>+      //cmd.disabled = !isEnabled; //XXXRatty: why isn't this working?
Because disabled is an XBL property only available on certain elements while hidden is a (Web)IDL property available on all elements.

>+    toggleCmd("Tools:BrowserContentToolbox", remoteEnabled && win.gMultiProcessBrowser);
This doesn't exist. Comment it out at least?

>+    <menuitem id="menu_devToolbox"
Could do with a separator before this.

>+  <broadcasterset id="mainBroadcasterSet">
These probably exist in Firefox because they have both the main menu and the app menu / hamburger thingy; we probably don't need them instead the attributes could be set directly on the menuitems.

>+    var gBrowser = this.browser.ownerDocument.defaultView.gBrowser;
>+    var tt = tmp.devtools.TargetFactory.forTab(gBrowser.selectedTab);
getBrowser().selectedTab

>+      if (this.isRemote) {
Never true, so at least comment it out.

>+    this.isRemote = gContextMenuContentData && gContextMenuContentData.isRemote;
Ditto.

>diff --git a/suite/installer/package-manifest.in b/suite/installer/package-manifest.in
[I didn't check these.]

>+<!ENTITY devtoolsInspect.label          "Inspect Element">
>+<!ENTITY devtoolsInspect.accesskey      "n">
Conflicts with Send Image I'm afraid. (Also we probably need to fix DOM Inspector at some point.)

Comment 9

[[e-mail]]

(In reply to comment #8)
> >+<!ENTITY devtoolsInspect.label          "Inspect Element">
> >+<!ENTITY devtoolsInspect.accesskey      "n">
> Conflicts with Send Image I'm afraid. (Also we probably need to fix DOM
> Inspector at some point.)
Then there's all those access keys - offhand I think Search The Web conflicts for instance.

Comment 10

[rexyrexy2]

(In reply to [e-mail] from comment #9)
> (In reply to comment #8)
> > >+<!ENTITY devtoolsInspect.label          "Inspect Element">
> > >+<!ENTITY devtoolsInspect.accesskey      "n">
> > Conflicts with Send Image I'm afraid. (Also we probably need to fix DOM
> > Inspector at some point.)
> Then there's all those access keys - offhand I think Search The Web
> conflicts for instance.


perhaps we could move Search the Web back on to Ctrl+Shift+F?

Comment 11

[Philip Chee]

Attachment: Screenshot of DevTools in action in SeaMonkey

Comment 12

[Tim Nguyen :ntim]

Comment on attachment 8703186 [details] [diff] [review]
Patch v2.0 Added Devtools styles to modern

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

::: suite/browser/webDeveloperOverlay.js
@@ +167,5 @@
> +    if (webIDEEnabled && showWebIDEWidget) {
> +      gDevToolsBrowser.installWebIDEWidget();
> +    } else {
> +      gDevToolsBrowser.uninstallWebIDEWidget();
> +    }

The WebIDE widget stuff really just adds a toolbar button in Firefox's UI if WebIDE was launched once. The code uses CustomizableUI.jsm (which I think is Firefox only).

For SM, you should be able to add the WebIDE button to the customization palette directly.

Comment 13

[Philip Chee]

Attachment: Patch v3.0 Fix nits added remaining styles.

Changes since the last patch:
* Added a Pref listener for "devtools.theme".
>     // Set an attribute on root element to make it possible
>     // to change colors based on the selected devtools theme.
* Moved all DevTools styles to webDeveloper.css.
** Mostly copied verbatim from mozilla/devtools/client/themes/commandline.inc.css.

>>+#developer-toolbar-toolbox-button[error-count]:before {
> Do you know how I can find all of the various elements so that I can comment
> on the CSS you've used? Thanks.

All Devtools styles apply to the "developer-toolbar" toolbar and descendent nodes it except:
* panel id="gcli-tooltip" class="gcli-panel" noautofocus="true" noautohide="true" height="1px"
* panel id="gcli-output" class="gcli-panel" noautofocus="true" noautohide="true" height="1px"
These two appear to be dynamically generated and are siblings to "developer-toolbar".

> +  updateDevtoolsThemeAttribute: function() {
> +    // Set an attribute on root element to make it possible
> +    // to change colors based on the selected devtools theme.
...
> +    document.documentElement.setAttribute("devtoolstheme", devtoolsTheme);
> +    // document.getElementById("developer-toolbar").setAttribute("devtoolstheme", devtoolsTheme);
Setting the attribute on the Devtools toolbar allows for more efficient CSS. Except that Devtools etc expect the attribute on the :root element.

> +    <toolbar id="developer-toolbar" xpfe="false" hidden="true"
> +             insertbefore="status-bar">
> +      <observes element="main-window" attribute="devtoolstheme"/>
I had a brainwave and added an observes child element to the toolbar.

> +#developer-toolbar-toolbox-button {
> +  list-style-image: url("chrome://devtools/skin/images/toggle-tools.png");
> +  -moz-image-region: rect(0px, 64px, 16px, 48px);
> +}
> +
> +#developer-toolbar-toolbox-button:hover > .toolbarbutton-icon {
> +  filter: brightness(120%);
> +}
> +
> +#developer-toolbar-toolbox-button:hover:active > .toolbarbutton-icon {
> +  filter: saturate(150%);
> +}
> +
> +#developer-toolbar-toolbox-button[checked=true] > .toolbarbutton-icon {
> +  filter: hue-rotate(180deg);
> +}
These styles are my own invention. The commandline.inc.css looks really terrible against both classic and modern.

> +#developer-toolbar-toolbox-button[error-count]:before {
> +  color: white;
> +  min-width: 16px;
> +  text-shadow: none;
> +  background-color: firebrick;
> +  border-radius: 2px;
> +  -moz-margin-end: 2px;
> +/*
> +  Firefox browser/themes/windows/browser.css
> +  color: #FDF3DE;
> +  min-width: 16px;
> +  text-shadow: none;
> +  background-image: linear-gradient(#B4211B, #8A1915);
> +  border-radius: 1px;
> +  -moz-margin-end: 5px;
> +  Firefox browser/themes/linux/browser.css
> +  color: #FDF3DE;
> +  min-width: 16px;
> +  text-shadow: none;
> +  background-image: linear-gradient(#B4211B, #8A1915);
> +  border-radius: 1px;
> +  -moz-margin-end: 2px;
> +*/
> +}
Didn't like the Firefox colours. Experimented with some different colours. Suggestions for improvements needed.

> +/* GCLI */
....
Almost verbatim from commandline.inc.css except I removed a few style rules that I disagreed with like -moz-appearance: none; and min-height: 32px; and line-height: 32px;

> +<?xml-stylesheet href="chrome://devtools/skin/devtools-browser.css" type="text/css"?>
* Sync latest changes in Firefox. Was using devtools/skin/common.css. After Devtools refactoring now using devtools-browser.css with far fewer styles hence less interference with our themes.

(In reply to [e-mail] from comment #8)

> Whoa, this is mostly great! It doesn't seem to like private windows though.
It's a mystery to me too. Devtools in Firefox private windows are working properly.

>>+  <toolbar id="developer-toolbar"
>>+           hidden="true"/>
> Nit: don't have to wrap if it fits on one line. (Also, not ideal to have
> this here instead of getting the overlay to insert it correctly, although
> moving the attributes here would be acceptable.)
Moved to webDeveloperOverlay using an insertbefore attribute.

>>+XPCOMUtils.defineLazyGetter(this, "BrowserToolboxProcess", function() {
> You can actually use defineLazyModuleGetter for this. (Do we actually use
> this one?)
Fixed. Yes.

>>+XPCOMUtils.defineLazyModuleGetter(this, "gDevTools",
>>+                                  "resource://devtools/client/framework/gDevTools.jsm");
> (Do we use this?)
Yes

>>+function openEyedropper() {
>>+  var eyedropper = new this.Eyedropper(this, { context: "menu",
>>+                                               copyOnSelect: true });
>>+  eyedropper.open();
>>+}
>>+
>>+Object.defineProperty(this, "Eyedropper", {
> (Do we use Eyedropper outside of openEyedropper?)
There is at least one Firefox extension that uses the Eyedropper - some sort of fancy colour picker. The author is rather keen to port it to SeaMonkey but is blocked on this.

>>+Object.defineProperty(this, "HUDService", {
> (Do we use this?)
This is used in several places in the Devtools client code.

>>-    if (uri && (uri.schemeIs("http") || uri.schemeIs("https")))
>>-      menuitem.removeAttribute("disabled");
>>-    else
>>-      menuitem.setAttribute("disabled", true);
>>+    var itemEnabled = uri && (uri.schemeIs("http") || uri.schemeIs("https"));
>>+    menuitem.disabled = !itemEnabled;
> Why this change?
Static in the attic. Reverted

>>+      //cmd.disabled = !isEnabled; //XXXRatty: why isn't this working?
> Because disabled is an XBL property only available on certain elements while
> hidden is a (Web)IDL property available on all elements.
Reverted.

>>+    toggleCmd("Tools:BrowserContentToolbox", remoteEnabled && win.gMultiProcessBrowser);
> This doesn't exist. Comment it out at least?
Added a comment.

>>+    <menuitem id="menu_devToolbox"
> Could do with a separator before this.
Fixed.

>>+  <broadcasterset id="mainBroadcasterSet">
> These probably exist in Firefox because they have both the main menu and the
> app menu / hamburger thingy; we probably don't need them instead the
> attributes could be set directly on the menuitems.
BTDTGTTS. Unfortunately the Devtools client code expects all these broadcasters to exist and gets rather unhappy when these go missing.

>>+    var gBrowser = this.browser.ownerDocument.defaultView.gBrowser;
>>+    var tt = tmp.devtools.TargetFactory.forTab(gBrowser.selectedTab);
> getBrowser().selectedTab
Fixed

>>+      if (this.isRemote) {
> Never true, so at least comment it out.
Commented out.

>>+    this.isRemote = gContextMenuContentData && gContextMenuContentData.isRemote;
> Ditto.
Commented out.

>>diff --git a/suite/installer/package-manifest.in b/suite/installer/package-manifest.in
> [I didn't check these.]
I just kept adding interfaces until Devtools stopped spamming the Error Console.

>>+<!ENTITY devtoolsInspect.label          "Inspect Element">
>>+<!ENTITY devtoolsInspect.accesskey      "n">
> Conflicts with Send Image I'm afraid.
It also conflicts with almost everything else. Suggestions?

> (Also we probably need to fix DOM Inspector at some point.)
Suggestions.

Comment 14

[Philip Chee]

(In reply to Tim Nguyen [:ntim] from comment #12)
> The WebIDE widget stuff really just adds a toolbar button in Firefox's UI if
> WebIDE was launched once. The code uses CustomizableUI.jsm (which I think is
> Firefox only).
> 
> For SM, you should be able to add the WebIDE button to the customization
> palette directly.
Thanks!

>> Whoa, this is mostly great! It doesn't seem to like private windows though.
> It's a mystery to me too. Devtools in Firefox private windows are working properly.
Devtools is totally broken in SeaMonkey private windows. The toolbox.xul is totally blank. Is there something special that we need to do for private windows?

Comment 15

[Tim Nguyen :ntim]

(In reply to Philip Chee from comment #14)
> >> Whoa, this is mostly great! It doesn't seem to like private windows though.
> > It's a mystery to me too. Devtools in Firefox private windows are working properly.
> Devtools is totally broken in SeaMonkey private windows. The toolbox.xul is
> totally blank. Is there something special that we need to do for private
> windows?

Do you have a try build ?

Comment 16

[Philip Chee]

Attachment: Dominator tree memory analysis

> Dominator trees give you fine-grained insight into memory retention.

Comment 17

[[e-mail]]

Comment on attachment 8708506 [details] [diff] [review]
Patch v3.0 Fix nits added remaining styles.

>+var ResponsiveUI = {
>+  toggle: () => {
>+    this.ResponsiveUIManager.toggle(window, getBrowser().selectedTab);
Because this is an arrow function, "this" is lexical - it's bound to the scope in which the function was declared (i.e. window). You need to switch this to function() { this....; } and similarly for the other tools.

>+html|*#gcli-tooltip-frame,
>+html|*#gcli-output-frame {
You didn't declare any @namespace in these files, so these rules won't work. You'll need a default and an html namespace.

The web console buttons are the wrong height in the Modern theme, because Modern defaults to large dropdown buttons like the Back and Forward buttons. In particular, the binding needs to be set back to the toolkit binding and the margin on the dropmarker needs to be removed. You're probably going to have to fix this in Modern's global.css as there doesn't seem to be anywhere more convenient.

Comment 19

[Philip Chee]

Attachment: Patch v4.0 Updated for changes in /mozilla/devtools/

>>+var ResponsiveUI = {
>>+  toggle: () => {
>>+    this.ResponsiveUIManager.toggle(window, getBrowser().selectedTab);
> Because this is an arrow function, "this" is lexical - it's bound to the 
> scope in which the function was declared (i.e. window). You need to switch 
> this to function() { this....; } and similarly for the other tools.
Fixed.

>>+html|*#gcli-tooltip-frame,
>>+html|*#gcli-output-frame {
> You didn't declare any @namespace in these files, so these rules won't work. 
> You'll need a default and an html namespace.
Fixed.

> The web console buttons are the wrong height in the Modern theme, because 
> Modern defaults to large dropdown buttons like the Back and Forward buttons. 
> In particular, the binding needs to be set back to the toolkit binding and 
> the margin on the dropmarker needs to be removed. You're probably going to 
> have to fix this in Modern's global.css as there doesn't seem to be anywhere 
> more convenient.
Working on this.

Updated for:
============
Bug 1241050 - Convert DeveloperToolbar.jsm to commonjs module
Updated nsContextMenu.js::inspectNode() due to:

Bug 1244120 - Enable browser_rules_content_02.js with e10s
[[Extracted a shared helper to open the browser context menu and choose
the 'inspect element' item. This helper works with e10s.
Adapted it a little bit so it waits for the right events in order to
make sure the inspector is ready.

This also involved modifying inspectNode in nsContextMenu.js to make it
wait until the node was selected and the node was ready.]]

-  inspectNode: function CM_inspectNode() {
+  inspectNode: function() {

Updated package-manifest.in for:
Bug 1248600 - Remove registerBrowserWindow/forgetBrowserWindow calls from /browser/. r=jryans
+@RESPATH@/components/devtools-startup.manifest
+@RESPATH@/components/devtools-startup.js

Bug 1248601 - Add the <xul:toolbar> of the Developer Toolbar dynamically

Comment 20

[Ian Neal]

Comment on attachment 8726728 [details] [diff] [review]
Patch v4.0 Updated for changes in /mozilla/devtools/

r/a=me for landing

Comment 21

[Philip Chee]

http://hg.mozilla.org/comm-central/rev/70c2c9d7851e

Comment 22

[Adrian Kalla [:adriank]]

Verified working with my local Linux c-c build (https://l10n.mozilla-community.org/~akalla/unofficial/seamonkey/nightly/latest-comm-central-linux64/)