Open Bug 451578 Opened 13 years ago Updated 24 days ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set
trivial

Tracking

()

People

(Reporter: sgautherie, Unassigned, Mentored)

References

()

Details

(Keywords: good-first-bug, Whiteboard: [patchlove] [lang=js] [See comments 0 and 14])

Attachments

(3 files, 6 obsolete files)

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);
}}
What is the gain from this change? Does it have universal support among the developers?
There is potentially quite a lot of this pattern.
http://mxr.mozilla.org/comm-central/search?string=.getService%28%29&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central
Assignee: nobody → acelists
(In reply to :aceman from comment #1)
> What is the gain from this change?

"Optimized" code: the result is the same.

> Does it have universal support among the developers?

It should. You can try it on a few files...
OK, let's try it.
Keywords: meta
Summary: Replace |.getService() .QueryInterface(iid)| by |.getService(iid)| → [meta] Replace |.getService() .QueryInterface(iid)| by |.getService(iid)|
On the other hand, many of them will go away just by converting to Services.jsm so I'd rather not do that now on whole Core and fixing the spots without converting to Services would probably pass through the reviewers.
So I need to find which spots are worth to do now.
...whouldn't pass...
(In reply to :aceman from comment #4)
> many of them will go away just by converting to Services.jsm

Good point ;-)
Depends on: 720356, 664838
Summary: [meta] Replace |.getService() .QueryInterface(iid)| by |.getService(iid)| → Replace |.getService() .QueryInterface(iid)| by |.getService(iid)|
Whiteboard: [good first bug][mentor=sgautherie][lang=js]
I am not going to do this globally in Core as I can not test it properly (only building TB).
Assignee: acelists → nobody
Sir, i would like to contribute to this bug if it is not assigned to anyone. In that case, will you explain the bug in detail ?
Hello, 

I would like to fix this bug.
Since sgautherie doesn't seem to be responding, I'll take over that mentoring.
Whiteboard: [good first bug][mentor=sgautherie][lang=js] → [good first bug][mentor=Yoric][lang=js]
Hello Yoric, 
Felt extremely happy to see the bug active. I had commented about a year ago. I would like to work on it.
Two things I would like to mention:
\1. Within few hours I would be boarding a flight to return from SUMMIT 2013. So it might take 18-20 hours for me to return to this bug.
\2. This bug requires Find-and-replace strategy. Instead of doing it manually for all 91 instances, I would like to write a simple script or a java program that would automatically replace all the instances in all the files of mozilla-central.
Automatically detecting the instances of .getService() is a good idea (although dxr can already help you there). As mentioned in comment 4, wherever this is applicable, it would be better to convert the code to use Services.jsm instead of calling into xpcom directly, so it might not be possible to write a simple script to cover these cases, though.
Attached patch bug-451578-fix (obsolete) — Splinter Review
From what I could interpret, I need to import < Services.jsm > in those files where < .getServices() > is used. 
Hence I have done changes only in 2 files and if I am going correct, I will makes the changes in rest of them as well. 
Thanks !
Attachment #815040 - Flags: feedback?(dteller)
Comment on attachment 815040 [details] [diff] [review]
bug-451578-fix

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

Well, importing Services.jsm is not sufficient, you need to use it.
So, for instance, in browser_aboutHome.js, you should replace
 Cc["@mozilla.org/observer-service;1"]
       .getService(Ci.nsIObserverService)
with
 Services.obs

You can find the full list of services included in Services.jsm here: https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Services.jsm
Attachment #815040 - Flags: feedback?(dteller)
Attached patch sample_patchv2 (obsolete) — Splinter Review
Attachment #815065 - Flags: feedback?
As per name, it is a sample patch. Once the approach is correct, I will make the changes to rest of instances. Also | nsICategoryManager | has no Service Accessor in Services.jsm
Comment on attachment 815065 [details] [diff] [review]
sample_patchv2

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

It's on the right way :)
Also, please mark your previous patches as obsolete when it's the case.

::: browser/base/content/browser.js
@@ +3237,5 @@
>    toOpenWindowByType("global:console", "chrome://global/content/console.xul");
>  }
>  
>  function BrowserDownloadsUI()
>  {

I believe that you forgot to import Services.jsm in this file.
Attachment #815065 - Flags: feedback? → feedback+
Attached patch patchv1 (obsolete) — Splinter Review
Repaired | .getService() | instances. 
Regarding | .QueryInterface() |, i got so many instances at 
http://mxr.mozilla.org/mozilla-central/search?string=QueryInterface%28%29&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

So thought of asking you before proceeding with it.
Attachment #815040 - Attachment is obsolete: true
Attachment #815065 - Attachment is obsolete: true
Attachment #815452 - Flags: review?(dteller)
Comment on attachment 815452 [details] [diff] [review]
patchv1

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

Generally speaking, you should cut this in several smaller patches, to be able to test (and land) them progressively.
I haven't reviewed the whole patch. Generally, this looks like on the right track, with a few fixes detailed below.
Now, I'd like you to test and fix your patch before we proceed. Eventually, we might also decide to replace some calls to |Cu.import("...Services.jsm")| by |XPCOM.defineLazyModuleGetter|, on a case-by-case basis.

::: addon-sdk/source/lib/sdk/test/harness.js
@@ +22,5 @@
>  const unit = require("../deprecated/unit-test");
>  const test = require("../../test");
>  const url = require("../url");
>  
> +Cu.import("resource://gre/modules/Services.jsm");

Have you tested that this works?
Addon-sdk code behaves differently from everything I know.

@@ +28,2 @@
>  var cService = Cc['@mozilla.org/consoleservice;1'].getService()
>                 .QueryInterface(Ci.nsIConsoleService);

You could also replace this by Services.console, iirc.

::: build/bloatcycle.html
@@ +7,5 @@
>    From mozilla/toolkit/content
>    These files did not have a license
>  */
>  
> +Cu.import("resource://gre/modules/Services.jsm");

Have you tested this?

::: netwerk/test/httpserver/httpd.js
@@ +38,5 @@
>    "HttpServer",
>  ];
>  
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

