[web-ui settings] Fixes and improvements for settings page searching.

[web-ui settings] Fixes and improvements for settings page searching.

 - Fixed a bug that prevented searching from the last page of the settings window.
 - Fixed appearance of the extensions page title.
 - Stop the extensions page from listening to NAV_ENTRY_COMMITTED, this event is no longer needed
   since the migration and was generating extension loads for every settings page change.
 - Prevent redundant searches from occurring when the user first starts typing into the search field.
 - Use custom 'searchChanged' events rather than key/mouse events for customizing extensions list
   appearance during a search.

BUG=99047
TEST=See reproduction steps in bug.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=107033

[csilv, 2011-10-19 23:03:21]

+jhawkins, +finnur for review

[Finnur, 2011-10-20 10:00:58]

LGTM, with caveat (see below).

http://codereview.chromium.org/8353022/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/extension_list.js (right):

http://codereview.chromium.org/8353022/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/extension_list.js:66:
this.searchChangedHandler_.bind(this));
Ooh, me like.

http://codereview.chromium.org/8353022/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/8353022/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:128: prevVisible =
page.visible;
For this change I don't have enough context to judge. Hoping other reviewer has
it. Same problem with search_page.js.

http://codereview.chromium.org/8353022/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/search_page.js (right):

http://codereview.chromium.org/8353022/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/search_page.js:399: // Cleanup the
recursion-prevension variable.
nit: typo: prevention, not prevension.

[csilv, 2011-10-20 17:49:13]

http://codereview.chromium.org/8353022/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/8353022/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:128: prevVisible =
page.visible;
On 2011/10/20 10:00:59, Finnur wrote:
> For this change I don't have enough context to judge. Hoping other reviewer
has
> it. Same problem with search_page.js.

This was the bug that was causing searches to fail if the last page was
selected.  It should only be setting prevVisible based on the target page.  Let
me see if I can make the code more readable by adjusting the variable name
and/or adding a comment.

http://codereview.chromium.org/8353022/diff/1/chrome/browser/resources/option...
File chrome/browser/resources/options/search_page.js (right):

http://codereview.chromium.org/8353022/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/search_page.js:399: // Cleanup the
recursion-prevension variable.
On 2011/10/20 10:00:59, Finnur wrote:
> nit: typo: prevention, not prevension.

Done.

[csilv, 2011-10-21 21:30:50]

ping jhawkins, when you have time.

[James Hawkins, 2011-10-21 23:17:30]

LGTM with nit.

http://codereview.chromium.org/8353022/diff/4001/chrome/browser/resources/opt...
File chrome/browser/resources/options/search_page.js (right):

http://codereview.chromium.org/8353022/diff/4001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_page.js:288: this.insideSetSearchText_ =
true;
Declare and document this variable.

[csilv, 2011-10-22 00:08:37]

http://codereview.chromium.org/8353022/diff/4001/chrome/browser/resources/opt...
File chrome/browser/resources/options/search_page.js (right):

http://codereview.chromium.org/8353022/diff/4001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_page.js:288: this.insideSetSearchText_ =
true;
On 2011/10/21 23:17:30, James Hawkins wrote:
> Declare and document this variable.

I'm a little unclear.  It is documented already on line 286.  I can declare it
outside of this function, but I'd prefer to document it locally since it's only
used by this function.

[James Hawkins, 2011-10-22 00:19:33]

http://codereview.chromium.org/8353022/diff/4001/chrome/browser/resources/opt...
File chrome/browser/resources/options/search_page.js (right):

http://codereview.chromium.org/8353022/diff/4001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_page.js:288: this.insideSetSearchText_ =
true;
On 2011/10/22 00:08:37, csilv wrote:
> On 2011/10/21 23:17:30, James Hawkins wrote:
> > Declare and document this variable.
> 
> I'm a little unclear.  It is documented already on line 286.  I can declare it
> outside of this function, but I'd prefer to document it locally since it's
only
> used by this function.

Perhaps, but it is technically visible to other methods.  I believe the best
readability for members of an object (even when only used in one method) is to
declare and  document them at the object level (and document it should only be
used in |setSearchText_|.

[csilv, 2011-10-22 01:58:15]

http://codereview.chromium.org/8353022/diff/4001/chrome/browser/resources/opt...
File chrome/browser/resources/options/search_page.js (right):

http://codereview.chromium.org/8353022/diff/4001/chrome/browser/resources/opt...
chrome/browser/resources/options/search_page.js:288: this.insideSetSearchText_ =
true;
On 2011/10/22 00:19:33, James Hawkins wrote:
> On 2011/10/22 00:08:37, csilv wrote:
> > On 2011/10/21 23:17:30, James Hawkins wrote:
> > > Declare and document this variable.
> > 
> > I'm a little unclear.  It is documented already on line 286.  I can declare
it
> > outside of this function, but I'd prefer to document it locally since it's
> only
> > used by this function.
> 
> Perhaps, but it is technically visible to other methods.  I believe the best
> readability for members of an object (even when only used in one method) is to
> declare and  document them at the object level (and document it should only be
> used in |setSearchText_|.

Done.

[James Hawkins, 2011-10-22 06:57:56]

LGTM, thanks!

[commit-bot: I haz the power, 2011-10-24 23:26:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csilv@chromium.org/8353022/12001

[commit-bot: I haz the power, 2011-10-25 01:03:21]

Change committed as 107033