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

RESOLVED FIXED in Firefox 61

Status

()

enhancement
P5
normal
RESOLVED FIXED
a year ago
a year ago

People

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

Tracking

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

60 Branch
Firefox 61
Points:
---
Dependency tree / graph

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

a year ago
I would like to work on this.
Reporter

Comment 11

a year ago
Thanks! Let me know if you have any questions!
Assignee: nobody → gilad.bau
Status: NEW → ASSIGNED
Assignee

Comment 12

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

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

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

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

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

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

Comment 18

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

a year ago
Keywords: checkin-needed
Attachment #8960992 - Attachment is obsolete: true

Comment 19

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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2569c92fd524
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.