381343 - Move SeaMonkey's debug/QA UI elements to be built as an extension in-tree

Status: VERIFIED FIXED

(SeaMonkey :: Build Config, defect)

Description

[Mark Banner (:standard8)]

We have various debug/QA elements that we don't need for releases, but tend to stay included anyway (just bits commented out).

Once suiterunner is available on trunk, we can move these elements out of the current areas where they are included into an extension built in-tree using overlays etc.

This would give us the following advantages:

- one item to disable for release builds/branches
- chrome files are easier to remove from the build (currently they just get left in) and hence our release download sizes will be slightly smaller
- an xpi could be made available for testers of release builds/branches to allow  them to use the Debug/QA menus and other areas that trunk has.

Comment 1

[Mark Banner (:standard8)]

Attachment: Proof of concept

Proof of concept patch. Moves the debug prefs panes to their own extension. All we'd need to disable for releases is the building of suite/debugQA.

Comment 2

[Robert Kaiser]

As we're using the "pre" postfixes on version starting with suiterunner, we could just magically go and not build that extension when the version number contains no "pre" string (perhaps, if in addition to that MOZILLA_OFFICIAL is set, so one can do a private build of a release tag and get the extension).
We could also go with some even more elaborate string comparison to even include it in alphas and betas (i.e. exclude it when no alphabetic characters are present, or the version matches [0-9\.]+ - or whatever Makefile magic allows us)

Comment 3

[Mark Banner (:standard8)]

Attachment: Patch v2

I think I'm ready to start asking reviews for this now. Note however, that in the unlikely even that we've completed reviewing this before the suiterunner transition, I'll only check it in after the transition on trunk.

Currently it only removes the debug items from navigatorOverlay.xul and prefs - still have /editor to do (and anything else we find/want to). I think its in the state now where we can get the basic structure in and that will simplify the later patches without adding too much complexity here.

Most of this patch is moving files to a new location for building in an extension. The rest is the extension setup and various overlays to make it work properly.

I'll attach a cvs moves files in a moment for the files that we need to move to go with this patch.

Comment 4

[Mark Banner (:standard8)]

Attachment: required cvs moves

Comment 5

[Mark Banner (:standard8)]

Comment on attachment 266099 [details] [diff] [review]
Patch v2

Robert, could you check you're happy from the l10n aspect please (I forgot to mention earlier I couldn't get the % substitution working correctly for the locale, so I left it as it was).

Also, I think we can leave sorting out the ifdef for DIRS = debugQA till later. This patch will be a big improvement on our current methods to disable debug code anyway.

Comment 6

[Robert Kaiser]

Comment on attachment 266099 [details] [diff] [review]
Patch v2


>Index: suite/debugQA/Makefile.in
>===================================================================

>+# We should really pull SEAMONKEY_VERSION from suite/config/version.txt
>+# but we can't be assured that we've even pulled those files.
>+# So we hardcode them.
>+XULAPP_DEFINES = \
>+    -DSEAMONKEY_VERSION=$(SEAMONKEY_VERSION) \
>+    $(NULL)

This comment sounds wrong. client.mk ensures that suite/config/version.txt is pulled and SEAMONKEY_VERSION is always read directly from there, so the actual XULAPP_DEFINES line is probably as correct as it can be, and the comment is useless.


>Index: suite/debugQA/jar.mn
>===================================================================
>+  locale/en-US/debugQA/debugQAPrefsOverlay.dtd            (locales/en-US/debugQAPrefsOverlay.dtd)
>+  locale/en-US/debugQA/pref-debug.dtd                     (locales/en-US/pref-debug.dtd)
>+  locale/en-US/debugQA/pref-debug1.dtd                    (locales/en-US/pref-debug1.dtd)
>+  locale/en-US/debugQA/pref-debug2.dtd                    (locales/en-US/pref-debug2.dtd)

I don't really like that.
I can tell you why the %-substitution doesn't work - you need to be in the jar.mn inside of locales/ for it to work correctly.

I'm still not sure how to do this best though. Hardcoding it to en-US looks hackish, but making it localizable makes builds go red if --enable-ui-locale is set to a locale that doesn't include a L10n for this.
It would be nice if we had a mechanism that could make L10n for such an extension optional at build time, just falling back to en-US if the whole extension is missing from the L10n dir and only breaking if it's present but incomplete.
I need to talk to Pike and bsmedberg if we find a usable solution for such cases.

