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

Status: RESOLVED DUPLICATE of bug 1263313

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

Description

[Andrew Swan [:aswan]]

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

Comment 1

[Andrew Swan [:aswan]]

Blair, do you know anything about the code in question here and whether its safe to remove it?

Comment 2

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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).

Comment 3

[deepesh00]

I want to work on that bug . Please assign it to me.

Comment 4

[deepesh00]

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.

Comment 5

[Andrew Swan [:aswan]]

Yep, looks like that should be removed.

Comment 6

[deepesh00]

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

Comment 7

[deepesh00]

Attachment: Modified 2 files and removed 1 file to remove the unused function retrieveRecommendedAddons()

Comment 8

[deepesh00]

This is my first patch , so please make any suggestions and give a review if possible .

Comment 9

[Andrew Swan [:aswan]]

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)

Comment 10

[Andrew Swan [:aswan]]

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...

Comment 11

[Vishnu (:malayaleecoder)]

Deepesh, have a look at the suggestions given by Andrew and submit another patch incorporating the necessary changes :)

Comment 12

[deepesh00]

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 ?

Comment 13

[deepesh00]

http://pastebin.com/xuxbMpDG

Comment 14

[Andrew Swan [:aswan]]

(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

[deepesh00]

Attachment: Removed unused AddonRepository.retrieveRecommendedAddons()

Bug 1265578: Removed unused AddonRepository.retrieveRecommendedAddons()

Comment 16

[Andrew Swan [:aswan]]

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!

Comment 17

[Vishnu (:malayaleecoder)]

Deepak, almost there. Please have a look at what Andrew said and submit an updated patch :)

Comment 18

[Andreas Wagner [:TheOne] [use NI]]

Deepak, are you still working on this bug?

Comment 19

[Vishnu (:malayaleecoder)]

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 :)

Comment 20

[Andreas Wagner [:TheOne] [use NI]]

Thanks!

Comment 21

[shrvan]

Hi ! I would like to work on this bug . Please would you assign it to me .

Comment 22

[shrvan]

Attachment: Bug1265578_1.diff

I have done the changes on the updated tree . Build is also running as expected .

Comment 23

[Andrew Swan [: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

Comment 24

[Vishnu (:malayaleecoder)]

Shrvan, Have look at Andrew's comment and please update the patch.

Comment 25

[Andrew Williamson [:eviljeff]]

Shrvan: are you still able to work on this bug?

Comment 26

[Vishnu (:malayaleecoder)]

Andrew: He conveyed that he would not be working on this bug. Removing the assignee.

Comment 27

[Santiago Paez [:tiago]]

Attachment: Bug 1265578 - Remove unused 'retrieveRecommendedAddons()' from AddonRepository.jsm.

Review commit: https://reviewboard.mozilla.org/r/144070/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/144070/

Comment 28

[Andrew Swan [:aswan]]

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)

Comment 29

[meghasharma4910]

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

Comment 30

[Andrew Swan [:aswan]]

(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...

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/69be89a46ead
Remove unused 'retrieveRecommendedAddons()' from AddonRepository.jsm. r=aswan

Comment 32

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/b5954d91e026 for eslint failures, https://treeherder.mozilla.org/logviewer.html#?job_id=126767811&repo=autoland

Comment 33

[Andrew Swan [:aswan]]

tiago, would you like to update this?

Comment 34

[Santiago Paez [:tiago]]

Hi aswan!

Yes, I'd like to update it :)

Comment 35

[Andrew Swan [:aswan]]

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

Comment 36

[Andrew Swan [:aswan]]

This ended up getting rolled into bug 1263313