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

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Attachment: patch

also added a basic test for the prompting functionality

Comment 2

[Frank Yan (:fryn)]

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. :)

Comment 3

[:Gavin Sharp [email: [e-mail]]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/be7eba63e2ad

Comment 4

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/be7eba63e2ad

Comment 5

[Patrick McManus [:mcmanus]]

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 :)!

Comment 6

[Patrick McManus [:mcmanus]]

(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)

Comment 7

[:Gavin Sharp [email: [e-mail]]]

(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.)

Comment 8

[Patrick McManus [:mcmanus]]

(In reply to Gavin Sharp (use [e-mail] 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 :)

Comment 9

[:Gavin Sharp [email: [e-mail]]]

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! :)

Comment 10

[:Gavin Sharp [email: [e-mail]]]

FWIW, I backed the test out in bug 738804.