Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Pale Moon compatibility #107

Closed
wants to merge 6 commits into from
Closed

Add Pale Moon compatibility #107

wants to merge 6 commits into from

Conversation

wolfbeast
Copy link

This PR will add compatibility of FireFTP with Pale Moon 25.0+
Since Pale Moon uses the pre-FF32 API that was switched away from in FireFTP 2.0.21, I had to split the code paths, restoring the previous state for Pale Moon in its own XUL/js modules.

I've tested the result on both Pale Moon 25.0.1 and FF33.0, and both seem to be happy with this.

@mimecuvalo
Copy link
Owner

Thanks for starting up support for Pale moon! This is a good start.

We'll need to whip this patch up into shape though before it lands. It currently looks like you're copy/pasting a lot of code :P

What specifically are you adding that adds Pale moon support in the code - can you point out the relevant code blocks? We should extend the code and avoid copy/pasting.

@wolfbeast
Copy link
Author

I didn't see an easy and organized way to do this otherwise. Pale Moon needs commit 41d32bb backed out completely, and the way I've done this is to make sure the PM compatible version with that commit backed out of baseprotocol.js is called. To walk up the chain to where this distinction can be made in the chrome.manifest, I've had to duplicate a few more files (including calling fireftp-pm.xul from the overlay js instead of just the base file).

Using separate files for separate target applications is common practice for multiple targets in a single XPI, since it allows separated development for the different target applications, without making a complex in-file weave of code for different applications.

If you know of a clear, easy way to avoid file duplication here that is still future-proof, I'd be interested to learn.

The basic code blocks changed are:

  • chrome.manifest: use a different overlay per target with application=GUID
  • overay.xul: include product-specific overlay.js
    No other changes
  • overlay.js: make sure to call the correct fireftp.xul for FF or PM, respectively
    Load() uses chrome://fireftp/content/fireftp-pm.xul instead of chrome://fireftp/content/
  • fireftp-pm.xul: make sure to call the compatible baseprotocol.js file (baseprotocol-pm.js for PM)
    No other changes
  • baseprotocol-pm.js: backed out FF32+-exclusive code that doesn't work on Pale Moon.

Basically:
On FF: chrome.manifest -> fireftpoverlay.xul -> fireftpoverlay.js -> Load() -> fireftp.xul -> baseprotocol.js
On PM: chrome.manifest -> fireftpoverlay-pm.xul -> fireftpoverlay-pm.js -> Load() -> fireftp-pm.xul -> baseprotocol-pm.js

@mimecuvalo
Copy link
Owner

Ok, thank you for the explanation. So, what really needs to be done, to code this up properly and avoid duplication of code is to take the commit 41d32bb and actually rework such that it is backwards-compatible with Pale Moon. It would need to do a check for the existence of support for the new code (check for the presence of Components.interfaces.nsICacheStorageService) and then take the appropriate path in the code to the do the right thing as necessary.

This way, we avoid copying a whole new fireftp.xul and baseProtocol.js. Instead the code would be smart and pick one storage over the other as necessary.

It's curious though that you point out that commit b/c caching, while making things much more performant, isn't strictly necessary for FireFTP to work (it'll just refetch everything all the time). What I'm saying is that since all that code is wrapped in a try/catch it shouldn't matter that it breaks or not really. Pale Moon in theory should still work despite that. Perhaps there's something else that's breaking for you? What errors do you see when trying to run it?

@wolfbeast
Copy link
Author

Well, it's clear that the current release code is not compatible with either previous versions of FF or Pale Moon. The FF32 caching commit is the only commit that breaks it. Simply backing it out provides a fully working extension. I think people who stay with older versions of Firefox would also appreciate a working FireFTP version so if you can find a way to avoid the error and simply disable what was put in after a try{}, that would help more than just Pale Moon users ;)

The error thrown is:
Error message= TypeError: gSiteManager is undefined
URL= chrome://fireftp/content/js/etc/sessionsPasswords.js
Line Number= 303

Which I traced back to the baseprotocol.js file as the culprit. The site manager breaks when trying to initialize (and it's impossible to create a new connection or connect as a result).
Since I'm not sure what exactly you did in the commit, I'm going to have to defer to you if you want to fix it up that way.

@mimecuvalo
Copy link
Owner

You're right - the latest version isn't backwards compatible with previous versions of Firefox (but for those users, there are older versions readily available: https://addons.mozilla.org/en-US/firefox/addon/fireftp/versions )

I tend to make FireFTP backwards compatible only for small changes - and there are these of kinds of breaking changes every couple releases or so these days so the policy is really to just move forward with Firefox, and the older versions will be compatible.

The same might apply to Pale Moon - I imagine your code base will want to stay relatively in sync with Firefox - so eventually your code base will want this newer caching code (unless you've completely forked off?). So for Pale Moon users I would just recommend using an older version of the plugin:
https://addons.mozilla.org/en-US/firefox/addon/fireftp/versions/
(2.0.19).

@wolfbeast
Copy link
Author

We've completely forked off. That's been the reason for the GUID change and all ;) Implementing the new caching code you had to switch to for FF32 is not on our roadmap. Hence my initial patch here splitting the code paths for Pale Moon, as well, since the divergence between Pale Moon and Firefox will only increase from this point forward.

Staying on an older version of FireFTP and having it go stale is really not something we'd want, so that is why I'm trying to work this out with you to be able to keep updated with your releases while having compatibility with Pale Moon.

@mimecuvalo
Copy link
Owner

Ok, gotcha. I can help to rework the patch but it'll take me several weeks - 2 months to get to (since I have a fulltime job). If you need it sooner than that, I'm happy to take a patch in the meantime that basically takes 41d32bb and just does if statements and branches to the appropriate code as necessary. That's the correct way of fixing this. It's as I said above:
"It would need to do a check for the existence of support for the new code (check for the presence of Components.interfaces.nsICacheStorageService) and then take the appropriate path in the code to the do the right thing as necessary."

@wolfbeast
Copy link
Author

I don't think there's a terrible hurry here. As you said, people can, for the time being, use 2.0.19 on Pale Moon.
I myself simply don't have the time either to do this in the way you describe, since I have a lot of browser core work to do on my end. So, thanks in advance for your time! I'm sure it will be done "when it's done" ;)

You can close this pull request if you don't plan on taking it this way.

@Lootyhoof
Copy link
Contributor

Good news: Pale Moon 27 (based more-or-less on Firefox 38), currently in alpha, is mostly compatible with current FireFTP. The only part of this PR that still applies is cb1cdf4 (though minVersion would need to be 27.0.0a1 for maximum compatibility at the moment). :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants