Closed Bug 957184 Opened 8 years ago Closed 7 years ago

Replace |.getService() .QueryInterface(iid)| by |.getService(iid)| in comm-central

Categories

(MailNews Core :: Backend, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 31.0

People

(Reporter: aceman, Assigned: hessamms)

References

(Blocks 1 open bug, )

Details

(Whiteboard: [good first bug][mentor=aceman][lang=js])

Attachments

(1 file, 3 obsolete files)

It looks like there are still some occurences of this after conversion to Services.jsm

+++ This bug was initially created as a clone of Bug #451578 +++

Example:
{{
-          strBundleService =
-          Components.classes["@mozilla.org/intl/stringbundle;1"].getService(); 
-          strBundleService = 
-          strBundleService.QueryInterface(Components.interfaces.nsIStringBundleService);

+      gStrBundleService =
+        Components.classes["@mozilla.org/intl/stringbundle;1"]
+                  .getService(Components.interfaces.nsIStringBundleService);
}}
Attached patch 957184-v1.patch (obsolete) — Splinter Review
hope I didn't miss anything.
Attachment #8396998 - Flags: feedback?
Comment on attachment 8396998 [details] [diff] [review]
957184-v1.patch

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

::: editor/ui/composer/content/ComposerCommands.js
@@ +717,5 @@
>  {
>    try {
>      var mimeService = null;
> +    mimeService = Components.classes["@mozilla.org/mime;1"].
> +                  getService(Components.interfaces.nsIMIMEService);

When you split expressions like this, try to align the dots in a column
so the ".getService" should be under ".classes" .

::: suite/common/sidebar/customize-panel.js
@@ +5,5 @@
>  
>  // the rdf service
>  var RDF = '@mozilla.org/rdf/rdf-service;1'
> +RDF = Components.classes[RDF].
> +      getService(Components.interfaces.nsIRDFService);

I would write this as:
var RDF = Components.classes[@"mozilla.org/rdf/rdf-service;1"].getService(Components.interfaces.nsIRDFService);
As the '@mozilla.org/rdf/rdf-service;1' string is not used later on a constant (we overwrite it immediately), so I do not see the point of splitting it out of the expression.

But we'll ask a Seamonkey peer on what the preferred style is.
Attachment #8396998 - Flags: feedback? → feedback?(philip.chee)
So the alignment should look like this:
mimeService = Components.classes["@mozilla.org/mime;1"]
                        .getService(Components.interfaces.nsIMIMEService);
Assignee: nobody → hessamms
(In reply to :aceman from comment #2)
> Comment on attachment 8396998 [details] [diff] [review]
> 957184-v1.patch
> 
> Review of attachment 8396998 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: editor/ui/composer/content/ComposerCommands.js
> @@ +717,5 @@
> >  {
> >    try {
> >      var mimeService = null;
> > +    mimeService = Components.classes["@mozilla.org/mime;1"].
> > +                  getService(Components.interfaces.nsIMIMEService);
> 
> When you split expressions like this, try to align the dots in a column
> so the ".getService" should be under ".classes" .
> 
Fixed.
> ::: suite/common/sidebar/customize-panel.js
> @@ +5,5 @@
> >  
> >  // the rdf service
> >  var RDF = '@mozilla.org/rdf/rdf-service;1'
> > +RDF = Components.classes[RDF].
> > +      getService(Components.interfaces.nsIRDFService);
> 
> I would write this as:
> var RDF =
> Components.classes[@"mozilla.org/rdf/rdf-service;1"].getService(Components.
> interfaces.nsIRDFService);
> As the '@mozilla.org/rdf/rdf-service;1' string is not used later on a
> constant (we overwrite it immediately), so I do not see the point of
> splitting it out of the expression.
> 
> But we'll ask a Seamonkey peer on what the preferred style is.
I'm confused too why it's written that way, anyway I created a new patch.
Attached patch 957184-v2.patch (obsolete) — Splinter Review
Attachment #8396998 - Attachment is obsolete: true
Attachment #8396998 - Flags: feedback?(philip.chee)
Attachment #8397739 - Flags: feedback?(acelists)
Comment on attachment 8397739 [details] [diff] [review]
957184-v2.patch

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

Thanks, looks great now!
So we can try to get a review. I try Ehsan for /editor, Florian for /im, mkmelin for /mail and Neil for /mailnews and /suite.

::: im/content/engineManager.js
@@ +425,5 @@
>    },
>  
>    getSourceIndexFromDrag: function () {
>      var dragService = Cc["@mozilla.org/widget/dragservice;1"].
> +                      getService(Ci.nsIDragService);

We usually put '.' on the next line but dot on line end seems to be the style in /im so you can keep this.

::: suite/common/sidebar/customize-panel.js
@@ +4,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  // the rdf service
> +
> +var RDF = Components.classes["@mozilla.org/rdf/rdf-service;"]

Remove the empty line.
Attachment #8397739 - Flags: review?(neil)
Attachment #8397739 - Flags: review?(mkmelin+mozilla)
Attachment #8397739 - Flags: review?(florian)
Attachment #8397739 - Flags: review?(ehsan)
Attachment #8397739 - Flags: feedback?(acelists)
Attachment #8397739 - Flags: feedback+
Comment on attachment 8397739 [details] [diff] [review]
957184-v2.patch

(In reply to :aceman from comment #6)

> ::: im/content/engineManager.js
> @@ +425,5 @@
> >    },
> >  
> >    getSourceIndexFromDrag: function () {
> >      var dragService = Cc["@mozilla.org/widget/dragservice;1"].
> > +                      getService(Ci.nsIDragService);
> 
> We usually put '.' on the next line but dot on line end seems to be the
> style in /im so you can keep this.

Context here: '.' is usually on the next line in im/ but this specific file was a direct copy of http://mxr.mozilla.org/mozilla-central/source/browser/components/search/content/engineManager.js and we avoid doing cosmetic changes to files we have imported from other places.
Attachment #8397739 - Flags: review?(florian) → review+
Comment on attachment 8397739 [details] [diff] [review]
957184-v2.patch

(ehsan is mozilla/editor anyway)


>diff --git a/suite/common/permissions/cookieViewer.js b/suite/common/permissions/cookieViewer.js
...
>+Components.utils.import("resource://gre/modules/Services.jsm");
...
>+  cookiemanager = Services.cookies;
>+  permissionmanager = Services.perms;
>+  promptservice = Services.prompt;
(This file isn't actually currently in use, so don't bother patching it if you don't want to, but if you do want to start using Services here, you shouldn't copy Services properties into local variables, instead you should replace all uses of those variables with their Services equivalent.)
Attachment #8397739 - Flags: review?(neil)
Attachment #8397739 - Flags: review?(ehsan)
Attachment #8397739 - Flags: review+
OK, then I suggest to do just this:
-  cookiemanager = Components.classes["@mozilla.org/cookiemanager;1"].getService();
-  cookiemanager = cookiemanager.QueryInterface(Components.interfaces.nsICookieManager);
-  permissionmanager = Components.classes["@mozilla.org/permissionmanager;1"].getService();
-  permissionmanager = permissionmanager.QueryInterface(Components.interfaces.nsIPermissionManager);
-  promptservice = Components.classes["@mozilla.org/embedcomp/prompt-service;1"].getService();
-  promptservice = promptservice.QueryInterface(Components.interfaces.nsIPromptService);
+  cookiemanager = Components.classes["@mozilla.org/cookiemanager;1"]
                             .getService(Components.interfaces.nsICookieManager)
+  permissionmanager = Components.classes["@mozilla.org/permissionmanager;1"]
                                 .getService(nsIPermissionManager);
+  promptservice = Components.classes["@mozilla.org/embedcomp/prompt-service;1"]
                             .getService(Components.interfaces.nsIPromptService);

and no other changes to that file. I think Seamonky didn't yet switch wholesale to Services.jsm as TB did, so we can just do what is needed for this bug.
Attached patch 957184-v3.patch (obsolete) — Splinter Review
Thank you for the review.
Attachment #8397739 - Attachment is obsolete: true
Attachment #8397739 - Flags: review?(mkmelin+mozilla)
Attachment #8398543 - Flags: review?(acelists)
Comment on attachment 8398543 [details] [diff] [review]
957184-v3.patch

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

So you removed touching the cookieViewer.js file, OK.

I am not a reviewer, but we just need to get review from mkmelin for the /mail file and we are ready for checkin.

But where did the /im files get lost in this last patch?
Attachment #8398543 - Flags: review?(mkmelin+mozilla)
Attachment #8398543 - Flags: review?(acelists)
Attachment #8398543 - Flags: feedback+
Attached patch 957184-v4.diffSplinter Review
Oops. I recently started using TortoiseHg and I messed up my local repository. sorry.
added cookieviewer and /im. Hope I didn't miss anything in the new patch. :P
Attachment #8398543 - Attachment is obsolete: true
Attachment #8398543 - Flags: review?(mkmelin+mozilla)
Attachment #8398804 - Flags: review?(mkmelin+mozilla)
>> But we'll ask a Seamonkey peer on what the preferred style is.
> I'm confused too why it's written that way, anyway I created a new patch.
The RDF code is ancient Netscape era code when every thing was being re-written from the ground up and RDF might still have been buggy like hades.

Thanks muchly for thinking of us.
Comment on attachment 8398804 [details] [diff] [review]
957184-v4.diff

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

mail/ bits look good to me. r=mkmelin
Attachment #8398804 - Flags: review?(mkmelin+mozilla) → review+
Comment on attachment 8398804 [details] [diff] [review]
957184-v4.diff

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

Yes, it seems the missing files are put back.
Attachment #8398804 - Flags: feedback+
Looks like this is done, congrats.
Status: NEW → ASSIGNED
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/11cd943edd27
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 31.0
You need to log in before you can comment on or make changes to this bug.