dom-ui settings: Enable searching for sub-pages...

dom-ui settings:
- enable searching for sub-pages and overlays.
- search-related performance improvements.

BUG=59267
TEST=Verify search find results in sub-pages.  Control(button) highlighting TBD.

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

[James Hawkins, 2010-12-23 01:16:29]

http://codereview.chromium.org/5992004/diff/8001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/5992004/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:248: * @param {OptionsPage}
page Page to register.
s/page/subPage/

http://codereview.chromium.org/5992004/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:249: * @param {OptionsPage}
parentPage Parent page to register.
'Parent page to register' ? Seems like 'Associated parent page' is more along
the correct lines.

http://codereview.chromium.org/5992004/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:250: * @param {Array}
associatedControls Array of control elements associated with
Hmm, from just reading this description, I have no idea what this third
parameter actually means/does.

[csilv, 2010-12-23 02:59:45]

http://codereview.chromium.org/5992004/diff/8001/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/5992004/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:248: * @param {OptionsPage}
page Page to register.
On 2010/12/23 01:16:29, James Hawkins wrote:
> s/page/subPage/

Done.

http://codereview.chromium.org/5992004/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:249: * @param {OptionsPage}
parentPage Parent page to register.
On 2010/12/23 01:16:29, James Hawkins wrote:
> 'Parent page to register' ? Seems like 'Associated parent page' is more along
> the correct lines.

Done.

http://codereview.chromium.org/5992004/diff/8001/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:250: * @param {Array}
associatedControls Array of control elements associated with
On 2010/12/23 01:16:29, James Hawkins wrote:
> Hmm, from just reading this description, I have no idea what this third
> parameter actually means/does.

Tweaked the wording, PTAL.

[James Hawkins, 2010-12-23 19:29:44]

+arv for good measure

http://codereview.chromium.org/5992004/diff/9004/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/5992004/diff/9004/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:240: * Fine an enclosing
section for an element if it exists.
s/Fine/Find/

http://codereview.chromium.org/5992004/diff/9004/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:255: * Registers a new Sub tab
page.
Can you remove tab here? We don't really refer to them as tabs anymore.

http://codereview.chromium.org/5992004/diff/9004/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:258: * @param {Array}
associatedControls Array of control elements that lead to
It really seems like we should expose an API to add associated controls when
they're created.  This seems very disconnected and likely to break when someone
adds a new sub-page.  What are your thoughts on that?

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

http://codereview.chromium.org/5992004/diff/9004/chrome/browser/resources/opt...
chrome/browser/resources/options/search_page.js:101: var page, key, length,
childDiv, i;
Why did you pull these out of scope?

[csilv, 2010-12-23 19:56:23]

http://codereview.chromium.org/5992004/diff/9004/chrome/browser/resources/opt...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/5992004/diff/9004/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:240: * Fine an enclosing
section for an element if it exists.
On 2010/12/23 19:29:44, James Hawkins wrote:
> s/Fine/Find/

Done.

http://codereview.chromium.org/5992004/diff/9004/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:255: * Registers a new Sub tab
page.
On 2010/12/23 19:29:44, James Hawkins wrote:
> Can you remove tab here? We don't really refer to them as tabs anymore.

Done.

http://codereview.chromium.org/5992004/diff/9004/chrome/browser/resources/opt...
chrome/browser/resources/options/options_page.js:258: * @param {Array}
associatedControls Array of control elements that lead to
On 2010/12/23 19:29:44, James Hawkins wrote:
> It really seems like we should expose an API to add associated controls when
> they're created.  This seems very disconnected and likely to break when
someone
> adds a new sub-page.  What are your thoughts on that?

I think associating the controls when the page is registered is the ideal
approach. Registration time (ie: options.js) is when the page structure is
defined, and thus a logical time to define what controls are associated with a
particular sub-page... just as we define the parentPage relationship for
sub-pages.

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

http://codereview.chromium.org/5992004/diff/9004/chrome/browser/resources/opt...
chrome/browser/resources/options/search_page.js:101: var page, key, length,
childDiv, i;
On 2010/12/23 19:29:44, James Hawkins wrote:
> Why did you pull these out of scope?

Attempts to re-use variables.  There's no benefit to moving key out of scope in
this method, but 'i' will benefit from (some) re-use.

[arv (Not doing code reviews), 2010-12-23 20:38:22]

LGTM

