517291 - [Extension] Bandwagon can work with Seamonkey 2

Status: RESOLVED FIXED

(addons.mozilla.org Graveyard :: Collector Extension, enhancement, P5)

Description

[Igor Velkov]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.9.3a1pre) Gecko/20090912 SeaMonkey/2.1a1pre
Build Identifier: 

Bandwagon works ok with Seamonkey 2.x version if compatibility info declared.

Reproducible: Always

Comment 1

[Igor Velkov]

Attachment: declare Seamonkey 2 compatibility

Comment 2

[Philip Chee]

See the patches in Bug 509080 for other js files you need to change.
Basically look for string like:
appname == "Firefox"
and replace with:
appname == "Firefox" || appname == "SeaMonkey"

Comment 3

[Igor Velkov]

Hope Brian includes patch for bug 509080. Then I can make not conflict patch that changes same places.

Comment 4

[Brian King [:kinger]]

Comment on attachment 401266 [details] [diff] [review]
declare Seamonkey 2 compatibility

Phil, I'm not going to take that until the code forks you mention are cleaned up to work with SM and it actually works.

There may also be some server code that is TB specific but I am not sure.

Comment 5

[Philip Chee]

Brian, is there any more work to be done in bug 509080? If not then Igor can proceed to come up with a new patch.

> There may also be some server code that is TB specific but I am not sure.

In all cases I've looked at, we would want to take the Firefox code branch since we have (from the Bandwagon perspective) all the browser/tabbrowser APIs that the extension relies on, and of course we have exactly the same toolkit APIs as Firefox 3.5.3.

Comment 6

[Wil Clouser [:clouserw]]

Who should this be assigned to?

Comment 7

[Philip Chee]

Igor can you own this bug on our behalf for the time being? Thanks.

Comment 8

[Igor Velkov]

ok

Comment 9

[Igor Velkov]

Attachment: add seamonkey support

Comment 10

[Philip Chee]

Remind me again where the SVN repository is and how to build the extension.

Comment 11

[Philip Chee]

Never mind I found http://svn.mozilla.org/addons/trunk/bandwagon and build.sh

Comment 12

[Philip Chee]

Comment on attachment 415732 [details] [diff] [review]
add seamonkey support

This diff doesn't appear to have been created with svn diff and didn't apply at all. Even after editing the diff it didn't apply cleanly against svn.mozilla.org/addons/trunk/bandwagon.

