Closed Bug 1223341 Opened 4 years ago Closed 4 years ago

Add the Firefox Devtools to the SeaMonkey UI

Categories

(SeaMonkey :: General, defect)

defect
Not set

Tracking

(seamonkey2.45 fixed, seamonkey2.46 fixed)

VERIFIED FIXED
seamonkey2.45
Tracking Status
seamonkey2.45 --- fixed
seamonkey2.46 --- fixed

People

(Reporter: philip.chee, Assigned: philip.chee)

References

(Blocks 2 open bugs)

Details

(Keywords: dev-doc-needed)

Attachments

(3 files, 4 obsolete files)

Make the Firefox Devtools show up and work in SeaMonkey
Blocks: 1223344
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)
Duplicate of this bug: 574955
Duplicate of this bug: 760799
No longer blocks: 1223344
Depends on: 1022354, 1223344
Attachment #8685398 - Attachment description: Patch v1.0 add glue code for the Firefox Devtools. → Patch 0.1WIP add glue code for the Firefox Devtools.
Attached patch Patch v1.0 ready for review (obsolete) — Splinter 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.
Attachment #8685398 - Attachment is obsolete: true
Attachment #8691242 - Flags: review?(neil)
Depends on: 1226570
> 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.
Attachment #8691242 - Attachment is obsolete: true
Attachment #8691242 - Flags: review?(neil)
Attachment #8703186 - Flags: review?(neil)
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.)
Attachment #8703186 - Flags: review?(neil) → feedback+
(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.
(In reply to neil@parkwaycc.co.uk 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 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.
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 neil@parkwaycc.co.uk 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.
Attachment #8703186 - Attachment is obsolete: true
Attachment #8708506 - Flags: review?(neil)
(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?
Flags: needinfo?(ntim.bugs)
(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 ?
Flags: needinfo?(ntim.bugs) → needinfo?(philip.chee)
> Dominator trees give you fine-grained insight into memory retention.
Flags: needinfo?(philip.chee)
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.
Attachment #8708506 - Flags: review?(neil) → review-
Duplicate of this bug: 1243308
>>+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
Attachment #8708506 - Attachment is obsolete: true
Attachment #8726728 - Flags: review?(neil)
Comment on attachment 8726728 [details] [diff] [review]
Patch v4.0 Updated for changes in /mozilla/devtools/

r/a=me for landing
Attachment #8726728 - Flags: review+
http://hg.mozilla.org/comm-central/rev/70c2c9d7851e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.45
Verified working with my local Linux c-c build (https://l10n.mozilla-community.org/~akalla/unofficial/seamonkey/nightly/latest-comm-central-linux64/)
Status: RESOLVED → VERIFIED
See Also: → 1263925
See Also: → 1265485
Blocks: 1273222
See Also: → 1243550
See Also: → 1284394
See Also: → 1284504
You need to log in before you can comment on or make changes to this bug.