The default bug view has changed. See this FAQ.

Removing search engines from preferences disabled 'Restore defaults' button

VERIFIED FIXED in Firefox 52

Status

()

Firefox
Search
P1
normal
VERIFIED FIXED
3 months ago
2 months ago

People

(Reporter: arni2033, Assigned: florian)

Tracking

({regression})

46 Branch
Firefox 54
regression
Points:
---

Firefox Tracking Flags

(firefox51 wontfix, firefox52 verified, firefox53 verified, firefox54 verified, firefox-esr45 unaffected)

Details

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
>>>   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.
(Reporter)

Updated

3 months ago
No longer blocks: 1277113
(Reporter)

Updated

3 months ago
Component: Untriaged → Search
Duplicate of this bug: 1332671
I doubt we can get the regressing patch author to work on this, Florian can you take a look?
Flags: needinfo?(florian)
Priority: -- → P1
Summary: Search engine can't notice the change if I shoot them from the back (needs better summary) → Removing search engines from preferences disabled 'Restore defaults' button
status-firefox51: --- → affected
status-firefox52: --- → affected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr45: --- → unaffected
See Also: → bug 1228318
(Assignee)

Comment 3

2 months ago
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.
Attachment #8829920 - Flags: review?(standard8)
(Assignee)

Updated

2 months ago
Assignee: nobody → florian
Status: NEW → ASSIGNED
Also note that adding a new search engine (eg. "youtube") doesn't enable the "Restore defaults" button.
(Assignee)

Comment 5

2 months ago
(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.
Flags: needinfo?(florian)
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.
Attachment #8829920 - Flags: review?(standard8) → review+
(Assignee)

Comment 7

2 months ago
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

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c79f0867b4ec
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox54: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
(Assignee)

Comment 9

2 months ago
(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.
(Assignee)

Updated

2 months ago
status-firefox51: affected → wontfix
OS: Unspecified → All
Hardware: Unspecified → All
Version: Trunk → 46 Branch
(Assignee)

Comment 10

2 months ago
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
Attachment #8829920 - Flags: approval-mozilla-beta?
Attachment #8829920 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

2 months ago
Flags: qe-verify+
Comment on attachment 8829920 [details] [diff] [review]
Patch

looks safe enough, let's get this in aurora53 and beta52
Attachment #8829920 - Flags: approval-mozilla-beta?
Attachment #8829920 - Flags: approval-mozilla-beta+
Attachment #8829920 - Flags: approval-mozilla-aurora?
Attachment #8829920 - Flags: approval-mozilla-aurora+

Comment 12

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0e1d4619f4c6
status-firefox53: affected → fixed

Comment 13

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/aa531b366492
status-firefox52: affected → fixed

Comment 14

2 months ago
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]
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified

Updated

2 months ago
status-firefox51: wontfix → fixed

Comment 15

2 months ago
[testday-20170203]
The bug is resolved and hence verified. 
OS: windows 10.0
Browser: Firefox Beta 51.0
Verified fixed FX 52b3, 53.0a2 (2017-02-05) Win 7.
status-firefox51: fixed → wontfix
status-firefox52: fixed → verified
status-firefox53: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.