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

VERIFIED FIXED

Status

SeaMonkey
Build Config
VERIFIED FIXED
10 years ago
5 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 9 obsolete attachments)

605 bytes, patch
Andrew Schultz
: review+
Details | Diff | Splinter Review
45.16 KB, patch
Andrew Schultz
: review+
Robert Kaiser
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
1.91 KB, patch
mcsmurf
: review+
Details | Diff | Splinter Review
850 bytes, patch
standard8
: review+
Details | Diff | Splinter Review
9.54 KB, patch
neil@parkwaycc.co.uk
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
731 bytes, patch
neil@parkwaycc.co.uk
: review+
Details | Diff | Splinter Review
447 bytes, patch
Robert Kaiser
: review+
neil@parkwaycc.co.uk
: superreview+
Details | Diff | Splinter Review
26.60 KB, patch
Robert Kaiser
: review+
standard8
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

10 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 265437 [details] [diff] [review]
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

10 years ago
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)
(Assignee)

Comment 3

10 years ago
Created attachment 266099 [details] [diff] [review]
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.
Attachment #265437 - Attachment is obsolete: true
Attachment #266099 - Flags: review?(ajschult)
(Assignee)

Comment 4

10 years ago
Created attachment 266100 [details] [diff] [review]
required cvs moves
Attachment #266100 - Flags: review?(ajschult)
(Assignee)

Comment 5

10 years ago
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.
Attachment #266099 - Flags: review?(kairo)
(Assignee)

Updated

10 years ago
Blocks: 59655

Comment 6

10 years ago
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...
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.
(Assignee)

Comment 8

10 years ago
(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.
(Assignee)

Comment 9

10 years ago
Created attachment 267197 [details] [diff] [review]
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.
Attachment #266099 - Attachment is obsolete: true
Attachment #267197 - Flags: review?(kairo)
Attachment #267197 - Flags: review?(ajschult)
Attachment #266099 - Flags: review?(kairo)
Attachment #266099 - Flags: review?(ajschult)
(Assignee)

Updated

10 years ago
Attachment #266100 - Attachment description: required cvs movies → required cvs moves

Comment 10

10 years ago
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.
Attachment #267197 - Flags: review?(kairo) → review+
(Assignee)

Comment 11

10 years ago
(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

10 years ago
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?
Attachment #267197 - Flags: review?(ajschult) → review+

Updated

10 years ago
Attachment #266100 - Flags: review?(ajschult) → review+

Comment 13

10 years ago
I guess the packaging list for the windows installer also needs updating
(Assignee)

Comment 14

10 years ago
(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...
(Assignee)

Comment 15

10 years ago
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.
Attachment #267197 - Flags: superreview?(neil)

Comment 16

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

Hmm.  Yes, it's there now.

Comment 17

10 years ago
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?
Attachment #267197 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 18

10 years ago
(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.
(Assignee)

Updated

10 years ago
Depends on: 383737

Comment 19

10 years ago
We have more L10n as we have the moved files as well :)
(Assignee)

Comment 20

10 years ago
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

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

Comment 22

10 years ago
Created attachment 269226 [details] [diff] [review]
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).
(Assignee)

Comment 23

10 years ago
Created attachment 269227 [details] [diff] [review]
Add debug QA packages to the installer.
Attachment #269227 - Flags: review?
(Assignee)

Comment 24

10 years ago
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?
Attachment #269227 - Flags: review? → review?(bugzilla)
(Assignee)

Comment 25

10 years ago
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.
Attachment #269227 - Attachment is obsolete: true
Attachment #269227 - Flags: review?(bugzilla)
(Assignee)

Updated

10 years ago
Depends on: 385377
(Assignee)

Comment 26

10 years ago
Created attachment 269493 [details] [diff] [review]
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.
Attachment #269493 - Flags: review?(bugzilla)
(Assignee)

Comment 27

10 years ago
Created attachment 269530 [details] [diff] [review]
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

10 years ago
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

10 years ago
<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 on attachment 269493 [details] [diff] [review]
Add debugQA to windows installer and move palmsync into its own section (checked in)

Looks good.
Attachment #269493 - Flags: review?(bugzilla) → review+
(Assignee)

Updated

10 years ago
Blocks: 385377
No longer depends on: 385377
(Assignee)

Updated

10 years ago
Attachment #269226 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #267197 - Attachment description: Patch v3 → Patch v3 (checked in)
(Assignee)

Updated

10 years ago
Attachment #269493 - Attachment description: Add debugQA to windows installer and move palmsync into its own section → Add debugQA to windows installer and move palmsync into its own section (checked in)

Comment 31

10 years ago
Created attachment 270171 [details] [diff] [review]
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.
Attachment #270171 - Flags: review?(bugzilla)
(Assignee)

Comment 32

10 years ago
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.
Attachment #270171 - Flags: review?(bugzilla) → review+

Comment 33

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

I checked this in with autoconf.mk removed - nice catch!
Attachment #270171 - Attachment description: correct locale override → correct locale override (checked in)

Comment 34

10 years ago
(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).
(Assignee)

Updated

10 years ago
Depends on: 386965
(Assignee)

Updated

10 years ago
Blocks: 387050
(Assignee)

Updated

10 years ago
Depends on: 386463
(Assignee)

Comment 35

10 years ago
Created attachment 272389 [details] [diff] [review]
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.
Attachment #272389 - Flags: superreview?(neil)
Attachment #272389 - Flags: review?(neil)

Comment 36

10 years ago
(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

10 years ago
Created attachment 273381 [details] [diff] [review]
Fixes build id but doesn't move it
Attachment #272389 - Attachment is obsolete: true
Attachment #272389 - Flags: superreview?(neil)
Attachment #272389 - Flags: review?(neil)
(Assignee)

Comment 38

10 years ago
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.
Attachment #272389 - Attachment is obsolete: false
Attachment #272389 - Flags: superreview?(neil)
Attachment #272389 - Flags: review?(neil)
(Assignee)

Updated

10 years ago
Attachment #273381 - Attachment is obsolete: true

Comment 39

10 years ago
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...
Attachment #272389 - Flags: superreview?(neil)
Attachment #272389 - Flags: superreview-
Attachment #272389 - Flags: review?(neil)
Attachment #272389 - Flags: review+
(Assignee)

Comment 40

10 years ago
(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

10 years ago
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.
(Assignee)

Comment 42

10 years ago
(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.
(Assignee)

Comment 43

10 years ago
Created attachment 274173 [details] [diff] [review]
Part 3 v2 - move Build Id in titlebar to debugQA extension

Addressed comments, but please also see comment 40 and 42.
Attachment #274173 - Flags: superreview?(neil)
Attachment #274173 - Flags: review?(neil)

Comment 44

10 years ago
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
Attachment #274173 - Flags: superreview?(neil)
Attachment #274173 - Flags: superreview-
Attachment #274173 - Flags: review?(neil)
(Assignee)

Comment 45

10 years ago
Created attachment 274268 [details] [diff] [review]
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.
Attachment #274268 - Flags: superreview?(neil)
Attachment #274268 - Flags: review?(neil)

Comment 46

10 years ago
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
Attachment #274268 - Flags: superreview?(neil)
Attachment #274268 - Flags: superreview+
Attachment #274268 - Flags: review?(neil)
Attachment #274268 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #274268 - Attachment description: Part 3 v3 - move Build Id in titlebar to debugQA extension → Part 3 v3 - move Build Id in titlebar to debugQA extension (checked in)
(Assignee)

Comment 47

10 years ago
Created attachment 276276 [details] [diff] [review]
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.
Attachment #276276 - Flags: review?(neil)

Updated

10 years ago
Attachment #276276 - Flags: review?(neil) → review+
(Assignee)

Updated

10 years ago
Attachment #276276 - Attachment description: Change homepage location to point to the wiki → Change homepage location to point to the wiki (checked in)
(Assignee)

Comment 48

10 years ago
Created attachment 277311 [details] [diff] [review]
Part 2 - move composer options (cvs moves)

This moves some of the Composer files that are debug-only to debugqa.
Attachment #277311 - Flags: superreview?(neil)
Attachment #277311 - Flags: review?(kairo)
(Assignee)

Comment 49

10 years ago
Created attachment 277312 [details] [diff] [review]
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).
Attachment #269530 - Attachment is obsolete: true
Attachment #277312 - Flags: superreview?(neil)
Attachment #277312 - Flags: review?(kairo)

Comment 50

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

looks good. r=me
Attachment #277311 - Flags: review?(kairo) → review+

Comment 51

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

Why rename the files?
(Assignee)

Comment 52

10 years ago
(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

10 years ago
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.
Attachment #277312 - Flags: superreview?(neil) → superreview+

Updated

10 years ago
Attachment #277311 - Flags: superreview?(neil) → superreview+
(Assignee)

Updated

10 years ago
Depends on: 392941
(Assignee)

Comment 54

10 years ago
Created attachment 277946 [details] [diff] [review]
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.
Attachment #272389 - Attachment is obsolete: true
Attachment #274173 - Attachment is obsolete: true
Attachment #277312 - Attachment is obsolete: true
Attachment #277946 - Flags: superreview+
Attachment #277946 - Flags: review?(kairo)
Attachment #277312 - Flags: review?(kairo)

Comment 55

10 years ago
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.
Attachment #277946 - Flags: review?(kairo) → review+
(Assignee)

Updated

10 years ago
Attachment #277946 - Attachment description: Part 2 - move composer debug options to debugQA v2 → Part 2 - move composer debug options to debugQA v2 (checked in)
(Assignee)

Comment 56

10 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED

Updated

9 years ago
Duplicate of this bug: 59655

Updated

9 years ago
Duplicate of this bug: 103144

Updated

8 years ago
Status: RESOLVED → VERIFIED

Updated

5 years ago
Depends on: 782243
You need to log in before you can comment on or make changes to this bug.