Removing search engines from preferences disabled 'Restore defaults' button

VERIFIED FIXED in Firefox 52

Status

()

Firefox
Search
P1
normal
VERIFIED FIXED
6 months ago
5 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

6 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

6 months ago
No longer blocks: 1277113
(Reporter)

Updated

6 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

5 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

5 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

5 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

5 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

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

Comment 9

5 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

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

Comment 10

5 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

5 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

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

Comment 13

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

Comment 14

5 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

5 months ago
status-firefox51: wontfix → fixed

Comment 15

5 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.