1278074 - remove STEEL uses in Thunderbird

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[:aceman]

Remove all uses of STEEL interfaces in base Thunderbird.
Proof of concept whether the migration is doable.

Comment 1

[:aceman]

In addition to conversions mentioned in bug 1278067 I also do the changes from bug 989307.

Comment 2

[:aceman]

Attachment: patch

This is what it would take for Thunderbird (including /im and /calendar). Removes all uses of Application.* .

Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d67d6292f7da9ade6bfcc4e19024ce642f1b2264

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 8760044 [details] [diff] [review]
patch

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

Looks like there are more of the below wrong default pattern elsewhere too

::: mail/base/content/glodaFacetView.js
@@ +359,5 @@
>        queryExplanation.setQuery(this.collection.query);
>      // we like to sort them so should clone the list
>      this.faceters = this.facetDriver.faceters.concat();
>  
> +    this._timelineShown = !Services.prefs.getBoolPref("gloda.facetview.hidetimeline", true);

nsIPrefBranch doesn't let you have a default value like this

Comment 4

[:aceman]

Using the restartApplication() from BrowserUtils and killing all the Application.* uses solves Neil's comment in bug 1090880 comment 3.

Comment 5

[aleth [:aleth]]

Comment on attachment 8760044 [details] [diff] [review]
patch

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

Looks sensible to me.

::: im/content/preferences/advanced.js
@@ -8,5 @@
>  Components.utils.import("resource://gre/modules/ctypes.jsm");
>  Components.utils.import("resource://gre/modules/Services.jsm");
>  Components.utils.import("resource://gre/modules/LoadContextInfo.jsm");
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> -Components.utils.import("resource://gre/modules/BrowserUtils.jsm");

Why did you remove this? (I agree it looks like it's not used, but was there another reason?)

Comment 6

[:aceman]

Attachment: patch v1.1

Good catch about the defaults passed into getBoolPref(). I've verified that all the fetched prefs do exist in mailnews.js or all-thunderbird.js, and also often they are already fetched at other places (with getBoolPref). So we do not need to rely on the default.

Comment 7

[:aceman]

(In reply to aleth [:aleth] from comment #5)
> ::: im/content/preferences/advanced.js
> @@ -8,5 @@
> >  Components.utils.import("resource://gre/modules/ctypes.jsm");
> >  Components.utils.import("resource://gre/modules/Services.jsm");
> >  Components.utils.import("resource://gre/modules/LoadContextInfo.jsm");
> >  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> > -Components.utils.import("resource://gre/modules/BrowserUtils.jsm");
> 
> Why did you remove this? (I agree it looks like it's not used, but was there
> another reason?)

I noticed it only because I was importing BrowserUtils elsewhere in this patch so I looked around a bit:)
It looks unneeded to me, and the changeset that added it didn't seem to use it either (it may be just copypasted from FF). It may be that also some of the other imports are unneeded.
But yes, it basically is not required for this patch so tell me if I should drop it :)

Comment 8

[Magnus Melin [:mkmelin]]

Comment on attachment 8760089 [details] [diff] [review]
patch v1.1

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

