1119872 - "restore default engines" button should also reset the selected engine to the default

Status: VERIFIED FIXED

(Firefox :: Search, defect, P3)

Description

[:Gavin Sharp [email: [e-mail]]]

See bug 1113692 comment 1. It's generally confusing to have this button only re-add the default engines, and not also reset your selected engine to the default.

Comment 1

[Jacob Crabtree]

Hey, I would love to work on this bug It will take until tomorrow for me to really kick off work on it, and it's my first bug, so I'd really appreciate a little guidance on it.

Comment 2

[Jacob Crabtree]

I'm guessing the file I would want to change would be C:\mozilla-source\mozilla-central\obj-i686-pc-mingw32\dist\bin\browser\chrome\browser\content\browser\search\engineManager.js , correct? Specifically adding code to the restoreDefaultEngines function. What is the actual function that sets an engine as the one being used in the search bar though, and what parameters does it need passed?

Comment 3

[:Gavin Sharp [email: [e-mail]]]

Hey Jacob - thanks for looking in to this. And sorry for the slow response.

You're right that engineManager.js is the right place to look. Specifically, its "restoreDefaultEngines" method:

https://hg.mozilla.org/mozilla-central/annotate/eab4a81e4457/browser/components/search/content/engineManager.js#l340

Looking into this a bit further, though, and it looks like I may have been wrong in marking this as a "good first bug", because changing that function to reset the default engine is actually a little bit involved.

Given that you're looking for a first bug, perhaps you'd be interested in taking another bug instead? Perhaps bug 510516?

Otherwise, I can walk you through the changes needed here, but multiple files would need changing and we may need a few cycles of feedback before getting a final patch - up to you!

Comment 4

[Jacob Crabtree]

I'd still like to attempt this bug. I'm not terribly well versed in javascript but I can understand the code, so I'd like to give it a shot. 

As far as I can tell, I'd want to call the changeEngine function:
https://hg.mozilla.org/mozilla-central/annotate/eab4a81e4457/browser/components/search/content/engineManager.js#l360
So, I think I should call that before the restore defaults button is disabled, correct?

I would pass it three values, and this is where my understanding starts to break down. Would I be passing it just the plain old name/string of the default mozilla search engine (is that Yahoo or Google?), or is there another value I should be giving this function?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Florian, can you take over mentorship of this bug?

Comment 6

[Florian Quèze [:florian]]

You need to edit the restoreDefaultEngines function in http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/in-content/search.js

Please ignore the engineManager.js file, it's part of the old search UI that we will remove soon.

