Implement a support action type which resets all Firefox search providers and settings back to the factory default.

WIP test still missing.

This patch is supposed to be applied after patch for bug 1075160 lands.

restoreDefaultEngines() does't currently reset the selected engine, so we'd likely want to fix that too. Bug 1119872 is related.

Comment on attachment 8586044 [details] [diff] [review] bugzilla-1075157-2.patch Review of attachment 8586044 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/selfsupport/SelfSupportService.js @@ +67,5 @@ > > clearUserPref: function(name) { > Services.prefs.clearUserPref(name); > }, > + nit: please avoid adding trailing whitespace. @@ +69,5 @@ > Services.prefs.clearUserPref(name); > }, > + > + restoreDefaultSearchEngines: function() { > + Services.search.restoreDefaultEngines(); Like Gavin said in comment 5, this won't reset the selected engine. You'll either need to change the restoreDefaultEngines implementation in the search service, or need to add more code here. ::: browser/components/selfsupport/test/browser_1075157.js @@ +1,1 @@ > +function test() { I guess for the test you'll want to get a list of all the visible engines, and the currently selected engine, then hide some engines, change the selected engine, then call your restoreDefaultSearchEngines method, then check that the list of visible engines and the selected engine are identical to what you had initially.

(related to https://bugzilla.mozilla.org/show_bug.cgi?id=1155259?) (I think this is related to an action on the Support pages, but it seems like language and UI should an could be unified)

Aside from tests, is the patch ok?

Comment on attachment 8594864 [details] [diff] [review] bugzilla-1075157-3.patch Review of attachment 8594864 [details] [diff] [review]: ----------------------------------------------------------------- Looks reasonable. Note that I'm not a reviewer for the dom/webidl/MozSelfSupport.webidl file. Also, technically I'm not a reviewer for netwerk/, but I suspect the nsIBrowserSearchService.idl file really wants to live in toolkit/ and just ended up in netwerk/ accidentally, so I'm fine with reviewing that file. ::: netwerk/base/nsIBrowserSearchService.idl @@ +365,5 @@ > + * > + * @returns The corresponding nsISearchEngine object, or null if it doesn't > + * exist. > + */ > + nsISearchEngine originalDefaultEngine(); This is actually implemented as a getter rather than a function, so it should be declared as: readonly attribute nsISearchEngine originalDefaultEngine; Then I don't think the documentation comments for attributes includes the @returns line; you can probably merge the 2 sentences of the documentation comment into one.

(In reply to Florian Quèze [:florian] [:flo] from comment #9) > Looks reasonable. Note that I'm not a reviewer for the > dom/webidl/MozSelfSupport.webidl file. I will do the SelfSupport parts - i'm not familiar with the search services intricacies though.

WIP

Change Assert.notDeepEqual to use ObjectUtils, the same way Assert.deepEqual does.

Comment on attachment 8597332 [details] [diff] [review] bugzilla-1075157-5.patch The tests of this patch rely on Assert.notDeepEqual, so previous patch should be applied first.

I think I would prefer leaving "originalDefaultEngine" as an implementation detail and just offering a resetSelectedEngineToDefault() method (better name welcome) that just does this.selectedEngine == this._originalDefaultEngine.

(In reply to :Gavin Sharp [email: gavin@gavinsharp.com] from comment #15) > I think I would prefer leaving "originalDefaultEngine" as an implementation > detail and just offering a resetSelectedEngineToDefault() method (better > name welcome) that just does this.selectedEngine == > this._originalDefaultEngine. Why? One advantage I see to exposing originalDefaultEngine is that it lets consumers check if the current engine is the original default or was changed by the user. Possible other name: resetDefaultEngine(). Note: if there wasn't all the existing history of this code, my real preference would be to keep currentEngine as it is today, and make defaultEngine readonly and return today's _originalDefaultEngine. That's the meaning I understood/assumed for these 2 properties before I started actually reading the code. I think this would be significantly less confusing.

Comment on attachment 8597332 [details] [diff] [review] bugzilla-1075157-5.patch Review of attachment 8597332 [details] [diff] [review]: ----------------------------------------------------------------- This looks ok for the SelfSupport changes, Florian will have to judge about the searchengine specific parts (and their use). ::: browser/components/selfsupport/test/browser_selfsupportAPI.js @@ +57,5 @@ > + Assert.deepEqual(Services.search.getVisibleEngines(), visibleEnginesOriginal, "default visible engines set should be reset"); > + > + // 2. change the default search engine > + Services.search.defaultEngine = visibleEnginesOriginal[3]; > + Assert.notEqual(Services.search.defaultEngine, defaultEngineOriginal, "default engine should be different"); Why not check for |equal(Services.search.defaultEngine, visibleEnginesOriginal[3])|? @@ +64,5 @@ > + Assert.deepEqual(Services.search.getVisibleEngines(), visibleEnginesOriginal, "default visible engines set should be reset"); > + > + // 3. remove an engine > + Services.search.removeEngine(visibleEnginesOriginal[2]); > + Assert.notDeepEqual(Services.search.getVisibleEngines(), visibleEnginesOriginal, "default visible engines set should be different"); The list might be different for various reasons. Why not check specifically that visibleEnginesOriginal[2] is not in the list of visible engines? @@ +69,5 @@ > + MozSelfSupport.resetSearchEngines(); > + Assert.equal(Services.search.defaultEngine, defaultEngineOriginal, "default engine should be reset"); > + Assert.deepEqual(Services.search.getVisibleEngines(), visibleEnginesOriginal, "default visible engines set should be reset"); > + > + // 4. add en angine Nit: "an engine"

Changed as per comments from Gavin and Georg.

Removed an unwanted change in the comment of a test.

Comment on attachment 8598575 [details] [diff] [review] bugzilla-1075157-7.patch Review of attachment 8598575 [details] [diff] [review]: ----------------------------------------------------------------- This looks ok to me from the selfsupport side, i'm leaving the search specific bits to Florian. ::: browser/components/selfsupport/test/browser_selfsupportAPI.js @@ +48,5 @@ > + > +function test_resetSearchEngines() > +{ > + let defaultEngineOriginal = Services.search.defaultEngine; > + let visibleEnginesOriginal = Services.search.getVisibleEngines(); You could just make those two const. @@ +53,5 @@ > + > + // 1. do nothing on unchanged search configuration > + MozSelfSupport.resetSearchEngines(); > + Assert.equal(Services.search.defaultEngine, defaultEngineOriginal, "default engine should be reset"); > + Assert.deepEqual(Services.search.getVisibleEngines(), visibleEnginesOriginal, "default visible engines set should be reset"); Nit: Some lines here are getting rather long, you could just put the messages on a new line for those. @@ +67,5 @@ > + > + // 3. remove an engine > + let engineRemoved = visibleEnginesOriginal[2]; > + Services.search.removeEngine(engineRemoved); > + Assert.ok(!Services.search.getVisibleEngines().some(engine => engine == engineRemoved), "removed engine should not be visible any more"); Just |find(engine) == -1| instead of |some(...)|.

Comment on attachment 8598624 [details] [diff] [review] bugzilla-1075157-8.patch Wrapped long lines, and changed .some to .indexOf in assertion, as per gfritzsche's comment. Carrying over his r+.

Comment on attachment 8598621 [details] [diff] [review] bugzilla-1075157-8.patch Review of attachment 8598621 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/selfsupport/test/browser_selfsupportAPI.js @@ +6,5 @@ > > const prefExistingName = "extensions.hotfix.id"; > Assert.ok(Preferences.has(prefExistingName), "pref should exist"); > + Assert.ok(!Preferences.isSet(prefExistingName), > + "pref should not be user-set"); nit: Not sure why you are changing this line, but if you make the parameter wrap, please align it with the first one. @@ +59,5 @@ > + MozSelfSupport.resetSearchEngines(); > + Assert.equal(Services.search.defaultEngine, defaultEngineOriginal, > + "default engine should be reset"); > + Assert.deepEqual(Services.search.getVisibleEngines(), visibleEnginesOriginal, > + "default visible engines set should be reset"); Please align this parameter with the first one. Also applies to the other deepEqual calls, and the Assert.ok call. ::: netwerk/base/nsIBrowserSearchService.idl @@ +281,5 @@ > */ > readonly attribute bool isInitialized; > > /** > + * Resets the default engine to its original value nit: add a '.' at the end of the sentence. @@ +283,5 @@ > > /** > + * Resets the default engine to its original value > + */ > + void resetOriginalDefaultEngine(); I don't think the word 'Original' here helps to understand what the method does. ::: toolkit/components/search/nsSearchService.js @@ +3304,5 @@ > return this.getEngineByName(defaultEngine); > }, > > + resetOriginalDefaultEngine: function SRCH_SVC__resetOriginalDefaultEngine() { > + this._defaultEngine = this._originalDefaultEngine; You don't want to set _defaultEngine directly, please use the defaultEngine setter instead so that observers are notified.

Changed according to :florian's comments on previous patch.

Comment on attachment 8598680 [details] [diff] [review] bugzilla-1075157-9.patch Carrying over r+ from :gfritzche in bugzilla-1075157-7.patch (now obsolete).

Comment on attachment 8598680 [details] [diff] [review] bugzilla-1075157-9.patch Review of attachment 8598680 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the name of the method changed, and a green try server build. ::: netwerk/base/nsIBrowserSearchService.idl @@ +283,5 @@ > > /** > + * Resets the default engine to its original value. > + */ > + void resetOriginalDefaultEngine(); Let's name this resetToOriginalDefaultEngine.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=687c38d07378 Changed function name. Waiting for green light before asking review again.

Comment on attachment 8598722 [details] [diff] [review] bugzilla-1075157-10.patch Build and test finished but some failures happen which I can't relate to any of my changes. Any suggestions from here?

Comment on attachment 8598722 [details] [diff] [review] bugzilla-1075157-10.patch (In reply to André Reinald from comment #30) > Comment on attachment 8598722 [details] [diff] [review] > bugzilla-1075157-10.patch > > Build and test finished but some failures happen which I can't relate to any > of my changes. Any suggestions from here? These failures looked unrelated and intermittent. To be sure I retriggered these jobs, and indeed the second runs were green.

Comment on attachment 8598722 [details] [diff] [review] bugzilla-1075157-10.patch r+ from :gfritzche in comment 20 and :florian in comments 28.

Patches apply in order.

This is being rejected because it needs a DOM peer's review. Can the bug numbers be added to the commit messages for both patches while we get that?

Comment on attachment 8598722 [details] [diff] [review] bugzilla-1075157-10.patch Support action, twin to bug 1075160.

Comment on attachment 8598722 [details] [diff] [review] bugzilla-1075157-10.patch Review of attachment 8598722 [details] [diff] [review]: ----------------------------------------------------------------- rs=me on the webidl change.

r+ from :florian, :gfritzche and bobbyholley@gmail.com Added bug number in commit message.

r+ from :gfritzche Added bug number in commit message.

Checkin needed of: 1. Attachment #8600510 [details] [diff] 2. Attachment #8600509 [details] [diff]

https://hg.mozilla.org/integration/fx-team/rev/cc9ff30e82d6 https://hg.mozilla.org/integration/fx-team/rev/c6380188c418

Backed out for causing Android debug mochitest crashes/asserts. Not sure which of the two patches was responsible. https://hg.mozilla.org/integration/fx-team/rev/b4ddf5fe16f6 https://treeherder.mozilla.org/logviewer.html#?job_id=2925666&repo=fx-team https://treeherder.mozilla.org/logviewer.html#?job_id=2925578&repo=fx-team https://treeherder.mozilla.org/logviewer.html#?job_id=2925579&repo=fx-team https://treeherder.mozilla.org/logviewer.html#?job_id=2925580&repo=fx-team

This is mostly Desktop-specific changes. There is a single change to nsIBrowserSearchService though, maybe this was missing a clobber? Otherwise i don't see a trigger for these failures.

I'm on pto today. A green try push would make a strong case for what you're arguing, though ;)

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

Just to make sure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f2a051e573cc As you can see there are LOTS of failures with NO patch applied. What's wrong?

(In reply to André Reinald from comment #44) > Started : > https://treeherder.mozilla.org/#/jobs?repo=try&revision=ae18d32aa048 Looks good. The fact that this is green suggests some sort of clobber issue, which is a build system bug if true. Typically if an IDL change were at fault, it would be due to a missing UUID bump (and therefore the build system being unaware anything had changed). However, this patch clearly did bump the nsIBrowserSearchService UUID. So I think the right thing to do here is to re-land this with a touch of the CLOBBER file in the root of the source tree. Additionally, please file a Core::Build Config bug blocking bug 941904 so that the root cause of this can be investigated and fixed. (In reply to André Reinald from comment #45) > As you can see there are LOTS of failures with NO patch applied. > What's wrong? We have lots of intermittent failures. Story of my life. Also, FWIW, you could have run a much more efficient Try push by only checking Android debug mochitests since that was the only platform of concern with the previous backout. Running extra platforms and test suites causes delays for other developers, so please avoid doing so if it isn't necessary. https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices

https://hg.mozilla.org/integration/fx-team/rev/d2417b679160 https://hg.mozilla.org/integration/fx-team/rev/d544be8b3dc3

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #46) > Additionally, please file a Core::Build Config bug blocking bug 941904 > so that the root cause of this can be investigated and fixed.

https://hg.mozilla.org/mozilla-central/rev/d2417b679160 https://hg.mozilla.org/mozilla-central/rev/d544be8b3dc3

https://hg.mozilla.org/mozilla-central/rev/d2417b679160 https://hg.mozilla.org/mozilla-central/rev/d544be8b3dc3

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #48) > (In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #46) > > Additionally, please file a Core::Build Config bug blocking bug 941904 > > so that the root cause of this can be investigated and fixed. Done: bug 1161993. Thanks.