Last Comment Bug 736878 - keyword.URL reset prompt doesn't properly re-set prefs if default engine is modified
: keyword.URL reset prompt doesn't properly re-set prefs if default engine is m...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: General (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Firefox 14
Assigned To: :Gavin Sharp [email: gavin@gavinsharp.com]
:
Mentors:
Depends on:
Blocks: 718088
  Show dependency treegraph
 
Reported: 2012-03-18 12:13 PDT by :Gavin Sharp [email: gavin@gavinsharp.com]
Modified: 2013-11-13 02:19 PST (History)
4 users (show)
gavin.sharp: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (6.38 KB, patch)
2012-03-18 14:13 PDT, :Gavin Sharp [email: gavin@gavinsharp.com]
fryn: review+
Details | Diff | Splinter Review

Description :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-18 12:13:09 PDT
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: gavin@gavinsharp.com] 2012-03-18 14:13:02 PDT
Created attachment 607006 [details] [diff] [review]
patch

also added a basic test for the prompting functionality
Comment 2 Frank Yan (:fryn) 2012-03-19 16:47:12 PDT
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: gavin@gavinsharp.com] 2012-03-19 18:04:31 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/be7eba63e2ad
Comment 4 Mounir Lamouri (:mounir) 2012-03-20 03:47:42 PDT
https://hg.mozilla.org/mozilla-central/rev/be7eba63e2ad
Comment 5 Patrick McManus [:mcmanus] PTO until Sep 6 2012-03-21 19:00:00 PDT
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] PTO until Sep 6 2012-03-21 19:58:14 PDT
(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: gavin@gavinsharp.com] 2012-03-21 22:22:34 PDT
(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] PTO until Sep 6 2012-03-21 22:33:27 PDT
(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 :)
Comment 9 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-03-21 22:44:37 PDT
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: gavin@gavinsharp.com] 2012-03-26 08:18:01 PDT
FWIW, I backed the test out in bug 738804.

Note You need to log in before you can comment on or make changes to this bug.