I have some suggestions but ignore them if you want. I'm leaving work early
today so I will not be able to take a second look until next week.

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/options.js (right):

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options.js:73: null);
can you make this param optional?

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:64: (rootPage &&
rootPage.sticky && targetPage.parentPage);
useless parentheses

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:246: while (node =
node.parentNode) {
node.parentElement since you don't care about non elements.

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:247: if (node.nodeType ==
document.ELEMENT_NODE &&
Node.Element_NODE

... if you use parentNode you can skip the nodeType check

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:248:
node.nodeName.toLowerCase() == 'section')
Use tagName?

I would skip the  toLowerCase and compare with 'SECTION' since tagName is always
upper case in HTML

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:259: *   this sub-page.  The
first item is typically a button in a root-level
indent +2

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:264: associatedControls) {
Can you make this optional?

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:283: associatedControls) {
here too

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:493: * @return {boolean} True
if this page is sticky.
I've been using @type for getters and setters since they look like properties
and not functions... I don't know the official stance on this

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

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/search_page.js:103: for (key in pagesToSearch)
{
I find it easier to read with var but it is up to you

[csilv, 2010-12-23 21:46:07]

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/options.js (right):

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options.js:73: null);
On 2010/12/23 20:38:22, arv wrote:
> can you make this param optional?

Done here and below.

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
File chrome/browser/resources/options/options_page.js (right):

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:64: (rootPage &&
rootPage.sticky && targetPage.parentPage);
On 2010/12/23 20:38:22, arv wrote:
> useless parentheses

Done.

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:246: while (node =
node.parentNode) {
On 2010/12/23 20:38:22, arv wrote:
> node.parentElement since you don't care about non elements.

parentElement?  I can't find any references to that other than mention that it's
supported in IE.

Nevertheless, it's probably unnecessary anyway.  parentNode is always going to
return an element or null.  So my extra checking of the nodeType is unnecessary.
 Will simplify.

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:247: if (node.nodeType ==
document.ELEMENT_NODE &&
On 2010/12/23 20:38:22, arv wrote:
> Node.Element_NODE
> 
> ... if you use parentNode you can skip the nodeType check

Done.

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:248:
node.nodeName.toLowerCase() == 'section')
On 2010/12/23 20:38:22, arv wrote:
> Use tagName?
> 
> I would skip the  toLowerCase and compare with 'SECTION' since tagName is
always
> upper case in HTML

Done.

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:259: *   this sub-page.  The
first item is typically a button in a root-level
On 2010/12/23 20:38:22, arv wrote:
> indent +2

Done.

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:264: associatedControls) {
On 2010/12/23 20:38:22, arv wrote:
> Can you make this optional?

Done.

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:283: associatedControls) {
On 2010/12/23 20:38:22, arv wrote:
> here too

Done.

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/options_page.js:493: * @return {boolean} True
if this page is sticky.
On 2010/12/23 20:38:22, arv wrote:
> I've been using @type for getters and setters since they look like properties
> and not functions... I don't know the official stance on this

Done.

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

http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
chrome/browser/resources/options/search_page.js:103: for (key in pagesToSearch)
{
On 2010/12/23 20:38:22, arv wrote:
> I find it easier to read with var but it is up to you

Moved a few vars inline that will have no or negligible performance impact.

[arv (Not doing code reviews), 2010-12-28 18:03:26]

On Thu, Dec 23, 2010 at 13:46,  <csilv@chromium.org> wrote:

>
http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
> chrome/browser/resources/options/options_page.js:246: while (node =
> node.parentNode) {
> On 2010/12/23 20:38:22, arv wrote:
>>
>> node.parentElement since you don't care about non elements.
>
> parentElement?  I can't find any references to that other than mention
> that it's supported in IE.
>
> Nevertheless, it's probably unnecessary anyway.  parentNode is always
> going to return an element or null.  So my extra checking of the
> nodeType is unnecessary.  Will simplify.
>
>
http://codereview.chromium.org/5992004/diff/22001/chrome/browser/resources/op...
> chrome/browser/resources/options/options_page.js:247: if (node.nodeType
> == document.ELEMENT_NODE &&
> On 2010/12/23 20:38:22, arv wrote:
>>
>> Node.Element_NODE
>
>> ... if you use parentNode you can skip the nodeType check

WebKit has parentElement as well and the only difference from
parentNode is that parentNode will include the document node.

assertTrue(document.documentElement.parentNode, document)
assertNull(document.documentElement.parentElement)


-- 
erik