Conveniently, a resetToOriginalDefaultEngine method has recently been added on the search service (http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsIBrowserSearchService.idl?rev=d544be8b3dc3#284). You'll want to call it with something like:
  Services.search.resetToOriginalDefaultEngine();

Comment 7

[Jacob Crabtree]

Sorry if it takes me a bit to get back with you on this bug. Possibly a few more days, I'm graduating High School soon so things are a little crazy. Hopefully over the weekend I'll have time to take a closer look at it. Regardless, thanks for the info so far Florian!

Comment 8

[Jeremy Francispillai]

Hi, I would like to take a look at this bug. Thanks

Comment 9

[Jeremy Francispillai]

Attachment: bug1119872.patch

I added in the change that you proposed with the resetToOriginalDefaultEngine() function. This changes the search engine back to the default however, it did not show the change in the dropdown menu. This was because in the buildDefaultEngineDropDown function, the dropdown menu defaulted to show what was previously selected and only show the current default if no search engine was selected in the menu. I believe that this is pointless as the current default engine is always updated whenever a new engine is selected in the dropdown menu, so there is never a case where you would want to use the selected engine over the actual current default engine. Even if in theory there was a case where the selected engine should be picked over the currentEngine and the selected engine is not equal to the currentEngine, this would not be good since the selected engine in the dropdown menu would not properly reflect the actual search engine that is being used, so this should never be the case. I therefore removed the portion in buildDefaultEngineDropDown() where it first attempts to set the current engine to the one that is selected in the dropdown menu. This fixes my problem and shouldn't change any of the previous functionality in any way.

Comment 10

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/701fb57ec4c7afea9dae110aee653f6d457e9bb1
Bug 1119872 - Changed "Restore default engines" button action to also reset the selected engine to the default engine, r=florian.

Comment 11

[Florian Quèze [:florian]]

Comment on attachment 8685762 [details] [diff] [review]
bug1119872.patch

Sorry for the delay in reviewing this, I was off last week. Thanks for the patch, it looks good to me.

(In reply to Jeremy Francispillai from comment #9)

> in the buildDefaultEngineDropDown function, the dropdown
> menu defaulted to show what was previously selected and only show the
> current default if no search engine was selected in the menu. I believe that
> this is pointless as the current default engine is always updated whenever a
> new engine is selected in the dropdown menu, so there is never a case where
> you would want to use the selected engine over the actual current default
> engine.

In case you are curious about the reason why the code was this way: I think it's a leftover from when we had preferences in a separate dialog that had Apply/OK/Cancel buttons. Now that all changes apply immediately, it's indeed pointless and we can remove it.

># HG changeset patch
># User Jeremy Francispillai <[e-mail]>
># Parent  e2a910c048dc82fc3be53475f18e7f81f03e377b
>Changed "Restore default engines" button action to also reset the selected engine to the default engine.

In the future, could you please make the commit message follow this format?
Bug NNNNNNN - <message>, r=<reviewer>

Comment 12

[Florian Quèze [:florian]]

As comment 10 shows, I pushed the patch to the fx-team repository; assuming everything goes fine, it should be merged to mozilla-central within about a day, and then be in the next Nightly.

If you would be interested in working on another bug in the same code area, bug 1122618 is related. If you would like to try touching a different area of the code, I can suggest bug 1170193.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/701fb57ec4c7

Comment 14

[Camelia Badau [:cbadau], Desktop QA]

Verified fixed on latest Nightly 45.0a1 (buildID: 20151118030232).

Comment 15

[Simona Badau, Desktop QA]

Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Firefox/45.0
Build ID: 20151120030227

If the restored default engine is other than Google the default engine is restored only after restarting Firefox. Used the following scenario:
1. Launch Firefox
2. Go to about:preferences#search and set Amazon as the default engine
3. In the One-click search engines list -> select Amazon -> click the "Remove" button
4. Click on the "Restore Default Search Engines" button 

Expected Results:
Amazon is placed back in the "One Click search engines" list and is set back as the Default engine.

Actual Results:
Amazon is placed back in the "One Click search engines" list but is not set back as the Default engine (Google remains the default engine).
It's set as the default engine only after restarting Firefox.

Should I file a new bug to cover this scenario or Re-open this one?

Comment 16

[Florian Quèze [:florian]]

(In reply to Simona B from comment #15)

> Expected Results:
> Amazon is placed back in the "One Click search engines" list and is set back
> as the Default engine.

The expected result is that Google should stay the default engine, even after a restart.

> Actual Results:
> Amazon is placed back in the "One Click search engines" list but is not set
> back as the Default engine (Google remains the default engine).

This is as expected.

> [Amazon i]s set as the default engine only after restarting Firefox.

This is unexpected, please file a bug covering this. The actual fix may turn out to the be same as for bug 1165341, but having a possibly duplicate bug won't hurt; forgetting this would. Thanks for catching this! :)

Comment 17

[Florian Quèze [:florian]]

(In reply to Florian Quèze [:florian] [:flo] from comment #16)
> (In reply to Simona B from comment #15)

> > [Amazon i]s set as the default engine only after restarting Firefox.
> 
> This is unexpected, please file a bug covering this.

This was filed as bug 1227045.