Last Comment Bug 863617 - Fix leftovers from switch to Services.jsm and mailServices.js
: Fix leftovers from switch to Services.jsm and mailServices.js
Status: RESOLVED FIXED
:
Product: Thunderbird
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 24.0
Assigned To: Sebastian H. [:aryx][:archaeopteryx]
:
Mentors:
Depends on:
Blocks: 720356 720358
  Show dependency treegraph
 
Reported: 2013-04-19 01:24 PDT by Sebastian H. [:aryx][:archaeopteryx]
Modified: 2013-07-23 04:34 PDT (History)
2 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
+
fixed


Attachments
patch, v2 (13.76 KB, patch)
2013-04-19 01:24 PDT, Sebastian H. [:aryx][:archaeopteryx]
mconley: review+
Details | Diff | Splinter Review
patch, v3, r=mconley [checked in, comment 5] (13.84 KB, patch)
2013-05-19 11:25 PDT, Sebastian H. [:aryx][:archaeopteryx]
no flags Details | Diff | Splinter Review
fix smtpServer falls, v1 (4.20 KB, patch)
2013-06-07 15:16 PDT, Sebastian H. [:aryx][:archaeopteryx]
mconley: review+
Details | Diff | Splinter Review
fix smtpServer calls, v2, r=mconley [checked in, comment 11] (4.29 KB, patch)
2013-07-09 12:10 PDT, Sebastian H. [:aryx][:archaeopteryx]
standard8: approval‑comm‑aurora+
Details | Diff | Splinter Review

Description Sebastian H. [:aryx][:archaeopteryx] 2013-04-19 01:24:15 PDT
Created attachment 739464 [details] [diff] [review]
patch, v2

aceman found some code which can be converted to Services.jsm / mailServices.js. Successful Thunderbird-Try run: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=00293112bed4
Comment 1 :aceman 2013-04-30 11:34:57 PDT
OK, this seems to cover some of the remaining users, but after applying the patch I still see these occurrences:

mailnews:
news/test/unit/test_uriParser.js:  let nntpService = Cc["@mozilla.org/messenger/nntpservice;1"]
news/test/unit/test_getNewsMessage.js:    var nntpService = Cc["@mozilla.org/messenger/nntpservice;1"]
compose/test/unit/test_nsSmtpService1.js:const SmtpServiceContractID = "@mozilla.org/messengercompose/smtp;1";
extensions/bayesian-spam-filter/test/unit/test_traits.js:  Cc["@mozilla.org/messenger/filter-plugin;1?name=bayesianfilter"]
extensions/bayesian-spam-filter/test/unit/test_traits.js:        Cc["@mozilla.org/messenger/filter-plugin;1?name=bayesianfilter"]
extensions/bayesian-spam-filter/test/unit/test_msgCorpus.js:  Cc["@mozilla.org/messenger/filter-plugin;1?name=bayesianfilter"]

mail:
base/content/plugins.js:                                   "@mozilla.org/xre/app-info;1",
base/content/aboutDialog.js:                                     "@mozilla.org/extensions/blocklist;1",
base/test/unit/test_windows_font_migration.js:  let sysInfo = Cc["@mozilla.org/system-info;1"]

