Last Comment Bug 763106 - convert /mail/components/preferences/*.js to Services.jsm and MailServices.js
: convert /mail/components/preferences/*.js to Services.jsm and MailServices.js
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: Preferences (show other bugs)
: Trunk
: All All
: -- minor (vote)
: Thunderbird 18.0
Assigned To: :aceman
:
Mentors:
Depends on: 718139 806124
Blocks: 720356 720358
  Show dependency treegraph
 
Reported: 2012-06-08 16:35 PDT by :aceman
Modified: 2012-10-27 12:17 PDT (History)
2 users (show)
ryanvm: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch (48.02 KB, patch)
2012-09-02 12:51 PDT, :aceman
no flags Details | Diff | Review
patch v2 (48.44 KB, patch)
2012-09-13 12:26 PDT, :aceman
mconley: review+
Details | Diff | Review
patch v3 (47.77 KB, patch)
2012-09-24 13:10 PDT, :aceman
acelists: review+
Details | Diff | Review

Description :aceman 2012-06-08 16:35:39 PDT
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 2012-09-02 12:51:35 PDT
Created attachment 657702 [details] [diff] [review]
patch
Comment 2 Mike Conley (:mconley) - (Away until June 29th) 2012-09-12 11:48:43 PDT
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 2012-09-12 13:15:10 PDT
(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) - (Away until June 29th) 2012-09-13 07:29:23 PDT
(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 2012-09-13 12:26:14 PDT
Created attachment 660927 [details] [diff] [review]
patch v2

Thanks, done.
Comment 6 Mike Conley (:mconley) - (Away until June 29th) 2012-09-24 12:28:27 PDT
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 2012-09-24 13:10:34 PDT
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:)
Comment 8 Ryan VanderMeulen [:RyanVM] 2012-09-24 15:16:12 PDT
https://hg.mozilla.org/comm-central/rev/089215e2d64f

Note You need to log in before you can comment on or make changes to this bug.