Support action: reset search

RESOLVED FIXED in Firefox 40

Status

Firefox Health Report
Client: Desktop
RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: Benjamin Smedberg, Assigned: André Reinald)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

31 Branch
Firefox 40
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 13 obsolete attachments)

7.24 KB, patch
André Reinald
: review+
Details | Diff | Splinter Review
1.35 KB, patch
André Reinald
: review+
Details | Diff | Splinter Review
(Reporter)

Description

4 years ago
Implement a support action type which resets all Firefox search providers and settings back to the factory default.
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Comment 3

3 years ago
Created attachment 8586044 [details] [diff] [review]
bugzilla-1075157-2.patch

WIP test still missing.
Attachment #8586043 - Attachment is obsolete: true
Attachment #8586043 - Flags: feedback?(florian)
Attachment #8586044 - Flags: feedback?(florian)
(Assignee)

Comment 4

3 years ago
This patch is supposed to be applied after patch for bug 1075160 lands.
(Assignee)

Updated

3 years ago
Assignee: nobody → areinald
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.
Attachment #8586044 - Flags: feedback?(florian) → feedback+
OS: Linux → All
Hardware: x86_64 → All
(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)
(Assignee)

Comment 8

3 years ago
Created attachment 8594864 [details] [diff] [review]
bugzilla-1075157-3.patch

