Closed Bug 1506338 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox :: Pocket, enhancement, P5)

Product:
Firefox
Component:
Pocket
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 65
Tracking Flags:
Tracking Status
firefox65 --- fixed

People

(Reporter: dao, Assigned: maxluoxiii, Mentored)

Details

(Keywords: good-first-bug, Whiteboard: [lang=js])

Attachments

(1 file, 1 obsolete file)

Bug 1506338 - Use gBrowser instead of getBrowser() in pocket code
47 bytes, text/x-phabricator-request
Details | Review 
+++ 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
Hi can I work on this as my first bug?
(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?
(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
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
Is anyone still working on this?
(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)
(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?
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?
(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?
(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)
Component: Preferences → Pocket
(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.
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
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f5ada684f144
Use gBrowser instead of getBrowser() in pocket code r=dao
https://hg.mozilla.org/mozilla-central/rev/f5ada684f144
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox65: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: