Improve "paste & search" code

RESOLVED FIXED

Status

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: kairo, Assigned: philip.chee)

Tracking

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
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)

Updated

8 years ago
Assignee: nobody → philip.chee
Status: NEW → ASSIGNED
Depends on: 599833
(Assignee)

Comment 1

8 years ago
Created attachment 519106 [details] [diff] [review]
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 ?
Attachment #519106 - Flags: review?(neil)

Comment 2

8 years ago
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]
(Assignee)

Comment 3

8 years ago
Created attachment 520124 [details] [diff] [review]
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.
Attachment #519106 - Attachment is obsolete: true
Attachment #519106 - Flags: review?(neil)
Attachment #520124 - Flags: review?(neil)

Comment 4

8 years ago
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+
(Assignee)

Comment 5

8 years ago
Created attachment 520200 [details] [diff] [review]
[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
Attachment #520200 - Flags: review+
(Assignee)

Updated

8 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Assignee)

Updated

8 years ago
Blocks: 643404
You need to log in before you can comment on or make changes to this bug.