1122618 - Default search engine drop down in preferences isn't updated when something else changes the default search engine

Status: VERIFIED FIXED

(Firefox :: Search, defect, P3)

Description

[Petruta Horea [:phorea], Desktop QA]

Having about:preferences#search page opened and then switching the default search engine using Ctrl / Cmd + up/down keyboard shortcuts doesn't show the selected search engine unless refreshing the page.

Reproduces on all branches, all platforms.

Comment 1

[Petruta Horea [:phorea], Desktop QA]

This also reproduces when adding a search engine from https://addons.mozilla.org/en-US/firefox/search/?atype=4 and set it as default.

Comment 3

[Jeremy Francispillai]

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

Comment 4

[Jeremy Francispillai]

Attachment: bug1122618.patch

It took a while to figure out how to update the page whenever the search engine changed, but after I found it, it was a simple one line addition.

Comment 5

[Florian Quèze [:florian]]

Comment on attachment 8690576 [details] [diff] [review]
bug1122618.patch

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

Thanks for the patch!

::: browser/components/preferences/in-content/search.js
@@ +180,5 @@
>          gEngineView.invalidate();
>          break;
>        case "engine-removed":
>        case "engine-current":
> +        gSearchPane.buildDefaultEngineDropDown();

This will execute this line for both the "engine-current" and the "engine-removed" notifications. Is this intended? For the "engine-removed" case, if the user clicked the 'remove' button from the search preferences, buildDefaultEngineDropDown will have already been called from the removeEngine method. If the engine was removed from somewhere else (eg. by an add-on, or by the search preferences opened in another window), calling buildDefaultEngineDropDown isn't enough because the engine hasn't been removed from gEngineView._engineStore.

There are 2 possible ways forward here:
- either you want to go for the smallest patch that fixes this bug as described in comment 0, in which case the patch should be adapted so that buildDefaultEngineDropDown is called only for the engine-current notification.
- or you want to further cleanup/fix the search.js code, and separate the code actually performing changes from the code displaying changes. Currently the two are entangled, but the code would be significantly simpler if changes were only displayed when receiving notifications. If you want to work on this, the search way to test is to open 'about:preferences#search' in 2 different windows, make changes to the preferences in one window and check that both windows keep displaying the same thing at all times.

Comment 6

[Jeremy Francispillai]

Hi Florian, I will take a closer look at this in the next few days when I have time.

Comment 7

[Jeremy Francispillai]

Attachment: bug1122618.txt

Hi i had some time today and I changed around the code to get rid of the above problem that you specified where I was also calling buildDefaultEngineDropDown when an engine was removed. I also changed the code responsible for the removal of an engine so that removing the actual engine is separated from displaying the changes. The code for making the change and displaying the change is also separated for changing the current default engine as well as for adding a new engine. I didn't separate the two operations for the cases where engines are moved around (change engine order by drag and drop, or reordering engines when "restore default engines" is clicked) because I cannot think of a good way to update the display once an engine has been  moved. One possible idea that I had for this was to call the Services.search.getVisibleEngines() function and then compare the position of the moved engine in the returned list compared to the displayed list. I didn't implement this because I do not think that it is very efficient and I don't know if there is a better way. Let me know if there is a good way to do this as well as if my current changes make sense. I have attached a diff of my changes so far.

Comment 8

[Florian Quèze [:florian]]

Comment on attachment 8696306 [details] [diff] [review]
bug1122618.txt

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

Sorry for the delay, I somehow missed the new patch in my bugmail. In the future, please don't hesitate to set a feedback flag on attachments when you expect some input from me/a reviewer.

Your current changes make sense :-). It's fine with me if you just address the 2 following comments and don't further expand the scope to fix the move/restore default cases.

I don't see any solution to the restore default case that wouldn't significantly expand the scope of the patch. The only idea I have right now is to move most of the code of the restoreDefaultEngines method to nsSearchService, so that search.js can listen to a single notification for the whole thing, and rebuild the tree and the drop down. If we really want to pursue this, I think it would be better in a separate bug.