+    switch (appname) {
+        case "Thunderbird": {
+            var ioServ = Bandwagon.Util.Cc["@mozilla.org/network/io-service;1"]
+        case "Seamonkey": {
+                openUILinkIn(url, "tab");
+        default: { // Firefox
+            var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
+                               .getService(Components.interfaces.nsIWindowMediator);
+            var mainWindow = wm.getMostRecentWindow("navigator:browser");

Pity bandwagon needs to support Thunderbird 2 since Thunderbird 3 has openUILink(url, event). In any case I think you can use the convenience wrapper |messenger.launchExternalURL(url);|

By the way Firefox also has |openUILinkIn(url, "tab");|

-    if (Bandwagon.Util.getHostEnvironmentInfo().appName == "Firefox")
+    if (Bandwagon.Util.getHostEnvironmentInfo().appName == "Firefox" || 
+        Bandwagon.Util.getHostEnvironmentInfo().appname == "SeaMonkey")

How about:
if (/(Firefox|SeaMonkey)/.test(Bandwagon.Util.getHostEnvironmentInfo().appName)) { ... }

-        <em:maxVersion>3.5.*</em:maxVersion>
+        <em:maxVersion>3.7.*</em:maxVersion>

Leave the Firefox maxVersion at 3.6.*

+      </Description>
+    </em:targetApplication>
+    
+     <!-- Seamonkey -->
+    <em:targetApplication>
+      <Description>
+        <em:id>{92650c4d-4b8e-4d2a-b7eb-24ecf4f6b63a}</em:id>
+        <em:minVersion>2.0a1</em:minVersion>
+        <em:maxVersion>3.0</em:maxVersion>

SeaMonkey maxVersion 3.0.*
(or 3.1a1pre if you want to support trunk testers).

Comment 13

[Philip Chee]

Igor, thinking about this further I think a minimal patch that only does the essential would be better.

So don't change the Thunderbird code.

+        case "Seamonkey": {
+                openUILinkIn(url, "tab");

Dont't have a separate branch for SeaMonkey. We can use the same code patch as Firefox since our tabbrowser API is compatible with Firefox as far as Bandwagon is concerned.

The rest of my comments hold.

And thanks for taking ownership of this bug.

Comment 14

[Igor Velkov]

My patch was produced for public release.
Now I build from svn trunk - and it can't login. No errors in js error log.
Something important was broken from release.

Comment 15

[Igor Velkov]

Bloody mess… Previosly I use "case "Seamonkey": {", and it work fine.
Now I see "nothing working", and, when I trace with venkman, look into, and see that appname="SeaMonkey".
Wtf?

It remember me to my previous patch for weave. See https://bugzilla.mozilla.org/show_bug.cgi?id=526521#c17, last paragraph.

Edward Lee wrote: "We've run into issues before when switching on the AppInfo.name instead of AppInfo.ID".

Maybe, bandwagon also affected by "issues"?

Comment 16

[Philip Chee]

nsIXULAppInfo has returned "SeaMonkey" (capital M) as the appname since SeaMonkey 1.0. But as you say now that Bandwagon supports Fennec which at some point will start identifying itself as "Firefox", the bandwagon developers (like the weave developers) will have to bite the bullet and switch to using appId from appname.

Comment 17

[Igor Velkov]

Adding into Bandwagon.Controller.CollectionsPane._openURL
    if (appname == "SeaMonkey")
	{
		openUILinkIn(url, "tab");
	}
not work - just not open new tab. No errors in error console.

all work for sm and ff that:

Bandwagon.Controller.CollectionsPane._openURL = function(url)
{
    Bandwagon.Logger.debug("Opening URL " + url);

    var appname = Bandwagon.Util.getHostEnvironmentInfo().appName;

    if (appname == "Thunderbird")
    {
        var ioServ = Bandwagon.Util.Cc["@mozilla.org/network/io-service;1"]
            .getService(Bandwagon.Util.Ci.nsIIOService);
        var resolvedURI = ioServ.newURI(url, null, null);
        var extps = Bandwagon.Util.Cc["@mozilla.org/uriloader/external-protocol-service;1"]
            .getService(Bandwagon.Util.Ci.nsIExternalProtocolService);
        extps.loadURI(resolvedURI, null);
    }
    else if (appname == "Fennec")
    {
        top.Browser.addTab(url, true);
        top.BrowserUI.show(0);
    }
	else // firefox, seamonkey, other
	{
        var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
                           .getService(Components.interfaces.nsIWindowMediator);
        var mainWindow = wm.getMostRecentWindow("navigator:browser");
    
        if (mainWindow)
        {
            var tab = mainWindow.getBrowser().addTab(url);
            mainWindow.getBrowser().selectedTab = tab;
            mainWindow.focus();
        }
        else
        {
            window.open(url);
        }
    }
}
    Bandwagon.Logger.debug("Opening URL " + url);

    var appname = Bandwagon.Util.getHostEnvironmentInfo().appName;

    if (appname == "Firefox" || appname == "SeaMonkey")
    {
        var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
                           .getService(Components.interfaces.nsIWindowMediator);
        var mainWindow = wm.getMostRecentWindow("navigator:browser");
    
        if (mainWindow)
        {
            var tab = mainWindow.getBrowser().addTab(url);
            mainWindow.getBrowser().selectedTab = tab;
            mainWindow.focus();
        }
        else
        {
            window.open(url);
        }
    }
    else if (appname == "Thunderbird")
    {
        var ioServ = Bandwagon.Util.Cc["@mozilla.org/network/io-service;1"]
            .getService(Bandwagon.Util.Ci.nsIIOService);
        var resolvedURI = ioServ.newURI(url, null, null);
        var extps = Bandwagon.Util.Cc["@mozilla.org/uriloader/external-protocol-service;1"]
            .getService(Bandwagon.Util.Ci.nsIExternalProtocolService);
        extps.loadURI(resolvedURI, null);
    }
    else if (appname == "Fennec")
    {
        top.Browser.addTab(url, true);
        top.BrowserUI.show(0);
    }
}

work both on seamonkey and firefox.
I think Firefox and Seamonkey should work not by exact id, but by default. Nobody knows what another app will be added in future, and why there should be exact handle for every?

If I replace firefox part to "openUILinkIn(url, "tab");" - it not work in firefox.

Comment 18

[Igor Velkov]

Code from previous comment should be 

Bandwagon.Controller.CollectionsPane._openURL = function(url)
{
    Bandwagon.Logger.debug("Opening URL " + url);

    var appname = Bandwagon.Util.getHostEnvironmentInfo().appName;

    if (appname == "Thunderbird")
    {
        var ioServ = Bandwagon.Util.Cc["@mozilla.org/network/io-service;1"]
            .getService(Bandwagon.Util.Ci.nsIIOService);
        var resolvedURI = ioServ.newURI(url, null, null);
        var extps = Bandwagon.Util.Cc["@mozilla.org/uriloader/external-protocol-service;1"]
            .getService(Bandwagon.Util.Ci.nsIExternalProtocolService);
        extps.loadURI(resolvedURI, null);
    }
    else if (appname == "Fennec")
    {
        top.Browser.addTab(url, true);
        top.BrowserUI.show(0);
    }
	else // firefox, seamonkey, other
	{
        var wm = Components.classes["@mozilla.org/appshell/window-mediator;1"]
                           .getService(Components.interfaces.nsIWindowMediator);
        var mainWindow = wm.getMostRecentWindow("navigator:browser");
    
        if (mainWindow)
        {
            var tab = mainWindow.getBrowser().addTab(url);
            mainWindow.getBrowser().selectedTab = tab;
            mainWindow.focus();
        }
        else
        {
            window.open(url);
        }
    }
}

Comment 19

[Igor Velkov]

> -    if (Bandwagon.Util.getHostEnvironmentInfo().appName == "Firefox")
> +    if (Bandwagon.Util.getHostEnvironmentInfo().appName == "Firefox" || 
> +        Bandwagon.Util.getHostEnvironmentInfo().appname == "SeaMonkey")
> 
> How about:
> if
> (/(Firefox|SeaMonkey)/.test(Bandwagon.Util.getHostEnvironmentInfo().appName)) 

Slow and processor-consumable regex code. Not easy to understandable by human - "what does it mean?". And I not see why it can be better...
Sure, I am not a developer, but do you realy think it will be better than 2 simple comparison?

> Leave the Firefox maxVersion at 3.6.*

Why not support trunk - currently 3.7a1pre ?

Comment 20

[Philip Chee]

>         openUILinkIn(url, "tab");
> not work - just not open new tab. No errors in error console.

Right In the Addons manager openUILinkIn() isn't available. However openURL() is and since this is part of toolkit it should be available for all toolkit apps including Thunderbird, Firefox and SeaMonkey (I don't know about Fennec) the three cases for FX/TB/SM with one branch to openURL()?

> Slow and processor-consumable regex code. Not easy to understandable by human -
> "what does it mean?". And I not see why it can be better...
OK. Good point.

>> Leave the Firefox maxVersion at 3.6.*
> Why not support trunk - currently 3.7a1pre ?

That's up to Brian King to decide, not us.

Comment 21

[Igor Velkov]

Attachment: wip 2

It works with current trunk. 
Not look into Bandwagon.Controller.BrowserOverlay.init and Bandwagon.Controller.BrowserOverlay.open
yet. Possible will try to make firefox as default instead choosen one there too.

Comment 22

[Brian King [:kinger]]

(In reply to comment #20)
> >> Leave the Firefox maxVersion at 3.6.*
> > Why not support trunk - currently 3.7a1pre ?
> 
> That's up to Brian King to decide, not us.

We don't have QA coverage, and are only targeting public releases.

Comment 23

[Brian King [:kinger]]

(In reply to comment #16)
> nsIXULAppInfo has returned "SeaMonkey" (capital M) as the appname since
> SeaMonkey 1.0. But as you say now that Bandwagon supports Fennec which at some
> point will start identifying itself as "Firefox", the bandwagon developers
> (like the weave developers) will have to bite the bullet and switch to using
> appId from appname.

I've filed bug 532941. Thanks for the heads-up guys.

Comment 24

[Igor Velkov]

What this function should do?
Bandwagon.Controller.CollectionsPane.doUpgradeToFirefoxN = function(version)
{
    Bandwagon.Logger.info("in Bandwagon.Controller.CollectionsPane.doUpgradeToFirefoxN() with version = " + version);
    var appname = Bandwagon.Util.getHostEnvironmentInfo().appName;
    var upUrl = (appname == "Firefox") ? Bandwagon.Controller.CollectionsPane.firefoxUpgradeUrl : Bandwagon.Controller.CollectionsPane.thunderbirdUpgradeUrl;
    Bandwagon.Controller.CollectionsPane._openURL(upUrl);
}
and very similar Bandwagon.Controller.CollectionsPane.doDownloadFirefoxNBeta

I see there wrong method - binary assignment "for firefox" and "not firefox --> it's thunderbird". 
Suppose switch - case should be there, but I don't know what should assigns for other clients.

Comment 25

[Igor Velkov]

Attachment: wip3

it works.

Comment 26

[Philip Chee]

Comment on attachment 416223 [details] [diff] [review]
wip3

> +	else // firefox, seamonkey, other
> +	{

> +		Bandwagon.Util.getHostEnvironmentInfo().appName == "SeaMonkey")

> +		case "SeaMonkey":

>      </em:targetApplication>
>  
> +	<!-- Seamonkey -->

Some tabs have crept in to your patch:

> +        <em:minVersion>2.0a1</em:minVersion>
> +        <em:maxVersion>2.1a1</em:maxVersion>

Nit: maxVersion 2.0.*. It doesn't matter whether we want to be risky and use 2.1a1 or not. It is bandwagon policy to only support officially released versions and we must respect that.

> Bandwagon.Controller.CollectionsPane.doUpgradeToFirefoxN = function(version)
> and very similar Bandwagon.Controller.CollectionsPane.doDownloadFirefoxNBeta

We need to create some extra strings:

>     this.firefoxUpgradeUrl = "http://www.mozilla.com/en-US/firefox/all.html";
>     this.firefoxUpgradeBetaUrl = "http://www.mozilla.com/en-US/firefox/all-beta.html";
>     this.thunderbirdUpgradeUrl = "http://www.mozillamessaging.com/en-US/thunderbird/all.html";
>     this.thunderbirdUpgradeBetaUrl = "http://www.mozillamessaging.com/en-US/thunderbird/early_releases/";

e.g.

this.seamonkeyUpgradeUrl = "http://www.seamonkey-project.org/releases/";

Unfortunately we don't seem to have a beta landing page. The closest we have currently is the /latest-comm-central-trunk/ directory on the FTP server.

this.seamonkeyUpgradeBetaUrl = "http://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/latest-comm-central-trunk/";

> I see there wrong method - binary assignment "for firefox" and "not firefox -->
> it's thunderbird". 
> Suppose switch - case should be there, but I don't know what should assigns for
> other clients.

You need to re-write the logic using a switch statement.

>    Bandwagon.Controller.CollectionsPane._openURL(upUrl);
Add a |if (upUrl)| here for future apps that don't have an update landing page.

Note I haven't been able to trigger these methods so I couldn't test these.

Comment 27

[Robert Kaiser]

(In reply to comment #26)
> this.seamonkeyUpgradeUrl = "http://www.seamonkey-project.org/releases/";
> 
> Unfortunately we don't seem to have a beta landing page. The closest we have
> currently is the /latest-comm-central-trunk/ directory on the FTP server.

That would have nightlies, what it seems to want is prereleases, though.
Our prereleases are usually also mentioned at and linked from the releases/ page, but I can create a redirect that always send one to the newest alpha/beta if one exists. Would that help or be wanted here?

Comment 28

[Igor Velkov]

Attachment: v3

Comment 29

[Igor Velkov]

I not see when xxxUpgradeUrl and xxxUpgradeBetaUrl calls. Maybe, it can just be ignored?

Comment 30

[Philip Chee]

Comment on attachment 416680 [details] [diff] [review]
v3

Almost there. Only a few minor nits to fix.

> +	else // firefox, seamonkey, other
> +	{
Still tab characters on these two lines.

> +		Bandwagon.Util.getHostEnvironmentInfo().appName == "SeaMonkey")
tab character on this line.

> +		case "SeaMonkey":
tab character on this line.

> +    this.otherUpgradeUrl = "http://www.mozilla.org/projects/";
> +    this.otherUpgradeBetaUrl = "http://www.mozdev.org/";
Comment: I don't know if mozdev is acceptable for an official addons.mozilla.org extension. I'll let Brian King decide.

> -    var upUrl = (appname == "Firefox") ? Bandwagon.Controller.CollectionsPane.firefoxUpgradeUrl : Bandwagon.Controller.CollectionsPane.thunderbirdUpgradeUrl;
> -    Bandwagon.Controller.CollectionsPane._openURL(upUrl);
> +    switch (appname) {
> +        case "Firefox":
> +            Bandwagon.Controller.CollectionsPane._openURL(Bandwagon.Controller.CollectionsPane.firefoxUpgradeUrl);
Keep the |var upUrl| and only call Bandwagon.Controller.CollectionsPane._openURL() once at the end of the method.

> +            // or comment out this for other products
Remove this comment please.

> I not see when xxxUpgradeUrl and xxxUpgradeBetaUrl calls. Maybe, it can just be
> ignored?

These are called from bandwagon.xml. I have triggered these manually and they work without error.

Thank you Igor for stepping up to fix this bug.

Comment 31

[Igor Velkov]

Attachment: v4

implemented

Comment 32

[Philip Chee]

Comment on attachment 416756 [details] [diff] [review]
v4

Yes. Everything works. Just a last nit to fix, but I'll give r+ on condition you fix this in the final patch before checkin.

> +    this.seamonkeyUpgradeBetaUrl = "http://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/latest-comm-central-trunk/";
comment. Until KaiRo implements a beta landing page, I guess this will have to do.

> +    var upUrl;

> +        default:
> +            upUrl = Bandwagon.Controller.CollectionsPane.otherUpgradeUrl;

> +    if (upUrl) Bandwagon.Controller.CollectionsPane._openURL(upUrl);

If you have a default then you don't need the if () test. You should also remove the default branch in the switch statement and just do:

var upUrl = Bandwagon.Controller.CollectionsPane.otherUpgradeUrl;

Comment 33

[Igor Velkov]

I think Brian will decide about default behavior, and default url. I only propose an idea what can be there.

If I just comment out a line with intial assignment of a string, but keep final assignment - there will be assignmrnt of an unitialized variable - will it be ok?

Comment 34

[Philip Chee]

> I think Brian will decide about default behavior, and default url. I only
> propose an idea what can be there.

OK.

> If I just comment out a line with intial assignment of a string, but keep final
> assignment - there will be assignmrnt of an unitialized variable - will it be
> ok?

No. Always define your variables (var upUrl;) otherwise you will be referring to a javascript global variable and not a variable inside the scope of that function.

Comment 35

[Igor Velkov]

I ask about 
> +    this.seamonkeyUpgradeBetaUrl = "http://ftp.mozilla.org/pub/mozilla.org/seamonkey/nightly/latest-comm-central-trunk/";
comment. Until KaiRo implements a beta landing page, I guess this will have to
do.

I think I can't just comment out this string. I should do 
this.seamonkeyUpgradeBetaUrl = null;
or this.seamonkeyUpgradeBetaUrl = "";

Or not?

Comment 36

[Brian King [:kinger]]

(In reply to comment #27)
> That would have nightlies, what it seems to want is prereleases, though.
> Our prereleases are usually also mentioned at and linked from the releases/
> page, but I can create a redirect that always send one to the newest alpha/beta
> if one exists. Would that help or be wanted here?

I think so, yes.

Comment 37

[Brian King [:kinger]]

Comment on attachment 416756 [details] [diff] [review]
v4

>+        default:
>+            upUrl = Bandwagon.Controller.CollectionsPane.otherUpgradeUrl;

Default here should be:
Bandwagon.Controller.CollectionsPane.firefoxUpgradeUrl

>+        default:
>+            upUrl = Bandwagon.Controller.CollectionsPane.otherUpgradeBetaUrl;

Default here should be:
Bandwagon.Controller.CollectionsPane.firefoxUpgradeBetaUrl

r=me with those changes. I tested the patch and it works fine. Thanks for doing this.

Comment 38

[Igor Velkov]

Attachment: recommendations implemented

Hope this patch can be checked in.
I think better checkin as possibly (now), and correct seamonkeyUpgradeBetaUrl leter, when KaiRo implement beta landing page.

Comment 39

[Robert Kaiser]

Comment on attachment 416941 [details] [diff] [review]
recommendations implemented

>+    this.seamonkeyUpgradeBetaUrl = "http://www.seamonkey-project.org/releases/";

Please use http://www.seamonkey-project.org/releases/beta - this one will redirect the the main releases page for now, but can be redirected on the server-side as needed.

Comment 40

[Igor Velkov]

Attachment: [for checkin] v6 r=philip.chee r=kinger

Comment 41

[Brian King [:kinger]]

Checked in.

http://viewvc.svn.mozilla.org/vc?revision=57698&view=revision

Please file new bugs for other SM issues.