1487854 - browser.search.get should use getVisibleEngines method

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P5)

Description

[kernp25]

But it uses Services.search.getEngines method instead and then filter the hidden engines out.

Comment 1

[kernp25]

What do you think?

Comment 2

[Mike Kaply [:mkaply]]

No idea why I did it that way, but yeah, probably worth a one liner.

Comment 3

[Caitlin Neiman [:caitmuenster]]

If this is your first contribution, please refer to https://wiki.mozilla.org/Add-ons/Contribute/Code on how to get started.

Comment 4

[Arshad Kazmi [:arshadkazmi42]]

I am interested to work on this.

So I explored a bit about this and found this file

https://searchfox.org/mozilla-central/source/browser/components/extensions/parent/ext-search.js#38

I think, changes needs to be made in this file

But I am not so sure, about what types of test cases will be required to test this.
Or it should just pass the existing test cases which is already there?

Comment 5

[Mike Kaply [:mkaply]]

(In reply to Arshad Kazmi from comment #4)
> I am interested to work on this.
> 
> So I explored a bit about this and found this file
> 
> https://searchfox.org/mozilla-central/source/browser/components/extensions/
> parent/ext-search.js#38
> 
> I think, changes needs to be made in this file

You are correct.

> But I am not so sure, about what types of test cases will be required to
> test this.
> Or it should just pass the existing test cases which is already there?

There should not need to be a new test for this. Just passing the existing test should be enough.

Comment 6

[Arshad Kazmi [:arshadkazmi42]]

For this checking this test case is enough? 
Or is there any other test case which needs to be validated too?

https://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_search.js

Comment 7

[Rob Wu [:robwu]]

Yes, verifying that the following test passes is sufficient:
mach test browser/components/extensions/test/browser/browser_ext_search.js

Comment 8

[Arshad Kazmi [:arshadkazmi42]]

Attachment: Bug 1487854 - Use getVisibleEngines in browser.search.get

Comment 9

[Rob Wu [:robwu]]

Comment on attachment 9008659 [details]
Bug 1487854 - Use getVisibleEngines in browser.search.get

Rob Wu [:robwu] has approved the revision.

Comment 10

[Andrew Swan [:aswan]]

Comment on attachment 9008659 [details]
Bug 1487854 - Use getVisibleEngines in browser.search.get

Andrew Swan [:aswan] has approved the revision.

Comment 11

[Arshad Kazmi [:arshadkazmi42]]

should I add check-in needed flag in this now?

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05fcdb6d47cf
Use getVisibleEngines in browser.search.get r=robwu, aswan

Comment 13

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/05fcdb6d47cf