dom-ui settings: Implement search capability for top level panels....

dom-ui settings:
- Implement search capability for top level panels.
- Code cleanups for options_page.js

BUG=59267
TEST=Verify search field presents results based on user input.

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

[csilv, 2010-12-02 20:45:40]

+jhawkins for review.
New CL to replace the previous.  All search logic is now self-contained in the
search page.

[James Hawkins, 2010-12-02 23:11:44]

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

http://codereview.chromium.org/5538002/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:71: page.visible = name ==
pageName;
Can you just move this to the above for loop that has the same loop condition?

http://codereview.chromium.org/5538002/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:87: if (page.didShowPage)
Is there a way for a page to listen to class changes? This seems involved.  I'm
thinking of something like:

onClassChange = function(change) {
  if (change == remove hidden)
     I'm now visible.
  else if (change == add hidden)
     I'm now hidden.
}

I have no idea if there is a mechanism for this though.

http://codereview.chromium.org/5538002/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:135: OptionsPage.handleKeyDown_
= function(e) {
s/handleKeyDown_/onKeyDown_/

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

http://codereview.chromium.org/5538002/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/search_page.js:55: this.setSearchActive_(true);
It looks like setSearchActive_ is synonymous with the page being visible. Can we
just use `var active = amIVisible()`?

http://codereview.chromium.org/5538002/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/search_page.js:61: willHidePage: function() {
You're using willHidePage here but didShowPage above; do we want didHidePage
here, and if not, what's the diff?

http://codereview.chromium.org/5538002/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/search_page.js:126: if (!this.searchActive_)
When can setSearchText_() be called when search is not active?

[csilv, 2010-12-02 23:40:21]

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

http://codereview.chromium.org/5538002/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:71: page.visible = name ==
pageName;
On 2010/12/02 23:11:44, James Hawkins wrote:
> Can you just move this to the above for loop that has the same loop condition?

No.  The reason is that the 'willShowPage' and 'willHidePage' events need to be
sent before changes are made to the DOM.  If we combine the loops, the messages
will be sent in the middle of changes and in the case of SearchPage, this would
be a problem.

http://codereview.chromium.org/5538002/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:87: if (page.didShowPage)
On 2010/12/02 23:11:44, James Hawkins wrote:
> Is there a way for a page to listen to class changes? This seems involved. 
I'm
> thinking of something like:
> 
> onClassChange = function(change) {
>   if (change == remove hidden)
>      I'm now visible.
>   else if (change == add hidden)
>      I'm now hidden.
> }
> 
> I have no idea if there is a mechanism for this though.

Yes, there is a mechanism for this in cr, but that doesn't help me because I
really need to get event notifications before and after all pages have had their
DOM modified.

http://codereview.chromium.org/5538002/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:135: OptionsPage.handleKeyDown_
= function(e) {
On 2010/12/02 23:11:44, James Hawkins wrote:
> s/handleKeyDown_/onKeyDown_/

Done.

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

http://codereview.chromium.org/5538002/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/search_page.js:55: this.setSearchActive_(true);
On 2010/12/02 23:11:44, James Hawkins wrote:
> It looks like setSearchActive_ is synonymous with the page being visible. Can
we
> just use `var active = amIVisible()`?
> 

setSearchActive_() performs a bunch of dom manipulation as the search page goes
in/out of active state.  Does that answer your question...?

http://codereview.chromium.org/5538002/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/search_page.js:61: willHidePage: function() {
On 2010/12/02 23:11:44, James Hawkins wrote:
> You're using willHidePage here but didShowPage above; do we want didHidePage
> here, and if not, what's the diff?

The reason is that when search is made active, we want to get the notification
after the dom has been changed for all pages.  When search is made inactive, we
want that notification before the dom has changed for all pages.  This allows us
to apply search specific changes to the dom, and then to undo those changes at
the appropriate time.

http://codereview.chromium.org/5538002/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/search_page.js:126: if (!this.searchActive_)
On 2010/12/02 23:11:44, James Hawkins wrote:
> When can setSearchText_() be called when search is not active?

Perhaps never. I need to test whether an explicit set of the search field (ie:
$('searchField').value = '') will cause the onsearch handler to be invoked.  As
long as that handler is not called I can remove this condition.

[James Hawkins, 2010-12-03 00:02:09]

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

http://codereview.chromium.org/5538002/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/options_page.js:71: page.visible = name ==
pageName;
On 2010/12/02 23:40:21, csilv wrote:
> On 2010/12/02 23:11:44, James Hawkins wrote:
> > Can you just move this to the above for loop that has the same loop
condition?
> 
> No.  The reason is that the 'willShowPage' and 'willHidePage' events need to
be
> sent before changes are made to the DOM.  If we combine the loops, the
messages
> will be sent in the middle of changes and in the case of SearchPage, this
would
> be a problem.

Can you elucidate the problem?  I'm not understanding the need yet.

[csilv, 2010-12-03 00:03:30]

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

http://codereview.chromium.org/5538002/diff/1/chrome/browser/resources/option...
chrome/browser/resources/options/search_page.js:126: if (!this.searchActive_)
On 2010/12/02 23:40:21, csilv wrote:
> On 2010/12/02 23:11:44, James Hawkins wrote:
> > When can setSearchText_() be called when search is not active?
> 
> Perhaps never. I need to test whether an explicit set of the search field (ie:
> $('searchField').value = '') will cause the onsearch handler to be invoked. 
As
> long as that handler is not called I can remove this condition.

After testing, removed this check, it's not needed.

[csilv, 2010-12-03 00:24:19]

Applied changes based on conversation.  Please take a look.

[James Hawkins, 2010-12-03 21:36:38]

LGTM

http://codereview.chromium.org/5538002/diff/15001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_page.js (right):

http://codereview.chromium.org/5538002/diff/15001/chrome/browser/resources/op...
chrome/browser/resources/options/search_page.js:101: // sections (ie titles,
button strips).
nit: add a comment describing why we order this before setting visibility of the
page.

[csilv, 2010-12-03 22:44:07]

http://codereview.chromium.org/5538002/diff/15001/chrome/browser/resources/op...
File chrome/browser/resources/options/search_page.js (right):

http://codereview.chromium.org/5538002/diff/15001/chrome/browser/resources/op...
chrome/browser/resources/options/search_page.js:101: // sections (ie titles,
button strips).
On 2010/12/03 21:36:38, James Hawkins wrote:
> nit: add a comment describing why we order this before setting visibility of
the
> page.

Done.

[James Hawkins, 2010-12-06 18:59:59]

On 2010/12/03 22:44:07, csilv wrote:
>
http://codereview.chromium.org/5538002/diff/15001/chrome/browser/resources/op...
> File chrome/browser/resources/options/search_page.js (right):
> 
>
http://codereview.chromium.org/5538002/diff/15001/chrome/browser/resources/op...
> chrome/browser/resources/options/search_page.js:101: // sections (ie titles,
> button strips).
> On 2010/12/03 21:36:38, James Hawkins wrote:
> > nit: add a comment describing why we order this before setting visibility of
> the
> > page.
> 
> Done.

SLGTM