(Ignoring mozmill itself and testpilot.) Are these intentionally left out or inconvertible?
Comment 2 Sebastian H. [:aryx][:archaeopteryx] 2013-05-01 02:53:06 PDT
(In reply to :aceman from comment #1)
> OK, this seems to cover some of the remaining users, but after applying the
> patch I still see these occurrences:
> 
> mailnews:
> news/test/unit/test_uriParser.js:  let nntpService =
> Cc["@mozilla.org/messenger/nntpservice;1"]
> news/test/unit/test_getNewsMessage.js:    var nntpService =
> Cc["@mozilla.org/messenger/nntpservice;1"]
These are tests for the news interface itself, so they shouldn't be changed.

> compose/test/unit/test_nsSmtpService1.js:const SmtpServiceContractID =
> "@mozilla.org/messengercompose/smtp;1";
This is a test for the smtp interface itself, so it shouldn't be changed.

> extensions/bayesian-spam-filter/test/unit/test_traits.js: 
> Cc["@mozilla.org/messenger/filter-plugin;1?name=bayesianfilter"]
> extensions/bayesian-spam-filter/test/unit/test_traits.js:       
> Cc["@mozilla.org/messenger/filter-plugin;1?name=bayesianfilter"]
> extensions/bayesian-spam-filter/test/unit/test_msgCorpus.js: 
> Cc["@mozilla.org/messenger/filter-plugin;1?name=bayesianfilter"]
These are tests for the junk interface itself, so they shouldn't be changed.


> mail:
> base/content/plugins.js:                                  
> "@mozilla.org/xre/app-info;1",
This calls nsICrashReporter which is not covered by Services.jsm

> base/content/aboutDialog.js:                                    
> "@mozilla.org/extensions/blocklist;1",
blocklist has been added to Services.jsm after this bug has been added and is already covered by bug 864838.

> base/test/unit/test_windows_font_migration.js:  let sysInfo =
> Cc["@mozilla.org/system-info;1"]
This calls nsIWriteablePropertyBag2 which is not covered by Services.jsm

> (Ignoring mozmill itself and testpilot.) Are these intentionally left out or
> inconvertible?
mozmill: As far as I know mozmill had been used before for Firefox, so I regarded it as shared code because the main repository should get the patches and Thunderbird get them downstream.

testpilot: Gregg Lind (tp developer) and squib (ported tp to tb) agreed last week that tp shouldn't ship anymore in its current state. As far as I know the new version 2 is a rewrite, so patching version 1 is a waste of time.
Comment 3 Mike Conley (:mconley) - (Needinfo me!) 2013-05-18 09:21:41 PDT
Comment on attachment 739464 [details] [diff] [review]
patch, v2

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

Looks good. r=me with my nit fixed. Thanks Sebastian!

::: mail/base/content/phishingDetector.js
@@ +269,5 @@
>        var dialogMsg = bundle.getFormattedString("confirmPhishingUrl", 
>                          [brandShortName, unobscuredHostNameValue], 2);
>  
>        const nsIPS = Components.interfaces.nsIPromptService;
> +      return !Services.prompt.confirmEx(window, titleMsg, dialogMsg, nsIPS.STD_YES_NO_BUTTONS + nsIPS.BUTTON_POS_1_DEFAULT, 

This looks like it can be broken up more neatly:

return !Services.prompt.confirmEx(window, titleMsg, dialogMsg,
                                  nsIPS.STD_YES_NO_BUTTONS +
                                  nsIPS.BUTTON_POS_1_DEFAULT,
                                  "", "", "", "", {});

Also note the trimmed whitespace after BUTTON_POS_1_DEFAULT,
Comment 4 Sebastian H. [:aryx][:archaeopteryx] 2013-05-19 11:25:12 PDT
Created attachment 751491 [details] [diff] [review]
patch, v3, r=mconley [checked in, comment 5]
Comment 5 Ryan VanderMeulen [:RyanVM] 2013-05-20 05:02:03 PDT
https://hg.mozilla.org/comm-central/rev/8fc54bbfaa0b
Comment 6 :aceman 2013-05-30 12:55:48 PDT
Looks like these still need update... smtpService got removed :(
/mailnews/base/prefs/content/aw-done.js
    line 128 -- var smtpServer = parent.smtpService.defaultServer;

/mailnews/base/prefs/content/aw-outgoing.js
    line 48 -- if (parent.smtpService.defaultServer && !smtpCreateNewServer) {
    line 49 -- smtpServer = parent.smtpService.defaultServer;
    line 85 -- var smtpServer = parent.smtpService.defaultServer;
Comment 7 Sebastian H. [:aryx][:archaeopteryx] 2013-06-07 15:16:20 PDT
Created attachment 759980 [details] [diff] [review]
fix smtpServer falls, v1
Comment 8 Mike Conley (:mconley) - (Needinfo me!) 2013-07-08 19:39:47 PDT
Comment on attachment 759980 [details] [diff] [review]
fix smtpServer falls, v1

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

r=me with the vars switched to lets. We don't like vars. :)

::: mailnews/base/prefs/content/aw-done.js
@@ +126,5 @@
>          setDivTextFromForm("server.type", incomingServerType.toUpperCase());
>  
>          var smtpServerName="";
>          if (pageData.server && pageData.server.smtphostname) {
> +          var smtpServer = MailServices.smtp.defaultServer;

Please change var to let

::: mailnews/base/prefs/content/aw-outgoing.js
@@ +82,5 @@
>      if (boxToShow)
>        boxToShow.removeAttribute("hidden");
>  
>      var smtpNameInput = document.getElementById("smtpusername");
> +    var smtpServer = MailServices.smtp.defaultServer;

let, not var
Comment 9 :aceman 2013-07-08 23:35:32 PDT
Don't forget to get this into TB24.
Comment 10 Sebastian H. [:aryx][:archaeopteryx] 2013-07-09 12:10:12 PDT
Created attachment 772803 [details] [diff] [review]
fix smtpServer calls, v2, r=mconley [checked in, comment 11]
Comment 11 Mark Banner (:standard8) 2013-07-11 06:08:01 PDT
https://hg.mozilla.org/comm-central/rev/65c5bfb05484
Comment 12 Mark Banner (:standard8) 2013-07-11 06:09:08 PDT
Note: please make a clear statement why we should track it, having to dig through comments isn't easy.
Comment 13 :aceman 2013-07-15 12:32:05 PDT
Comment on attachment 772803 [details] [diff] [review]
fix smtpServer calls, v2, r=mconley [checked in, comment 11]

[Approval Request Comment]
Regression caused by (bug #): one of the mailServices conversion bugs reachable from bug 720358.
User impact if declined: Account wizard possibly not finishing properly
Testing completed (on c-c, etc.): TB24
Risk to taking this patch (and alternatives if risky): probably none
Comment 14 Mark Banner (:standard8) 2013-07-23 04:34:51 PDT
https://hg.mozilla.org/releases/comm-aurora/rev/0adb0914b405

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