This probably won't work, as Cu is defined below.

::: netwerk/test/httpserver/test/head_utils.js
@@ +9,5 @@
>   * <runxpcshelltests.py>.
>   */
> + 
> +Cu.import("resource://gre/modules/Services.jsm");
> + 

Please don't add whitespaces.

::: netwerk/test/unit/test_cookiejars_safebrowsing.js
@@ +39,5 @@
>  var checkCookiePath = "/checkcookie";
>  var safebrowsingUpdatePath = "/safebrowsingUpdate";
>  var safebrowsingRekeyPath = "/safebrowsingRekey";
>  var httpserver;
>  

Have you imported Services.jsm?
Does this work?

::: netwerk/test/unit/test_unix_domain.js
@@ +17,5 @@
>  const threadManager = Cc["@mozilla.org/thread-manager;1"].getService();
>  
>  const allPermissions = parseInt("777", 8);
>  
> +Cu.import("resource://gre/modules/Services.jsm");

You are calling Services before importing it, that probably won't work.

::: services/sync/tps/extensions/mozmill/resource/stdlib/httpd.js
@@ +10,5 @@
>   * httpd.js.
>   */
>  
>  Components.utils.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");

Have you checked whether Cu is defined here?

::: toolkit/components/passwordmgr/test/test_bug_627616.html
@@ +4,5 @@
>    <title>Test bug 627616</title>
>    <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>        
>    <script type="text/javascript" src="prompt_common.js"></script>
>    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +  Cu.import("resource://gre/modules/Services.jsm");

I strongly doubt that this can work.
Attachment #815452 - Flags: review?(dteller) → feedback+
Hi,

I am interested in working on this bug. So, please can you assign this bug to me.

Thanks in advance,

Regards,
A.Anup
Anup, that depends. Amod, are you still working on this?
Flags: needinfo?(amod.narvekar)
No, I have just started working on this bug. OK so, if that is the case I will stop it now and see for one more bug.

Sir, when will you be available in IRC?
I am working on this bug.. I will resume the work within a day or two.
I will self-assign the bug to avoid any confusion.
Flags: needinfo?(amod.narvekar)
Assignee: nobody → amod.narvekar
Any news of that bug, Amod?
Flags: needinfo?(amod.narvekar)
I'll split comm-central work into a separate bug as it can be worked on in parallel.
No longer depends on: 664838
Depends on: 957184
I will resume the bug within a while.
Sorry for delay.
Flags: needinfo?(amod.narvekar)
Sorry Yoric, Since I am busy with my academics, I wont be able to find time before March 2, 2014. If you feel it is too long to wait, you are free to assign it to someone more deserving. 
Thank you. Looking forward to work with you again after I return.
Flags: needinfo?(sgautherie.bz)
I am back from the break. So I would be working on this bug now.
Flags: needinfo?(sgautherie.bz)
Any news, Amod?
Flags: needinfo?(amod.narvekar)
yes. Sorry for the delay. I will resume the work within a day.
Flags: needinfo?(amod.narvekar)
Since the bug is split, What steps do I need to take now ?
Your current patch does not contain anything for comm-central (Thunderbird and Seamonkey) so you can continue where you left off without any changes. The patch only contains files for mozilla-central so if you do any more changes just keep them inside the mozilla-central tree.
You probably just need to address problems from comment 19.
Status: NEW → ASSIGNED
Summary: Replace |.getService() .QueryInterface(iid)| by |.getService(iid)| → Replace |.getService() .QueryInterface(iid)| by |.getService(iid)| in mozilla-central
Attached patch patchv1_to_look_for_.rej_files (obsolete) — Splinter Review
I have returned to this bug after a very long time. So while importing the patch, I found many .rej files. So I am uploading the patch just to locate those unsaved changes. Hence I am NOT ASKING FOR REVIEW.
Attachment #815452 - Attachment is obsolete: true
Attachment #8408396 - Flags: review?(dteller)
Comment on attachment 8408396 [details] [diff] [review]
patchv1_to_look_for_.rej_files

Well, if you're not asking for review, please don't set the flag that asks for review.
Attachment #8408396 - Flags: review?(dteller)
I dint ask for the review in the comment where I mentioned I want to check for the .rej files. When I had done with it, I asked for your review.
Please accept my apologies in case of any misunderstanding. :)
Comment on attachment 8408396 [details] [diff] [review]
patchv1_to_look_for_.rej_files

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

There is progress, thanks.

Since your patch is hitting lots of code, it would be useful if you could divide it in several subpatches, one per toplevel directory:
i.e. one for netwerk/*, one for browser/*, etc. This will simplify reviewing, testing and landing.

Also, please run the tests. I don't want to spend time reviewing patches that simply have no chance of working.

::: addon-sdk/source/lib/sdk/test/harness.js
@@ +29,5 @@
>    resolve();
>    return promise;
>  }
>  
> +var cService = Services.console;

This module doesn't import Services, so you can't use it. Generally, don't attempt to use Services in the addon-sdk, it uses a different module system.

Also, please test your patches, I'm almost sure that this would have shown up in a test.

::: build/bloatcycle.html
@@ +21,5 @@
>  }
>  
>  function canQuitApplication()
>  {
> +  var os = Services.obs;

This file doesn't have access to Services. Anyway, please don't touch files in build/.

::: dom/tests/unit/test_geolocation_provider.js
@@ +3,5 @@
>  const Cu = Components.utils;
>  const Cr = Components.results;
>  
>  Cu.import("resource://testing-common/httpd.js");
> +Cu.import("resource://gre/modules/Services.jsm");

What's the point of importing Services if you don't use it?

::: netwerk/test/httpserver/httpd.js
@@ +47,5 @@
>  const CC = Components.Constructor;
>  
>  const PR_UINT32_MAX = Math.pow(2, 32) - 1;
>  
> +Cu.import("resource://gre/modules/Services.jsm");

Please add a second argument |this| to this import. Theoretically, we should do it everywhere, for compatibility with B2G.

::: netwerk/test/httpserver/test/head_utils.js
@@ +8,5 @@
>   * Loads _HTTPD_JS_PATH file, which is dynamically defined by
>   * <runxpcshelltests.py>.
>   */
> +
> +Cu.import("resource://gre/modules/Services.jsm");

