Use gBrowser instead of getBrowser() in browser/components/pocket/content/main.js

RESOLVED FIXED in Firefox 65

Status

()

enhancement
P5
normal
RESOLVED FIXED
5 months ago
5 months ago

People

(Reporter: dao, Assigned: maxluoxiii, Mentored)

Tracking

({good-first-bug})

Trunk
Firefox 65
Points:
---

Firefox Tracking Flags

(firefox65 fixed)

Details

(Whiteboard: [lang=js])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 months ago
+++ This bug was initially created as a clone of Bug #1110069 +++

Bug 448572 deprecated getBrowser() a long time ago. We should be using gBrowser instead.

The instances that should be replaced can be found here:

https://searchfox.org/mozilla-central/rev/a3894a4dcfb5d42f2e6eee6cb9faf7141879ef1a/browser/components/pocket/content/main.js#520,524
(Assignee)

Comment 1

5 months ago
Hi can I work on this as my first bug?
(Reporter)

Comment 2

5 months ago
(In reply to Maxiwell Luo from comment #1)
> Hi can I work on this as my first bug?

Sure! Do you have the source code? Have you built Firefox yet?
(Assignee)

Comment 3

5 months ago
(In reply to Dão Gottwald [::dao] from comment #2)
> (In reply to Maxiwell Luo from comment #1)
> > Hi can I work on this as my first bug?
> 
> Sure! Do you have the source code? Have you built Firefox yet?

I haven't yet, but I should follow the instructions here, right?
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Windows_Prerequisites

Comment 4

5 months ago
Hey , looking for a good first bug to work on. Saw this one and it seemed like a good start to get involved in the community. I've built Firefox successfully, is there anything specific i should know before starting other than replacing the deprecated getBorwser() call (shown in the link above)
Priority: -- → P5

Comment 5

5 months ago
Is anyone still working on this?

Comment 6

5 months ago
(In reply to andrewc.goupil from comment #4)
> Hey , looking for a good first bug to work on. Saw this one and it seemed
> like a good start to get involved in the community. I've built Firefox
> successfully, is there anything specific i should know before starting other
> than replacing the deprecated getBorwser() call (shown in the link above)

I have begun looking at this bug, and have updated my local copy of mozilla-central, and the directory pocket does not seem to exist. Any guidance with this issue would be greatly appreciated.
Flags: needinfo?(dao+bmo)

Comment 7

5 months ago
(In reply to andrewc.goupil from comment #6)
> (In reply to andrewc.goupil from comment #4)
> > Hey , looking for a good first bug to work on. Saw this one and it seemed
> > like a good start to get involved in the community. I've built Firefox
> > successfully, is there anything specific i should know before starting other
> > than replacing the deprecated getBorwser() call (shown in the link above)
> 
> I have begun looking at this bug, and have updated my local copy of
> mozilla-central, and the directory pocket does not seem to exist. Any
> guidance with this issue would be greatly appreciated.

Did you build it?

Comment 8

5 months ago
Yes I did.(In reply to JZ from comment #7)
> (In reply to andrewc.goupil from comment #6)
> > (In reply to andrewc.goupil from comment #4)
> > > Hey , looking for a good first bug to work on. Saw this one and it seemed
> > > like a good start to get involved in the community. I've built Firefox
> > > successfully, is there anything specific i should know before starting other
> > > than replacing the deprecated getBorwser() call (shown in the link above)
> > 
> > I have begun looking at this bug, and have updated my local copy of
> > mozilla-central, and the directory pocket does not seem to exist. Any
> > guidance with this issue would be greatly appreciated.
> 
> Did you build it?
(Assignee)

Comment 9

5 months ago
(In reply to JZ from comment #5)
> Is anyone still working on this?

I've modified it locally, but I'm not entirely sure on how I should test it. I've built and run Firefox and everything seems to still be working, but I'm not sure how to do more rigorous testing to ensure that nothing has been broken. Should I be using xpcshell or something else?
(Reporter)

Comment 10

5 months ago
(In reply to Maxiwell Luo from comment #3)
> (In reply to Dão Gottwald [::dao] from comment #2)
> > (In reply to Maxiwell Luo from comment #1)
> > > Hi can I work on this as my first bug?
> > 
> > Sure! Do you have the source code? Have you built Firefox yet?
> 
> I haven't yet, but I should follow the instructions here, right?
> https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/
> Build_Instructions/Windows_Prerequisites

Yeah.

(In reply to andrewc.goupil from comment #6)
> (In reply to andrewc.goupil from comment #4)
> > Hey , looking for a good first bug to work on. Saw this one and it seemed
> > like a good start to get involved in the community. I've built Firefox
> > successfully, is there anything specific i should know before starting other
> > than replacing the deprecated getBorwser() call (shown in the link above)
> 
> I have begun looking at this bug, and have updated my local copy of
> mozilla-central, and the directory pocket does not seem to exist. Any
> guidance with this issue would be greatly appreciated.

The pocket code is in browser/components/pocket/. But Maxiwell was first, so I'll assign this bug them for now.

(In reply to Maxiwell Luo from comment #9)
> (In reply to JZ from comment #5)
> > Is anyone still working on this?
> 
> I've modified it locally, but I'm not entirely sure on how I should test it.
> I've built and run Firefox and everything seems to still be working, but I'm
> not sure how to do more rigorous testing to ensure that nothing has been
> broken. Should I be using xpcshell or something else?

Running tests in browser/components/pocket/test might be a good idea; I don't think more testing is needed here.
Assignee: nobody → maxluoxiii
Flags: needinfo?(dao+bmo)
(Reporter)

Updated

5 months ago
Component: Preferences → Pocket
(Assignee)

Comment 11

5 months ago
(In reply to Dão Gottwald [::dao] from comment #10)
> Running tests in browser/components/pocket/test might be a good idea; I don't think more testing is needed here.

I ran the tests with "./mach mochitest -f browser browser/components/pocket/test/", can you confirm that this is the correct way to run the tests? The output ended with:

Overall Summary
===============

mochitest-browser
~~~~~~~~~~~~~~~~~
Ran 29 checks (5 tests, 24 subtests)
Expected results: 29
OK

so I'm assuming the tests passed. If this is the case, how should I submit the patch?
Sorry for asking so many questions about this, I don't have much experience with Javascript testing frameworks.
(Reporter)

Comment 12

5 months ago
Yep, that's correct. See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch for how to submit a patch.
Attachment #9026581 - Attachment is obsolete: true
Attachment #9026875 - Attachment description: Bug 1506338 - Correct gBrowser usage as an object rather than as a function → Bug 1506338 - Use gBrowser instead of getBrowser() in pocket code

Comment 15

5 months ago
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f5ada684f144
Use gBrowser instead of getBrowser() in pocket code r=dao

Comment 16

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f5ada684f144
Status: NEW → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
You need to log in before you can comment on or make changes to this bug.