keyword.URL reset prompt doesn't properly re-set prefs if default engine is modified

RESOLVED FIXED in Firefox 14

Status

()

Firefox
General
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Gavin, Assigned: Gavin)

Tracking

Trunk
Firefox 14
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

The keyword.URL reset prompt implemented in bug 718088 offers to re-set the search engine to the build's default search engine. However if the build's default engine has been modified, we don't undo that change when the user opts in to the re-set. That means that there are cases where we offer to reset to Google, but when you click the button, you don't actually get Google. We can fix this by also resetting the defaultenginename pref.
Created attachment 607006 [details] [diff] [review]
patch

also added a basic test for the prompting functionality
Assignee: nobody → gavin.sharp
Status: NEW → ASSIGNED
Attachment #607006 - Flags: review?(fryn)
Blocks: 718088

Comment 2

6 years ago
Comment on attachment 607006 [details] [diff] [review]
patch

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

Callbacks and executeSoons abound, but it looks correct to me. :)
Attachment #607006 - Flags: review?(fryn) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/be7eba63e2ad
Flags: in-testsuite+
Target Milestone: --- → Firefox 14
https://hg.mozilla.org/mozilla-central/rev/be7eba63e2ad
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
The test in this checkin actually launches a real search via

https://www.google.com/othersearchfoo+bar

I'm pretty sure you don't want to be doing that out of mochitest-other.

otoh I think it may have just found a bug for me in an uncommited patch set. Thanks :)!
(In reply to Patrick McManus [:mcmanus] from comment #5)
> The test in this checkin actually launches a real search via
> 
> https://www.google.com/othersearchfoo+bar
> 

(it generally somehow gets canceled btw (page nav'd away maybe?) - which is part of the fun of my bug... but that's going to be a timing thing, it could certainly go out to the network)
(In reply to Patrick McManus [:mcmanus] from comment #5)
> The test in this checkin actually launches a real search via
> 
> https://www.google.com/othersearchfoo+bar
> 
> I'm pretty sure you don't want to be doing that out of mochitest-other.

Why is it a problem, out of curiosity? I suppose there is a potential for side-effects in other tests, but this test doesn't depend on the load succeeding in any way.

(I'll add a workaround to avoid triggering the load - I just didn't bother because I didn't think it would be a problem.)
(In reply to Gavin Sharp (use gavin@gavinsharp.com for email) from comment #7)

> Why is it a problem, out of curiosity?

I think the risk is that you could jam up some queues (e.g. the dns queue) if there was no WAN for the test and that would interfere with resolving local names like example.org .. but mostly I thought the policy was no WAN activity in the testbase, it really doesn't matter to me though.. mostly I was surprised to see it so noted it here.

if you're fine with it then I probably should just adjust my expectation. been there before :)
No, you're right that it's best to avoid in general. I just thought I might be able to get away with it in this case - the kind of dangerous attitude that leads to random orange! :)
FWIW, I backed the test out in bug 738804.
You need to log in before you can comment on or make changes to this bug.