Closed Bug 1278067 Opened 3 years ago Closed 2 years ago

remove STEEL module from Thunderbird

Categories

(Thunderbird :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 57.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

(Keywords: addon-compat, dev-doc-needed)

Attachments

(1 file, 1 obsolete file)

14.35 KB, patch
aceman
: review+
Details | Diff | Splinter Review
Bug 1090880 has removed FUEL module from Firefox. The Thunderbird equivalent is STEEL. But in contrast to FUEL, it does almost nothing. It only adds some .platformIs* methods to check the platform (OS). These can easily be replaced by AppConstants.platform these days.

Calls to Application.prefs can be replaced by Services.prefs.
Application.console can be replaced by Services.prefs.

Thus it seems STEEL itself just duplicates already existing interfaces (surely, maybe those didn't exist when STEEL started).

I'll make another bug with patch to remove just the uses of STEEL in Thunderbird to see how feasible this is. Then in the bug here we can decide when to remove STEEL itself to give addons some time.
The rationale for the removal would be that we should not have to maintain one more interface that differs from Firefox at these times when m-c is removing/changing other more important components.

There are about 70 files in addons that reference steelIApplication directly, for various purposes (e.g. detecting if running under TB, or Application.console.* or
Depends on: 1278074
Attached patch patch (obsolete) — Splinter Review
Sample of what it would take to remove STEEL itself. It can be seen it a very think wrapper of extIApplication.

This is not for review/landing yet, we need to decide on the strategy when to do that.
Attachment #8760045 - Flags: feedback?(rkent)
One important difference between Thunderbird and Firefox is that FF maintains addon-compat flags, which then result in addons getting scanned for offending code, and addon authors notified. We have not maintained that capability, so when we remove functionality, what happens is lots of addons just break with no warning. Many are not being maintained as well. Very few are fixed before a few releases go by.

70 references to STEEL in addons could be a lot.

This issue is only going to get worse, as FF tries to move addons away from XUL overlays and XPCOM toward technologies that we do not support.

So I'd like some comments about how removing this fits into a strategy of maintaining addon compatibility withing Thunderbird. Have the changes in FF already broken us wrt STEEL? Do we have reasonable alternatives that do not result in our addons simply getting silently broken?
BTW adding "addon-compat" to this bug does not result in someone writing the code to check addons for compatibility like it does in FF.
(In reply to Kent James (:rkent) from comment #3)
> One important difference between Thunderbird and Firefox is that FF
> maintains addon-compat flags, which then result in addons getting scanned
> for offending code, and addon authors notified. We have not maintained that
> capability, so when we remove functionality, what happens is lots of addons
> just break with no warning. Many are not being maintained as well. Very few
> are fixed before a few releases go by.

In bug 1278074 I add a depreciation warning that should be shown whenever somebody uses Application. So maintainters should be notified. Firefox also did that.

> So I'd like some comments about how removing this fits into a strategy of
> maintaining addon compatibility withing Thunderbird. Have the changes in FF
> already broken us wrt STEEL? Do we have reasonable alternatives that do not
> result in our addons simply getting silently broken?

The changes in FF have not YET broken our STEEL. But as Neil pointed out, that is unless m-c removes extApplication.js . That file is only included in STEEL and Seamonkey's SMILE.

There are NO uses of Application.* in m-c that I can see. There seems to be a single use of it in
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_fuel.js
This file mentions to test FUEL, however it may be just a forgotten remnant as FUEL is gone. It actually uses extApplication.

So it is a question for how long this file/support will survive.

(In reply to Kent James (:rkent) from comment #4)
> BTW adding "addon-compat" to this bug does not result in someone writing the
> code to check addons for compatibility like it does in FF.

Sure, it was meant in the sense, that this bug will affect addons so it should be mentioned in the release notes, or "TB 52 changes for developers" page.

So as I said, we can remove all uses of STEEL in TB itself, but leave STEEL shipped until it is possible (with the depreciation warning). That is why I split it into 2 bugs.
Comment on attachment 8760045 [details] [diff] [review]
patch

So with your explanation, the intent as I understand it is to deprecate STEEL with a warning message and remove it post-TB52. If that is the plan, I can feedback+

Just as an example, with Exchange Web Services I can specify 2007 as the version of the protocol and it still works in modern versions of Exchange Server, 9 years later. There is a value to stability in protocols, and giving app developers lots of time to respond.
Attachment #8760045 - Flags: feedback?(rkent) → feedback+
If Kent is in favour for the removal, it shouldn't be hard get an r+ for this since it's a straight forward removal. So, Aceman, are we going ahead with this?
This is not urgent unless m-c removes extApplication.js.
The deprecation message was already done in bug 1278074. So now we leave addon devs some time to notice the message and update their addons. I agree with the plan of landing this patch after TB52.
There is a non-trivial amount of addons using this so I think this leaves them about a year of support on release branch (up to release of TB59). In that time they should be able to easily update (there are replacement APIs), they just need to get to it.
What we need to do until then is to update https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Toolkit_API/STEEL and make it have a deprecation message in the same way https://developer.mozilla.org/en-US/docs/Mozilla/Tech/Toolkit_API/FUEL has. That is why this has the "dev-doc-needed" flag. Does anybody have perms to edit that page?
Edited.
I can edit the page, too. I've added this bug as a reference.
Still about 100 hits of steelIApplication in all addons. Compared to 400 hits of fuelIApplication which does not exist anymore ;) Looks like some addons are abandoned.
Depends on: 1354205
Comment on attachment 8760045 [details] [diff] [review]
patch

Ok, due to movement in bug 1354205, we may need to proceed here and finally remove STEEL. We need a high level decision whether to remove STEEL (it is not used in TB) or move extApplication.js (and whatever else is needed to /mailnews).
Seamonkey also needs it (and probably much more as SMILE is much larger than the stub STEEL is). So if Seamonkey wants to adopt extApplication.js we could follow and keep STEEL.
Attachment #8760045 - Flags: review?(rkent)
Attachment #8760045 - Flags: review?(mkmelin+mozilla)
Attachment #8760045 - Flags: feedback?(frgrahl)
Comment on attachment 8760045 [details] [diff] [review]
patch

Review of attachment 8760045 [details] [diff] [review]:
-----------------------------------------------------------------

Pretty clear to me we should just remove it. 
It may break some extensions, but in the grand scheme of things, this is not a major worry.
Attachment #8760045 - Flags: review?(mkmelin+mozilla) → review+
I came in a little late into the SeaMonkey game and don't know much about the history. Looking it the removal patch I would rather keep it for now in SeaMonkey and remove it later when porting Web Extensions. We already have a few other tasks at hand which need to be addressed first. IanN should know better what would be best for SeaMonkey.
Flags: needinfo?(iann_bugzilla)
Kent has already agreed to the removal of STEEL, see comment #6 and the f+. Why ask again?

You know I appreciate our SM friend a lot, but why would we follow them? Since they maintain a suite of applications, they naturally have a lot of things we don't have. So I would just remove STEEL from mail/ and whatever they want to do they can do in suite/ code.
Attachment #8760045 - Flags: review?(rkent) → review+
Is SM using Steel? They don't have it in their package-manifest.in so how can they possibly be using it?
On the other hand, you may be able to migrate Seamonkey away from using the SMILE APIs (see what we did in bug 1278074) and then you are done. There are only *14* addons referencing smileIApplication, which is much less than those using STEEL or FUEL, so you probably can just ignore that part.
(In reply to Jorg K (GMT+2) from comment #17)
> Is SM using Steel? They don't have it in their package-manifest.in so how
> can they possibly be using it?

They have SMILE which seems to be FUEL+STEEL merged. So they use extApplication.js again, similar to STEEL.

(In reply to Jorg K (GMT+2) from comment #16)
> You know I appreciate our SM friend a lot, but why would we follow them?

IF SM decides to migrate extApplication.js into mailnews for their needs, we can just continue to use it, keeping STEEL for addons. Why break addons if we can avoid it. They already break too often.
>> Why break addons if we can avoid it. 

That is what worries me the most. Mozilla already acts as if thy do not exist. I personally don't want SeaMonkey to go down the same path at least as long as a release based on ESR 59 is possible. After this everything need to change anyway.

But don't worry. If you don't want it we can copy it under suite. Already did this with the help viewer and old console. Appreciate the heads up.
Attachment #8760045 - Flags: feedback?(frgrahl) → feedback+
(In reply to :aceman from comment #19)
> IF SM decides to migrate extApplication.js into mailnews for their needs, we
> can just continue to use it, keeping STEEL for addons. Why break addons if
> we can avoid it. 

Everything has a maintenance burden. It's not reasonable to put it in mailnews just because SeaMonkey would like to keep it for a short while, and in the process essentially add the Thunderbird maintenance burden.
(In reply to Frank-Rainer Grahl (:frg) from comment #20)
> >> Why break addons if we can avoid it. 
> 
> That is what worries me the most. Mozilla already acts as if thy do not
> exist. I personally don't want SeaMonkey to go down the same path at least
> as long as a release based on ESR 59 is possible. After this everything need
> to change anyway.

But in this case I think SM does not have this problem. Removing this, you affect only 14 addons (on AMO).
TB will affect 100 addons and it still seems we decided to do it. FF killed ~400 addons with FUEL.
Where is the list of affected addons?
Attached patch patch v1.1Splinter Review
Unbitrotted the patch.
Attachment #8760045 - Attachment is obsolete: true
Attachment #8893552 - Flags: review+
(In reply to Wayne Mery (:wsmwk, NI for questions) from comment #23)
> Where is the list of affected addons?

https://dxr.mozilla.org/addons , but it is tedious to get their names, they are listed by some AMO ID.
I have a list of add-ons with ID and number of installations, can't you join tables in Excel/L/O Calc?
Reading comment #0 it should be easy for actively maintained add-ons to replace STEEL since "it does almost nothing".
For the record, I posted about the removal into mozilla.dev.apps.thunderbird on 14th of March 2017 and there were no responses.

So if we want to land the removal, the patch is ready.
Coming up ;-)
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/969a4b75e9e5
remove STEEL module from Thunderbird. r=mkmelin
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Gone ;-)
Target Milestone: --- → Thunderbird 57.0
Not sure any info is needed from me now.
Flags: needinfo?(iann_bugzilla)
(In reply to Ian Neal from comment #32)
> Not sure any info is needed from me now.

You guys need to decide quickly whether you at least temporarily move extApplication.js into /suite because bug 	
1090880 has landed and the file is removed. Or whether you can remove SMILE fast so that you are not busted at next build. But it does not need to be in this bug.
You need to log in before you can comment on or make changes to this bug.