576869 - Make venkman XPCOM components use new manifests and data tables

Status: RESOLVED FIXED

(Other Applications Graveyard :: Venkman JS Debugger, defect)

Description

[Ian Neal]

Bug 568691 has changed XPCOM registration a lot, venkman is now broken and when trying to compile you get:
mozilla/config/rules.mk:1822: *** .js component without matching .manifest.  Stop.
In the extensions/venkman directory.

Comment 1

[Justin Wood (:Callek)]

Of note for this bug, is to be cautious of what observers venkman currently registers for. as not all of them are valid anymore for extensions.

Comment 2

[Ian Neal]

(In reply to comment #1)
> Of note for this bug, is to be cautious of what observers venkman currently
> registers for. as not all of them are valid anymore for extensions.

See https://developer.mozilla.org/en/Observer_Notifications for details

Comment 4

[[e-mail]]

Attachment: Minimal fix

As per bug 576745 this is a patch that makes the minimum number of changes to make the command line handler and source view etc. work on trunk. Note that I noticed that Venkman wasn't registering its command line handler ideally on pre-trunk toolkit builds so I rolled that fix in to the patch too.

Comment 5

[[e-mail]]

Comment on attachment 457084 [details] [diff] [review]
Minimal fix

>+% category command-line-handler m-venkman @mozilla.org/commandlinehandler/general-startup;1?type=venkman
Perhaps this should be a-venkman to make this execute as early in startup as possible?

Comment 6

[[e-mail]]

(In reply to comment #5)
>>+% category command-line-handler m-venkman @mozilla.org/commandlinehandler/general-startup;1?type=venkman
>Perhaps this should be a-venkman to make this execute as early in startup as possible?
Although profile-after-change observers would probably get notified first... would we be better off adding code to nsSuiteGlue.js to do this on app startup, or perhaps even C++ code to do this on xpcom startup?

Comment 7

[:Gijs (he/him)]

(In reply to comment #6)
> (In reply to comment #5)
> >>+% category command-line-handler m-venkman @mozilla.org/commandlinehandler/general-startup;1?type=venkman
> >Perhaps this should be a-venkman to make this execute as early in startup as possible?
> Although profile-after-change observers would probably get notified first...
> would we be better off adding code to nsSuiteGlue.js to do this on app startup,
> or perhaps even C++ code to do this on xpcom startup?

Venkman has never had C++ code. It would make me sad (and our build process a lot harder) if that had to change. For now, AIUI, we can get parity with current (pre-4.0) builds by taking the patch you have here, is that correct?

This patch will also make KaiRo unhappy, I'm afraid. The perf hit that this causes is not really avoidable without significant work, though.

Comment 8

[[e-mail]]

(In reply to comment #7)
> (In reply to comment #6)
> > (In reply to comment #5)
> > >>+% category command-line-handler m-venkman @mozilla.org/commandlinehandler/general-startup;1?type=venkman
> > >Perhaps this should be a-venkman to make this execute as early in startup as possible?
> > Although profile-after-change observers would probably get notified first...
> > would we be better off adding code to nsSuiteGlue.js to do this on app startup,
> > or perhaps even C++ code to do this on xpcom startup?
> 
> Venkman has never had C++ code. It would make me sad (and our build process a
> lot harder) if that had to change. For now, AIUI, we can get parity with
> current (pre-4.0) builds by taking the patch you have here, is that correct?
> 
> This patch will also make KaiRo unhappy, I'm afraid. The perf hit that this
> causes is not really avoidable without significant work, though.
I believe this doesn't init JSD until at best the commandline handler runs, and then only if someone sets the initAtStartup pref to 2, whereas pre-4.0 we would unavoidably init JSD at app startup. As it's an extension, Venkman can no longer itself start anything before profile-after-change; it was on the SeaMonkey side that I was trying to suggest adding something earlier to init JSD.

Comment 9

[:Gijs (he/him)]

Huh, OK. Can we take this patch and file a followup bug to deal with improving this? Also, if you already know, can you tell us/me how we could improve this for apps where we *are* "just" an add-on?

Comment 10

[Robert Kaiser]

I think there really needs to be some mechanism for an add-on to turn on JSD from the actual start of the app, of course depending on some pref (and I think it never should turn that one of without explicitely asking the user) or, perhaps, depending on a special commandline switch.

I on one hand have seen that my normal build definitely feels faster since the XPCOM registry changes, and I assume that's because JSD is off now, on the other hand I've been seriously bitten by not being able to debug scripts that have been loaded before opening venkman (in my test profile), as I have started to use it nowdays, esp. with my Data Manager work.
I'd love to have both cases work well, the "I have venkman installed but am not using it (right now) so I want full speed" and the "I really want to debug, I don't care for speed right now, just let me do it" case.

Comment 11

[[e-mail]]

(In reply to comment #10)
> I think there really needs to be some mechanism for an add-on to turn on JSD
> from the actual start of the app, of course depending on some pref (and I think
> it never should turn that one of without explicitely asking the user) or,
> perhaps, depending on a special commandline switch.
Unfortunately this is impossible because of the way startup works.
First, xpcom-startup fires. Then, app-startup fires. Then, the profile, including prefs, is loaded. Then, profile-after-change fires. Finally command lines are run. So even if you set the (hidden?) initAtStartup pref with a current trunk build, you only get code that is loaded after the command lines.

About the only thing that is available that early on is an environment variable.

Comment 12

[Robert Kaiser]

Bah, we suck.

One would think that if we design an application platform, we'd provide a good way to debug it reasonably.

Comment 13

[Tony Mechelynck [:tonymec]]

..."then the profile, including prefs; then profile-after-change...
So it would be conceivable to act on a pref (changed in prefs.js while the app was not running, maybe) before any extension, including Geasemonkey, Data Manager, or whatever, gets a chance to do anything, wouldn't it? Or would even that be too late? Then there remains the possibility of an environment variable, MOZ_JSDEBUG=1 or whatever...

Comment 14

[Karsten Düsterloh]

(In reply to comment #11)
> First, xpcom-startup fires. Then, app-startup fires. Then, the profile,
> including prefs, is loaded. Then, profile-after-change fires. Finally command
> lines are run.

What's the first in this chain where JS stuff can appear?

Shaver claimed in the md.platform newsgroup (iirc) that "it's being solved as part of the JSDv2 effort", but never felt like answering what this effort actually is or which bugs it happens in. :-(

Comment 15

[[e-mail]]

(In reply to comment #14)
> (In reply to comment #11)
> > First, xpcom-startup fires. Then, app-startup fires. Then, the profile,
> > including prefs, is loaded. Then, profile-after-change fires. Finally command
> > lines are run.
> What's the first in this chain where JS stuff can appear?
In theory, the app could have an xpcom-startup JS component. In practice I don't actually know of any xpcom-startup components, JS or otherwise. We do have JS app-startup components (session store, suite glue etc.)

Comment 16

[Justin Wood (:Callek)]

Turning on this "hurt" for us, on an app level shouldn't be too hard. However we have no ability to check for extension enable/disable state until at least profile-after-change. Since until then we can't get the extensions sqlite file reliably. (Unless we decide we _want_ to hardcode a profile-finding-logic that already exists in the normal paths, slowing down our startup even more. I'm not in favor of that).

Perhaps shaver can point us at a project-wiki-page, or bug number(s) for the JSD2 coming work; (and potentially some indication if he expects it in the Gecko 2.0 timeframe).

Comment 17

[Colby Russell :crussell (no longer Mozilla-ing)]

(In reply to comment #16)
https://wiki.mozilla.org/Platform/JSDebugv2, bug 560314; I don't know what happened to bug 449452.

Comment 18

[Benjamin Smedberg]

As it stands, users who want to debug application code should stick venkman in distribution/bundles and it should continue to register for the earliest possible notification (I think that's xpcom-startup). Doing it in an extension isn't going to work. Yes, JSD2 will make it possible to debug from a separate process, and we'll get there but not for the immediate need I think you want.

Comment 19

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Attachment: Larger Fix

This patch doesn't really fix the start-debugging-early problems, but it does adapt the existing components more completely.

This may depend on bug 579178 to correctly package the manifest stuff.

Comment 20

[[e-mail]]

Comment on attachment 461277 [details] [diff] [review]
Larger Fix

>+Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
[Always assuming this is acceptable in Venkman, which it might not be]

>+function NSGetFactory(cid) {
>+    startupInit();
>+    return (XPCOMUtils.generateNSGetFactory(components))(cid);
> }
NSGetFactory is called once for every component. This means you call both startupInit and generateNSGetFactory each time. Is that what you want?

Comment 21

[Georg Maaß]

There is a bug in attachment 461277 [details] [diff] [review]:

JSDProtocolHandler.prototype.QueryInterface =
function QueryInterface(aIID)
{
    for each (let face in this.getInterfaces({})) {
        if (iid.equals(face)) {
            return this;
        }
    }
    throw Components.results.NS_ERROR_NO_INTERFACE;
};

There is no variable iid. The name of the function argument is aIID not iid.

Comment 22

[Georg Maaß]

In my XulRunner application I detect with the patch of attachment 461277 [details] [diff] [review] and the bug from comment 21 fixed a different behaviour when using XulRunner 1.9.3 and latest XulRunner 2.0. With XulRunner 1.9.3 in Venkman I see all scripts independent from whether they belong to a window opened before Venkman or after. With latest XulRunner 2.0 in Venkman I see only scripts from Venkman and from windows opened after Venkman but not from windows opened before. If I later invoke loading a script into an already open window, this script will be shown too. Is it possible to make Venkman to recognize scripts that have been loaded before the Window of Venkman has been opened?

Comment 24

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 461277 [details] [diff] [review]
Larger Fix

yeah, commment 21 found a bug in this patch so r- even if it's a good beginning

Comment 25

[:Gijs (he/him)]

Attachment: Step 1: Clean up the attic (aka: die, rdf, die)

So I'm trying to deal with this, and all this stuff was in the way. This is a lot easier to review than what comes next, so I'd rather have it out of the way now (next patch may take a while (read: hours - 1 day) to appear while I tweak it, though).

Comment 26

[:Gijs (he/him)]

Attachment: Step1b: Don't forget the localizers when doing that

Oops.


(and yes, these patches will remove support for pre-gecko-1.9.1 apps)

Comment 27

[:Gijs (he/him)]

Attachment: Step1b: Don't forget the localizers when doing that

Eh, without that top change. Sorry for spam.

Comment 28

[Karsten Düsterloh]

Comment on attachment 463821 [details] [diff] [review]
Step 1: Clean up the attic (aka: die, rdf, die)

Ah, them cherry trees... ;-)

Comment 29

[Robert Kaiser]

Comment on attachment 463825 [details] [diff] [review]
Step1b: Don't forget the localizers when doing that

yay for the cleanup!

Comment 30

[:Gijs (he/him)]

Attachment: Step 1c: Actually, this build system can use updating, too

So, the perl build system doesn't like all these newfangled thingummywhatsits we're sticking in jar.mn, and the attempts to move to python (from bug 569839) but still use SH break rather magically on my machine. After asking Callek (who wrote the patch there) about it, it seems like just moving to python altogether, like CZ has done, will be easiest. This patch ought to do the trick.

Comment 31

[:Gijs (he/him)]

Attachment: Step 2: Improve Venkman

In terms of the component and its registration (about 1/3-1/2 of the patch), this:

- Adds component registration instructions to jar.mn (these won't be dealt with correctly under the old perl system, hence patch 1c)
- Rewrites the component to use XPCOMUtils
- Now uses a boolean pref to toggle startup of JSD on profile-after-change. This is early enough for things like browser chrome, but too late for JS components (browser ones or add-on ones)
- Separates the commandline handling and the pref checking (as profile-after-change is still a bit earlier than commandline handling)
- Keeps backwards compat with 1.9.1 (Fx 3.5 / SM 2.0), but not before that. I have to admit I haven't actually tested on 3.5, only on Fx 3.6, but things seemed to work, and to my knowledge I'm not using any APIs that are new in 3.6...

In terms of Venkman itself:
- We now take new jsdScripts from function hooks. The code has been updated to deal with this, and the mayhem they cause. Basically, Venkman keeps track of scripts partly by checking when the 'toplevel' jsdScript (which doesn't have a function name) comes in through onScriptCreated. It then "seals" the script instance, so that when you reload a webpage and get a newer version of the script, it keeps those separate in a new script instance. This also helps it figure out when you're eval'ing scripts (because it means new jsdScripts arrive for scripts which have already been sealed). All of this logic breaks down if you start adding random jsdScripts from a functionHook. To cope with this:
* Venkman keeps track if a scriptInstance was created from a callHook.
* If so, the script is not sealed unless a new named script from onScriptCreated (rather than the functionHook) arrives, signalling there is a new complete script.
* Special magic happens for XBL, which is absolutely freaking crazy (AFAICT it always creates new scripts for every call to its methods, which then gets into onScriptCreated, which messes up any logic you could possibly think of)
* We display these scripts before they're sealed (because that might never happen). In this case not all lines may be marked as executable, because not all the jsdScripts are known to JSD/Venkman yet. However, you can set a future breakpoint, and it will get hit when the function gets called (!).

- I fixed some related other bugs explicitly (bug 534222, bug 358286) or implicitly (bug 483282, bug 483685). There might be one or two others I forgot about already.


All of this actually means you should be able to debug scripts which are currently hard or impossible to debug in Venkman, namely ones which load before JSD's app-startup handler fires, and run code while the UI is up.

However good this sounds, please be careful when reviewing. I'm not sure how sane I still am after this stuff, and the code might have suffered from that. :-)

Comment 32

[:Gijs (he/him)]

Attachment: XPI with all these changes

Comment 33

[Karsten Düsterloh]

Comment on attachment 463897 [details] [diff] [review]
Step 1c: Actually, this build system can use updating, too

Just some nits:

makexpi.py:
>+    except WindowsError, ex:
>+        if ex.errno != 2:
>+            raise
>+def mkdir(dir):

Missing empty line between the functions.

>+    """
>+    acts like mkdir -p
>+    """
>+    try:
>+        os.makedirs(dir)
>+    except os.error:
>+        pass # dont error out if there dir already exists

s/there/the/

>+# Extract version number.
>+version = None
>+versionFile = open(joinpath(fedir, '..', 'version.txt'), 'r')

This will die (instead of dumping the error hint later) if fedir exists, but version.txt doesn't:
  Traceback (most recent call last):
    File "makexpi.py", line 172, in <module>
      versionFile = open(joinpath(fedir, '..', 'version.txt'), 'r')
  IOError: [Errno 2] No such file or directory: '/tmp/../version.txt'
(I set fedir to /tmp).
Not actually a problem, since caught version.txt errors would end the script anyway, but not really nice either.

>+# Make Firefox updates.
>+echo("  Updating Firefox Extension files")

Not just Firefox' anymore. ;-)

Comment 34

[Karsten Düsterloh]

Comment on attachment 463918 [details] [diff] [review]
Step 2: Improve Venkman

Does Venkman actually have a coding style policy, eg like <https://wiki.mozilla.org/index.php?title=SeaMonkey:MailNews:CodingStyle>? (Comments in parens below are on some irregularities I noticed.)

>diff --git a/js/venkman-service.js b/js/venkman-service.js
> function handler_QI(iid)
> {
>+    var ifaces = this.getInterfaces({});
>+    for (var face in ifaces) {

(Most Venkman code seems to prefer the braces-on-their-own-line style, so it may be useful for readability to be consistent with that - here and below.)

>+CLineService.prototype.getHelperForLanguage =
>+function getHelperForLanguage()
>+{
>+    return null;
>+};

JavaScript 1.8/Gecko 1.9 allows for even more brevity with simple functions like this:

CLineService.prototype.getHelperForLanguage = function getHelperForLanguage() null;

>+        var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                .getService(Components.interfaces.nsIPrefBranch);
>+        var prefValue = false;

You may want to consider using let for sub-scope variable definitions (works since JS 1.7/Gecko 1.8.something).

>+/**
>+* XPCOMUtils.generateNSGetFactory was introduced in Mozilla 2 (Firefox 4).
>+* XPCOMUtils.generateNSGetModule is for Mozilla 1.9.1 and 1.9.2 (Firefox 3.5/3.6).
>+*/
>+if (XPCOMUtils.generateNSGetFactory)
>+    var NSGetFactory = XPCOMUtils.generateNSGetFactory(components);
>+else
>+    var NSGetModule = XPCOMUtils.generateNSGetModule(components);

Wow, this is gross. (Or a cool JS feature, in other light? *g*)
This means that both variables will be declared in global scope, but may be uninitialized depending upon the value of a probably non-existing function object!
At least the check should be      
  if ("generateNSGetFactory" in XPCOMUtils)

>@@ -859,13 +860,14 @@ function cmdFinish (e)
>+    console._areStepping = true;

It'd help occasional later code readers if you could explain the purpose/usage of _areStepping somewhere (in the source).

>+function scv_hookScriptCHInstanceUpdated(e)
>+{
>+    // This is a variation of the script instance sealed notification.
>+    // For script instances created from a call hook, we have no way of knowing
>+    // when we have 'all' the scripts, so we continuously update them.
>+    var url = e.scriptInstance.url;
>+    if (!url || url.search (/^(x-jsd|javascript)/) == 0)

You should also match the : after the scheme, just in case (like you do below for chrome:).


In toto, this looks good, but SM keeps crashing on me atm, so I can't test it for real now - more tomorrow.

Comment 35

[:Gijs (he/him)]

(In reply to comment #34)
> Comment on attachment 463918 [details] [diff] [review]
> Step 2: Improve Venkman
> 
> Does Venkman actually have a coding style policy, eg like
> <https://wiki.mozilla.org/index.php?title=SeaMonkey:MailNews:CodingStyle>?
> (Comments in parens below are on some irregularities I noticed.)
There's a ChatZilla one that we broadly follow, AIUI. You're right that braces go on their own line. Will fix...

 
> <snip>
> >+        var prefService = Components.classes["@mozilla.org/preferences-service;1"]
> >+                                .getService(Components.interfaces.nsIPrefBranch);
> >+        var prefValue = false;
> 
> You may want to consider using let for sub-scope variable definitions (works
> since JS 1.7/Gecko 1.8.something).

I don't like 'let'. It will break parsing of the file in older versions of SM, and in plain function scope it is, to my knowledge, useless, and doesn't behave any different from 'var'. Or so the MDC page seems to imply!
 
> >+/**
> >+* XPCOMUtils.generateNSGetFactory was introduced in Mozilla 2 (Firefox 4).
> >+* XPCOMUtils.generateNSGetModule is for Mozilla 1.9.1 and 1.9.2 (Firefox 3.5/3.6).
> >+*/
> >+if (XPCOMUtils.generateNSGetFactory)
> >+    var NSGetFactory = XPCOMUtils.generateNSGetFactory(components);
> >+else
> >+    var NSGetModule = XPCOMUtils.generateNSGetModule(components);
> 
> Wow, this is gross. (Or a cool JS feature, in other light? *g*)
> This means that both variables will be declared in global scope, but may be
> uninitialized depending upon the value of a probably non-existing function
> object!

There is no way not to do that, unless you can tell me the name of the global scope object in JS components (in a browser, you could have done 'window[foo] = ...', which only assigns without declaring, but given that 'window' doesn't exist, that won't work here).
 
> >@@ -859,13 +860,14 @@ function cmdFinish (e)
> >+    console._areStepping = true;
> 
> It'd help occasional later code readers if you could explain the purpose/usage
> of _areStepping somewhere (in the source).

Good call.
 
> In toto, this looks good, but SM keeps crashing on me atm, so I can't test it
> for real now - more tomorrow.

Great, thanks! :-)

Comment 36

[Robert Kaiser]

(In reply to comment #34)
> >+/**
> >+* XPCOMUtils.generateNSGetFactory was introduced in Mozilla 2 (Firefox 4).
> >+* XPCOMUtils.generateNSGetModule is for Mozilla 1.9.1 and 1.9.2 (Firefox 3.5/3.6).
> >+*/
> >+if (XPCOMUtils.generateNSGetFactory)
> >+    var NSGetFactory = XPCOMUtils.generateNSGetFactory(components);
> >+else
> >+    var NSGetModule = XPCOMUtils.generateNSGetModule(components);
> 
> Wow, this is gross. (Or a cool JS feature, in other light? *g*)

Well, AFAIK, this is exactly the same block of code we are using everywhere in the cases where compat is needed, and which IIRC has been set up by bsmedberg as the example for how to do this.

Comment 37

[Karsten Düsterloh]

Comment on attachment 463918 [details] [diff] [review]
Step 2: Improve Venkman

(Basically, my other profile was broken, so no crashing issues here.)

This works as intended by the scope of this bug. 

I do have issues with hitting breakpoints under certain circumstances (open messenger.xml, set breakpoint in OnLoad, open browser, close messenger, open messenger.xml again -> breakpoint not hit), but I need to investigate those and file new bugs for them if necessary then.

Comment 38

[Karsten Düsterloh]

(In reply to comment #35)
> I don't like 'let'. 
...
> in plain function scope it is, to my knowledge, useless, and doesn't
> behave any different from 'var'.

That's correct, that's why I said "sub-scope". ;-)

> There is no way not to do that, unless you can tell me the name of the 
> global scope object in JS components (in a browser, you could have done
> 'window[foo] = ...', which only assigns without declaring, but given that
> 'window' doesn't exist, that won't work here).

Yeah, I know. My comment wasn't a rejection, just a "OMFG". ;-)

(In reply to comment #36)
> > Wow, this is gross. (Or a cool JS feature, in other light? *g*)
> 
> Well, AFAIK, this is exactly the same block of code we are using everywhere

That doesn't make it any better! ;-)

> in the cases where compat is needed, 

That doesn't make it any better! ;-)

> and which IIRC has been set up by bsmedberg

That doesn't make it any better! ;-)

> as the example for how to do this.

That doesn't make it any better! ;-)

Comment 39

[Benjamin Smedberg]

I'm not sure I understand the objection. You could, of course, declare both of them at global scope:

var NSGetFactory, NSGetModule;
and then use the "if" to merely initialize one or the other. But the way I've got it is correct and IMO a fairly elegant way to do the dynamic detection.

Comment 40

[Karsten Düsterloh]

(In reply to comment #39)
> the way I've got it is correct and IMO a fairly elegant way to do the
> dynamic detection.

Yes, fairly. You can't do much better in this brevity, of course.

Comment 41

[[e-mail]]

Comment on attachment 463918 [details] [diff] [review]
Step 2: Improve Venkman

>+        var prefService = Components.classes["@mozilla.org/preferences-service;1"]
>+                                .getService(Components.interfaces.nsIPrefBranch);
>+        var prefValue = false;
>+        try {
>+            prefValue = prefService.getBoolPref(this.prefNameForStartup);
>+        } catch (ignore) {}
> 
>+        if (cmdLine.handleFlag("venkman", false) || prefValue)
This pref check seems to be new? I only mention it because SeaMonkey will default to launching any command line handler if it sees a matching pref.
(So if someone sets general.startup.venkman it will find the @mozilla.org/commandlinehandler/general-startup;1?type=venkman component and invoke it with the -venkman command line argument.)

Comment 42

[:Gijs (he/him)]

(In reply to comment #41)
> Comment on attachment 463918 [details] [diff] [review]
> Step 2: Improve Venkman
> 
> >+        var prefService = Components.classes["@mozilla.org/preferences-service;1"]
> >+                                .getService(Components.interfaces.nsIPrefBranch);
> >+        var prefValue = false;
> >+        try {
> >+            prefValue = prefService.getBoolPref(this.prefNameForStartup);
> >+        } catch (ignore) {}
> > 
> >+        if (cmdLine.handleFlag("venkman", false) || prefValue)
> This pref check seems to be new? I only mention it because SeaMonkey will
> default to launching any command line handler if it sees a matching pref.
> (So if someone sets general.startup.venkman it will find the
> @mozilla.org/commandlinehandler/general-startup;1?type=venkman component and
> invoke it with the -venkman command line argument.)

Where does this code live for SM 2.0? I didn't find it at the time.

Also, that won't work on Fx. Just trying to keep feature-parity...

Comment 43

[:Gijs (he/him)]

(In reply to comment #37)
> Comment on attachment 463918 [details] [diff] [review]
> Step 2: Improve Venkman
> 
> (Basically, my other profile was broken, so no crashing issues here.)
> 
> This works as intended by the scope of this bug. 
> 
> I do have issues with hitting breakpoints under certain circumstances (open
> messenger.xml, set breakpoint in OnLoad, open browser, close messenger, open
> messenger.xml again -> breakpoint not hit), but I need to investigate those and
> file new bugs for them if necessary then.

That's XBL right, from the filename? That's always been Really Hard at best, and impossible at worst. There are bugs on file about this, but I didn't want to try to do too many things at once...

Comment 44

[Robert Kaiser]

From a project management POV, do you have an ETA for this landing?
We have been in a freeze for the current SM alpha since Wednesday morning, and this seems to be the last blocker left, so we need to come to a decision if we do builds or even ship the alpha with a broken venkman, do some relbranch magic, land the patch there and ship that, or wait for this to land (which I would prefer if it can land very soon).
Of course, venkman should not be pressed for SeaMonkey release plans, but we need to find some deal on our side, so an ETA would be helpful. Thanks for that and your hard work on this issue.

Comment 45

[:Gijs (he/him)]

(In reply to comment #44)
> From a project management POV, do you have an ETA for this landing?
> We have been in a freeze for the current SM alpha since Wednesday morning, and
> this seems to be the last blocker left, so we need to come to a decision if we
> do builds or even ship the alpha with a broken venkman, do some relbranch
> magic, land the patch there and ship that, or wait for this to land (which I
> would prefer if it can land very soon).
> Of course, venkman should not be pressed for SeaMonkey release plans, but we
> need to find some deal on our side, so an ETA would be helpful. Thanks for that
> and your hard work on this issue.

I was/am about to land it. Was out most of yesterday, or I would have done it right after I'd gotten that review... :-)

Comment 46

[Robert Kaiser]

(In reply to comment #45)
> I was/am about to land it. Was out most of yesterday, or I would have done it
> right after I'd gotten that review... :-)

Cool, thanks, good to hear!

Comment 47

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #45)
> I was/am about to land it.

git apply -v showed some whitespace errors. You might want to fix them upon check-in. I'm pretty sure Neil would have told, too you if he had reviewed the patches. ;-)

Comment 48

[:Gijs (he/him)]

Fixed in:
changeset:   646:15d7d14e6846
changeset:   647:67060e64e26b
changeset:   648:8be664ec9d4c
changeset:   649:289e582190e8

Thanks everyone. Put the version at .87.5, will commit some other patches which are lying around bugs, and then set to .88.

(In reply to comment #47)
> (In reply to comment #45)
> > I was/am about to land it.
> 
> git apply -v showed some whitespace errors. You might want to fix them upon
> check-in. I'm pretty sure Neil would have told, too you if he had reviewed the
> patches. ;-)

Next time please be more specific than this. Esp. in Python 'whitespace errors' can be serious, but clearly what you pointed out wasn't. I believe I fixed all the trailing whitespace in the last (code) patch.

Comment 49

[Jens Hatlak (:InvisibleSmiley)]

(In reply to comment #48)
> Next time please be more specific than this. Esp. in Python 'whitespace errors'
> can be serious, but clearly what you pointed out wasn't. I believe I fixed all
> the trailing whitespace in the last (code) patch.

Well, sorry I know nothing about Python. I just used the same terminology as git apply -v. Sure I could have checked more thoroughly, but then I'm not a reviewer, I just happened to spot it.

Comment 50

[Georg Maaß]

When you do cosmetics like white space clean up, you may also do semicolon clean up by adding all missing semicolns to make it possible to compress the resulting js code to single line exprssions.