1637931 - Remove test functions from SearchService.idl

Status: RESOLVED FIXED

(Firefox :: Search, task, P3)

Description

[Dale Harvey (:daleharvey)]

I added a bunch of test only functions to nsISearchService.idl because I didnt realise we could call them via |Services.search.wrappedJSObject|

Comment 1

[Dale Harvey (:daleharvey)]

To explain the issue, I added 4 functions @ https://searchfox.org/mozilla-central/source/toolkit/components/search/nsISearchService.idl#212 to the .idl file that are only used in JavaScript tests, we can search for those function calls, for instance: https://searchfox.org/mozilla-central/search?q=Services.search.reset%28%29&path=&case=false&regexp=false
and replace them with calls to Services.search.wrappedJSObject.reset() and remove the idl definitions

Comment 2

[Kanishk]

Hey, I'm new here and would like to work on this bug.
Seems straightforward. What tests can I run locally to make sure that everything is working fine after I modify the files?

Comment 3

[Kanishk]

Attachment: Bug 1637931 - replace instances of Services.search.(reset/makeEngineFromConfig/ensureBuiltinExtension) with Services.search.wrappedJSObject.(...) in test files. r=daleharvey

Depends on D81457

Comment 4

[Kanishk]

Sorry about the "Depends on D81457" in my previous comment. It seems one of my commits for an unrelated bug interfered while committing this. How do I revert this?

Comment 5

[Kanishk]

I submitted a patch where I replaced the instances of reset, makeEngineFromConfig and ensureBuiltinExtension with wrappedJSObject.(...).

I noticed that the function reInit() is used in some non-test files as well (toolkit/components/search/(SearchCache.jsm, SearchService.jsm)).

How should I proceed? Should I replace only the test occurrences of search.reInit() with search.wrappedJSObject.reInit() ? And is it safe to remove the reInit definition from nslSearchService.idl? I haven't removed any of the definitions in this patch.

Comment 6

[Kanishk]

Depends on D81457

Sorry for the mess of attachments notifications. Two of my unrelated REVs got tangled up, but it should be fixed now.

Comment 7

[Dale Harvey (:daleharvey)]

Awesome thank you, If you run the tests @

$ ./mach test toolkit/components/search/tests/xpcshell/test_

and they pass it should be good although ultimately will need to push it to try @ https://firefox-source-docs.mozilla.org/tools/try/index.html

Looking at those usages of reInit we can probably skip that for now, they arent really supposed to be production code but they could be, might want to look at that seperately

Comment 8

[Mark Banner (:standard8)]

On the reInit side, I'll be looking at that in bug 1559530, which I'll hopefully get to in the next couple of weeks. I hope to remove it completely, but we'll have to see when we get there.

Comment 9

[Kanishk]

Attachment: Bug 1637931 - remove reset, makeEngineFromConfig, ensureBuiltinExtension from nsISearchService.idl. r=daleharvey

Depends on D81500

Comment 10

[Kanishk]

I submitted the patch for review, but it created 2 revisions:

• D81500(replacing with wrappedJSObject.(...)) and
• child REV D81549(removing reset, makeEngineFromConfig, ensureBuiltinExtension from nsISearchService.idl)

I guess this happened because I had them as separate consecutive commits. I should have done hg commit --amend instead?

Let me know if I need to change anything.

Comment 11

[Dale Harvey (:daleharvey)]

Hi Kanishk

So these patches look good to me, good job cheers, if you can figure out how to merge those commits it would make more sense as a single commit (I dont know hg so cant help you there sorry)

The next step is to push this to try to make sure that we arent missing any usages that get broken when we remove from the .idl, there is documentation for how to do this @ https://firefox-source-docs.mozilla.org/tools/try/index.html and https://firefox-source-docs.mozilla.org/tools/try/configuration.html

When you push, since you are changing an idl if you could remember to push with ./mach try auto --no-artifact this will make sure that the idl files get rebuilt

Thanks, will try to keep an eye out in #introductions for any questions

Comment 12

[Kanishk]

Attachment: Bug 1637931 - Remove testonly functions from nsISearchService.idl r=daleharvey

Depends on D81500

Comment 13

[Kanishk]

Sorry about the mess, the complete patch is: https://phabricator.services.mozilla.com/D81665.
I'm yet to test it on the try server, I was just looking into the procedure to obtain level 1 commit access.
I'll update once I push this to try.

Comment 14

[Kanishk]

I have opened a bug as a request for Level 1 commit access as described here https://www.mozilla.org/en-US/about/governance/policies/commit/

https://bugzilla.mozilla.org/show_bug.cgi?id=1649412

Can you please comment on that bug as a voucher?

Comment 15

[Dale Harvey (:daleharvey)]

Have vouched, sorry its release day so its going a little slowly, in the meantime I pushed it to try myself so will see how its gone soon

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6afaad8b5df9f9f27952aac4c89eaaf5563bae9

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e7262a324ee
Remove testonly functions from nsISearchService.idl r=daleharvey

Comment 17

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/1e7262a324ee