Status

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: kairo, Assigned: philip.chee)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

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.
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Depends on: 599833
Posted patch Patch v1.0 refactor. (obsolete) — Splinter Review
>          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 ?
Attachment #519106 - Flags: review?(neil)
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]
> >+.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.
Attachment #519106 - Attachment is obsolete: true
Attachment #519106 - Flags: review?(neil)
Attachment #520124 - Flags: review?(neil)
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.
Attachment #520124 - Flags: review?(neil) → review+
> >+        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
Attachment #520200 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 643404
You need to log in before you can comment on or make changes to this bug.