Closed Bug 886021 Opened 11 years ago Closed 11 years ago

Rollup updates for SeaMonkey web search. Sync with Firefox.

Categories

(SeaMonkey :: Search, defect)

defect
Not set
normal

Tracking

(seamonkey2.22 fixed)

RESOLVED FIXED
seamonkey2.22
Tracking Status
seamonkey2.22 --- fixed

People

(Reporter: philip.chee, Assigned: philip.chee)

Details

Attachments

(1 file, 2 obsolete files)

Relevant Firefox bugs:
Bug 587780 - add "purpose" argument to getSubmission.
  [Not ported. Only consumer appears to be the Firefox Health Reporter which we don't have.]

Bug 697377 - Use Asynchronous FormHistory.jsm for content/search.xml.
  Bug 566746, use improved callback api for asyncronous form history methods.
Bug 738818 - part 3: Make changing currentEngine also change defaultEngine, including for about:home
Bug 875042 - Remove unnecessary setTimeout from search-textbox constructor.
  Bug 875297 - Move the contents of the initialize method into the constructor of the search-textbox binding.
Bug 408251 - Move updating of form history from handleSearchCommand to doSearch to make sure that mouse-triggered search bar queries end up recorded in form history.
Bug 493051 - Avoid having addEngine select the engine by default, by adding an optional callback to let callers observe the successful addition of the engine.
Bug 860560 make sure that defaultEngine and currentEngine stay in sync.
Attached patch Patch v1.0 consolidated changes. (obsolete) — Splinter Review
> -        prompt.alert(window, dtitle, eduplicate ? emsg : bmsg);
> +        Services.prompt.alert(window, dtitle, eduplicate ? emsg : bmsg);
Wonder why this never showed up in the error console.

> +++ b/suite/common/search/search-panel.xul
....
> +<?xml-stylesheet href="chrome://communicator/content/search/searchbarBindings.css"?>
Can't think of a better place to put this.

> +      <property name="currentEngine">
> +        <setter><![CDATA[
> +          var ss = this.searchService;
> +          ss.defaultEngine = ss.currentEngine = val;
> +          return val;
> +        ]]></setter>
Bug 738818 - part 3: Make changing currentEngine also change defaultEngine.

>        <method name="doSearch">
>          <parameter name="aData"/>
>          <parameter name="aWhere"/>
>          <body><![CDATA[
> +          // Save the current value in the form history.
Bug 697377 - Use Asynchronous FormHistory.jsm for content/search.xml.
Bug 408251 - Move updating of form history from handleSearchCommand to doSearch.

> +          // Select the installed engine if the installation succeeds
Bug 493051 - Avoid having addEngine select the engine by default, by adding an optional callback to let callers observe the successful addition of the engine.

> +          var installCallback = {
> +            // Fat arrow functions bind |this| lexically.
> +            onSuccess: engine => this.currentEngine = engine
Or perhaps:

var installCallback = {
  onSuccess: function(engine) {
    this.currentEngine = engine;
  }.bind(this)
};

> -        setTimeout(function(a) { a.initialize(); }, 0, this);
> +
> +        if (this._prefBranch.getBoolPref("browser.urlbar.clickSelectsAll"))
> +          this.setAttribute("clickSelectsAll", true);
Bug 875042 - Remove unnecessary setTimeout from search-textbox constructor.
Bug 875297 - Move the contents of the initialize method into the constructor of the search-textbox binding.

>              case "cmd_clearhistory":
> -              return this._formHistSvc.nameExists("searchbar-history");
>              case "cmd_togglesuggest":
>                return true;
Async form history. In Bug 697377 discussed using a blocking call but decided that this was counter productive.

> +  <binding id="sidebar-search-box"
> +      extends="chrome://global/content/bindings/autocomplete.xml#autocomplete">
Adds the enhanced context menu with paste & search, clear form history, and toggle suggestions.

> +      case "browser-search-engine-modified":
> +        if (data != "engine-default" && data != "engine-current") {
> +          break;
> +        }
> +        // Enforce that the search service's defaultEngine is always equal to
> +        // its currentEngine. The search service will notify us any time either
> +        // of them are changed (either by directly setting the relevant prefs,
> +        // i.e. if add-ons try to change this directly, or if the
> +        // nsIBrowserSearchService setters are called).
Bug 860560 make sure that defaultEngine and currentEngine stay in sync.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Attachment #766310 - Flags: review?(neil)
Comment on attachment 766310 [details] [diff] [review]
Patch v1.0 consolidated changes.

patch needs more baking.
Attachment #766310 - Flags: review?(neil)
Attached patch Patch v1.1 consolidated changes. (obsolete) — Splinter Review
Changes since the last patch:

> +  <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
needs goDoCommand() for the sidebar Paste and Search context menu.

> -                     onpopupshowing="if (document.commandDispatcher.focusedElement != this.parentNode.firstChild)
> -                                       this.parentNode.firstChild.focus();
> +                     onpopupshowing="var input =
> +                                       this.parentNode.getElementsByAttribute('anonid', 'input')[0];
> +                                     if (document.commandDispatcher.focusedElement != input)
> +                                       input.focus();
From Bug 869086 - Make textbox.xml resilient against the changes in bug 653881.

=========================
From Comment 1:

> -        prompt.alert(window, dtitle, eduplicate ? emsg : bmsg);
> +        Services.prompt.alert(window, dtitle, eduplicate ? emsg : bmsg);
Wonder why this never showed up in the error console.

> +++ b/suite/common/search/search-panel.xul
....
> +<?xml-stylesheet href="chrome://communicator/content/search/searchbarBindings.css"?>

Adds the enhanced context menu with paste & search, clear form history, and toggle suggestions.
Can't think of a better place to put this CSS.

> +      <property name="currentEngine">
> +        <setter><![CDATA[
> +          var ss = this.searchService;
> +          ss.defaultEngine = ss.currentEngine = val;
> +          return val;
> +        ]]></setter>
Bug 738818 - part 3: Make changing currentEngine also change defaultEngine.

>        <method name="doSearch">
>          <parameter name="aData"/>
>          <parameter name="aWhere"/>
>          <body><![CDATA[
> +          // Save the current value in the form history.
Bug 697377 - Use Asynchronous FormHistory.jsm for content/search.xml.
Bug 408251 - Move updating of form history from handleSearchCommand to doSearch.

> +          // Select the installed engine if the installation succeeds
Bug 493051 - Avoid having addEngine select the engine by default, by adding an optional callback to let callers observe the successful addition of the engine.

> +          var installCallback = {
> +            // Fat arrow functions bind |this| lexically.
> +            onSuccess: engine => this.currentEngine = engine
Or perhaps:

var installCallback = {
  onSuccess: function(engine) {
    this.currentEngine = engine;
  }.bind(this)
};

> -        setTimeout(function(a) { a.initialize(); }, 0, this);
> +
> +        if (this._prefBranch.getBoolPref("browser.urlbar.clickSelectsAll"))
> +          this.setAttribute("clickSelectsAll", true);
Bug 875042 - Remove unnecessary setTimeout from search-textbox constructor.
Bug 875297 - Move the contents of the initialize method into the constructor of the search-textbox binding.

>              case "cmd_clearhistory":
> -              return this._formHistSvc.nameExists("searchbar-history");
>              case "cmd_togglesuggest":
>                return true;
Async form history. In Bug 697377 discussed using a blocking call but decided that this was counter productive.

> +  <binding id="sidebar-search-box"
> +      extends="chrome://global/content/bindings/autocomplete.xml#autocomplete">
Adds the enhanced context menu with paste & search, clear form history, and toggle suggestions.

> +      case "browser-search-engine-modified":
> +        if (data != "engine-default" && data != "engine-current") {
> +          break;
> +        }
> +        // Enforce that the search service's defaultEngine is always equal to
> +        // its currentEngine. The search service will notify us any time either
> +        // of them are changed (either by directly setting the relevant prefs,
> +        // i.e. if add-ons try to change this directly, or if the
> +        // nsIBrowserSearchService setters are called).
Bug 860560 make sure that defaultEngine and currentEngine stay in sync.
Attachment #766310 - Attachment is obsolete: true
Attachment #768235 - Flags: review?(neil)
Comment on attachment 768235 [details] [diff] [review]
Patch v1.1 consolidated changes.

> <?xml-stylesheet href="chrome://communicator/skin/" type="text/css"?>
> <?xml-stylesheet href="chrome://communicator/skin/search/search.css" type="text/css"?>
>+<?xml-stylesheet href="chrome://communicator/content/search/searchbarBindings.css"?>
Nit: content CSS belongs before skin CSS

>   <script type="application/javascript" src="chrome://communicator/content/search/search-panel.js"/>
>   <script type="application/javascript" src="chrome://communicator/content/utilityOverlay.js"/>
>+  <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
Nit: global script belongs first

>+          // Select the installed engine if the installation succeeds
>+          var installCallback = {
>+            // Fat arrow functions bind |this| lexically.
>+            onSuccess: engine => this.currentEngine = engine
>+          };
>           this.searchService.addEngine(target.getAttribute("uri"), type,
>-                                       target.getAttribute("image"), false);
>+                                       target.getAttribute("image"), false,
>+                                       installCallback);
The other way to achieve this is to pass this as the callback and add nsISearchInstallCallback (compare nsIBrowserSearchInitObserver).

>+      <property name="FormHistory" readonly="true">
>+        <getter><![CDATA[
>+          if (!this._formHistory)
>+            this._formHistory =
>+              (Components.utils.import("resource://gre/modules/FormHistory.jsm", {})).FormHistory;
>+          return this._formHistory;
>+        ]]></getter>
>+      </property>
I'd rather just import FormHistory.jsm into this in the constructor.

>             case "cmd_clearhistory":
>-              return this._formHistSvc.nameExists("searchbar-history");
[Sigh.]

>+  <binding id="sidebar-search-box"
So what's this solving? It looks like it's duplicating a boatload of code, which is a bad idea.
>>+  <binding id="sidebar-search-box"
> So what's this solving? It looks like it's duplicating a boatload of code, which is a bad idea.

I'm adding the paste and search context menu to the sidebar search box.
When I tried to use the existing <binding id="searchbar-textbox"> I ran into several problems/errors:

1. Some js error about undefined.
>       <constructor><![CDATA[
>         if (document.getBindingParent(this).parentNode.parentNode.localName ==
>             "toolbarpaletteitem")
>           return;

2. I don't want to override onTextEntered in the sidebar search box because then things stop working.
>       <!-- override |onTextEntered| in autocomplete.xml -->
>       <method name="onTextEntered">

3. A whole bunch of keyboard handlers that don't work because we are not a child of the main window searchbar.
>       <handler event="keypress" keycode="VK_UP" modifiers="accel"
>                phase="capturing"
>                action="document.getBindingParent(this).selectEngine(event, false);"/>
And anyway the sidebar has a different UI to handle search engine selection.
(In reply to Philip Chee from comment #5)
> I'm adding the paste and search context menu to the sidebar search box.
> When I tried to use the existing <binding id="searchbar-textbox"> I ran into
> several problems/errors:
Most of these can be dealt with by splitting the existing binding into two (OK so I suck at naming) let's call them searchpanel-textbox and searchbar-textbox. searchbar-textbox extends searchpanel-textbox except that it also includes the onTextEntered override and the handlers.

> 1. Some js error about undefined.
> >       <constructor><![CDATA[
> >         if (document.getBindingParent(this).parentNode.parentNode.localName ==
> >             "toolbarpaletteitem")
> >           return;
So, this is trying to avoid constructing the searchbar-textbox during toolbar customisation. But, we do want to construct the searchpanel-textbox. I guess the easiest way to solve this is an extra null-check at the appropriate point.
Comment on attachment 768235 [details] [diff] [review]
Patch v1.1 consolidated changes.

r- based on previous comments.

> .searchbar-textbox {
>   -moz-binding: url("chrome://communicator/content/search/search.xml#searchbar-textbox");
> }
> 
> .textbox-input-box {
>   -moz-binding: url("chrome://communicator/content/search/search.xml#input-box-search");
> }
> 
>+.sidebar-search-box {
>+  -moz-binding: url("chrome://communicator/content/search/search.xml#sidebar-search-box");
>+}
(Nit: Under my scheme as mentioned previously this rule would belong before the searchbar-textbox.)
Attachment #768235 - Flags: review?(neil) → review-
>>+<?xml-stylesheet href="chrome://communicator/content/search/searchbarBindings.css"?>
> Nit: content CSS belongs before skin CSS
Fixed.

>>+  <script type="application/javascript" src="chrome://global/content/globalOverlay.js"/>
> Nit: global script belongs first
Fixed.

>>+                                       target.getAttribute("image"), false,
>>+                                       installCallback);
> The other way to achieve this is to pass this as the callback and add
> nsISearchInstallCallback (compare nsIBrowserSearchInitObserver).
Fixed.

> I'd rather just import FormHistory.jsm into this in the constructor.
Fixed.

> Most of these can be dealt with by splitting the existing binding into two (OK 
> so I suck at naming) let's call them searchpanel-textbox and searchbar-textbox. 
> searchbar-textbox extends searchpanel-textbox except that it also includes the 
> onTextEntered override and the handlers.
Now split into "search-textbox" (basic) and "searchbar-textbox" (extended).

> 1. Some js error about undefined.
>>       <constructor><![CDATA[
>>         if (document.getBindingParent(this).parentNode.parentNode.localName ==
>>             "toolbarpaletteitem")
>>           return;

> So, this is trying to avoid constructing the searchbar-textbox during toolbar 
> customisation. But, we do want to construct the searchpanel-textbox. I guess 
> the easiest way to solve this is an extra null-check at the appropriate point.
Fixed.

>>+.sidebar-search-box {
>>+  -moz-binding: url("chrome://communicator/content/search/search.xml#sidebar-search-box");
>>+}
> (Nit: Under my scheme as mentioned previously this rule would belong before the
> searchbar-textbox.)
Fixed.
Attachment #768235 - Attachment is obsolete: true
Attachment #769631 - Flags: review?(neil)
Comment on attachment 769631 [details] [diff] [review]
Patch v1.2 address review issues.

>+      <method name="onSuccess">
>+        <parameter name="aEngine"/>
>+        <body><![CDATA[
>+          this.currentEngine = engine;
aEngine, no? r=me with that fixed.

>-        this._prefBranch.getBoolPref("browser.search.suggest.enabled")
>+        this._prefBranch.getBoolPref("browser.search.suggest.enabled");
Nit: Although these are indeed technically expression statements, because we use them as expressions, I'd prefer them not to end with a semicolon.

>+.sidebar-search-box {
>+  -moz-binding: url("chrome://communicator/content/search/search.xml#search-textbox");
Nit: rename the class to search-textbox too please.
Attachment #769631 - Flags: review?(neil) → review+
>>+          this.currentEngine = engine;
> aEngine, no? r=me with that fixed.
Fixed.

>>-        this._prefBranch.getBoolPref("browser.search.suggest.enabled")
>>+        this._prefBranch.getBoolPref("browser.search.suggest.enabled");
> Nit: Although these are indeed technically expression statements, because we use them as expressions, I'd prefer them not to end with a semicolon.
Fixed.

>>+.sidebar-search-box {
>>+  -moz-binding: url("chrome://communicator/content/search/search.xml#search-textbox");
> Nit: rename the class to search-textbox too please.
Fixed.

Pushed to comm-central
https://hg.mozilla.org/comm-central/rev/d20b9bdc60b2
Target Milestone: --- → seamonkey2.22
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.