Aside from tests, is the patch ok?
Attachment #8586044 - Attachment is obsolete: true
Attachment #8594864 - Flags: feedback?(florian)
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.
Attachment #8594864 - Flags: feedback?(florian) → feedback+
(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.
(Assignee)

Comment 11

3 years ago
Created attachment 8596739 [details] [diff] [review]
bugzilla-1075157-4.patch

WIP
Attachment #8594864 - Attachment is obsolete: true
(Assignee)

Comment 12

3 years ago
Created attachment 8597330 [details] [diff] [review]
assert-notDeepEqual.patch

Change Assert.notDeepEqual to use ObjectUtils, the same way Assert.deepEqual does.
Attachment #8597330 - Flags: review?(gfritzsche)
(Assignee)

Comment 13

3 years ago
Created attachment 8597332 [details] [diff] [review]
bugzilla-1075157-5.patch
Attachment #8596739 - Attachment is obsolete: true
Attachment #8597332 - Flags: review?(gfritzsche)
Attachment #8597332 - Flags: review?(florian)
(Assignee)

Comment 14

3 years ago
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"
Attachment #8597332 - Flags: review?(gfritzsche) → feedback+
Attachment #8597330 - Flags: review?(gfritzsche) → review+
(Assignee)

Comment 18

3 years ago
Created attachment 8598570 [details] [diff] [review]
bugzilla-1075157-6.patch

Changed as per comments from Gavin and Georg.
Attachment #8597332 - Attachment is obsolete: true
Attachment #8597332 - Flags: review?(florian)
Attachment #8598570 - Flags: review?(gfritzsche)
Attachment #8598570 - Flags: review?(florian)
(Assignee)

Comment 19

3 years ago
Created attachment 8598575 [details] [diff] [review]
bugzilla-1075157-7.patch

Removed an unwanted change in the comment of a test.
Attachment #8598570 - Attachment is obsolete: true
Attachment #8598570 - Flags: review?(gfritzsche)
Attachment #8598570 - Flags: review?(florian)
Attachment #8598575 - Flags: review?(gfritzsche)
Attachment #8598575 - Flags: review?(florian)
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(...)|.
Attachment #8598575 - Flags: review?(gfritzsche) → review+
Comment hidden (obsolete)
Comment hidden (obsolete)
(Assignee)

Comment 23

3 years ago
Created attachment 8598624 [details] [diff] [review]
bugzilla-1075157-8.patch
Attachment #8598575 - Attachment is obsolete: true
Attachment #8598621 - Attachment is obsolete: true
Attachment #8598575 - Flags: review?(florian)
Attachment #8598621 - Flags: review?(florian)
Attachment #8598624 - Flags: review?(florian)
(Assignee)

Comment 24

3 years ago
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+.
Attachment #8598624 - Flags: review+
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.
Attachment #8598621 - Attachment is obsolete: false
Attachment #8598621 - Flags: feedback+
(Assignee)

Updated

3 years ago
Attachment #8598621 - Attachment is obsolete: true
(Assignee)

Comment 26

3 years ago
Created attachment 8598680 [details] [diff] [review]
bugzilla-1075157-9.patch

Changed according to :florian's comments on previous patch.
Attachment #8598624 - Attachment is obsolete: true
Attachment #8598624 - Flags: review?(florian)
Attachment #8598680 - Flags: review?(florian)
(Assignee)

Comment 27

3 years ago
Comment on attachment 8598680 [details] [diff] [review]
bugzilla-1075157-9.patch

Carrying over r+ from :gfritzche in bugzilla-1075157-7.patch (now obsolete).
Attachment #8598680 - Flags: review+
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.
Attachment #8598680 - Flags: review?(florian) → review+
(Assignee)

Comment 29

3 years ago
Created attachment 8598722 [details] [diff] [review]
bugzilla-1075157-10.patch

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

Changed function name. Waiting for green light before asking review again.
Attachment #8598680 - Attachment is obsolete: true
(Assignee)

Comment 30

3 years ago
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?
Attachment #8598722 - Flags: feedback?(florian)
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.
Attachment #8598722 - Flags: feedback?(florian)
(Assignee)

Comment 32

3 years ago
Comment on attachment 8598722 [details] [diff] [review]
bugzilla-1075157-10.patch

r+ from :gfritzche in comment 20 and :florian in comments 28.
Attachment #8598722 - Flags: review+
(Assignee)

Comment 33

3 years ago
Patches apply in order.
Keywords: checkin-needed
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?
Flags: needinfo?(areinald)
Keywords: checkin-needed
(Assignee)

Comment 35

3 years ago
Comment on attachment 8598722 [details] [diff] [review]
bugzilla-1075157-10.patch

Support action, twin to bug 1075160.
Flags: needinfo?(areinald)
Attachment #8598722 - Flags: review?(bobbyholley)
Comment on attachment 8598722 [details] [diff] [review]
bugzilla-1075157-10.patch

Review of attachment 8598722 [details] [diff] [review]:
-----------------------------------------------------------------

rs=me on the webidl change.
Attachment #8598722 - Flags: review?(bobbyholley) → review+
(Assignee)

Comment 37

3 years ago
Created attachment 8600509 [details] [diff] [review]
bugzilla-1075157-10.patch

r+ from :florian, :gfritzche and bobbyholley@gmail.com
Added bug number in commit message.
Attachment #8598722 - Attachment is obsolete: true
Attachment #8600509 - Flags: review+
(Assignee)

Comment 38

3 years ago
Created attachment 8600510 [details] [diff] [review]
assert-notDeepEqual.patch

r+ from :gfritzche
Added bug number in commit message.
Attachment #8597330 - Attachment is obsolete: true
Attachment #8600510 - Flags: review+
(Assignee)

Comment 39

3 years ago
Checkin needed of:
1. Attachment #8600510 [details] [diff]
2. Attachment #8600509 [details] [diff]
Keywords: checkin-needed
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.
Flags: needinfo?(ryanvm)
Status: NEW → ASSIGNED
I'm on pto today. A green try push would make a strong case for what you're arguing, though ;)
Flags: needinfo?(ryanvm)
(Assignee)

Comment 45

3 years ago
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?
Flags: needinfo?(ryanvm)
(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
Flags: needinfo?(ryanvm)
(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.
Flags: needinfo?(areinald)
https://hg.mozilla.org/mozilla-central/rev/d2417b679160
https://hg.mozilla.org/mozilla-central/rev/d544be8b3dc3
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 40
(Assignee)

Comment 51

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

Updated

3 years ago
Flags: needinfo?(areinald)
See Also: → bug 1161993
You need to log in before you can comment on or make changes to this bug.