Closed
Bug 1445895
Opened 6 years ago
Closed 6 years ago
Replace BrowserUtils.makeURI with Services.io.newURI in Feeds.jsm
Categories
(Firefox :: General, enhancement, P5)
Tracking
()
RESOLVED
FIXED
Firefox 61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: johannh, Assigned: gilad.bau, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: good-first-bug)
Attachments
(1 file, 4 obsolete files)
885 bytes,
patch
|
johannh
:
review+
|
Details | Diff | Splinter Review |
BrowserUtils.makeURI (which is deprecated) in Feeds.jsm can be replaced with Services.io.newURI: https://searchfox.org/mozilla-central/rev/8976abf9cab8eb4661665cc86bd355cd08238011/browser/modules/Feeds.jsm#77 For instructions on how to get your local build of Firefox up and running and submit your patch, see https://developer.mozilla.org/en-US/docs/Introduction. Please leave a comment if you would like to be assigned to this bug and feel free to ask questions here or via IRC if you're stuck.
Comment 1•6 years ago
|
||
Can I work on this?
Reporter | ||
Comment 2•6 years ago
|
||
(In reply to Trisha from comment #1) > Can I work on this? Hi Trisha, this bug is rather something for someone entirely new to Firefox development. We have a number of "good next bugs" that would suit you better, I believe. How about bug 1442303 or bug 1437885? Thanks!
Comment 3•6 years ago
|
||
Hi Johann , I would like to work on this !
Comment 4•6 years ago
|
||
Please review it!
Attachment #8960091 -
Flags: review?(prathikshaprasadsuman)
Updated•6 years ago
|
Assignee: nobody → shreyasonawane54
Status: NEW → ASSIGNED
Comment 5•6 years ago
|
||
Hi Shreya! Thank you for picking this up. You have to upload a patch for review, not the file you changed. Please create a patch using Mercurial and upload it. You can generate a patch after you've made changes to file(s) by using 'hg commit'. I recommend using mozReview to upload your patch. You can read all about configuring Mercurial to use mozReview here: https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/install-mercurial.html#mozreview-install-mercurial Feel free to ping me on IRC if you have any questions. You can find me on the #fx-team channel.
Updated•6 years ago
|
Attachment #8960091 -
Flags: review?(prathikshaprasadsuman)
Comment 6•6 years ago
|
||
Attachment #8960091 -
Attachment is obsolete: true
Attachment #8960907 -
Flags: review?(prathikshaprasadsuman)
Comment 7•6 years ago
|
||
Attachment #8960907 -
Attachment is obsolete: true
Attachment #8960907 -
Flags: review?(prathikshaprasadsuman)
Attachment #8960992 -
Flags: review?(prathikshaprasadsuman)
Comment 8•6 years ago
|
||
Comment on attachment 8960992 [details] [diff] [review] Patch1.txt Hi Shreya, you can export your patch to get the diff of what you changed. What you have uploaded is just the full Feeds.jsm file. If your commit is at the tip then you can run `hg export -r tip > 1445895.patch` then you can upload the 1445895.patch file.
Attachment #8960992 -
Flags: review?(prathikshaprasadsuman)
Reporter | ||
Comment 9•6 years ago
|
||
Hi Shreya, are you still trying to get the patch uploaded? Do you need more help with that?
Flags: needinfo?(shreyasonawane54)
Reporter | ||
Updated•6 years ago
|
Assignee: shreyasonawane54 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(shreyasonawane54)
Assignee | ||
Comment 10•6 years ago
|
||
I would like to work on this.
Reporter | ||
Comment 11•6 years ago
|
||
Thanks! Let me know if you have any questions!
Assignee: nobody → gilad.bau
Status: NEW → ASSIGNED
Assignee | ||
Comment 12•6 years ago
|
||
One small (big) question: after I made the change, how can I test this? I feel a bit lost in what exactly this jsm module does..
Reporter | ||
Comment 13•6 years ago
|
||
(In reply to gilad.bau from comment #12) > One small (big) question: after I made the change, how can I test this? I > feel a bit lost in what exactly this jsm module does.. Yeah, sorry, that module isn't exactly straightforward to understand or test. The function you're amending is used in a couple of places: https://searchfox.org/mozilla-central/search?q=isValidFeed&redirect=false One of them is the feed view in page info, so if you go to a news site that supports RSS and select Tools > Page Info, you should see a "Feeds" tab with the list of feeds. I hope that helps. When working on bugs you should usually understand what they're supposed to do, but good first bugs tend to be a little different since they're mostly about getting your environment set up. :) Thanks!
Assignee | ||
Comment 14•6 years ago
|
||
Thanks! I already found the file, but didn't know what it affected until now. I'm attaching my patch. Are you the reviewer?
Reporter | ||
Comment 15•6 years ago
|
||
Comment on attachment 8968333 [details] [diff] [review] bugfix_1445895.patch Review of attachment 8968333 [details] [diff] [review]: ----------------------------------------------------------------- Oh, sorry, I didn't notice this. For the future, if you want to get review you need to set the review? flag on patch attachments (or use mozreview https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview-user.html). Thanks for the patch, this looks great! I'm going to set the checkin-needed flag so that someone checks it in for us. Usually we would also do a try run (https://wiki.mozilla.org/ReleaseEngineering/TryServer) but this is a really simple patch and I assume you ran it locally, right? :)
Attachment #8968333 -
Flags: review+
Reporter | ||
Comment 16•6 years ago
|
||
Also a note on the commit message format, the correct format is: Bug ###### - Short description of patch in imperative form. r=reviewer So in your case: Bug 1445895 - Replace BrowserUtils.makeURI with Services.io.newURI in Feeds.jsm. r=johannh Can you amend your patch with the correct message (using `hg commit --amend`) and upload it again here? Thanks!
Flags: needinfo?(gilad.bau)
Assignee | ||
Comment 17•6 years ago
|
||
Attachment #8968333 -
Attachment is obsolete: true
Flags: needinfo?(gilad.bau)
Reporter | ||
Comment 18•6 years ago
|
||
Comment on attachment 8969188 [details] [diff] [review] bugfix_1445895.patch Review of attachment 8969188 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8969188 -
Flags: review+
Reporter | ||
Updated•6 years ago
|
Keywords: checkin-needed
Updated•6 years ago
|
Attachment #8960992 -
Attachment is obsolete: true
Comment 19•6 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/2569c92fd524 Replace BrowserUtils.makeURI with Services.io.newURI in Feeds.jsm. r=johannh
Keywords: checkin-needed
Comment 20•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2569c92fd524
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in
before you can comment on or make changes to this bug.
Description
•