The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 18.0

Status

Thunderbird
Preferences
--
minor
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

(Blocks: 2 bugs)

Trunk
Thunderbird 18.0
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

47.77 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
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();
(Assignee)

Comment 1

5 years ago
Created attachment 657702 [details] [diff] [review]
patch
Attachment #657702 - Flags: review?(mconley)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
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?
(Assignee)

Comment 3

5 years ago
(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.
(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.
(Assignee)

Comment 5

5 years ago
Created attachment 660927 [details] [diff] [review]
patch v2

Thanks, done.
Attachment #657702 - Attachment is obsolete: true
Attachment #657702 - Flags: review?(mconley)
Attachment #660927 - Flags: review?(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?
Attachment #660927 - Flags: review?(mconley) → review+
(Assignee)

Comment 7

5 years ago
Created attachment 664178 [details] [diff] [review]
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:)
Attachment #660927 - Attachment is obsolete: true
Attachment #664178 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/089215e2d64f
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0

Updated

5 years ago
Depends on: 806124
You need to log in before you can comment on or make changes to this bug.