, this

::: netwerk/test/unit/test_authpromptwrapper.js
@@ +7,5 @@
>  
>  const nsIAuthInformation = Components.interfaces.nsIAuthInformation;
>  const nsIAuthPromptAdapterFactory = Components.interfaces.nsIAuthPromptAdapterFactory;
>  
> +Cu.import("resource://gre/modules/Services.jsm");

, this

::: netwerk/test/unit/test_unix_domain.js
@@ +2,5 @@
>  
>  const CC = Components.Constructor;
> +const Cu = Components.utils;
> +
> +Cu.import("resource://gre/modules/Services.jsm");

, this

::: security/manager/pki/resources/content/resetpassword.js
@@ +5,5 @@
>  const nsPK11TokenDB = "@mozilla.org/security/pk11tokendb;1";
>  const nsIPK11TokenDB = Components.interfaces.nsIPK11TokenDB;
>  const nsIDialogParamBlock = Components.interfaces.nsIDialogParamBlock;
>  
> +Cu.import("resource://gre/modules/Services.jsm");

, this

::: services/sync/tps/extensions/mozmill/resource/stdlib/httpd.js
@@ +27,5 @@
>  const CC = Components.Constructor;
>  
>  const PR_UINT32_MAX = Math.pow(2, 32) - 1;
>  
> +Cu.import("resource://gre/modules/Services.jsm");

, this

::: testing/mochitest/server.js
@@ +10,5 @@
>  
>  // Disable automatic network detection, so tests work correctly when
>  // not connected to a network.
> +
> +Cu.import("resource://gre/modules/Services.jsm");

Let's not introduce dependencies to Services in this directory.

::: testing/xpcshell/head.js
@@ +9,5 @@
>   * See http://developer.mozilla.org/en/docs/Writing_xpcshell-based_unit_tests
>   * for more information.
>   */
>  
> +Cu.import("resource://gre/modules/Services.jsm");

Nor here.

::: toolkit/components/help/content/contextHelp.js
@@ +1,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +Cu.import("resource://gre/modules/Services.jsm");

, this

