Last Comment Bug 1327953 - Removing search engines from preferences disabled 'Restore defaults' button
: Removing search engines from preferences disabled 'Restore defaults' button
Status: VERIFIED FIXED
: regression
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: 46 Branch
: All All
P1 normal (vote)
: Firefox 54
Assigned To: Florian Quèze [:florian] [:flo]
:
: Florian Quèze [:florian] [:flo]
Mentors:
: 1332671 (view as bug list)
Depends on:
Blocks: 1122618
  Show dependency treegraph
 
Reported: 2017-01-01 03:15 PST by arni2033
Modified: 2017-02-06 00:09 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
wontfix
verified
verified
verified
unaffected


Attachments
Patch (1.20 KB, patch)
2017-01-24 09:28 PST, Florian Quèze [:florian] [:flo]
standard8: review+
jcristau: approval‑mozilla‑aurora+
jcristau: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description User image arni2033 2017-01-01 03:15:12 PST
>>>   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 1 User image Panos Astithas [:past] 2017-01-24 06:08:34 PST
*** Bug 1332671 has been marked as a duplicate of this bug. ***
Comment 2 User image Panos Astithas [:past] 2017-01-24 06:11:02 PST
I doubt we can get the regressing patch author to work on this, Florian can you take a look?
Comment 3 User image Florian Quèze [:florian] [:flo] 2017-01-24 09:28:35 PST
Created attachment 8829920 [details] [diff] [review]
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 User image Paul Silaghi, QA [:pauly] 2017-01-25 00:48:39 PST
Also note that adding a new search engine (eg. "youtube") doesn't enable the "Restore defaults" button.
Comment 5 User image Florian Quèze [:florian] [:flo] 2017-01-25 02:20:04 PST
(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 User image Mark Banner (:standard8) 2017-01-27 02:36:24 PST
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 User image Florian Quèze [:florian] [:flo] 2017-01-29 23:14:57 PST
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 User image Carsten Book [:Tomcat] 2017-01-30 07:08:03 PST
https://hg.mozilla.org/mozilla-central/rev/c79f0867b4ec
Comment 9 User image Florian Quèze [:florian] [:flo] 2017-01-31 09:08:08 PST
(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 User image Florian Quèze [:florian] [:flo] 2017-01-31 09:13:49 PST
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 User image Julien Cristau [:jcristau] 2017-02-01 04:32:44 PST
Comment on attachment 8829920 [details] [diff] [review]
Patch

looks safe enough, let's get this in aurora53 and beta52
Comment 13 User image Carsten Book [:Tomcat] 2017-02-01 07:30:44 PST
https://hg.mozilla.org/releases/mozilla-beta/rev/aa531b366492
Comment 14 User image Forhad Hossain 2017-02-01 10:04:34 PST
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 User image fahimazulfath.a 2017-02-03 08:30:09 PST
[testday-20170203]
The bug is resolved and hence verified. 
OS: windows 10.0
Browser: Firefox Beta 51.0
Comment 16 User image Paul Silaghi, QA [:pauly] 2017-02-06 00:09:50 PST
Verified fixed FX 52b3, 53.0a2 (2017-02-05) Win 7.

Note You need to log in before you can comment on or make changes to this bug.