Add remove option to Search Engines Policy

VERIFIED FIXED in Firefox -esr60

Status

()

enhancement
VERIFIED FIXED
Last year
Last year

People

(Reporter: kanika16047, Assigned: kanika16047)

Tracking

Trunk
Firefox 62
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr60 verified, firefox62 verified)

Details

Attachments

(1 attachment)

No description provided.
Assignee: nobody → kanika16047
Assignee: kanika16047 → ksaini
Comment on attachment 8984848 [details]
Bug 1466484 - Added remove to SearchEngines Policy

https://reviewboard.mozilla.org/r/250650/#review257040

Great! Just two suggestions:

- The new block of code that handles the Remove parameter could be added before the Add parameter. That way, if someone misconfigures one search engine, they can specify it to be removed and re-added in one single step instead of two.

- (the other suggestion is in the code below)

With these changes done, this will be ready to land as soon as you add some testcases for the Remove param.

::: browser/components/enterprisepolicies/Policies.jsm:682
(Diff revision 1)
> +                engine = Services.search.getEngineByName(engineName);
> +                if (!engine) {
> +                  throw "No engine by that name could be found";
> +                }

it will probably be a common case where someone adds an engine to the Remove section, but they don't clean that up from the policies afterwards.. So the code will try to remove it again every time. Which is fine, but there's no need to log this as an error. So if the engine can't be find by name, you can just silently continue to the next item in the list.

Ah, and one thing to check: the built-in engines can't be removed. You should test that the code works fine in case someone specifies a built-in one (i.e., it won't remove it and it doesn't break anything else)
Attachment #8984848 - Flags: review?(felipc) → review+
Are Google, Wikipedia, Twitter, Bing etc all built-in engines? Because the user is allowed to remove them via about:preferences. 
Wouldn't we want the same for the policy? :)
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/cc12983789d7
Added remove to SearchEngines Policy r=Felipe
(In reply to Kanika Saini from comment #3)
> Are Google, Wikipedia, Twitter, Bing etc all built-in engines? Because the
> user is allowed to remove them via about:preferences. 
> Wouldn't we want the same for the policy? :)

So this was a bit confusing.. For the user, it looks like it was removed, but they are actually just hidden from the list (so that they can be restored).

I thought the the call to removeEngine would have problems with that, but I checked and it actually just do the right thing (see here [1]).

So everything was fine and I just pushed the patch.

[1] https://searchfox.org/mozilla-central/rev/6eea08365e7386a2b81c044e7cc8a3daa51d8754/toolkit/components/search/nsSearchService.js#3932-3937
https://hg.mozilla.org/mozilla-central/rev/cc12983789d7
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Comment on attachment 8984848 [details]
Bug 1466484 - Added remove to SearchEngines Policy

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: Policy Related
Fix Landed on Version: Firefox 62
Risk to taking this patch (and alternatives if risky): Low, policy only
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8984848 - Flags: approval-mozilla-esr60?
Comment on attachment 8984848 [details]
Bug 1466484 - Added remove to SearchEngines Policy

Policy engine fix needed on ESR60. Approved for ESR 60.2.
Attachment #8984848 - Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Flags: qe-verify+
Blocks: 1479870
This bug was covered by the overall testing efforts invested in the New Enterprise Policies feature.

Marking this as verified fixed using Firefox 62.0b16 (BuildId:20180809104529) and the ESR try build (provided in comment 10) on Windows 10 64bit and macOS 10.13.4
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.