::: browser/components/preferences/in-content/search.js
@@ +371,5 @@
>      Services.search.moveEngine(aEngine.originalEngine, aNewIndex);
>    },
>  
>    removeEngine: function ES_removeEngine(aEngine) {
> +    var index = -1;

Can this be simplified using https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/findIndex ?

::: toolkit/components/search/nsSearchService.js
@@ +3991,4 @@
>        // Since we removed an engine, we need to update the preferences.
>        this._saveSortedEngineList();
>      }
> +    

nit: please avoid adding trailing whitespace.

Comment 9

[Jeremy Francispillai]

Attachment: bug1122618updated.patch

Here is my updated patch. I changed the code to find the index using array.findIndex. I can no longer find the whitespace that you were referring to, but I will pay attention to whitespace in my future patches. Thanks

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8703748 [details] [diff] [review]
bug1122618updated.patch

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

Thanks for the updated patch. The following comments are mostly cosmetic details :-).

For the commit message, we indicate the reviewer using irc nicks rather than the full name, eg. for me it would be r=florian.

::: browser/components/preferences/in-content/search.js
@@ +371,5 @@
>      Services.search.moveEngine(aEngine.originalEngine, aNewIndex);
>    },
>  
>    removeEngine: function ES_removeEngine(aEngine) {
> +    var index = this._engines.findIndex(function(element){ return element.name == aEngine.name; });

function(element){ return element.name == aEngine.name; }
can be simplified to:
  element => element.name == aEngine.name

Additionally, to avoid calling the 'name' getter repeatedly on the aEngine object, you can store it in a variable before calling findIndex.

nit: in new code we tend to replace 'var' with 'let'. I would do it here.

::: toolkit/components/search/nsSearchService.js
@@ +4019,4 @@
>        // Since we removed an engine, we need to update the preferences.
>        this._saveSortedEngineList();
>      }
> +    

You still have trailing whitespace on this line.

Comment 11

[Jeremy Francispillai]

Attachment: bug1122618updated.patch

Added the specified changes.

Comment 12

[Florian Quèze [:florian]]

Comment on attachment 8704231 [details] [diff] [review]
bug1122618updated.patch

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

Thanks! I'll test this one last time tomorrow before setting the r+ flag.

::: browser/components/preferences/in-content/search.js
@@ +371,5 @@
>      Services.search.moveEngine(aEngine.originalEngine, aNewIndex);
>    },
>  
>    removeEngine: function ES_removeEngine(aEngine) {
> +    let aEngineName = aEngine.name;

The 'a' prefix means 'argument', we don't use it for local variables (and we don't really use that prefix anymore in new code these days).

Comment 13

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/fx-team/rev/503ebfe13cd2f079829f4a4c8b7e860b4989403f
Bug 1122618 - Added change to rebuild the defaultEngineDropDown whenever the default engine is changed. Separated the code performing the changes and displaying the changes when removing/adding engines. r=florian.

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/503ebfe13cd2

Comment 15

[Florian Quèze [:florian]]

Hi Jeremy. Thanks for fixing this bug. If you would like suggestions for another bug to look at, I can propose bug 1170193 or bug 1214284.

Comment 16

[Jeremy Francispillai]

Hi Florian, thanks for the help. I am currently back in school right now and I won't have a lot of free time for the next little while. I will pick back up where I left off in when I have some time.

Comment 17

[Florian Quèze [:florian]]

(In reply to Jeremy Francispillai from comment #16)
> Hi Florian, thanks for the help.

You are welcome!

> I am currently back in school right now and
> I won't have a lot of free time for the next little while. I will pick back
> up where I left off in when I have some time.

No worries, feel free to pick another bug whenever you have time and feel like it.

Comment 18

[Iulia Cristescu, QA [:JuliaC] (away, please needinfo? [e-mail])]

I verified the bug. This is verified fixed on:
- 46.0b7 (20160330161941),
- 47.0a2 (2016-04-01),
- 48.0a1 (2016-03-31),
using Windows 7 Ultimate x64, Ubuntu 14.04 x86 and Mac OS X 10.11 Beta.