689878 - Refactor the code in search.js as there are couple of loose global methods

Status: RESOLVED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Raymond Lee [:raymondlee]]

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

Attachment: 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]]

Comment on attachment 563324 [details] [diff] [review]
v1

Passed Try

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

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

Attachment: 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]]

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

(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] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/def30095d7be

Comment 8

[Rob Campbell [:rc] (:robcee)]

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