::: mail/base/content/mailWindow.js
@@ +425,5 @@
>  function loadStartPage(aForce)
>  {
>    // If the preference isn't enabled, then don't load anything.
>    if (!aForce &&
> +      !Services.prefs.getBoolPref("mailnews.start_page.enabled"))

nit: this should now fit nicely on one row

::: mail/base/content/mailWindowOverlay.js
@@ +3032,5 @@
>      let brandName = this.brandBundle.getString("brandShortName");
>      let remoteContentMsg = this.stringBundle.getFormattedString("remoteContentBarMessage",
>                                                                  [brandName]);
>  
> +    let buttonLabel = this.stringBundle.getString((AppConstants.platform == "macosx") ?

win, not macosx

@@ +3037,2 @@
>        "remoteContentPrefLabel" : "remoteContentPrefLabelUnix");
> +    let buttonAccesskey = this.stringBundle.getString((AppConstants.platform == "win") ?

this too, but I guess it matters less since macs don't show access keys (i think)

::: mail/components/about-support/content/prefs.js
@@ +106,5 @@
>  }
>  
>  function getModifiedPrefs() {
>    // We use the low-level prefs API to identify prefs that have been
> +  // modified, rather than extApplication.js::prefs.all since the latter is

don't know what this is

::: mail/components/preferences/permissions.js
@@ +355,5 @@
>  
>    onPermissionKeyPress: function (aEvent)
>    {
>      if (aEvent.keyCode == KeyEvent.DOM_VK_DELETE
> +        || ((AppConstants.platform == "macosx") && aEvent.keyCode == KeyEvent.DOM_VK_BACK_SPACE)

while you're here, put || on the previous line

Comment 9

[aleth [:aleth]]

Comment on attachment 8760089 [details] [diff] [review]
patch v1.1

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

::: mail/base/content/tabmail.xml
@@ +277,5 @@
>      <implementation implements="nsIController">
>        <constructor>
>          window.controllers.insertControllerAt(0, this);
>          this._restoringTabState = null;
> +        Components.utils.import("resource://gre/modules/AppConstants.jsm");

You can move the import to where you need AppConstants if you like.

::: mail/steel/steelApplication.js
@@ +11,2 @@
>  var APPLICATION_CID = Components.ID("f265021a-7f1d-4b4b-bdc6-9aedca4d8f13");
>  var APPLICATION_CONTRACTID = "@mozilla.org/steel/application;1";

I guess this can't be removed altogether due to addons?

@@ +33,5 @@
>  #include ../../mozilla/toolkit/components/exthelper/extApplication.js
>  
>  function Application() {
> +  Deprecated.warning("STEEL is deprecated, you should use the add-on SDK instead.",
> +                     "https://developer.mozilla.org/Add-ons/SDK/");

Isn't the Addon-SDK deprecated as well? Better to point people at AppConstants (assuming that's the main use case in extensions)

Comment 10

[:aceman]

(In reply to aleth [:aleth] from comment #9)
> ::: mail/steel/steelApplication.js
> @@ +11,2 @@
> >  var APPLICATION_CID = Components.ID("f265021a-7f1d-4b4b-bdc6-9aedca4d8f13");
> >  var APPLICATION_CONTRACTID = "@mozilla.org/steel/application;1";
> 
> I guess this can't be removed altogether due to addons?

Yes, that is discussed in bug 1278067. In the bug here I intentionally do not remove whole steel.

> @@ +33,5 @@
> >  #include ../../mozilla/toolkit/components/exthelper/extApplication.js
> >  
> >  function Application() {
> > +  Deprecated.warning("STEEL is deprecated, you should use the add-on SDK instead.",
> > +                     "https://developer.mozilla.org/Add-ons/SDK/");
> 
> Isn't the Addon-SDK deprecated as well? Better to point people at
> AppConstants (assuming that's the main use case in extensions)

That is what Firefox had in the message. But yes, I can add AppConstants too.

Comment 11

[:aceman]

(In reply to Magnus Melin from comment #8)
> ::: mail/base/content/mailWindowOverlay.js
> @@ +3032,5 @@
> > +    let buttonLabel = this.stringBundle.getString((AppConstants.platform == "macosx") ?
> 
> win, not macosx

Thanks, too much copy-pasting :)

> ::: mail/components/about-support/content/prefs.js
> @@ +106,5 @@
> >  }
> >  
> >  function getModifiedPrefs() {
> >    // We use the low-level prefs API to identify prefs that have been
> > +  // modified, rather than extApplication.js::prefs.all since the latter is
> 
> don't know what this is
The comment was talking about Application.prefs.all which we want to lose (will not exist if we do bug 1278067). But that functionality still exists in extApplication.js, in the method prefs.all . So I preserved the comment in case somebody gets the idea to use that. Maybe the comment was copied from Firefox (much of about:config we just port to TB, e.g. the GFX section is broken today and needs porting (there is a bug)).
Or should I remove the comment?

Comment 12

[aleth [:aleth]]

(In reply to :aceman from comment #10)
> (In reply to aleth [:aleth] from comment #9)
> > Isn't the Addon-SDK deprecated as well? Better to point people at
> > AppConstants (assuming that's the main use case in extensions)
> 
> That is what Firefox had in the message. But yes, I can add AppConstants too.

That message was probably from a time when the Add-on SDK was "the new way to write addons" ;) Today it would push web extensions, but that doesn't work for TB...

Comment 13

[:aceman]

(In reply to aleth [:aleth] from comment #12)
> That message was probably from a time when the Add-on SDK was "the new way
> to write addons" ;) Today it would push web extensions, but that doesn't
> work for TB...

See starting at bug 1090880 comment 21 (in 2016), it seems they still allow for Add-on SDK, but Webextensions are on top.

So yes, I can add that to the warning.

Comment 14

[:aceman]

(In reply to aleth [:aleth] from comment #9)
> ::: mail/base/content/tabmail.xml
> @@ +277,5 @@
> >      <implementation implements="nsIController">
> >        <constructor>
> >          window.controllers.insertControllerAt(0, this);
> >          this._restoringTabState = null;
> > +        Components.utils.import("resource://gre/modules/AppConstants.jsm");
> 
> You can move the import to where you need AppConstants if you like.

I hope constructor is only run once, but the function where AppConstants are used may be called often.

Comment 15

[:aceman]

Attachment: patch v1.2

Comment 16

[Magnus Melin [:mkmelin]]

Comment on attachment 8760318 [details] [diff] [review]
patch v1.2

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

::: mail/base/content/mailWindow.js
@@ +425,5 @@
>  function loadStartPage(aForce)
>  {
>    // If the preference isn't enabled, then don't load anything.
>    if (!aForce &&
> +      !Services.prefs.getBoolPref("mailnews.start_page.enabled"))

nit: looks like it could fit on one row

::: mail/steel/steelApplication.js
@@ +32,5 @@
>  
>  #include ../../mozilla/toolkit/components/exthelper/extApplication.js
>  
>  function Application() {
> +  Deprecated.warning("STEEL is deprecated, you should use AppConstants.jsm, Services.jsm or WebExtensions.",

but we don't support webextensions...! so drop that part, and maybe link to one of the .jsms

Comment 17

[:aceman]

Attachment: patch v1.3

Thanks fixed the depreciation message

Comment 18

[:aceman]

https://hg.mozilla.org/comm-central/rev/3af670f17709f87c653d9f4587a3b6b8094f6ff2