241079 - DOM Inspector: fix it; remove Tools, Window, Help menus, Preferences, and Quit keybinding

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Steffen Wilberg]

DOM inspector still has some unneeded and partly broken menus from SeaMonkey.
I'd like to rip the Tools, Window and Help menus altogether.
The "Preferences" (!) in the Edit (!) menu doesn't work. I don't think we need a
UI for these prefs anyway, so let's remove that as well. If anyone really needs
to modify these prefs, he can go to about:config and filter for "inspector".

Comment 1

[Steffen Wilberg]

Attachment: patch

Comment 2

[Steffen Wilberg]

Attachment: diff between original and forked files

I had to fork toolboxOverlay.xul and popupOverlay.xul. This is what I did to
the forked files.

Comment 3

[Steffen Wilberg]

Comment on attachment 146585 [details] [diff] [review]
patch

Ben, this is some cleanup in DOM inspector.
Have a look at the diff I attached to see how I modified the files I had to
fork.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

We really have to remove the clutter out of the menu. A patch for this is
available here and only needs a (s)r for checkin. 

Blocking 0.9?

Comment 5

[Steffen Wilberg]

Attachment: also remove the quit keybinding

This patch removes the keybinding from File->Quit (Ctrl-Q) as well. This was
done by forking inspector.xul and removing this line:
<key id="key_quit"/>

Comment 6

[Steffen Wilberg]

Comment on attachment 147888 [details] [diff] [review]
also remove the quit keybinding

Ben, this patch removes the Tools, Window and Help menus, as well as the
Preferences and the File->Quit keybinding from DOMi.

Comment 7

[Mike Connor [:mconnor]]

not a 0.9 blocker, plussing for 1.0 to get on the review radar

Comment 8

[Steffen Wilberg]

Attachment: also fix domi

This patch makes domi run again!

Comment 9

[Steffen Wilberg]

Attachment: diff -uw between original and forked files

This is how I modified the forked files.

Comment 10

[Steffen Wilberg]

Comment on attachment 148298 [details] [diff] [review]
diff -uw between original and forked files

Explanation:

>--- mozilla/extensions/inspector/resources/content/inspector.xul	2004-05-12 12:18:40.000000000 +0200
>-  <!ENTITY % dtd3 SYSTEM "chrome://communicator-platform/locale/platformCommunicatorOverlay.dtd"> %dtd3;
>+  <!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd"> %browserDTD;
platformCommunicatorOverlay.dtd is gone. We only need it for &closeCmd.key
(close Inspector), which is provided by browser.dtd as well.

>-<?xul-overlay href="chrome://communicator/content/utilityOverlay.xul"?>
This is only needed for the Quit keybinding which Ben wanted to be removed (bug
242911).

>-<?xul-overlay href="chrome://communicator/content/tasksOverlay.xul"?>
This is only needed for the Tools and Window menu, which have to go as well
(bug 242911).

Comment 11

[Steffen Wilberg]

Comment on attachment 148297 [details] [diff] [review]
also fix domi

Mike, Ben, can one of you please have a look? See comment 10 for explanation.

Comment 12

[Mike Connor [:mconnor]]

We need to figure something out wrt to making prefs work, too.

if we're forking inspector.xul how close are we to completely forking DOMi?

will look at this later today, hopefully

Comment 13

[Steffen Wilberg]

(In reply to comment #12)
> We need to figure something out wrt to making prefs work, too.
Inspector prefs UI has never worked in Firefox. We first need a concept
- if we need UI for these prefs at all,
- where these prefs should appear: I guess the main options dialog, but where
exactly? Additionally, a link from the inspector window.
I'd suggest to leave that to a separate bug.

> if we're forking inspector.xul how close are we to completely forking DOMi?
There are still a lot of files in mozilla/extensions/inspector/resources/content
which we didn't fork yet.

Comment 14

[PikeUK]

For the prefs, how about forking
chrome://inspector/content/prefs/pref-inspector.xul to make it a standalone
dialog then adding it as the optionsURL in Inspector's install.rdf so that it
can be accessed like the options box of any other extension?

Comment 15

[PikeUK]

See Bug 225644 for a pre-existing patch that does what I suggested in my last
comment, except it creates a wrapper for the file rather than forking it (looks
like it needs to be updated for the new extension system though).

Comment 16

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 148298 [details] [diff] [review]
diff -uw between original and forked files

>-  <!ENTITY % dtd3 SYSTEM "chrome://communicator-platform/locale/platformCommunicatorOverlay.dtd"> %dtd3;
>+  <!ENTITY % browserDTD SYSTEM "chrome://browser/locale/browser.dtd"> %browserDTD;

This seems excessive... browser.dtd contains a lot of stuff that inspector
doesn't need. Sounds like browser needs to have these items abstracted into a
file with a name something like "platformMenus.dtd". I won't die if it stays
this way though. 

The solution for preferences I think is to fix Inspector's Options UI to be a
standalone dialog (as mandated by the new Extension Manager) and have it be
opened by the appropriate menu item and the EM itself. 

Overall, I say if this patch works, let's use it - it's probably an acceptable
fix for the 1.0 branch, and in the interim on the trunk. 

I want to say that the dependencies of inspector on communicator and other
Seamonkey components is completely bogus. It's one thing for an extension to
overlay into sections of Seamonkey UI, it's another thing to rely on it to
exist. As Seamonkey slowly shuffles off on its way towards retirement Inspector
should become more autonomous.

Comment 17

[Ben Goodger (use ben at mozilla dot org for email)]

Summary: I'm fine with this patch if Mike is and it fixes inspector.

Comment 18

[Mike Connor [:mconnor]]

Comment on attachment 148297 [details] [diff] [review]
also fix domi

works fine, for now.  Ultimately what ben brought up needs to be addressed, but
this works without an issue, will land this today.

Comment 19

[Mike Connor [:mconnor]]

checked in branch and trunk, twiddle blocking flags since this really became a
"fix DOMi" patch

we should really file a followup bug about DOMi and its bogus dependencies,
forking sucks.

thanks Steffen!

Comment 20

[Steffen Wilberg]

*** Bug 239559 has been marked as a duplicate of this bug. ***

Comment 21

[Christopher Aillon (sabbatical, not receiving bugmail)]

It would *really* have been nice if the DOM Inspector folks were informed of this...

Comment 22

[Bill Mason]

Verified trunk 20040523 PC/WinXP

Comment 23

[Ryan Polk (Quark)]

*** Bug 240930 has been marked as a duplicate of this bug. ***

Comment 24

[Ryan Polk (Quark)]

*** Bug 242748 has been marked as a duplicate of this bug. ***