I'm a bit undecided what to do about review here...

Comment 7

[Axel Hecht [:Pike]]

For make-jars.pl to do the right thing for %, you need to have -c given. That happens in config.mk, if LOCALE_SRCDIR is defined.
config.mk does that if relativesrcdir is defined, which may or may not be the right thing to do here. Just going for LOCALE_SRCDIR might be good enough.

Comment 8

[Mark Banner (:standard8)]

(In reply to comment #6)
> (From update of attachment 266099 [details] [diff] [review])
> >Index: suite/debugQA/Makefile.in
> >===================================================================
> 
> >+# We should really pull SEAMONKEY_VERSION from suite/config/version.txt
> >+# but we can't be assured that we've even pulled those files.
> >+# So we hardcode them.
> >+XULAPP_DEFINES = \
> >+    -DSEAMONKEY_VERSION=$(SEAMONKEY_VERSION) \
> >+    $(NULL)
> 
> This comment sounds wrong. client.mk ensures that suite/config/version.txt is
> pulled and SEAMONKEY_VERSION is always read directly from there, so the actual
> XULAPP_DEFINES line is probably as correct as it can be, and the comment is
> useless.

Yeah, I was following what's said everywhere else which is probably wrong now as well.

Comment 9

[Mark Banner (:standard8)]

Attachment: Patch v3 (checked in)

Fixes locales jar.mn issue. I ended up moving this jar.mn into a locales folder as relativesrcdir seemed to mess up the non-localized files.

cvs moves are still the same.

Comment 10

[Robert Kaiser]

Comment on attachment 267197 [details] [diff] [review]
Patch v3 (checked in)

This looks good to me now from an L10n POV (I reviewed it only from looking at the patch and looking at the locale stuff - I'll leave the rest to ajschult), with one small comment:

>Index: suite/debugQA/locales/Makefile.in
>===================================================================
>+DEFINES += -DAB_CD=$(AB_CD)

I think we probably want to hardcode this to en-US for the moment, along with an according comment that we don't want to force localizers to provide L10n for this extension, even though it is theoretically localizable, and we can easily make it localizable by setting this to $(AB_CD) once we have the infrastructure to make L10n optional for built-in extensions like this one.

Comment 11

[Mark Banner (:standard8)]

(In reply to comment #10)
> (From update of attachment 267197 [details] [diff] [review])
> with one small comment:
> >Index: suite/debugQA/locales/Makefile.in
> >===================================================================
> >+DEFINES += -DAB_CD=$(AB_CD)
> I think we probably want to hardcode this to en-US for the moment, along with
> an according comment that we don't want to force localizers to provide L10n for
> this extension ...

Agreed, I'll either do that at check-in or next update depending on ajschult's comments.

Comment 12

[Andrew Schultz]

Comment on attachment 267197 [details] [diff] [review]
Patch v3 (checked in)

This works and looks good.

I noticed that debugQA doesn't show up in the list of add-ons while DOM Inspector does.  Should it?

Comment 13

[Andrew Schultz]

I guess the packaging list for the windows installer also needs updating

Comment 14

[Mark Banner (:standard8)]

(In reply to comment #12)
> (From update of attachment 267197 [details] [diff] [review])
> This works and looks good.
> I noticed that debugQA doesn't show up in the list of add-ons while DOM
> Inspector does.  Should it?

It should be in there. Otherwise (in theory) it isn't working...

Comment 15

[Mark Banner (:standard8)]

Comment on attachment 267197 [details] [diff] [review]
Patch v3 (checked in)

I'll sort out a windows packaging list change in a separate patch. Requesting sr.

Comment 16

[Andrew Schultz]

> It should be in there. Otherwise (in theory) it isn't working...

Hmm.  Yes, it's there now.

Comment 17

[[e-mail]]

Comment on attachment 267197 [details] [diff] [review]
Patch v3 (checked in)

Is it worth doing L10 for 3 entities? Or maybe leave them in the pref dtd?

Comment 18

[Mark Banner (:standard8)]

(In reply to comment #17)
> (From update of attachment 267197 [details] [diff] [review])
> Is it worth doing L10 for 3 entities? Or maybe leave them in the pref dtd?
> 
I think given I've already done the patch for it, and there would be the (small) risk of someone looking at a release version assuming they aren't used and removing them, we may as well just move those entities. We can always expand that file later if we move some of the other stuff to l10n.

Comment 19

[Robert Kaiser]

We have more L10n as we have the moved files as well :)

Comment 20

[Mark Banner (:standard8)]

I've just landed the files that are new for this. I'll land the rest once the tree is open and I've got the packaging file patch ready.

Comment 21

[[e-mail]]

(In reply to comment #19)
>We have more L10n as we have the moved files as well :)
Ah, I didn't spot those, thanks.

Comment 22

[Mark Banner (:standard8)]

Attachment: Patch v3a - left to check in.

Just in case anyone wants to build with this, this is what's left of Patch v3a to check in (pending installer additions & cvs moves).

Comment 23

[Mark Banner (:standard8)]

Attachment: Add debug QA packages to the installer.

Comment 24

[Mark Banner (:standard8)]

Comment on attachment 269227 [details] [diff] [review]
Add debug QA packages to the installer.

Get a single match address...

I forgot to say last time, I haven't tested this but I believe it should work. Use Patch 3a if you want to check with the rest of the debuQA fully in place.

Don't know if we should also check what happens if debugQA isn't build/present in the output - I guess it'll just ignore it?

Comment 25

[Mark Banner (:standard8)]

Comment on attachment 269227 [details] [diff] [review]
Add debug QA packages to the installer.

I'm going to be doing a patch which allows optional installation of the debugQA and PalmSync in the windows installer, so we may as well leave this out for now.

Comment 26

[Mark Banner (:standard8)]

Attachment: Add debugQA to windows installer and move palmsync into its own section (checked in)

I've not had much success yet in getting the installer working for optional install of palmsync and debugQA (bug 385377). Therefore in the short term I'd like to land this which will update the packaging scripts for debugQA (and start to prepare a palmsync section for bug 385377).

This will allow me to get the rest of the debugQA code checked in (= complete the cvs moves), especially now the cvs copies are done.

Comment 27

[Mark Banner (:standard8)]

Attachment: Part 2 - move Composer debug options to debugQA extension

This is the next patch I've been working on, but I won't submit it for review until we've got the first debugQA fully in on the trunk.

This will move the debug options from editor/ui (composer) into the debugQA extension.

TextEditorAppShell.xul can possibly move completely as well, because that's only available in a debug menu. Any opinions on this would be welcomed.

Comment 28

[Robert Kaiser]

Regarding
http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/suite/locales/en-US/chrome/browser/navigator.dtd&rev=1.180&mark=6-9 I'm told that Firefox' Nightly Tester Tools extension adds "(Build 2007062404)" to the window title, so it's probably something we want to look into.

We might even want to "steal" more parts of that extension or somehow integrate with it, I guess. Its source is at http://svn.blueprintit.co.uk/viewvc/mozilla/firefox/buildid/trunk/

Comment 29

[Robert Kaiser]

<Mossop> NTT should already install on suiterunner though it doesnt attempt to set the title. But that could be changed
<Mossop> All it really does is change the titlemodifier attribute then tell firefox to update the titlebar

Comment 30

[Frank Wein [:mcsmurf]]

Comment on attachment 269493 [details] [diff] [review]
Add debugQA to windows installer and move palmsync into its own section (checked in)

Looks good.

Comment 31

[Robert Kaiser]

Attachment: correct locale override (checked in)

In my locale build with --enable-ui-locale=de I found out that our locale override doesn't work as expected yet. With a little help from Pike, I managed to make it work as expected. I also added comments so we know why those lines are there.

Comment 32

[Mark Banner (:standard8)]

Comment on attachment 270171 [details] [diff] [review]
correct locale override (checked in)

According to http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/config/config.mk&rev=3.374&mark=56-58#55 I think you should be able to remove the include of autoconf.mk. So r=me if you drop that line as well.

Comment 33

[Robert Kaiser]

Comment on attachment 270171 [details] [diff] [review]
correct locale override (checked in)

I checked this in with autoconf.mk removed - nice catch!

Comment 34

[[e-mail]]

(In reply to comment #29)
>All it really does is change the titlemodifier attribute
Which should be trivial to do with a preprocessed overlay (or dtd).

Comment 35

[Mark Banner (:standard8)]

Attachment: Part 3 - move Build Id in titlebar to debugQA extension

I've skipped part 2 for the time being - build ID is broken on trunk so that takes priority.

As we've no longer got the build id when we generate the files, I've written some js code that sets up the title modifier appropriately for the window.

Comment 36

[[e-mail]]

(In reply to comment #35)
>As we've no longer got the build id when we generate the files
Not true. Simply copy the GRE_BUILDID code from suite/app/Makefile.in

Comment 37

[[e-mail]]

Attachment: Fixes build id but doesn't move it

Comment 38

[Mark Banner (:standard8)]

Comment on attachment 272389 [details] [diff] [review]
Part 3 - move Build Id in titlebar to debugQA extension

Per discussion on irc - the extension may be moved (or even released separately) between builds, hence we can't hard-code the build id into the extension as per Neil's suggestion.

Comment 39

[[e-mail]]

Comment on attachment 272389 [details] [diff] [review]
Part 3 - move Build Id in titlebar to debugQA extension

>RCS file: /cvsroot/mozilla/suite/browser/hiddenWindow.xul,v
We don't need to overlay this, its title is never used.

> <!-- LOCALIZATION NOTE (mainWindow.title): DONT_TRANSLATE --> 
>-<!ENTITY mainWindow.title "&brandShortName; {&buildId.label;}"> 
>+<!ENTITY mainWindow.title "&brandShortName;">
> <!-- LOCALIZATION NOTE (mainWindow.titlemodifier) : DONT_TRANSLATE --> 
>-<!ENTITY mainWindow.titlemodifier "&brandShortName; {&buildId.label;}"> 
>+<!ENTITY mainWindow.titlemodifier "&brandShortName;">
> <!-- LOCALIZATION NOTE (mainWindow.titlemodifiermenuseparator): DONT_TRANSLATE -->
> <!ENTITY mainWindow.titlemodifiermenuseparator " - ">
Given the notes, I wonder why we still bother with these...

>+    window.addEventListener("load", debugQABuildIDOnLoad, true);
But you never remove it, so this runs on every subframe load...

>+      var appInfo = Components.classes["@mozilla.org/xre/app-info;1"]
>+                            .getService(Components.interfaces.nsIXULAppInfo);
Nit: .s don't line up.

>+      // Set the new title modifier
>+      document.documentElement.setAttribute("titlemodifier",
>+        bundle.getFormattedString("titlemodifier",
>+                                  [appInfo.name, appInfo.appBuildID]));
>+
>+      // Now ensure we update the title bar
>+      if (gBrowser)
>+        gBrowser.updateTitlebar();
This isn't nececessary, at least the first time this runs (see above), because at this point nothing will have loaded in the browser yet, so you can simply set both the document title and the title modifier at the same time.

>+  <window id="main-window">
>+    <stringbundle id="debugQANavigatorBundle"
>+                  src="chrome://debugQA/locale/debugQANavigatorOverlay.properties"/>
Please overlay the existing stringbundleset.

>+<!-- LOCALIZATION NOTE: the %S are replaced by app name and build ID respectively -->
>+titlemodifier=%S {Build Id: %S}
The note contains the correct case of ID...

Comment 40

[Mark Banner (:standard8)]

(In reply to comment #39)
> (From update of attachment 272389 [details] [diff] [review])
> > <!-- LOCALIZATION NOTE (mainWindow.title): DONT_TRANSLATE --> 
> >-<!ENTITY mainWindow.title "&brandShortName; {&buildId.label;}"> 
> >+<!ENTITY mainWindow.title "&brandShortName;">
> > <!-- LOCALIZATION NOTE (mainWindow.titlemodifier) : DONT_TRANSLATE --> 
> >-<!ENTITY mainWindow.titlemodifier "&brandShortName; {&buildId.label;}"> 
> >+<!ENTITY mainWindow.titlemodifier "&brandShortName;">
> > <!-- LOCALIZATION NOTE (mainWindow.titlemodifiermenuseparator): DONT_TRANSLATE -->
> > <!ENTITY mainWindow.titlemodifiermenuseparator " - ">
> Given the notes, I wonder why we still bother with these...

I've just checked in l10n repository. Apparently the titlemodifiermenuseparator is translated by the "lt" translation. So I think I'll leave that as an entity and remove the rest.

Comment 41

[Robert Kaiser]

IIRC, this is all in L10n because it's easier for rebranded distributions to change this way. I think Firefox also has it in L10n and we probably should just leave it there IMHO.

Comment 42

[Mark Banner (:standard8)]

(In reply to comment #39)
> (From update of attachment 272389 [details] [diff] [review])
> >+      // Set the new title modifier
> >+      document.documentElement.setAttribute("titlemodifier",
> >+        bundle.getFormattedString("titlemodifier",
> >+                                  [appInfo.name, appInfo.appBuildID]));
> >+
> >+      // Now ensure we update the title bar
> >+      if (gBrowser)
> >+        gBrowser.updateTitlebar();
> This isn't nececessary, at least the first time this runs (see above), because
> at this point nothing will have loaded in the browser yet, so you can simply
> set both the document title and the title modifier at the same time.

Actually I've just realised - it is necessary. Otherwise if you start up with "./seamonkey -url about:blank" then the title bar will still show just SeaMonkey if you don't have the updateTitlebar call in.

Comment 43

[Mark Banner (:standard8)]

Attachment: Part 3 v2 - move Build Id in titlebar to debugQA extension

Addressed comments, but please also see comment 40 and 42.

Comment 44

[[e-mail]]

Comment on attachment 274173 [details] [diff] [review]
Part 3 v2 - move Build Id in titlebar to debugQA extension

* KaiRo wants to keep the title entities
* Missing diffs for build.dtd
* Unnecessary to overlay <window>
* updateTitleBar not replaced with document.title

Comment 45

[Mark Banner (:standard8)]

Attachment: Part 3 v3 - move Build Id in titlebar to debugQA extension (checked in)

Incorporates all the comments, and removes the old build.dtd inclusions in navigator.xul/hiddenWindow.xul.

Comment 46

[[e-mail]]

Comment on attachment 274268 [details] [diff] [review]
Part 3 v3 - move Build Id in titlebar to debugQA extension (checked in)

>+      document.title = titlemodifier
Nit: missing semicolon

Comment 47

[Mark Banner (:standard8)]

Attachment: Change homepage location to point to the wiki (checked in)

I've set up a page on the wiki giving some brief information about the extension. I think the extension should point to that rather than the SeaMonkey main homepage.

Comment 48

[Mark Banner (:standard8)]

Attachment: Part 2 - move composer options (cvs moves)

This moves some of the Composer files that are debug-only to debugqa.

Comment 49

[Mark Banner (:standard8)]

Attachment: Part 2 - move composer debug options to debugQA

Patch to move the composer options to debugQA - note apply the moves first, then the patch.

Also note that the plain text editor appears broken in currently builds even without this patch. So I propose that we fix that under a different bug if there isn't one already (I haven't searched yet).

Comment 50

[Robert Kaiser]

Comment on attachment 277311 [details] [diff] [review]
Part 2 - move composer options (cvs moves)

looks good. r=me

Comment 51

[[e-mail]]

Comment on attachment 277311 [details] [diff] [review]
Part 2 - move composer options (cvs moves)

Why rename the files?

Comment 52

[Mark Banner (:standard8)]

(In reply to comment #51)
> (From update of attachment 277311 [details] [diff] [review])
> Why rename the files?
> 
I did it mainly so they fit better with the extension, I don't think there's any reason we have to do it.

Comment 53

[[e-mail]]

Comment on attachment 277312 [details] [diff] [review]
Part 2 - move composer debug options to debugQA

>+% overlay chrome://debugqa/content/debugQATextEditorShell.xul chrome://debugqa/content/debugQAEditorOverlay.xul
You don't need this, just add a xul-overlay PI to the xul directly.

>+    <menu id="debugMenu" label="&debugMenu.label;" insertafter="windowMenu,tasksMenu">
I think I'd prefer insertbefore="menu_Help" which saves you from having to mention two menus.

Comment 54

[Mark Banner (:standard8)]

Attachment: Part 2 - move composer debug options to debugQA v2 (checked in)

Updated patch to resolve any bit rot from the cvs copies and fix Neil's comments. Carrying forward sr.

Comment 55

[Robert Kaiser]

Comment on attachment 277946 [details] [diff] [review]
Part 2 - move composer debug options to debugQA v2 (checked in)

OK, the one issue I saw works now. I don't see anything happening on most menu entries in the debug menu of composer, but I guess either I don't know how to use them or they didn't work before the patch as well.
The patch itself looks good, so r=me.

Comment 56

[Mark Banner (:standard8)]

I believe all the Debug/QA UI that used to be disabled manually, is now built into the debugQA extension.

Although there are some bugs in the code that's been moved I believe they weren't caused by this bug, this bug is essentially fixed.

Therefore marking as fixed. Other issues/problems with the extension can be raised and dealt with in separate bugs.