Closed Bug 1265578 Opened 6 years ago Closed 4 years ago

Prune dead "recommended add-ons" code from AddonRepository.jsm

Categories

(Toolkit :: Add-ons Manager, defect, P4)

defect

Tracking

()

RESOLVED DUPLICATE of bug 1263313

People

(Reporter: aswan, Unassigned, Mentored)

Details

(Whiteboard: triaged[good first bug])

Attachments

(1 file, 3 obsolete files)

AddonRepository.jsm includes a method called `retrieveRecommendedAddons()` that appears to be obsolete.  If its unused, lets just take it out altogether.
Blair, do you know anything about the code in question here and whether its safe to remove it?
Flags: needinfo?(bmcbride)
I have a vague memory of this... IIRC, it was originally left in the tree because AMO's API still supported it, we weren't sure if we were going to use it again or not, and there were a couple of add-ons using it. IMO it's safe to remove (and will be good to excise dead code).
Flags: needinfo?(bmcbride)
Priority: -- → P4
Whiteboard: triaged
Mentor: aswan
Whiteboard: triaged → triaged[good first bug]
I want to work on that bug . Please assign it to me.
Assignee: nobody → deepeshiitb15
Mentor: malayaleecoder
Status: NEW → ASSIGNED
Do i have to also remove the whole test_bug424262.js file in toolkit/mozapps/extensions/test/xpcshell/test_bug424262.js to do this change because the function:retrieveRecommendedAddons() is being used here and i guess the whole code is to check if the function is valid or not? Please help.
Flags: needinfo?(amckay)
Flags: needinfo?(amckay) → needinfo?(aswan)
Yep, looks like that should be removed.
Flags: needinfo?(aswan)
How do i remove that file ? Should i just manually delete that file from my mozilla-central folder in my computer ?
This is my first patch , so please make any suggestions and give a review if possible .
Comment on attachment 8746229 [details] [diff] [review]
Modified 2 files and removed 1 file to remove the unused function retrieveRecommendedAddons()

Hi, thanks for the contribution!  I'll take a close look at the patch in a bit, but clearing unrelated flags for now (to request a review you just need to set the review? flag)
Attachment #8746229 - Flags: superreview?
Attachment #8746229 - Flags: feedback-
Comment on attachment 8746229 [details] [diff] [review]
Modified 2 files and removed 1 file to remove the unused function retrieveRecommendedAddons()

Review of attachment 8746229 [details] [diff] [review]:
-----------------------------------------------------------------

This looks very promising, thanks for your efforts so far, but there are a few more things needed to land this:
- First, please update the commit comment to explain what the commit does.  But also leave the bug number there, something like "Bug 1265578: Remove unused AddonRepository.retrieveRecommendedAddons()"
- When you removed test_bug424262.js, you also need to remove it from any test manifest files that reference it.  In this case, it needs to be removed from toolkit/mozapps/extensions/test/xpcshell/xpcshell-shared.ini
- test_AddonRepository.js is not passing with your changes.  If you have a successful build in your source tree, you should be able to run "mach test toolkit/mozapps/extensions/text/xpcshell/test_AddonRepository.js" to run it locally.  Its worth also checking that the other tests in toolkit/mozapps/test/ pass with your changes

Can you address these issues and upload an updated patch?  If you need help with building or with running tests locally, please feel free to ask here or on IRC.

::: toolkit/mozapps/extensions/test/xpcshell/test_AddonRepository.js
@@ +110,5 @@
>    repositoryStatus:       9999,
>    icons:                  {}
>  }];
>  
> +// Results of and searchAddons

Remove the word "and" here

@@ +254,5 @@
>  const TOTAL_RESULTS = 1111;
>  const MAX_RESULTS = SEARCH_RESULTS.length;
>  
>  // Used to differentiate between testing that a search success
> +// or a search failure for and searchAddons

