Closed
Bug 1238537
Opened 9 years ago
Closed 9 years ago
make nsBrowserContentHandler.js use Services.jsm
Categories
(Firefox :: General, defect)
Firefox
General
Tracking
()
RESOLVED
FIXED
Firefox 47
People
(Reporter: florian, Assigned: malayaleecoder, Mentored)
Details
(Whiteboard: [good first bug])
Attachments
(1 file, 3 obsolete files)
10.51 KB,
patch
|
florian
:
review+
|
Details | Diff | Splinter Review |
While working on bug 1237648, I noticed that nsBrowserContentHandler.js still calls getService in plenty of places where Services.* should be used instead.
The file is at http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserContentHandler.js
For example,
242 var wwatch = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
243 .getService(nsIWindowWatcher);
244
245 wwatch.openWindow(null, gBrowserContentHandler.chromeURL,
should become:
Services.ww.openWindow(...
The way to work on this is to search for 'getService' in nsBrowserContentHandler.js, and check in http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Services.jsm if the service is available on the Services object.
Assignee | ||
Comment 1•9 years ago
|
||
I would like to take this as my first bug. Can you please guide me through?
Reporter | ||
Comment 2•9 years ago
|
||
(In reply to malayaleecoder from comment #1)
> I would like to take this as my first bug. Can you please guide me through?
Sure. Do you already have a copy of the Firefox source code, and a local build?
Assignee | ||
Comment 3•9 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #2)
> (In reply to malayaleecoder from comment #1)
> > I would like to take this as my first bug. Can you please guide me through?
>
> Sure. Do you already have a copy of the Firefox source code, and a local
> build?
No, I don't. My system is currently running OSX 10.11.1!
Reporter | ||
Comment 4•9 years ago
|
||
(In reply to malayaleecoder from comment #3)
> (In reply to Florian Quèze [:florian] [:flo] from comment #2)
> > (In reply to malayaleecoder from comment #1)
> > > I would like to take this as my first bug. Can you please guide me through?
> >
> > Sure. Do you already have a copy of the Firefox source code, and a local
> > build?
>
> No, I don't. My system is currently running OSX 10.11.1!
Ok. You can find instructions about how to get the source code and how to build on this page: https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
Once you have your local build, the description of the bug here should give you indications about where to start looking inside the source code. Let me know if you need more help to get started.
Assignee | ||
Comment 5•9 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #4)
> (In reply to malayaleecoder from comment #3)
> > (In reply to Florian Quèze [:florian] [:flo] from comment #2)
> > > (In reply to malayaleecoder from comment #1)
> > > > I would like to take this as my first bug. Can you please guide me through?
> > >
> > > Sure. Do you already have a copy of the Firefox source code, and a local
> > > build?
> >
> > No, I don't. My system is currently running OSX 10.11.1!
>
> Ok. You can find instructions about how to get the source code and how to
> build on this page:
> https://developer.mozilla.org/en-US/docs/Simple_Firefox_build
>
> Once you have your local build, the description of the bug here should give
> you indications about where to start looking inside the source code. Let me
> know if you need more help to get started.
I have got the local build and have started working on this!
Assignee | ||
Comment 6•9 years ago
|
||
Hello Florian,
Very sorry for the delay. I have attached an initial patch. Many getService call objects are not listed in Services.jsm. Please go through and suggest me further improvements.
Thanks :)
Assignee: nobody → malayaleecoder
Status: NEW → ASSIGNED
Flags: needinfo?(florian)
Reporter | ||
Comment 7•9 years ago
|
||
Comment on attachment 8718720 [details] [diff] [review]
Bug1238537_v1.diff
Review of attachment 8718720 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks for the patch, this is going in the right direction!
Other things that can be simplified:
321 var prefb = Components.classes["@mozilla.org/preferences-service;1"]
322 .getService(nsIPrefBranch);
(also at line 497)
455 var ios = Components.classes["@mozilla.org/network/io-service;1"]
456 .getService(Components.interfaces.nsIIOService);
750 var fl = Components.classes["@mozilla.org/file/directory_service;1"]
751 .getService(Components.interfaces.nsIProperties);
::: browser/components/nsBrowserContentHandler.js
@@ -59,5 @@
>
> function resolveURIInternal(aCmdLine, aArgument) {
> var uri = aCmdLine.resolveURI(aArgument);
> - var urifixup = Components.classes["@mozilla.org/docshell/urifixup;1"]
> - .getService(nsIURIFixup);
The nsIURIFixup constant, defined a few lines above (line 40 currently) is now unused, please remove it :-).
The same comment applies for nsIWindowWatcher and nsIWindowMediator (and nsIPrefBranch after updating the patch).
@@ +62,4 @@
>
> if (!(uri instanceof nsIFileURL)) {
> + return Services.uriFixup.createFixupURI(aArgument,
> + Services.uriFixup.FIXUP_FLAG_FIX_SCHEME_TYPOS);
Please preserve the alignment here: the second parameter should be aligned with the first one.
Alternatively, as "urifixup" was used 3 times in this function, you may want to prefer keeping the variable and just doing "var urifixup = Services.uriFixup;".
@@ +242,1 @@
> "_blank",
Please keep the parameters aligned.
Attachment #8718720 -
Flags: feedback+
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(florian)
Assignee | ||
Comment 8•9 years ago
|
||
Hello Florian,
Please check the attached patch. I have applied all that you have suggested in the previous comment. On running |mach build| I am getting a few errors, which I am unable to decipher. Can you help me out in finding what is causing this?
Thank You.
Attachment #8718720 -
Attachment is obsolete: true
Flags: needinfo?(florian)
Reporter | ||
Comment 9•9 years ago
|
||
Comment on attachment 8718906 [details] [diff] [review]
Bug1238537_v2.diff
Review of attachment 8718906 [details] [diff] [review]:
-----------------------------------------------------------------
::: browser/components/nsBrowserContentHandler.js
@@ +301,5 @@
> get chromeURL() {
> if (this.mChromeURL) {
> return this.mChromeURL;
> }
> + var prefb = Services.prefs;
We don't need this prefb variable (used only once here).
Reporter | ||
Comment 10•9 years ago
|
||
Thanks for the updated patch!
(In reply to malayaleecoder from comment #8)
> On running |mach build| I am getting a few errors,
> which I am unable to decipher. Can you help me out in finding what is
> causing this?
Could you please pastebin the error with some context?
Flags: needinfo?(florian)
Assignee | ||
Comment 11•9 years ago
|
||
Florian, I have made the above said changes in this attachment. Please check and tell me if any corrections are needed.
Thank You.
Attachment #8718906 -
Attachment is obsolete: true
Flags: needinfo?(florian)
Attachment #8719126 -
Flags: review?(florian)
Assignee | ||
Comment 12•9 years ago
|
||
(In reply to Florian Quèze [:florian] [:flo] from comment #10)
> Thanks for the updated patch!
>
> (In reply to malayaleecoder from comment #8)
> > On running |mach build| I am getting a few errors,
> > which I am unable to decipher. Can you help me out in finding what is
> > causing this?
>
> Could you please pastebin the error with some context?
It's fixed right now. The problem was that I made a mistake by replacing a file by another.
Thank You for your concern.
Reporter | ||
Comment 13•9 years ago
|
||
Comment on attachment 8719126 [details] [diff] [review]
Bug1238537_v3.diff
Review of attachment 8719126 [details] [diff] [review]:
-----------------------------------------------------------------
Thanks! Unfortunately, the patch no longer applies, most likely because of http://hg.mozilla.org/mozilla-central/rev/6c56f5dad3b2 Could you please update it?
::: browser/components/nsBrowserContentHandler.js
@@ +242,3 @@
> }
>
> function getMostRecentWindow(aType) {
It looks like this function is never actually used, so we could just remove it. Sorry for missing this during the previous review.
Attachment #8719126 -
Flags: review?(florian) → feedback+
Assignee | ||
Comment 14•9 years ago
|
||
Done. Please have a look :-)
Attachment #8719126 -
Attachment is obsolete: true
Reporter | ||
Comment 15•9 years ago
|
||
Comment on attachment 8719547 [details] [diff] [review]
Bug1238537_v4.diff
Review of attachment 8719547 [details] [diff] [review]:
-----------------------------------------------------------------
Looks good to me, thanks!
Attachment #8719547 -
Flags: review+
Reporter | ||
Comment 16•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/61db19d669b711a03ea2f9ac0c0967b78f5e708e
Bug 1238537 - make nsBrowserContentHandler.js use Services.jsm, r=florian.
Comment 17•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox47:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
Assignee | ||
Comment 18•9 years ago
|
||
Thank You Florian for guiding me through in fixing this bug!
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(florian)
You need to log in
before you can comment on or make changes to this bug.
Description
•