Last Comment Bug 764270 - Generalize use of Services.jsm in nsSearchSuggestions
: Generalize use of Services.jsm in nsSearchSuggestions
Status: RESOLVED FIXED
[mentor=Yoric][lang=js]
:
Product: Firefox
Classification: Client Software
Component: Search (show other bugs)
: unspecified
: All All
: -- enhancement (vote)
: Firefox 16
Assigned To: Ekanan Ketunuti
:
: Florian Quèze [:florian] [:flo]
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-06-12 22:56 PDT by David Teller [:Yoric] (please use "needinfo")
Modified: 2012-06-16 06:49 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (5.23 KB, patch)
2012-06-13 00:09 PDT, Ekanan Ketunuti
dteller: review+
Details | Diff | Splinter Review
patch1 (5.32 KB, patch)
2012-06-14 00:47 PDT, Ekanan Ketunuti
no flags Details | Diff | Splinter Review
patch2 (5.52 KB, patch)
2012-06-14 02:13 PDT, Ekanan Ketunuti
no flags Details | Diff | Splinter Review
patch2.1 (5.52 KB, patch)
2012-06-14 02:25 PDT, Ekanan Ketunuti
dteller: review+
Details | Diff | Splinter Review
patch2.2 (5.60 KB, patch)
2012-06-14 02:51 PDT, Ekanan Ketunuti
dteller: review+
gavin.sharp: review-
Details | Diff | Splinter Review
patch3.0 (7.15 KB, patch)
2012-06-15 00:02 PDT, Ekanan Ketunuti
gavin.sharp: review+
dteller: review+
Details | Diff | Splinter Review
patch for checkin (7.08 KB, patch)
2012-06-15 20:32 PDT, Ekanan Ketunuti
no flags Details | Diff | Splinter Review

Description David Teller [:Yoric] (please use "needinfo") 2012-06-12 22:56:17 PDT
As bug 726279, but for nsSearchSuggestions.js
Comment 1 Ekanan Ketunuti 2012-06-13 00:09:23 PDT
Created attachment 632583 [details] [diff] [review]
patch
Comment 2 David Teller [:Yoric] (please use "needinfo") 2012-06-13 00:17:34 PDT
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: gavin@gavinsharp.com] 2012-06-13 09:15:09 PDT
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 2012-06-14 00:47:08 PDT
Created attachment 633062 [details] [diff] [review]
patch1

is it like this? hope i did it correctly.

feedback please.
Comment 5 David Teller [:Yoric] (please use "needinfo") 2012-06-14 01:25:04 PDT
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 2012-06-14 02:13:00 PDT
Created attachment 633073 [details] [diff] [review]
patch2
Comment 7 Ekanan Ketunuti 2012-06-14 02:25:09 PDT
Created attachment 633076 [details] [diff] [review]
patch2.1
Comment 8 David Teller [:Yoric] (please use "needinfo") 2012-06-14 02:29:13 PDT
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 2012-06-14 02:51:14 PDT
Created attachment 633081 [details] [diff] [review]
patch2.2

address comment 8
Comment 10 David Teller [:Yoric] (please use "needinfo") 2012-06-14 02:56:27 PDT
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: gavin@gavinsharp.com] 2012-06-14 11:55:03 PDT
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 2012-06-15 00:02:55 PDT
Created attachment 633413 [details] [diff] [review]
patch3.0
Comment 13 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-06-15 11:04:00 PDT
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 2012-06-15 20:32:36 PDT
Created attachment 633765 [details] [diff] [review]
patch for checkin

thank you so much, yoric & gavin. :)
Comment 15 Geoff Lankow (:darktrojan) 2012-06-15 23:09:48 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccc8de9d7fc2
Comment 16 Ryan VanderMeulen [:RyanVM] 2012-06-16 06:49:50 PDT
https://hg.mozilla.org/mozilla-central/rev/ccc8de9d7fc2

Note You need to log in before you can comment on or make changes to this bug.