763106 - convert /mail/components/preferences/*.js to Services.jsm and MailServices.js

Status: RESOLVED FIXED

(Thunderbird :: Preferences, defect)

Description

[:aceman]

advanced.js:      var psvc = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
advanced.js:    var cacheService = Components.classes["@mozilla.org/network/cache-service;1"]
applications.js:  _prefSvc: Components.classes["@mozilla.org/preferences-service;1"]
applications.js:  _prefSvc      : Components.classes["@mozilla.org/preferences-service;1"]
applications.js:  _ioSvc        : Components.classes["@mozilla.org/network/io-service;1"]
applications.js:      Components.classes["@mozilla.org/observer-service;1"]
applications.js:    let promptSvc = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
attachmentReminder.js:    this.prefs = Components.classes["@mozilla.org/preferences-service;1"]
attachmentReminder.js:    this.promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
connection.js:        Components.classes["@mozilla.org/preferences-service;1"].
cookies.js:  _cm               : Components.classes["@mozilla.org/cookiemanager;1"]
cookies.js:    var os = Components.classes["@mozilla.org/observer-service;1"]
cookies.js:    var os = Components.classes["@mozilla.org/observer-service;1"]
cookies.js:    var psvc = Components.classes["@mozilla.org/preferences-service;1"]
downloads.js:    var fileLocator = Components.classes["@mozilla.org/file/directory_service;1"]
downloads.js:    var ios = Components.classes["@mozilla.org/network/io-service;1"]
fonts.js:      Components.classes["@mozilla.org/observer-service;1"]
general.js:    var formatter = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"].
general.js:    var formatter = Components.classes["@mozilla.org/toolkit/URLFormatterService;1"].
general.js:      var ios = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);
general.js:      var ioService = Components.classes["@mozilla.org/network/io-service;1"].getService(Components.interfaces.nsIIOService);
permissions.js:  _pm           : Components.classes["@mozilla.org/permissionmanager;1"]
permissions.js:      var ioService = Components.classes["@mozilla.org/network/io-service;1"]
permissions.js:      var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
permissions.js:    var os = Components.classes["@mozilla.org/observer-service;1"]
permissions.js:    var os = Components.classes["@mozilla.org/observer-service;1"]
security.js:    var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
security.js:    var junkmailPlugin = Components.classes["@mozilla.org/messenger/filter-plugin;1?name=bayesianfilter"]
security.js:      var promptService = Cc["@mozilla.org/embedcomp/prompt-service;1"].
sendoptions.js:    var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService();
sendoptions.js:    var promptService = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService();

Comment 1

[:aceman]

Attachment: patch

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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

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

Thanks for the patch, aceman! This looks really good. Just a few questions / comments - see below.

::: mail/components/preferences/applications.js
@@ +1238,5 @@
>                                                      [aHandlerInfo.plugin.name,
>                                                       this._brandShortName]);
> +      default:
> +        // Hopefully this never happens.
> +        Components.utils.reportError("No description for action found!");

If we're writing out the error, we might as well also provide the value that we weren't catching - aHandlerInfo.preferredAction, in this case.

@@ +1775,5 @@
>          let (preferredApp = aHandlerInfo.preferredApplicationHandler) {
>            if (this.isValidHandlerApp(preferredApp))
>              return this._getIconURLForHandlerApp(preferredApp);
> +
> +          return ICON_URL_APP;

Hm - this isn't logically equivalent. I guess this is fixing a bug? (The case where isValidHandlerApp is false and we return nothing)

::: mail/components/preferences/cookies.js
@@ +298,5 @@
>            return "";
>          if (aColumn.id == "domainCol")
>            return item.rawHost;
>          else if (aColumn.id == "nameCol")
> +          return ("name" in item) ? item.name : "";

So what are the cases where name is undefined?

::: mail/components/preferences/fonts.js
@@ +172,5 @@
>    mCharsetMenuInitialized: false,
>    readDefaultCharset: function()
>    {
>      if (!this.mCharsetMenuInitialized)
> +    { Services.obs.notifyObservers(null, "charsetmenu-selected", "mailedit");

This is a bit weird - I think this shouldn't be on the same line as the opening brace.

::: mail/components/preferences/general.js
@@ +56,5 @@
>    {
>      // convert the file url into a nsILocalFile
>      if (aFileURL)
>      {
> +      return Services.io.getProtocolHandler("file")

If you're doing it this way, I'd prefer:

return Services.io
               .getProtocolHandler("file")
               .QueryInterface(Components.interfaces.nsIFileProtocolHandler)
               .getFileFromURLSpec(aFileURL);

::: mail/components/preferences/permissions.js
@@ +74,5 @@
>    {
>      var textbox = document.getElementById("url");
>      var host = textbox.value.replace(/^\s*([-\w]*:\/+)?/, ""); // trim any leading space and scheme
>      try {
> +      var uri = Services.io.newURI("http://"+host, null, null);

Spaces on either side of the +

::: mail/components/preferences/security.js
@@ +64,4 @@
>        return;
>  
>      // otherwise go ahead and remove the training data
> +    MailServices.junk.resetTrainingData();

It looks like the old code handled a case where MailServices.junk was undefined. Does such a case not exist anymore?

Comment 3

[:aceman]

(In reply to Mike Conley (:mconley) from comment #2)
> @@ +1775,5 @@
> >          let (preferredApp = aHandlerInfo.preferredApplicationHandler) {
> >            if (this.isValidHandlerApp(preferredApp))
> >              return this._getIconURLForHandlerApp(preferredApp);
> > +
> > +          return ICON_URL_APP;
> 
> Hm - this isn't logically equivalent. I guess this is fixing a bug? (The
> case where isValidHandlerApp is false and we return nothing)

Yeah, this is solving a "function does not always return a value" strict warning. Any better proposals?

> ::: mail/components/preferences/cookies.js
> @@ +298,5 @@
> >            return "";
> >          if (aColumn.id == "domainCol")
> >            return item.rawHost;
> >          else if (aColumn.id == "nameCol")
> > +          return ("name" in item) ? item.name : "";
> 
> So what are the cases where name is undefined?
It seems is happens when the cookies are grouped by server and the server lines only have one cell (columns), while the cookie lines have 2. That is only my feeling. This solves strict warnings of "reference to undefined property".

> ::: mail/components/preferences/security.js
> @@ +64,4 @@
> >        return;
> >  
> >      // otherwise go ahead and remove the training data
> > +    MailServices.junk.resetTrainingData();
> 
> It looks like the old code handled a case where MailServices.junk was
> undefined. Does such a case not exist anymore?

Not sure so I add it back.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

(In reply to :aceman from comment #3)
> (In reply to Mike Conley (:mconley) from comment #2)
> > @@ +1775,5 @@
> > >          let (preferredApp = aHandlerInfo.preferredApplicationHandler) {
> > >            if (this.isValidHandlerApp(preferredApp))
> > >              return this._getIconURLForHandlerApp(preferredApp);
> > > +
> > > +          return ICON_URL_APP;
> > 
> > Hm - this isn't logically equivalent. I guess this is fixing a bug? (The
> > case where isValidHandlerApp is false and we return nothing)
> 
> Yeah, this is solving a "function does not always return a value" strict
> warning. Any better proposals?
> 

I think I'd prefer to drop the default from the switch statement, and just have the return show up at the end of the function.

> 
> > ::: mail/components/preferences/security.js
> > @@ +64,4 @@
> > >        return;
> > >  
> > >      // otherwise go ahead and remove the training data
> > > +    MailServices.junk.resetTrainingData();
> > 
> > It looks like the old code handled a case where MailServices.junk was
> > undefined. Does such a case not exist anymore?
> 
> Not sure so I add it back.

So I did a little digging around, and I don't think such a case exists. I think the original implementer was being overly cautious. I think we can remove it.

Sorry for giving you the runaround.

Comment 5

[:aceman]

Attachment: patch v2

Thanks, done.

Comment 6

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 660927 [details] [diff] [review]
patch v2

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

If below wasn't a mistake, r=me. Thanks!

::: mail/components/preferences/applications.js
@@ -37,5 @@
>  // was set by us to a custom handler icon and CSS should not try to override it.
>  const APP_ICON_ATTR_NAME = "appHandlerIcon";
>  
>  // CloudFile account tools used by gCloudFileTab.
> -Components.utils.import("resource://gre/modules/Services.jsm");

Why is this being removed?

Comment 7

[:aceman]

Attachment: patch v3

(In reply to Mike Conley (:mconley) from comment #6)
> ::: mail/components/preferences/applications.js
> >  // CloudFile account tools used by gCloudFileTab.
> > -Components.utils.import("resource://gre/modules/Services.jsm");
> 
> Why is this being removed?

It seems to work even without it. But I can't remember/prove right now why it works, so I'll put it back for safety:)

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/089215e2d64f