At the least, remove the word "and" here.  Even with that change, the comment doesn't make much sense, but it already didn't make sense before your changes...
Attachment #8746229 - Flags: review?(aswan) → review-
Deepesh, have a look at the suggestions given by Andrew and submit another patch incorporating the necessary changes :)
Flags: needinfo?(deepeshiitb15)
I am now getting 2 timeout errors while i run the ./mach test toolkit/mozapps/extensions/text/xpcshell/test_AddonRepository.js , how do i remove these timeout errors ?
(In reply to deepesh00 from comment #12)
> I am now getting 2 timeout errors while i run the ./mach test
> toolkit/mozapps/extensions/text/xpcshell/test_AddonRepository.js , how do i
> remove these timeout errors ?

Hey, sorry for the slow reply, feel free to click the "need more information" box and enter my address if you have a question, that way bugzilla will let me know that you're waiting for a reply from me.

It looks like the error is on line 362 of your test output:

 5:05.14 LOG: Thread-6 INFO "CONSOLE_MESSAGE: (error) [JavaScript Error: "ReferenceError: run_test_retrieveRecommended_fails is not defined" {file: "/Users/deepeshsingh/mozilla-central/obj-firefox/_tests/xpcshell/toolkit/mozapps/extensions/test/xpcshell/test_AddonRepository.js" line: 514}]"

These tests are chained together in a kind of awkward way -- code in run_test_getAddonsByID_succeeds calls run_test_retrieveRecommended_fails directly when it is finished.  but if you removed run_test_retrieveRecommended_fails, then you should change that call to instead call the next unremoved test case.
Bug 1265578: Removed unused AddonRepository.retrieveRecommendedAddons()
Flags: needinfo?(deepeshiitb15) → needinfo?(aswan)
Attachment #8749628 - Flags: review+
Flags: needinfo?(aswan)
Attachment #8749628 - Flags: review+
Hi, this is looking good.  A few things:
1. Since you provided an updated patch, can you mark your original patch obsolete (you should be able to do this from the "details" page for the attachment)
2. Regarding the needinfo? and review? flags, you should not set the review+ flag yourself, set the review? flag to ask somebody else to review your patch, then they will set review+ if the patch passes review.  Likewise, you can set feedback? if you want somebody to look at a patch but the patch isn't really ready for a formal review, or you can set needinfo? if you have any other question that you want somebody to respond to.
3. In your actual patch, please actually remove the file test_bug424262.js (as well as removing the reference to it in xpcshell-shared.ini).  Since we won't be running that test any more, there's no need to keep it around (and it will be in mercurial history if we ever want to go back to it)
4. Finally, I started a "try" run for you which will build and run the tests under toolkit/mozapps/extensions.  I've pasted the URL below where you can follow/check the results.  If tests all pass, then we'll be ready to land this once you address the above item.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d80ce732e7a1

Thanks for all the work so far, this is getting close!
Deepak, almost there. Please have a look at what Andrew said and submit an updated patch :)
Flags: needinfo?(deepeshiitb15)
Deepak, are you still working on this bug?
Andreas, sorry that I forgot to remove the assignee. Deepak had told me that he would not continue on this bug.

NI you, to get you notified :)
Assignee: deepeshiitb15 → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(deepeshiitb15) → needinfo?(awagner)
Thanks!
Flags: needinfo?(awagner)
Hi ! I would like to work on this bug . Please would you assign it to me .
Flags: needinfo?(malayaleecoder)
Assignee: nobody → shrvan.amlekar
Flags: needinfo?(malayaleecoder)
Attached patch Bug1265578_1.diff (obsolete) — Splinter Review
I have done the changes on the updated tree . Build is also running as expected .
Attachment #8803693 - Flags: review?(aswan)
Comment on attachment 8803693 [details] [diff] [review]
Bug1265578_1.diff

Review of attachment 8803693 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!  Just a couple little test things to clean up and this should be ready to go.

::: toolkit/mozapps/extensions/test/xpcshell/test_AddonRepository.js
@@ -526,4 @@
>      searchSucceeded: function(aAddonsList, aAddonCount, aTotalResults) {
>        do_check_eq(aTotalResults, -1);
>        check_results(aAddonsList, GET_RESULTS, aAddonCount, true);
> -      run_test_retrieveRecommended_fails();

This needs to be replaced with a chained call to the next test: run_test_searchAddons_fails()

::: toolkit/mozapps/extensions/test/xpcshell/xpcshell-shared.ini
@@ -72,4 @@
>  [test_bug406118.js]
>  # Bug 676992: test consistently hangs on Android
>  skip-if = os == "android"
> -[test_bug424262.js]

Please remove the actual file as well
Attachment #8803693 - Flags: review?(aswan) → review-
Attachment #8746229 - Attachment is obsolete: true
Attachment #8749628 - Attachment is obsolete: true
Shrvan, Have look at Andrew's comment and please update the patch.
Status: NEW → ASSIGNED
Flags: needinfo?(shrvan.amlekar)
Shrvan: are you still able to work on this bug?
Andrew: He conveyed that he would not be working on this bug. Removing the assignee.
Assignee: shrvan.amlekar → nobody
Status: ASSIGNED → NEW
Flags: needinfo?(shrvan.amlekar)
Comment on attachment 8872529 [details]
Bug 1265578 - Remove unused 'retrieveRecommendedAddons()' from AddonRepository.jsm.

https://reviewboard.mozilla.org/r/144070/#review147920

This looks good to me, thank you!  But please do a try run before landing (limiting it to the toolkit/mozapps/extensions directory would be fine)
Attachment #8872529 - Flags: review?(aswan) → review+
Has this bug been resolved or is it still open? If yes, I would like to take it up.
Flags: needinfo?(aswan)
Attachment #8803693 - Attachment is obsolete: true
(In reply to meghasharma4910 from comment #29)
> Has this bug been resolved or is it still open? If yes, I would like to take
> it up.

Hm, there was a patch that was reviewed but never checked in.  I'm going to try to test and land the existing patch.  I don't expect anything to go wrong, so I would recommend looking through other good first bugs...
Flags: needinfo?(aswan)
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/69be89a46ead
Remove unused 'retrieveRecommendedAddons()' from AddonRepository.jsm. r=aswan
tiago, would you like to update this?
Flags: needinfo?(tiago.paez11)
Hi aswan!

Yes, I'd like to update it :)
Flags: needinfo?(tiago.paez11)
It wasn't clear from your comment if you need some pointers or not but in case you do:
1. Pull a recent copy of mozilla-central
2. Use "hg rebase" to rebase your original patch onto recent central.
3. Run "mach eslint" and fix any errors
4. Commit the changes with "hg commit --amend"
5. Re-push the patch to reviewboard
This ended up getting rolled into bug 1263313
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: new-amo-search
You need to log in before you can comment on or make changes to this bug.