Remove unused variables from MockProvider (remains of deleted _delayCallback)
Categories
(Toolkit :: Add-ons Manager, task, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox71 | --- | fixed |
People
(Reporter: robwu, Assigned: abowler, Mentored)
References
(Blocks 1 open bug)
Details
(Keywords: good-first-bug)
Attachments
(2 files)
In https://hg.mozilla.org/mozilla-central/rev/112ff1e321a , the _delayCallback
method was removed from toolkit/mozapps/extensions/test/browser/head.js
.
The callbackTimers
and timerLocations
member variables are no longer used, and should be removed, together with https://searchfox.org/mozilla-central/rev/7556a400affa9eb99e522d2d17c40689fa23a729/toolkit/mozapps/extensions/test/browser/head.js#844-860
Reporter | ||
Updated•5 years ago
|
Reporter | ||
Comment 1•5 years ago
|
||
This is a good first bug, and can be fixed by removing the code that was referenced in comment 0, including the declarations of the variables in the same file.
To get started, set up a development environment following the instructions at:
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build
Comment 2•5 years ago
|
||
Hello Rob, I am new to Mozilla open source. I would like to fix this bug!
Reporter | ||
Comment 3•5 years ago
|
||
Hi Chaitanya, welcome!
The first two comments explain what needs to be done to resolve this bug. To get started, set up a development environment. Once you have written a patch, submit the patch. Feel free to message me if you have any questions.
Comment 4•5 years ago
|
||
Hey Rob,
I've followed all instructions in Setup guide, but I seem to be stuck at the final step.
I ran ./mach bootstrap which completed successfully.
Now I cannot find ./mach or build anywhere. Also the instructions run out here and I do not how to proceed.
Any help would be appreciated
Reporter | ||
Comment 5•5 years ago
|
||
Which operating system are you using? From the slashes I guess that you're using macOS or Linux, but correct me if I'm wrong.
Since you've successfully ran ./mach bootstrap
before, it is obvious that you have a copy of the mozilla-central
repository.
If ./mach build
does not work any more, then your current directory is not mozilla-central
, so you need to change the directory back to mozilla-central
.
When you know that you did really create mozilla-central
, but forgot about the location, then you can use the find
command to locate it again. E.g. find / -type d -name mozilla-central
.
Comment 6•5 years ago
|
||
Sorry I forgot to mention I'm running Linux on my system.
I tried running ./mach build after changing directory to /mozilla-central
The script is running now!
Comment 7•5 years ago
|
||
Just a quick update, the make script has done the job. I will get to fixing the bug now!
Comment 8•5 years ago
|
||
Hey Rob!
I have followed the steps and removed all instances of 'callbackTimers' and 'timerLocations' and also edited the 'MP_shutdown()' function.
I am trying to './mach build' but my laptop (6GB ram and enough free space) froze. I forced shutdown and ran './mach busted' which showed me a few bugs. Problem is I cannot find any support on how to fix these bugs
'''
Bug 1552120 - Deleted search extensions are not rebuilt
Bug 1557030 - "ERROR: Could not find libclang to generate rust bindings for C/C++"
Bug 1557157 - mach wpt - ValueError: Expecting : delimiter: line 9001 column 75 (char 765061)
Bug 1556371 - |mach doc | command is not working
Bug 1541424 - mach try fuzzy <path> is ignoring skip-if and disabled in mochitest.ini
Bug 1554987 - Invalid remote name: "hg::ssh://hg.mozilla.org/try"
error running ./mach try (fuzzy|chooser) on a git repo
Bug 1487552 - Provide further steps to devs for xcode-select --install on Mojave (AKA 'inttypes.h' file not found)
Bug 1500924 - Packaging fennec is broken until build is clobbered
Bug 1543447 - Multiple minutes to dump a stack from NS_ASSERTION in mochitests and reftests on OS X
Bug 1548948 - |mach install| fails to start x86 emulator
Bug 1522931 - OSX SDK version check doesn't work when using default SDK
Bug 1547040 - --try-test-paths isn't respected anymore
Bug 1547269 - Don't put LLVM in $PATH since we now prefer .mozbuild
Bug 1544295 - Local linux debug build fails to run (GLIBCXX_3.4.22 not found)
'''
Is there a fix for this?
Reporter | ||
Comment 9•5 years ago
|
||
The only time when I experienced a frozen system when compiling (not sure if it was Firefox) was when I ran out of memory.
Without other details, I would still guess that running out of memory is the most likely condition.
With artifact builds, the system requirements (and time to build!) are significantly lower. So unless you want to make change C++ code, I suggest that you enable artifact builds instead, following the instructions at: https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Build_Instructions/Simple_Firefox_build/Linux_and_MacOS_build_preparation#Let%E2%80%99s_Build_Firefox
Comment 10•5 years ago
|
||
Hey Rob!
Sorry I couldn't get back to you earlier. I have been reading up on Arcanist and Phabricator to understand the code review process.
But I am still uncertain as to what tests I must run before submitting my code for review. Also since you are the mentor on this bug, should I assign you as a reviewer or the module owner (Who I think is Narcis Beleuzu) ?
Thanks a lot
Comment 11•5 years ago
|
||
Just a quick update,
I ran mach build with artifact build enabled in my .mozconfig and it built succesfully on the second try. It didn't hang my system this time and was done much faster. After that I followed the link https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/So_you_just_built_Firefox for further steps. I am now running the xpcshell-test script.
Comment 12•5 years ago
|
||
Hey Rob!
So I ran mach test in the relevant folder (/mozilla-central/toolkit/mozapps/extensions/test/browser)
The overall summary : "Ran 6950 checks (6869 subtests, 81 tests) Expected results: 6942 Skipped: 5 tests Unexpected results: 3 subtest: 3 (3 fail) "
I am debugging the code now, there seem to be some issues with window closing which may be related to the MP_shutdown method
Reporter | ||
Comment 13•5 years ago
|
||
I don't expect any failures from removing the code. To see whether the issue is related to your change at all, you could temporarily revert your changes and see if the test still fails.
Since this is test-only code, I would look up the tests that use MockProvider and run those to check whether everything still works as expected. Or conversely, you could look at the list of failing tests and check whether they contain MockProvider.
Comment 14•5 years ago
|
||
Hey Rob!
You were right, when I run mach test on the original code with all instances of callbackTimers and timerLocations also returns the same failed tests. I have committed to changeset 476922 with the commit message including the bug id.
Do I need to submit it for review to you? Or is someone else responsible for the review? I have already read the docs on Phabricator and Arcanist, so it shouldn't take me long
Reporter | ||
Comment 15•5 years ago
|
||
I will be providing the first round of review. Once that looks right, I will find you a module peer to sign off on your patch.
Once you've created a Phabricator account and uploaded your patch, just add me as a reviewer.
(you've probably already seen https://wiki.mozilla.org/WebExtensions/Contribution_Onramp#Submitting_a_Patch )
Comment 16•5 years ago
|
||
Before this change unused variables callbackTimers and timerLocations were included in head.js, which were remains of removed method _delayCallback. The function MP_shutdown is now simplified as _delayCallback is removed.
Comment 17•5 years ago
|
||
I have added you as reviewer from username robwu
I would be happy to do anything else as required
Comment 18•5 years ago
|
||
Hey Chaitanya, how's it going with this bug?
Comment 19•5 years ago
|
||
Hey Chaitanya, since we haven't heard from you in awhile we're going to re-open this bug to other contributors. If you'd like to continue working on it, just let us know!
Assignee | ||
Comment 20•5 years ago
|
||
I would like to take on this bug if Chaitanya is not able to continue with it.
Can you please clarify if you would need the original changes to be done again or if there is a way to build off of Chaitanya's work.
Comment 21•5 years ago
|
||
Hey abowler2! If you go to the top of the bug you can see an attachment that links to Chaitanya's patch. You can take a look at the diff and also Rob's comments. :)
Rob's currently attending an event for the rest of the week, but if you want to submit a patch in Phabricator, he should be able to review it when he gets back next week.
Happy bug fixing!
Assignee | ||
Comment 22•5 years ago
|
||
Thank you.
I will go over what was worked on previously and Rob's comments and submit my patch once done.
Assignee | ||
Comment 23•5 years ago
|
||
Before this change / _delayCallback was removed. As a result the variables callbackTimers, timerLocations, and useAsyncCallback were no longer needed.
Reporter | ||
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 24•5 years ago
|
||
Thanks for your feedback Rob.
checkin-needed
Comment 25•5 years ago
|
||
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2340335f5458
remove unused variables in MockProvider r=robwu
Comment 26•5 years ago
|
||
bugherder |
Comment 27•5 years ago
|
||
Thanks so much for the patch, abowler2! 🎉 Your Mozillians profile has been vouched for and your contribution has been added to our recognition wiki: https://wiki.mozilla.org/Add-ons/Contribute/Recognition
Welcome onboard! We look forward to seeing you around.
Assignee | ||
Comment 28•5 years ago
|
||
Thank you!! I'm looking forward to continued contributions.
Description
•