300561 - default search engine pref breaks after rebranding

Status: RESOLVED INVALID

(SeaMonkey :: Location Bar, defect)

Description

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

user_pref("browser.search.defaultengine",
"engine:///home/bzbarsky/mozilla/nightly/mozilla/searchplugins/google.src");

This pref should not store the absolute path to the plugin, since the program
directory changed when we rebranded (and can change in other situations).  We
need to 1) set the pref to something safer and 2) make the code handle an
invalid path better.

Comment 1

[Andrew Schultz]

Attachment: handle invalid search engine (checked in)

This fixes the problem at hand.  Calling ensureDefaultEnginePrefs will nicely
recursively call updateEngines, so I made it bail, which seems to work.  If the
pref is already set to the default (pref doesn't change), it won't call
updateEngines, so this shouldn't create an infinite loop.

Comment 2

[[e-mail]]

Comment on attachment 189146 [details] [diff] [review]
handle invalid search engine (checked in)

Looks good as a workaround.

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Comment on attachment 189146 [details] [diff] [review]
handle invalid search engine (checked in)

Checked in.

Do we want to actually set a better value for the pref in this bug, or another
one?

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

No longer blocking 1.0a, since the checked in patch mitigates the major part of
the problem.

Comment 5

[Andrew Schultz]

Attachment: store pref as engine name

The only thing that makes me sadder than seeing the sidebar pop open is reading Search code.  :)

This patch makes us use the same prefs as firefox.  browser.search.defaultengine (the engine URI) is dropped in favor of browser.search.selectedEngine (the engine name).

Comment 6

[Andrew Schultz]

*** Bug 187293 has been marked as a duplicate of this bug. ***

Comment 7

[Karsten Düsterloh]

Comment on attachment 208464 [details] [diff] [review]
store pref as engine name

>Index: browser/resources/content/navigator.js
>===================================================================

This file is pretty much a nightmare with regard to coding conventions. :-(
We should follow our global conventions as far as possible without restructuring the entire file now, so
- begin function names with Uppercase letters
- use spaces after comma in function signatures and calls
- but keep the (imo ugly) curly-brace-at-the-end-of-line style


>+function getEngineByName(aName,aRDF,aDS)
>+function getSearchEnginePref()

Style nit, see intro.

>+  var rdf = Components.classes["@mozilla.org/rdf/rdf-service;1"]
>+              .getService(Components.interfaces.nsIRDFService);

Style nit: vertically align the dots before classes and getService.

>   try {
>+    var ds = rdf.GetDataSource("rdf:internetsearch");
>+  } catch (ex) {
>+    return null;
>   }
>+  if (!ds)
>+    return null;
>+  
>+  var selectedEngineName;
>+  try {

While JS actually doesn't care, please move the declaration of ds out of the try, like you did for selectedEngineName. 
You should probably init both to null/"" to show they're not undefined (but JS doesn't care).

>+    selectedEngineName = pref.
>+            getComplexValue("browser.search.selectedEngine",
>+                            Components.interfaces.nsIPrefLocalizedString).data;
>+  }
>+  catch (ex) { }
>+
>+  if (selectedEngineName) {
>+    var selectedEngineRes = getEngineByName(selectedEngineName,rdf,ds);
>+    if (selectedEngineRes)
>+      return selectedEngineRes;
>+  }
>+
>+  try {
>+    selectedEngineName = pref.
>+            getComplexValue("browser.search.defaultenginename",
>+                            Components.interfaces.nsIPrefLocalizedString).data;
>+  }
>+  catch (ex) { }
>+
>+  return getEngineByName(selectedEngineName,rdf,ds);

You're using getComplexValue three times in this file, so you should use a

  function GetComplexPrefValue(aPref)
  {
    var value = null;
    try {
      value = pref.getComplexValue(aPref, Components.interfaces.nsIPrefLocalizedString).data;
    } catch (ex) {}
    return value;
  }

to simplify the above to

  var selectedEngineName = GetComplexPrefValue("browser.search.selectedEngine") ||
                           GetComplexPrefValue("browser.search.defaultenginename");
  return getEngineByName(selectedEngineName, rdf, ds);

