The default bug view has changed. See this FAQ.

Generalize use of Services.jsm in nsSearchSuggestions

RESOLVED FIXED in Firefox 16

Status

()

Firefox
Search
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: Ekanan Ketunuti)

Tracking

unspecified
Firefox 16
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=Yoric][lang=js])

Attachments

(1 attachment, 6 obsolete attachments)

As bug 726279, but for nsSearchSuggestions.js
(Assignee)

Comment 1

5 years ago
Created attachment 632583 [details] [diff] [review]
patch
Assignee: nobody → ananuti
Status: NEW → ASSIGNED
Attachment #632583 - Flags: review?(dteller)
Comment on attachment 632583 [details] [diff] [review]
patch

Review of attachment 632583 [details] [diff] [review]:
-----------------------------------------------------------------

Wow, that was a quick patch!
Looks good to me.
Attachment #632583 - Flags: review?(dteller) → review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Keywords: checkin-needed
Comment on attachment 632583 [details] [diff] [review]
patch

>diff --git a/toolkit/components/search/nsSearchSuggestions.js b/toolkit/components/search/nsSearchSuggestions.js

>   get _strings() {
>     if (!this.__strings) {
>-      var sbs = Cc["@mozilla.org/intl/stringbundle;1"].
>-                getService(Ci.nsIStringBundleService);
>-
>-      this.__strings = sbs.createBundle(SEARCH_BUNDLE);
>+      this.__strings = Services.strings.createBundle(SEARCH_BUNDLE);
>     }
>     return this.__strings;
>   },
>   __strings: null,

You can just get rid of this property entirely, and use Services.strings directly.

>   _loadSuggestPref: function SAC_loadSuggestPref() {

Similarly, you can just inline this code now that it's a single line.
(Assignee)

Comment 4

5 years ago
Created attachment 633062 [details] [diff] [review]
patch1

is it like this? hope i did it correctly.

feedback please.
Attachment #633062 - Flags: review?(gavin.sharp)
(Assignee)

Updated

5 years ago
Attachment #633062 - Flags: review?(dteller)
Comment on attachment 633062 [details] [diff] [review]
patch1

Review of attachment 633062 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/search/nsSearchSuggestions.js
@@ +44,3 @@
>    get _strings() {
> +    return Services.strings.createBundle(SEARCH_BUNDLE);
> +  },

I am not quite sure what Gavin meant, but I assume he wanted something along the lines of
|_strings: Services.strings.createBundle(SEARCH_BUNDLE),|

@@ +47,2 @@
>  
> +  _loadSuggestPref: Services.prefs.getBoolPref(BROWSER_SUGGEST_PREF),

No, this has the wrong semantics. What you should do is change method |observe| so that it sets |this._suggestEnabled = prefService.getBoolpref(BROWSER_SUGGEST_PREF)| upon receiving |NS_PREFBRANCH_PREFCHANGE_TOPIC_ID|.
Attachment #633062 - Flags: review?(dteller)
(Assignee)

Updated

5 years ago
Attachment #633062 - Flags: review?(gavin.sharp)
(Assignee)

Comment 6

5 years ago
Created attachment 633073 [details] [diff] [review]
patch2
Attachment #633062 - Attachment is obsolete: true
Attachment #633073 - Flags: review?(dteller)
(Assignee)

Updated

5 years ago
Attachment #633073 - Flags: review?(dteller)
(Assignee)

Comment 7

5 years ago
Created attachment 633076 [details] [diff] [review]
patch2.1
Attachment #633073 - Attachment is obsolete: true
Attachment #633076 - Flags: review?(dteller)
Comment on attachment 633076 [details] [diff] [review]
patch2.1

Review of attachment 633076 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, with the small stuff below. Now, you should check if this is what Gavin wanted :)

::: toolkit/components/search/nsSearchSuggestions.js
@@ +36,5 @@
>  }
>  SuggestAutoComplete.prototype = {
>  
>    _init: function() {
>      this._addObservers();

My bad, I had missed this one: you should also inline 

|this._suggestEnabled = Services.prefs.getBoolPref(BROWSER_SUGGEST_PREF);|

here.

@@ +45,1 @@
>    _suggestEnabled: null,

I would prefer if you could leave the line of documentation you have removed.
Attachment #633076 - Flags: review?(gavin.sharp)
Attachment #633076 - Flags: review?(dteller)
Attachment #633076 - Flags: review+
(Assignee)

Comment 9

5 years ago
Created attachment 633081 [details] [diff] [review]
patch2.2

address comment 8
Attachment #633081 - Flags: review?(dteller)
Comment on attachment 633081 [details] [diff] [review]
patch2.2

Review of attachment 633081 [details] [diff] [review]:
-----------------------------------------------------------------

Fine with me, thank you very much. Now, let's wait for Gavin.
Attachment #633081 - Flags: review?(gavin.sharp)
Attachment #633081 - Flags: review?(dteller)
Attachment #633081 - Flags: review+
Attachment #633076 - Attachment is obsolete: true
Attachment #633076 - Flags: review?(gavin.sharp)
Comment on attachment 633081 [details] [diff] [review]
patch2.2

>diff --git a/toolkit/components/search/nsSearchSuggestions.js b/toolkit/components/search/nsSearchSuggestions.js

>-  get _strings() {
>-    if (!this.__strings) {
>-      var sbs = Cc["@mozilla.org/intl/stringbundle;1"].
>-                getService(Ci.nsIStringBundleService);
>-
>-      this.__strings = sbs.createBundle(SEARCH_BUNDLE);
>-    }
>-    return this.__strings;
>-  },
>-  __strings: null,
>+  _strings: Services.strings.createBundle(SEARCH_BUNDLE),

Sorry, I think I was confused with my previous suggestion. Let's make this:

get _suggestionLabel() {
  delete this._suggestionLabel;
  let bundle = Services.strings.createBundle("chrome://global/locale/search/search.properties");
  return this._suggestionLabel = bundle.GetStringFromName("suggestion_label");
}

and then get rid of SEARCH_BUNDLE and replace this._strings.GetStringFromName("suggestion_label") with just this._suggestionLabel, since it's the only use of _strings.
Attachment #633081 - Flags: review?(gavin.sharp) → review-
(Assignee)

Comment 12

5 years ago
Created attachment 633413 [details] [diff] [review]
patch3.0
Attachment #632583 - Attachment is obsolete: true
Attachment #633081 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #633413 - Flags: review?(gavin.sharp)
(Assignee)

Updated

5 years ago
Attachment #633413 - Flags: review?(dteller)
Attachment #633413 - Flags: review?(dteller) → review+
Comment on attachment 633413 [details] [diff] [review]
patch3.0

>diff --git a/toolkit/components/search/nsSearchSuggestions.js b/toolkit/components/search/nsSearchSuggestions.js

>   /**
>-   * this._strings is the string bundle for message internationalization.
>+   * this._suggestionLabel is the string bundle for message internationalization.
>    */

Just remove this comment, it's not really useful.

r=me with that change, and assuming that you've tested all of the relevant search suggestion functionality
Attachment #633413 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 14

5 years ago
Created attachment 633765 [details] [diff] [review]
patch for checkin

thank you so much, yoric & gavin. :)
Attachment #633413 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [mentor=Yoric][lang=js] → [autoland-try:633765: -b do -p win32 -u all -t none][mentor=Yoric][lang=js]
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
Whiteboard: [autoland-try:633765: -b do -p win32 -u all -t none][mentor=Yoric][lang=js] → [mentor=Yoric][lang=js]
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc8de9d7fc2
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ccc8de9d7fc2
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
You need to log in before you can comment on or make changes to this bug.