Replace BrowserUtils.makeURI with Services.io.newURI in Feeds.jsm

RESOLVED FIXED in Firefox 61

Status

()

P5
normal
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: johannh, Assigned: gilad.bau, Mentored)

Tracking

(Blocks: 1 bug, {good-first-bug})

60 Branch
Firefox 61
good-first-bug
Points:
---

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

a year ago
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?
(Reporter)

Comment 2

a year 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

a year ago
Hi Johann ,
I would like to work on this !

Comment 4

a year ago
Posted patch patch (obsolete) — Splinter Review
Please review it!
Attachment #8960091 - Flags: review?(prathikshaprasadsuman)

Updated

a year ago
Assignee: nobody → shreyasonawane54
Status: NEW → ASSIGNED

Comment 5

a year 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

a year ago
Attachment #8960091 - Flags: review?(prathikshaprasadsuman)

Comment 6

a year ago
Posted patch Patch (obsolete) — Splinter Review
Attachment #8960091 - Attachment is obsolete: true
Attachment #8960907 - Flags: review?(prathikshaprasadsuman)

Comment 7

a year ago
Posted patch Patch1.txt (obsolete) — Splinter Review
Attachment #8960907 - Attachment is obsolete: true
Attachment #8960992 - Flags: review?(prathikshaprasadsuman)
Attachment #8960907 - 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)
(Reporter)

Comment 9

a year ago
Hi Shreya, are you still trying to get the patch uploaded? Do you need more help with that?
Flags: needinfo?(shreyasonawane54)
(Reporter)

Updated

a year ago
Assignee: shreyasonawane54 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(shreyasonawane54)
(Assignee)

Comment 10

11 months ago
I would like to work on this.
(Reporter)

Comment 11

11 months ago
Thanks! Let me know if you have any questions!
Assignee: nobody → gilad.bau
Status: NEW → ASSIGNED
(Assignee)

Comment 12

11 months 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

11 months 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

11 months ago
Posted 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?
(Reporter)

Comment 15

11 months 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

11 months 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

11 months ago
Attachment #8968333 - Attachment is obsolete: true
Flags: needinfo?(gilad.bau)
(Reporter)

Comment 18

11 months 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

11 months ago
Keywords: checkin-needed
Attachment #8960992 - Attachment is obsolete: true

Comment 19

11 months 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

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2569c92fd524
Status: ASSIGNED → RESOLVED
Last Resolved: 11 months 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.