764270 - Generalize use of Services.jsm in nsSearchSuggestions

Status: RESOLVED FIXED

(Firefox :: Search, enhancement)

Description

[David Teller [:Yoric] - still alive but not very active]

As bug 726279, but for nsSearchSuggestions.js

Comment 1

[Ekanan Ketunuti]

Attachment: patch

Comment 2

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 3

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 4

[Ekanan Ketunuti]

Attachment: patch1

is it like this? hope i did it correctly.

feedback please.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

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|.

Comment 6

[Ekanan Ketunuti]

Attachment: patch2

Comment 7

[Ekanan Ketunuti]

Attachment: patch2.1

Comment 8

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 9

[Ekanan Ketunuti]

Attachment: patch2.2

address comment 8

Comment 10

[David Teller [:Yoric] - still alive but not very active]

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.

Comment 11

[:Gavin Sharp [email: [e-mail]]]

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.

Comment 12

[Ekanan Ketunuti]

Attachment: patch3.0

Comment 13

[:Gavin Sharp [email: [e-mail]]]

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

Comment 14

[Ekanan Ketunuti]

Attachment: patch for checkin

thank you so much, yoric & gavin. :)

Comment 15

[Geoff Lankow (:darktrojan)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc8de9d7fc2

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ccc8de9d7fc2