397869 - Sanify console.host usage

Status: RESOLVED FIXED

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

Description

[:Gijs (he/him)]

Currently we call "non-suite" "Firefox". That's kinda odd and strange and it's probably expected to mean different things in different places. It'd be good to comb through these usages and make sure they do what they should, even with suiterunner around the corner, and are reasonably semantic (in my mind, at least "Firefox" doesn't include Thunderbird, while in reality it does in the current code).

Comment 1

[Karsten Düsterloh]

This check is indeed quite fragile - it just does "if it doesn't have a Windows menu, it's Firefox"!

<http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/extensions/venkman/resources/content/venkman-static.js&rev=1.72&mark=637-642#630>

Comment 2

[:Gijs (he/him)]

Attachment: Patch

Steal code from ChatZilla, weee!

Comment 3

[Alex Vincent [:WeirdAl]]

Comment on attachment 297981 [details] [diff] [review]
Patch

>Index: mozilla/extensions/venkman/resources/content/venkman-static.js
>     var app = getService("@mozilla.org/xre/app-info;1", "nsIXULAppInfo");

You define getService() for the first time just to get this service.  I'd say it was worth it if you were using getService() in other ways.  As it is, I don't see it worth it for just the one call.

>+    // nsIXULAppInfo wasn't implemented before 1.8...
>+    if (app)
>+    {
...
>+    }
>+    else if ("getBrowserURL" in window)

I don't see us supporting anything before 1.8 at this stage.  Do you?

Comment 4

[:Gijs (he/him)]

(In reply to comment #3)
> (From update of attachment 297981 [details] [diff] [review])
> >Index: mozilla/extensions/venkman/resources/content/venkman-static.js
> >     var app = getService("@mozilla.org/xre/app-info;1", "nsIXULAppInfo");
> 
> You define getService() for the first time just to get this service.  I'd say
> it was worth it if you were using getService() in other ways.  As it is, I
> don't see it worth it for just the one call.

That's most definitely true, but I would prefer to define it and file a followup bug to start using it everywhere rather than using the direct XPCOM variant here, too.


> >+    // nsIXULAppInfo wasn't implemented before 1.8...
> >+    if (app)
> >+    {
> ...
> >+    }
> >+    else if ("getBrowserURL" in window)
> 
> I don't see us supporting anything before 1.8 at this stage.  Do you?

The install.rdf claims support for lots of things before 1.8 - and to my knowledge we still work there, too. So I'm not really willing to break things right now if I can help it. The code is well-tested, it has been in ChatZilla for a few years now, so I'm not too worried.

Comment 5

[Alex Vincent [:WeirdAl]]

Comment on attachment 297981 [details] [diff] [review]
Patch

r=ajvincent then.

Comment 6

[:Gijs (he/him)]

Thanks! Bug 413148 filed for the getService issue.

Comment 7

[:Gijs (he/him)]

Checking in mozilla/extensions/venkman/resources/content/venkman-static.js;
/cvsroot/mozilla/extensions/venkman/resources/content/venkman-static.js,v  <--  venkman-static.js
new revision: 1.76; previous revision: 1.75
done
Checking in mozilla/extensions/venkman/resources/content/venkman-utils.js;
/cvsroot/mozilla/extensions/venkman/resources/content/venkman-utils.js,v  <--  venkman-utils.js
new revision: 1.37; previous revision: 1.36
done