1327953 - Removing search engines from preferences disabled 'Restore defaults' button

Status: VERIFIED FIXED

(Firefox :: Search, defect, P1)

Description

[arni2033]

>>>   My Info:   Win7_64, Nightly 49, 32bit, ID 20160526082509
STR_1:
1. Open about:preferences#search in a clear profile
2. Select the last search engine
3. Click button "Remove" 3 times to delete selected search engine

AR:  Button "Restore Default Search Engines" is disabld
ER:  The button should be enabled, because 3 default search engines were deleted.

This is regression from bug 1122618. Regression range:
> https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=861971c670e1fe27c8bf43833831d7ce17b46f00&tochange=503ebfe13cd2f079829f4a4c8b7e860b4989403f@ Jeremy Francispillai:
It seems that this is a regresion caused by your change. Please have a look.

Comment 2

[Panos Astithas (he/him) [:past] (please ni?)]

I doubt we can get the regressing patch author to work on this, Florian can you take a look?

Comment 3

[Florian Quèze [:florian]]

Attachment: Patch

Attempting to access an index of an array right after removing it with .splice can't go well. Trivial fix, inspired from what moveEngine does a few lines above.

Comment 4

[Paul Silaghi, QA [:pauly]]

Also note that adding a new search engine (eg. "youtube") doesn't enable the "Restore defaults" button.

Comment 5

[Florian Quèze [:florian]]

(In reply to Paul Silaghi, QA [:pauly] from comment #4)
> Also note that adding a new search engine (eg. "youtube") doesn't enable the
> "Restore defaults" button.

The "Restore defaults" button is meant to unhide the default engines, it doesn't remove user-installed engines, so it's fine that adding an engine doesn't enable the button.

Comment 6

[Mark Banner (:standard8)]

Comment on attachment 8829920 [details] [diff] [review]
Patch

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

Looks good. Though it'd be nice to have this covered by a test.

Comment 7

[Florian Quèze [:florian]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c79f0867b4ece170ec6a2a1832839d22278c45b2
Bug 1327953 - Fix enabling the 'Restore Default Search Engines' button when removing a default engine, r=Standard8.

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c79f0867b4ec

Comment 9

[Florian Quèze [:florian]]

(In reply to Mark Banner (:standard8) from comment #6)
> Though it'd be nice to have this covered by a test.

Yes, it would be nice to have better test coverage for the Search pane. I started looking at it, and then was annoyed by the excessive complexity of all this code. I filed bug 1335467.

Comment 10

[Florian Quèze [:florian]]

Comment on attachment 8829920 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1122618
[User impact if declined]: No easy way to get a default engine back after removing it (reloading the preference UI will work as a workaround).
[Is this code covered by automated tests?]: Unfortunately, no. I filed bug 1335467 to improve this for the future.
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: see initial bug description.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low risk
[Why is the change risky/not risky?]: very simple patch
[String changes made/needed]: none

Comment 11

[Julien Cristau [:jcristau]]

Comment on attachment 8829920 [details] [diff] [review]
Patch

looks safe enough, let's get this in aurora53 and beta52

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0e1d4619f4c6

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/aa531b366492

Comment 14

[Forhad Hossain]

I have to reproduce this bug with Nightly 53.0a1 (2017-01-01) with instruction from comment 0 and Linux 32 Bit.

The Bug's fix is now verified on Latest Nightly 54.0a1 (2017-01-31)

Build ID:20170131110402
User Agent:Mozilla/5.0 (X11; Linux i686; rv:54.0) Gecko/20100101 Firefox/54.0 

[bugday-20170201]

Comment 15

[Fahima Zulfath]

[testday-20170203]
The bug is resolved and hence verified. 
OS: windows 10.0
Browser: Firefox Beta 51.0

Comment 16

[Paul Silaghi, QA [:pauly]]

Verified fixed FX 52b3, 53.0a2 (2017-02-05) Win 7.