640427 - Improve "paste & search" code

Status: RESOLVED FIXED

(SeaMonkey :: Search, defect)

Description

[Robert Kaiser]

Bug 401417 comment #32 by Neil:
>+        setTimeout(function (a) { a.init(); }, 0, this);
[I wonder why this is done on a timeout. Anyway, we can now use .bind(this)]

Bug 401417 comment #42 by Ratty:
> Maybe Ratty can address those in a follow-up patch.
Actually my plan is to move the current cut'n'paste code (and the searchbar
paste'n'go code) into a toolkit binding so that we can share this between
Firefox and SeaMonkey - and Thunderbird as well. I think Messaging Labs now has
a openSearch bar widget for Thunderbird that will open search results in a
content tab.



I don't know where this should go, but it should be filed, so putting it here for now.

Comment 1

[Philip Chee]

Attachment: Patch v1.0 refactor.

>          doCommand: function (aCommand) {
>            switch (aCommand) {
> +            case "cmd_pasteAndSearch":
> +              this.select();
> +              goDoCommand("cmd_paste");
> +              //this.fireEvent("textentered"); XXXRatty Doesn't work.
I don't know why fireEvent doesn't work here.
> +              this.onTextEntered();

> +      <handler event="keypress" keycode="VK_F4" phase="capturing">
> +      <![CDATA[
> +        if (/Mac/.test(navigator.platform))
> +          return this.openSearch();
> +      ]]>
> +      </handler>

What do I return if !Mac ?

Comment 2

[[e-mail]]

Comment on attachment 519106 [details] [diff] [review]
Patch v1.0 refactor.

>+.searchbar-textbox > .autocomplete-textbox-container > .textbox-input-box {
>+  -moz-binding: url("chrome://communicator/content/search/search.xml#input-box-search");
>+}
This belongs in searchbarBindings.css (and then .textbox-input-box suffices)

>+          this.selectEngine(event, (event.detail > 0));
[Hmm, I thought I'd already asked for^the ()s to be removed...]

>-            this._suggestMenuItem.setAttribute("checked", this._suggestEnabled);
You've moved the major user of the preference into a different binding. However this reduces the usefulness of the observer. Two observers would be overkill and reading one binding from the other would seem to be overly hacky so perhaps the best thing to do is to drop the observer completely.

>+              var param = this.getAttribute("autocompletesearchparam");
>+              return this._formHistSvc.nameExists(param);
Could use this.searchParam instead.

>+              //this.fireEvent("textentered"); XXXRatty Doesn't work.
That's because the search bar overrides onTextEntered to call itself directly.

>+      <handler event="keypress" keycode="VK_F4" phase="capturing">
>+      <![CDATA[
[Strange mix of styles in the file.] Most popular in other files seems to be either a) avoid CDATA if the body is small and doesn't needed b) put the <![CDATA[ on the same line and close with ]]></handler>

>+        if (/Mac/.test(navigator.platform))
!Mac, or return true if Mac, or you could even use ||

>+                     oncommand="var cmd = event.originalTarget.getAttribute('cmd'); if(cmd) { this.parentNode.doCommand(cmd); event.stopPropagation(); }">
Nit: space between if and (

>+            var prefs = Components.classes["@mozilla.org/preferences-service;1"]
>+                                  .getService(Components.interfaces.nsIPrefBranch2)
[Doesn't need to be 2 for this]

Comment 3

[Philip Chee]

Attachment: Patch v1.1 Use xbl:inherits to simplify code.

> >+.searchbar-textbox > .autocomplete-textbox-container > .textbox-input-box {
> >+  -moz-binding: url("chrome://communicator/content/search/search.xml#input-box-search");
> >+}
> This belongs in searchbarBindings.css (and then .textbox-input-box suffices)
Moved.

> >+          this.selectEngine(event, (event.detail > 0));
> [Hmm, I thought I'd already asked for^the ()s to be removed...]
Removed.

> >-            this._suggestMenuItem.setAttribute("checked", this._suggestEnabled);
> You've moved the major user of the preference into a different binding. However
> this reduces the usefulness of the observer. Two observers would be overkill
> and reading one binding from the other would seem to be overly hacky so perhaps
> the best thing to do is to drop the observer completely.
New approach using one observer and xbl:inherits.

> >+              var param = this.getAttribute("autocompletesearchparam");
> >+              return this._formHistSvc.nameExists(param);
> Could use this.searchParam instead.
Fixed.

> >+      <handler event="keypress" keycode="VK_F4" phase="capturing">
> >+      <![CDATA[
> [Strange mix of styles in the file.] Most popular in other files seems to be
> either a) avoid CDATA if the body is small and doesn't needed b) put the
> <![CDATA[ on the same line and close with ]]></handler>
I put the CDATA on the same line as I can never remember when a CDATA isn't needed.

> >+        if (/Mac/.test(navigator.platform))
> !Mac, or return true if Mac, or you could even use ||
Fixed.

> >+                     oncommand="var cmd = event.originalTarget.getAttribute('cmd'); if(cmd) { this.parentNode.doCommand(cmd); event.stopPropagation(); }">
> Nit: space between if and (
Fixed.

> >+            var prefs = Components.classes["@mozilla.org/preferences-service;1"]
> >+                                  .getService(Components.interfaces.nsIPrefBranch2)
> [Doesn't need to be 2 for this]
This part of the patch is gone.

Comment 4

[[e-mail]]

Comment on attachment 520124 [details] [diff] [review]
Patch v1.1 Use xbl:inherits to simplify code.

>+        return this.openSearch() || /Mac/.test(navigator.platform);
Wrong way around, need to test for /Mac/ first. r=me with that fixed.

Comment 5

[Philip Chee]

Attachment: [check-in comment 5] Patch v1.1a as pushed. r=Neil

> >+        return this.openSearch() || /Mac/.test(navigator.platform);
> Wrong way around, need to test for /Mac/ first. r=me with that fixed.
Fixed.

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/f7715f57e4d3