Closed Bug 1415727 Opened 5 years ago Closed 5 years ago

resetToOriginalDefaultEngine doesn't work if the original engine is hidden

Categories

(Firefox :: Search, defect, P2)

Product:
Firefox
Component:
Search
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 59
Tracking Flags:
Tracking Status
firefox59 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

Bug 1415727 - resetToOriginalDefaultEngine should unhide engine.
59 bytes, text/x-review-board-request
florian
: review+
Details 
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.
Is that needed in 57 ?
(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.
Priority: -- → P2
Whiteboard: [fxsearch]
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?
Attachment #8933474 - Flags: review?(florian) → review-
> 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?
(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 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.
Attachment #8933474 - Flags: review?(florian) → review+
Pushed by mozilla@kaply.com:
https://hg.mozilla.org/integration/autoland/rev/91033fe6389a
resetToOriginalDefaultEngine should unhide engine. r=florian
https://hg.mozilla.org/mozilla-central/rev/91033fe6389a
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Assignee: nobody → mozilla
You need to log in before you can comment on or make changes to this bug.