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)

60 Branch
enhancement

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)

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.
Can I work on this?
(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!
Hi Johann ,
I would like to work on this !
Attached patch patch (obsolete) — Splinter Review
Please review it!
Attachment #8960091 - Flags: review?(prathikshaprasadsuman)
Assignee: nobody → shreyasonawane54
Status: NEW → ASSIGNED
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.
Attachment #8960091 - Flags: review?(prathikshaprasadsuman)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8960091 - Attachment is obsolete: true
Attachment #8960907 - Flags: review?(prathikshaprasadsuman)
Attached patch Patch1.txt (obsolete) — Splinter Review
Attachment #8960907 - Attachment is obsolete: true
Attachment #8960907 - Flags: review?(prathikshaprasadsuman)
Attachment #8960992 - Flags: review?(prathikshaprasadsuman)
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)
Hi Shreya, are you still trying to get the patch uploaded? Do you need more help with that?
Flags: needinfo?(shreyasonawane54)
Assignee: shreyasonawane54 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(shreyasonawane54)
I would like to work on this.
Thanks! Let me know if you have any questions!
Assignee: nobody → gilad.bau
Status: NEW → ASSIGNED
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..
(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!
Attached patch bugfix_1445895.patch (obsolete) — Splinter Review
Thanks! I already found the file, but didn't know what it affected until now.

I'm attaching my patch. Are you the reviewer?
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+
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)
Attachment #8968333 - Attachment is obsolete: true
Flags: needinfo?(gilad.bau)
Comment on attachment 8969188 [details] [diff] [review]
bugfix_1445895.patch

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

Thanks!
Attachment #8969188 - Flags: review+
Keywords: checkin-needed
Attachment #8960992 - Attachment is obsolete: true
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
https://hg.mozilla.org/mozilla-central/rev/2569c92fd524
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.