The default bug view has changed. See this FAQ.

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.