1415727 - resetToOriginalDefaultEngine doesn't work if the original engine is hidden

Status: RESOLVED FIXED

(Firefox :: Search, defect, P2)

Description

[Mike Kaply [:mkaply]]

If you hide the default engine, then call resetToOriginalDefaultEngine, it resets to a different engine.

The function should unhide the original engine if it is hidden.

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Is that needed in 57 ?

Comment 2

[Florian Quèze [:florian]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #1)
> Is that needed in 57 ?

No. Nor in 58. It's just an improvement to make our API less surprising.

Comment 3

[Sylvestre Ledru [:Sylvestre]]

ok, merci

Comment 4

[Mike Kaply [:mkaply]]

Attachment: Bug 1415727 - resetToOriginalDefaultEngine should unhide engine.

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

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8933474 [details]
Bug 1415727 - resetToOriginalDefaultEngine should unhide engine.

https://reviewboard.mozilla.org/r/204404/#review210138

::: toolkit/components/search/nsSearchService.js:2837
(Diff revision 1)
>  
>      return this.getEngineByName(defaultEngine);
>    },
>  
>    resetToOriginalDefaultEngine: function SRCH_SVC__resetToOriginalDefaultEngine() {
> +    this.originalDefaultEngine.hidden = false;

Please don't call the originalDefaultEngine getter twice.

Should we cleanup existing callers that no longer need to do the .hidden = false dance?

Comment 6

[Mike Kaply [:mkaply]]

> Should we cleanup existing callers that no longer need to do the .hidden = false dance?

I checked that and the cases i found were enumerating through the entire default list and unhiding everything.

Did you find something different?

Comment 7

[Florian Quèze [:florian]]

(In reply to Mike Kaply [:mkaply] from comment #6)

> Did you find something different?

No, it looks like https://searchfox.org/mozilla-central/rev/7f45cb7cc0229398fc99849bdc150cb6462d6966/browser/components/nsBrowserGlue.js#2202 is the only "caller" that could be cleaned up.

Comment 8

[Mike Kaply [:mkaply]]

Comment on attachment 8933474 [details]
Bug 1415727 - resetToOriginalDefaultEngine should unhide engine.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204404/diff/1-2/

Comment 9

[Florian Quèze [:florian]]

Comment on attachment 8933474 [details]
Bug 1415727 - resetToOriginalDefaultEngine should unhide engine.

https://reviewboard.mozilla.org/r/204404/#review210994

::: toolkit/components/search/nsSearchService.js:2837
(Diff revisions 1 - 2)
>  
>      return this.getEngineByName(defaultEngine);
>    },
>  
>    resetToOriginalDefaultEngine: function SRCH_SVC__resetToOriginalDefaultEngine() {
> -    this.originalDefaultEngine.hidden = false;
> +    let originalDefaultEngine = this.originalDefaultEngine;;

Please remove the duplicated ';' here.

Comment 10

[Mike Kaply [:mkaply]]

Comment on attachment 8933474 [details]
Bug 1415727 - resetToOriginalDefaultEngine should unhide engine.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/204404/diff/2-3/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/91033fe6389a
resetToOriginalDefaultEngine should unhide engine. r=florian

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/91033fe6389a