Last Comment Bug 689878 - Refactor the code in search.js as there are couple of loose global methods
: Refactor the code in search.js as there are couple of loose global methods
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox Graveyard
Classification: Graveyard
Component: Panorama (show other bugs)
: Trunk
: All All
: -- normal
: Firefox 10
Assigned To: Raymond Lee [:raymondlee]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-28 01:46 PDT by Raymond Lee [:raymondlee]
Modified: 2016-04-12 14:00 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
v1 (47.56 KB, patch)
2011-09-29 00:22 PDT, Raymond Lee [:raymondlee]
ttaubert: review+
Details | Diff | Splinter Review
Patch for checkin (47.12 KB, patch)
2011-09-29 21:32 PDT, Raymond Lee [:raymondlee]
no flags Details | Diff | Splinter Review

Description Raymond Lee [:raymondlee] 2011-09-28 01:46:00 PDT
There are some loose global methods e.g isSearchEnabled(), ensureSearchShown(), performSearch().  It would be nice to encapsulate them into an object.
Comment 1 Raymond Lee [:raymondlee] 2011-09-29 00:22:18 PDT
Created attachment 563324 [details] [diff] [review]
v1

No functions change, just refactor the search code. 

Pushed to try
https://tbpl.mozilla.org/?tree=Try&rev=66dc0d227227
Comment 2 Raymond Lee [:raymondlee] 2011-09-29 10:07:50 PDT
Comment on attachment 563324 [details] [diff] [review]
v1

Passed Try
Comment 3 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-09-29 14:50:40 PDT
Comment on attachment 563324 [details] [diff] [review]
v1

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

Thanks, nice refactoring!

r=me with those few changes.

::: browser/base/content/tabview/search.js
@@ +99,5 @@
>    // Function: focus
>    // Given a <TabItem> or a <xul:tab>, focuses it and it's window.
>    focus: function TabUtils_focus(tab) {
>      // Convert a <TabItem> to a <xul:tab>
> +    if (tab.tab != undefined)

Better: if ("tab" in tab).

@@ +528,5 @@
> +    }
> +    iQ(window).keydown(this._currentHandler);
> +  },
> +
> +  createSearchTabMacher: function Search_createSearchTabMacher() {

There is a "t" missing.

@@ +542,5 @@
> +
> +  // ----------
> +  // Function: hideSearch
> +  // Hides search mode.
> +  hideSearch: function Search_hideSearch(event) {

With "Search.init" I think that should be "Search.hide" only.

@@ +581,5 @@
> +
> +  // ----------
> +  // Function: performSearch
> +  // Performs a search.
> +  performSearch: function Search_performSearch() {

Same here, only "Search.perform".

@@ +598,5 @@
> +  // Function: ensureSearchShown
> +  // Ensures the search feature is displayed.  If not, display it.
> +  // Parameters:
> +  //  - a boolean indicates whether this is triggered by a keypress or not
> +  ensureSearchShown: function Search_ensureSearchShown(activatedByKeypress) {

Maybe "ensureShown" only?
Comment 4 Raymond Lee [:raymondlee] 2011-09-29 21:32:57 PDT
Created attachment 563652 [details] [diff] [review]
Patch for checkin

(In reply to Tim Taubert [:ttaubert] from comment #3)
> 
> Review of attachment 563324 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Thanks, nice refactoring!
> 
> r=me with those few changes.
> 
> ::: browser/base/content/tabview/search.js
> @@ +99,5 @@
> >    // Function: focus
> >    // Given a <TabItem> or a <xul:tab>, focuses it and it's window.
> >    focus: function TabUtils_focus(tab) {
> >      // Convert a <TabItem> to a <xul:tab>
> > +    if (tab.tab != undefined)
> 
> Better: if ("tab" in tab).

Fixed

> 
> @@ +528,5 @@
> > +    }
> > +    iQ(window).keydown(this._currentHandler);
> > +  },
> > +
> > +  createSearchTabMacher: function Search_createSearchTabMacher() {
> 
> There is a "t" missing.

Fixed

> 
> @@ +542,5 @@
> > +
> > +  // ----------
> > +  // Function: hideSearch
> > +  // Hides search mode.
> > +  hideSearch: function Search_hideSearch(event) {
> 
> With "Search.init" I think that should be "Search.hide" only.

Updated

> 
> @@ +581,5 @@
> > +
> > +  // ----------
> > +  // Function: performSearch
> > +  // Performs a search.
> > +  performSearch: function Search_performSearch() {
> 
> Same here, only "Search.perform".

Updated

> 
> @@ +598,5 @@
> > +  // Function: ensureSearchShown
> > +  // Ensures the search feature is displayed.  If not, display it.
> > +  // Parameters:
> > +  //  - a boolean indicates whether this is triggered by a keypress or not
> > +  ensureSearchShown: function Search_ensureSearchShown(activatedByKeypress) {
> 
> Maybe "ensureShown" only?

Yes, updated
Comment 5 Raymond Lee [:raymondlee] 2011-09-29 21:33:40 PDT
Comment on attachment 563652 [details] [diff] [review]
Patch for checkin

Pushed to try and waiting for results

https://tbpl.mozilla.org/?tree=Try&rev=d975b17ceb72
Comment 6 Raymond Lee [:raymondlee] 2011-09-30 11:26:20 PDT
(In reply to Raymond Lee [:raymondlee] from comment #5)
> Comment on attachment 563652 [details] [diff] [review] [diff] [details] [review]
> Patch for checkin
> 
> Pushed to try and waiting for results
> 
> https://tbpl.mozilla.org/?tree=Try&rev=d975b17ceb72

Passed Try.  There are intermittents but not related to our code.
Comment 7 Tim Taubert [:ttaubert] (on PTO, back Aug 29th) 2011-10-01 01:35:05 PDT
https://hg.mozilla.org/integration/fx-team/rev/def30095d7be
Comment 8 Rob Campbell [:rc] (:robcee) 2011-10-04 05:20:04 PDT
https://hg.mozilla.org/mozilla-central/rev/def30095d7be

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