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

RESOLVED FIXED in Firefox 10

Status

Firefox Graveyard
Panorama
RESOLVED FIXED
6 years ago
a year ago

People

(Reporter: raymondlee, Assigned: raymondlee)

Tracking

Trunk
Firefox 10

Details

(Whiteboard: [fixed-in-fx-team])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
There are some loose global methods e.g isSearchEnabled(), ensureSearchShown(), performSearch().  It would be nice to encapsulate them into an object.
(Assignee)

Updated

6 years ago
Assignee: nobody → raymond
Status: NEW → ASSIGNED
(Assignee)

Comment 1

6 years ago
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
Attachment #563324 - Flags: review?(ttaubert)
(Assignee)

Comment 2

6 years ago
Comment on attachment 563324 [details] [diff] [review]
v1

Passed Try
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?
Attachment #563324 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 4

6 years ago
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
Attachment #563324 - Attachment is obsolete: true
(Assignee)

Comment 5

6 years ago
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
(Assignee)

Comment 6

6 years ago
(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.
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/def30095d7be
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/def30095d7be
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 10
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.