::: toolkit/components/passwordmgr/test/test_bug_627616.html
@@ +31,5 @@
>        }
>      });
>  
>      function init1() {
> +        var ios = Services.io;

Services is not defined. This would have showed up in a test.

::: toolkit/components/passwordmgr/test/test_prompt.html
@@ +6,5 @@
>    <script type="text/javascript" src="pwmgr_common.js"></script>
>    <script type="text/javascript" src="prompt_common.js"></script>
>    <script type="text/javascript" src="notification_common.js"></script>
>    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +  Cu.import("resource://gre/modules/Services.jsm");

That's not even syntactically correct. Please run the tests.
I'll stop my review here because the rest seems also syntactically incorrect.
Sorry Yoric for giving no updates for a month. And concerning my current academic endeavors, I wont get time for another 1 month. So I think it would be better to leave the bug. 
Thank you for helping me !
Status: ASSIGNED → NEW
Assignee: amod.narvekar → nobody
Mentor: dteller
Whiteboard: [good first bug][mentor=Yoric][lang=js] → [good first bug][lang=js]
Hi, 

I would like to work on this bug. So please can you assign it to me?

Thanks in advance,

Regards,
Anup
Assignee: nobody → allamsetty.anup
Changes made as requested in the whole mozilla-central (only a few are to be changed.) Most of them are already changed in the latest revisions of the code.
Attachment #8450367 - Flags: review?(dteller)
Comment on attachment 8450367 [details] [diff] [review]
Fix for /addon-sdk/*
[Checked in: Comment 45]

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

r=me if this passes all tests
Attachment #8450367 - Flags: review?(dteller) → review+
Attached file Link for a try request (obsolete) —
In this I am attaching the try request link which I pulled today.
Attachment #8450751 - Flags: review?(dteller)
That looks good. Do you have the rights to mark your bug as checkin-needed?
Keywords: metacheckin-needed
Attachment #8450751 - Attachment is obsolete: true
Attachment #8450751 - Flags: review?(dteller)
https://hg.mozilla.org/mozilla-central/rev/fda29051ad88
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/47eac113a15a51e6c805d17ab6cbbf616c9c5d60
Bug 451578 - Replace |.getService() .QueryInterface(iid)| by |.getService(iid)| in mozilla-central. r=Yoric
Attachment #8450367 - Attachment description: bug451578.patch → Fix for /addon-sdk/*
(In reply to Tim Taubert [:ttaubert] from comment #45)
> Status: NEW → RESOLVED

I don't think so : this patch fixed 2 instances only, "many" of them remain.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(In reply to Serge Gautherie (:sgautherie) from comment #47)
> (In reply to Tim Taubert [:ttaubert] from comment #45)
> > Status: NEW → RESOLVED
> 
> I don't think so : this patch fixed 2 instances only, "many" of them remain.

I think the patch which I've submitted earlier is correct because according to the bug we need to replace *.getService() .QueryInterface(iid)* with *.getService()*. The search which I've used for is http://mxr.mozilla.org/mozilla-central/search?string=.getService%28%29%2B.QueryInterface%28iid%29

Correct me if I am wrong.
Flags: needinfo?(bugzillamozillaorg_serge_20140323)
(In reply to Anup Kumar from comment #48)
> (In reply to Serge Gautherie (:sgautherie) from comment #47)
> > (In reply to Tim Taubert [:ttaubert] from comment #45)
> > > Status: NEW → RESOLVED
> > 
> > I don't think so : this patch fixed 2 instances only, "many" of them remain.
> 
> I think the patch which I've submitted earlier is correct because according
> to the bug we need to replace *.getService() .QueryInterface(iid)* with
> *.getService()*. The search which I've used for is
> http://mxr.mozilla.org/mozilla-central/search?string=.getService%28%29%2B.
> QueryInterface%28iid%29
> 
> Correct me if I am wrong.
You cannot search for the fixed string "iid" because there can be anything in the .QueryInterface() call. Also, the .getService().QueryInterface() call chain is often split into multiple lines and that is probably not found easily with such a search.

Also, there are cases like:
http://mxr.mozilla.org/mozilla-central/source/dom/tests/unit/test_geolocation_provider.js#86

So I think you should just search for ".getService()" (as I've done in comment 1) and manually review all hits.
(In reply to :aceman from comment #49)
> Also, there are cases like:
> http://mxr.mozilla.org/mozilla-central/source/dom/tests/unit/
> test_geolocation_provider.js#86
> 
> So I think you should just search for ".getService()" (as I've done in
> comment 1) and manually review all hits.

Thanks for correcting me. I forgot about the iid part of the search index. Coming to the search for *.getService()* this is the correct search right? 
http://dxr.mozilla.org/mozilla-central/search?q=getService()+ext%3Ajs&case=true
Attachment #8450367 - Attachment description: Fix for /addon-sdk/* → Fix for /addon-sdk/* [Checked in: Comment 45]
(In reply to Anup Kumar from comment #50)
> Coming to the search for *.getService()* this is the correct search right? 
> http://dxr.mozilla.org/mozilla-central/
> search?q=getService()+ext%3Ajs&case=true

JS files are most of the cases, but not all of them.
I'm not sure whether the search should be made with the beginning '.' or not, to catch all relevant cases...
Mentor: bugzillamozillaorg_serge_20140323
Status: REOPENED → ASSIGNED
Flags: needinfo?(bugzillamozillaorg_serge_20140323)
Whiteboard: [good first bug][lang=js] → [good first bug][lang=js] [See comments 0 and 14]
Made all the changes in the tests also including all the js files.
Attachment #8450367 - Attachment is obsolete: true
Attachment #8490789 - Flags: review?(dteller)
Comment on attachment 8490789 [details] [diff] [review]
Replaced all the .getService.QuertInterface( ) in js files too.

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

I had a quick look: these changes look good ;-)

::: toolkit/components/printing/content/printPageSetup.js
@@ -256,5 @@
>    var print_margin_left   = 0.5;
>    var print_margin_bottom = 0.5;
>    var print_margin_right  = 0.5;
>  
>    try {

If the following 'if's need to stay, maybe add a comment(s) to explain why.

@@ -260,5 @@
>    try {
>      gPrefs = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
>    
> -    gPrintService = Components.classes["@mozilla.org/gfx/printsettings-service;1"];
> -    if (gPrintService) {

I think you can't merge this 'if', because (iirc) the class may not be available if there is no printer. (To be confirmed.)

@@ -262,5 @@
>    
> -    gPrintService = Components.classes["@mozilla.org/gfx/printsettings-service;1"];
> -    if (gPrintService) {
> -      gPrintService = gPrintService.getService();
> -      if (gPrintService) {

I assume you can indeed merge this 'if'. (To be confirmed.)

::: toolkit/components/printing/content/printdialog.js
@@ -217,5 @@
>    var print_frametype     = gPrintSetInterface.kSelectedFrame;
>    var print_howToEnableUI = gPrintSetInterface.kFrameEnableNone;
>    var print_tofile        = "";
>  
>    try {

Same as 'printPageSetup.js'...
Comment on attachment 8450367 [details] [diff] [review]
Fix for /addon-sdk/*
[Checked in: Comment 45]

Anup, don't obsolete checked in patches.
Attachment #8450367 - Attachment is obsolete: false
"printing" can be disabled at compile time. If that removes this exact service, I don't know. But better do not remove the ifs in such code patterns.
Comment on attachment 8490789 [details] [diff] [review]
Replaced all the .getService.QuertInterface( ) in js files too.

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

The patch looks good to me.
If NS_PRINTING is undefined, `Components.classes["@mozilla.org/gfx/printsettings-service;1"]` will return `undefined` and the rest will be caught by the `try/catch`. Otherwise, as far as I can tell, the only bad thing that can happen is an error while instantiating the service, which will also cause an exception which will also be caught.

In other words, if the tests pass, I'm satisfied. Anup, can you run the tests on all platforms to make sure that everything is alright?
Attachment #8490789 - Flags: review?(dteller) → review+
Flags: needinfo?(allamsetty.anup)
(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #56)
> In other words, if the tests pass, I'm satisfied. Anup, can you run the
> tests on all platforms to make sure that everything is alright?

Hi Yoric,

I am sorry to say that I can't put a try request now because as I told you earlier I lost my public key. I need to file a bug for that and it might take a while. So please can you put the try request for me.

Regards,
Anup
Flags: needinfo?(allamsetty.anup)
Assignee: allamsetty.anup → nobody
Status: ASSIGNED → NEW
Whiteboard: [good first bug][lang=js] [See comments 0 and 14] → [patchlove] [good first bug][lang=js] [See comments 0 and 14]
I looked for all those functions in the mozilla-central codebase. I replaced the first case  in server.js

@@ -49,8 +49,7 @@
   httpserv.registerPathHandler("/bug482601/only_from_cache", bug482601_only_from_cache);
   httpserv.start(-1);
 
-  var obs = Cc["@mozilla.org/observer-service;1"].getService();
-  obs = obs.QueryInterface(Ci.nsIObserverService);
+  var obs = Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);
   obs.addObserver(observer, "http-on-examine-response", false);
   obs.addObserver(observer, "http-on-examine-merged-response", false);
   obs.addObserver(observer, "http-on-examine-cached-response", false);

There is another file IIRC but it requires a complete rewrite of the code used.

Can I be assigned to this ticket ?
(In reply to Stanislas Daniel Claude Dolcini from comment #60)

> I looked for all those functions in the mozilla-central codebase.

Compared to previous patch,
your patch misses 'test_net_failedtoprocess.html', at least.

> +  var obs =
> Cc["@mozilla.org/observer-service;1"].getService(Ci.nsIObserverService);

Did you try to do the replacements as in comment 14, where applicable?
(Please, read (again) all previous bug comments for added details about this bug.)

> Can I be assigned to this ticket ?

Done :-)
Assignee: nobody → dolcinis
Status: NEW → ASSIGNED
Target Milestone: mozilla33 → ---
Hello, thanks for the fast reply.

Actually I looked into js files, that's why I missed the html one(s)
> Did you try to do the replacements as in comment 14, where applicable? (Please, read (again) all previous bug comments for added details about this bug.)
Will do.

> Done :-)
Thanks !
Attached patch bug_451578.patchSplinter Review
This adds the Services.Jsm to all the files so that we can use lazy shortcuts.

There is one missing PrintingPrompt.
Attachment #8672992 - Attachment is obsolete: true
Attachment #8674913 - Flags: review+
Attachment #8674913 - Flags: review+ → review?(bugzillamozillaorg_serge_20140323)
Comment on attachment 8674913 [details] [diff] [review]
bug_451578.patch

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

(Sorry for the long delay.)

This seems promising.
Please add a link to a (successful) Try run.

> There is one missing PrintingPrompt

I'm not sure what you meant.

::: dom/tests/unit/test_geolocation_provider.js
@@ +84,4 @@
>      prefs.setBoolPref("dom.testing.ignore_ipc_principal", true);
>      prefs.setBoolPref("geo.wifi.scan", false);
>  
> +	var obs = Services.obs;

Nit: check that you are not adding more indentation than needed.
Attachment #8674913 - Flags: review?(bugzillamozillaorg_serge_20140323)
Unassigning due to inactivity in case someone wants to finish this.
Assignee: dolcinis → nobody
Status: ASSIGNED → NEW
Hey, can anyone tell me the current status of this issue?
Hey, I am trying to contribute to this bug. How to build the code ( a way to check if everything works fine?)?
Thank you.
Flags: needinfo?(bugzillamozillaorg_serge_20140323)
Hi,I wanted to know the status of the bug. Is it still active? I want to take it up as my first big
Thank You
Looks like it has stalled. You can take it if you wish.
Component: General → JavaScript Engine
Hey is it still open, I would like to take it as my first bug. If not can you recommend where should I go?
Yes, it seems you can take it. There is already an existing patch (2 of them) that need to be finished.
Hello! Is someone still working on it?, if not I would like to be assigned.
Hey! Akanksha bhardwaj you can take it if you want I was going to work on this but my laptop stopped working recently and also I have already done my first bug so if you are just starting it will be good for you. Thanks!
This is one of those catch-all sorts of bugs that's a whole-codebase concern.  It definitely is not a Core::JavaScript Engine concern.  Core::XPCOM because this is XPCOM use and I suppose I ought to avoid Core::General.
Component: JavaScript Engine → XPCOM
Hey! I would to like to work on this bug. As this bug is my first, can someone guide so as to where should I start from.
Keywords: good-first-bug
Whiteboard: [patchlove] [good first bug][lang=js] [See comments 0 and 14] → [patchlove] [lang=js] [See comments 0 and 14]

hey !!
Can I take this up ?
Thanks
mahak :)

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