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

RESOLVED DUPLICATE of bug 1263313

Status

()

P4
normal
RESOLVED DUPLICATE of bug 1263313
3 years ago
11 months ago

People

(Reporter: aswan, Unassigned, Mentored)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged[good first bug])

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

3 years ago
AddonRepository.jsm includes a method called `retrieveRecommendedAddons()` that appears to be obsolete.  If its unused, lets just take it out altogether.
(Reporter)

Comment 1

3 years ago
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)

Updated

3 years ago
Priority: -- → P4
Whiteboard: triaged

Updated

3 years ago
Mentor: aswan
Whiteboard: triaged → triaged[good first bug]

Comment 3

3 years ago
I want to work on that bug . Please assign it to me.
Assignee: nobody → deepeshiitb15
Mentor: malayaleecoder
Status: NEW → ASSIGNED

Comment 4

3 years ago
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.

Updated

3 years ago
Flags: needinfo?(amckay)

Updated

3 years ago
Flags: needinfo?(amckay) → needinfo?(aswan)
(Reporter)

Comment 5

3 years ago
Yep, looks like that should be removed.
Flags: needinfo?(aswan)

Comment 6

3 years ago
How do i remove that file ? Should i just manually delete that file from my mozilla-central folder in my computer ?

Comment 7

3 years ago
Created attachment 8746229 [details] [diff] [review]
Modified 2 files and removed 1 file to remove the unused function retrieveRecommendedAddons()
Attachment #8746229 - Flags: superreview?
Attachment #8746229 - Flags: review?(aswan)
Attachment #8746229 - Flags: feedback-

Comment 8

3 years ago
This is my first patch , so please make any suggestions and give a review if possible .
(Reporter)

Comment 9

3 years ago
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-
(Reporter)

Comment 10

3 years ago
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...
(Reporter)

Updated

3 years ago
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)

Comment 12

3 years ago
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 ?
(Reporter)

Comment 14

3 years ago
(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.

Comment 15

3 years ago
Created attachment 8749628 [details] [diff] [review]
Removed unused AddonRepository.retrieveRecommendedAddons()

Bug 1265578: Removed unused AddonRepository.retrieveRecommendedAddons()
Flags: needinfo?(deepeshiitb15) → needinfo?(aswan)
Attachment #8749628 - Flags: review+
(Reporter)

Updated

3 years ago
Flags: needinfo?(aswan)
Attachment #8749628 - Flags: review+
(Reporter)

Comment 16

3 years ago
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)

Comment 21

2 years ago
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)

Comment 22

2 years ago
Created attachment 8803693 [details] [diff] [review]
Bug1265578_1.diff

I have done the changes on the updated tree . Build is also running as expected .
Attachment #8803693 - Flags: review?(aswan)
(Reporter)

Comment 23

2 years ago
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-
(Reporter)

Updated

2 years ago
Attachment #8746229 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
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 hidden (mozreview-request)
(Reporter)

Comment 28

a year ago
mozreview-review
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+

Comment 29

a year ago
Has this bug been resolved or is it still open? If yes, I would like to take it up.

Updated

a year ago
Flags: needinfo?(aswan)
(Reporter)

Updated

a year ago
Attachment #8803693 - Attachment is obsolete: true
(Reporter)

Comment 30

a year ago
(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)

Comment 31

a year ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/69be89a46ead
Remove unused 'retrieveRecommendedAddons()' from AddonRepository.jsm. r=aswan
(Reporter)

Comment 33

a year ago
tiago, would you like to update this?
Flags: needinfo?(tiago.paez11)
Hi aswan!

Yes, I'd like to update it :)
Flags: needinfo?(tiago.paez11)
(Reporter)

Comment 35

a year ago
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
(Reporter)

Comment 36

11 months ago
This ended up getting rolled into bug 1263313
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1263313
You need to log in before you can comment on or make changes to this bug.