That said, "browser.search.defaultenginename" isn't /written/ anywhere, it's just set by default or users hacking their config, so the net effect for older profiles is actually losing their default search engine setting until they set it again - we probably should fallback to "browser.search.defaultengine" somewhere (once?).  
  
>+function getSearchUrl(attr,engineRes)

Style nit, see intro.

> {
>   var rdf = Components.classes["@mozilla.org/rdf/rdf-service;1"].getService(Components.interfaces.nsIRDFService); 
>   var ds = rdf.GetDataSource("rdf:internetsearch"); 
>-  var kNC_Root = rdf.GetResource("NC:SearchEngineRoot");
>-  var mPrefs = Components.classes["@mozilla.org/preferences-service;1"].getService(Components.interfaces.nsIPrefBranch);
>-  var defaultEngine = mPrefs.getCharPref("browser.search.defaultengine");
>-  var engineRes = rdf.GetResource(defaultEngine);
>   var prop = "http://home.netscape.com/NC-rdf#" + attr;
>   var kNC_attr = rdf.GetResource(prop);
>   var searchURL = readRDFString(ds, engineRes, kNC_attr);
>   return searchURL;
> }

In getSearchEnginePref above, you're doing several checks on ds - here you don't...
Why?

> function OpenSearch(tabName, searchStr, newWindowFlag)
...
>     defaultSearchURL = pref.getComplexValue("browser.search.defaulturl",
>                                             Components.interfaces.nsIPrefLocalizedString).data;

defaultSearchURL = GetComplexPrefValue("browser.search.defaulturl");

>+            var searchURL = getSearchUrl("actionButton",engineRes);

Style nit, see intro.

>+function BrowserSearchInternet(engineRes)
...
>+      var searchRoot = getSearchUrl("searchForm",engineRes);

Style nit, see intro.

>Index: browser/resources/content/urlbarBindings.xml
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/browser/resources/content/urlbarBindings.xml,v
>retrieving revision 1.34
>diff -u -1 -0 -r1.34 urlbarBindings.xml
>--- browser/resources/content/urlbarBindings.xml	13 Dec 2005 07:41:04 -0000	1.34
>+++ browser/resources/content/urlbarBindings.xml	14 Jan 2006 08:17:45 -0000
>@@ -203,84 +203,80 @@
>       <method name="updateEngines">

Most of the factoring out and style nits of navigator.js are valid here almost verbatim...
(But no uppercase method names, of course.)

>       </method>
> 
>-      <method name="ensureDefaultEnginePrefs">
>+      <method name="getEngineByName">
...
>       </method>

Do you see a (useful) way to avoid the duplication of the above navigator.js code?
Or rather: make navigator.js use the urlbarBindings.xml functions (if possible)?

>Index: components/search/resources/search-panel.js
>===================================================================
>-  var defaultEngine = nsPreferences.
>-    copyUnicharPref("browser.search.defaultengine");
>+  var defaultEngineURI = getSearchEnginePref().Value;

GetSearchEnginePref() could be null...

>+function getEngineByName(aName,aRDF,aDS)
>+function getSearchEnginePref()

That's the second time these functions are defined (->navigator.js), not counting the duplicated code parts in urlbarBindings.xml. We should really coalesce them...

>   try {
>     if (!engineURIs || engineURIs.length <= 1) {
>       // not called from sidebar or only one engine selected
>+      var searchEngineURI = null;

Why null if it is supposed to be a string?

>+      if (!searchEngineURI) {
>+        searchEngineURI = getSearchEnginePref().Value;

GetSearchEnginePref() could be null...


For testing, I set my default search engine to Ask Jeeve before applying the patch (=> browser.search.defaultengine pointed to AJ, browser.search.defaultenginename still said Google). After applying the patch, my default search engine fell back to Google (see comment above on that), but the pref panel (Navigator->Internet Search) still claimed AJ, because it still operates on the old pref...


Finally, two more sidenotes:
- please use -pu8 for diffs
- it'd be a good idea if the patch's filename would include the bug number ;-)

Comment 8

[Robert Kaiser]

ajschult, this is still ASSIGNED to you, any interest on still working on this bug?

Comment 9

[Andrew Schultz]

Unfortunately, no
==> default

Comment 10

[Philip Chee]

INVALID/WORKSFORME now that we are using the Firefox OpenSearch engine.