Open
Bug 451578
Opened 15 years ago
Updated 8 months ago
Replace |.getService() .QueryInterface(iid)| by |.getService(iid)| in mozilla-central
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
NEW
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)
1.97 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
12.21 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
8.96 KB,
patch
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 2•11 years ago
|
||
(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.
Reporter | ||
Comment 6•11 years ago
|
||
(In reply to :aceman from comment #4) > many of them will go away just by converting to Services.jsm Good point ;-)
I am not going to do this globally in Core as I can not test it properly (only building TB).
Assignee: acelists → nobody
Comment 8•11 years ago
|
||
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 ?
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]
Comment 11•10 years ago
|
||
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.
Comment 13•10 years ago
|
||
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)
Comment 15•10 years ago
|
||
Attachment #815065 -
Flags: feedback?
Comment 16•10 years ago
|
||
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+
Comment 18•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #815452 -
Attachment is patch: true
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+
Comment 20•10 years ago
|
||
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)
Comment 22•10 years ago
|
||
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?
Comment 23•10 years ago
|
||
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)
Updated•10 years ago
|
Assignee: nobody → amod.narvekar
Any news of that bug, Amod?
Flags: needinfo?(amod.narvekar)
![]() |
||
Comment 25•10 years ago
|
||
I'll split comm-central work into a separate bug as it can be worked on in parallel.
No longer depends on: 720356
Comment 26•10 years ago
|
||
I will resume the bug within a while. Sorry for delay.
Flags: needinfo?(amod.narvekar)
Comment 27•10 years ago
|
||
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)
Comment 28•9 years ago
|
||
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)
Comment 30•9 years ago
|
||
yes. Sorry for the delay. I will resume the work within a day.
Flags: needinfo?(amod.narvekar)
Comment 31•9 years ago
|
||
Since the bug is split, What steps do I need to take now ?
![]() |
||
Comment 32•9 years ago
|
||
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
Comment 33•9 years ago
|
||
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
Updated•9 years ago
|
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)
Comment 35•9 years ago
|
||
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.
Comment 37•9 years ago
|
||
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
Updated•9 years ago
|
Assignee: amod.narvekar → nobody
Assignee | ||
Updated•9 years ago
|
Mentor: dteller
Whiteboard: [good first bug][mentor=Yoric][lang=js] → [good first bug][lang=js]
Comment 38•9 years ago
|
||
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
Comment 39•9 years ago
|
||
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+
Attachment #8408396 -
Attachment is obsolete: true
Comment 41•9 years ago
|
||
In this I am attaching the try request link which I pulled today.
Attachment #8450751 -
Flags: review?(dteller)
Reposting in the body. Try: https://tbpl.mozilla.org/?tree=Try&rev=1ac67f7d7fd6
That looks good. Do you have the rights to mark your bug as checkin-needed?
Updated•9 years ago
|
Keywords: meta → checkin-needed
Comment 44•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/fda29051ad88
Keywords: checkin-needed
Reporter | ||
Updated•9 years ago
|
Attachment #8450751 -
Attachment is obsolete: true
Attachment #8450751 -
Flags: review?(dteller)
Comment 45•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fda29051ad88
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Comment 46•9 years ago
|
||
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
Reporter | ||
Updated•9 years ago
|
Attachment #8450367 -
Attachment description: bug451578.patch → Fix for /addon-sdk/*
Reporter | ||
Comment 47•9 years ago
|
||
(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 → ---
Comment 48•9 years ago
|
||
(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)
![]() |
||
Comment 49•9 years ago
|
||
(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.
Comment 50•9 years ago
|
||
(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
Reporter | ||
Updated•9 years ago
|
Attachment #8450367 -
Attachment description: Fix for /addon-sdk/* → Fix for /addon-sdk/*
[Checked in: Comment 45]
Reporter | ||
Comment 51•9 years ago
|
||
(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]
Comment 52•9 years ago
|
||
Made all the changes in the tests also including all the js files.
Attachment #8450367 -
Attachment is obsolete: true
Attachment #8490789 -
Flags: review?(dteller)
Reporter | ||
Comment 53•9 years ago
|
||
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'...
Reporter | ||
Comment 54•9 years ago
|
||
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
![]() |
||
Comment 55•9 years ago
|
||
"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)
Comment 57•9 years ago
|
||
(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)
Try results have been wiped a long time ago. Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=35d577bb7408
Updated•8 years ago
|
Assignee: allamsetty.anup → nobody
Status: ASSIGNED → NEW
Reporter | ||
Updated•8 years ago
|
Whiteboard: [good first bug][lang=js] [See comments 0 and 14] → [patchlove] [good first bug][lang=js] [See comments 0 and 14]
Comment 60•8 years ago
|
||
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 ?
Comment 61•8 years ago
|
||
Reporter | ||
Comment 62•8 years ago
|
||
(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 → ---
Comment 63•8 years ago
|
||
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 !
Comment 64•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #8674913 -
Flags: review+ → review?(bugzillamozillaorg_serge_20140323)
Reporter | ||
Comment 65•8 years ago
|
||
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)
Comment 66•5 years ago
|
||
Unassigning due to inactivity in case someone wants to finish this.
Assignee: dolcinis → nobody
Status: ASSIGNED → NEW
Comment 67•5 years ago
|
||
Hey, can anyone tell me the current status of this issue?
Comment 68•5 years ago
|
||
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)
Comment hidden (off-topic) |
Comment 70•5 years ago
|
||
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
![]() |
||
Comment 71•5 years ago
|
||
Looks like it has stalled. You can take it if you wish.
Updated•5 years ago
|
Component: General → JavaScript Engine
Comment 72•5 years ago
|
||
Hey is it still open, I would like to take it as my first bug. If not can you recommend where should I go?
![]() |
||
Comment 73•5 years ago
|
||
Yes, it seems you can take it. There is already an existing patch (2 of them) that need to be finished.
Comment 74•5 years ago
|
||
Hello! Is someone still working on it?, if not I would like to be assigned.
Comment 75•5 years ago
|
||
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!
Comment 76•5 years ago
|
||
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
Comment 77•5 years ago
|
||
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.
Updated•3 years ago
|
Keywords: good-first-bug
Whiteboard: [patchlove] [good first bug][lang=js] [See comments 0 and 14] → [patchlove] [lang=js] [See comments 0 and 14]
Comment 78•3 years ago
|
||
hey !!
Can I take this up ?
Thanks
mahak :)
Updated•8 months ago
|
Severity: trivial → S4
You need to log in
before you can comment on or make changes to this bug.
Description
•