MD Settings: Tune searching algorithm to skip certain subpages.

MD Settings: Tune searching algorithm to skip certain subpages.

 - Introducing a new CSS attribute 'skip-search' to mark elements that should be
   ignored.
 - Modifying searching algorithm, to skip such elements, both when searching but
   also when triggering a forced rendering.
 - Modify <settings-animated-pages> to carry over the 'skip-search' attribute
   from the <template is="dom-if"> to the <settings-subpage> stamped instance.
 - Remove settings-subpage's noAssociatedControl member variable since it is not
   necessary anymore.

BUG=635017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/3a94b6f16b9117a502adda26eddd53ef95832c27
Cr-Commit-Position: refs/heads/master@{#410432}

[dpapad, 2016-08-05 22:52:23]

Description was changed from

==========
MD Settings: Tune searching algorithm to skip certain subpages.

BUG=635017
==========

to

==========
MD Settings: Tune searching algorithm to skip certain subpages.

BUG=635017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-08-06 00:37:25]

Description was changed from

==========
MD Settings: Tune searching algorithm to skip certain subpages.

BUG=635017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Tune searching algorithm to skip certain subpages.

 - Introducing a new CSS attribute 'skip-search' to mark elements that should be
   ignored.
 - Modifying searching algorithm, to skip such elements, both when searching but
   also when triggering a forced rendering.
 - Remove settings-subpage noAssociatedControl member variable since it is not
   necessary anymore.

BUG=635017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-08-06 00:38:47]

Description was changed from

==========
MD Settings: Tune searching algorithm to skip certain subpages.

 - Introducing a new CSS attribute 'skip-search' to mark elements that should be
   ignored.
 - Modifying searching algorithm, to skip such elements, both when searching but
   also when triggering a forced rendering.
 - Remove settings-subpage noAssociatedControl member variable since it is not
   necessary anymore.

BUG=635017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Tune searching algorithm to skip certain subpages.

 - Introducing a new CSS attribute 'skip-search' to mark elements that should be
   ignored.
 - Modifying searching algorithm, to skip such elements, both when searching but
   also when triggering a forced rendering.
 - Modify <settings-animated-pages> to carry over the 'skip-search' attribute
   from the template to the <settings-subpage> stamped instance.
 - Remove settings-subpage noAssociatedControl member variable since it is not
   necessary anymore.

BUG=635017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-08-06 00:40:32]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-06 00:40:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-06 00:43:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-06 00:43:43]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dpapad, 2016-08-06 00:51:27]

Patchset #3 (id:40001) has been deleted

[dpapad, 2016-08-06 00:51:30]

Patchset #2 (id:20001) has been deleted

[dpapad, 2016-08-06 00:51:34]

Patchset #1 (id:1) has been deleted

[dpapad, 2016-08-06 00:51:42]

The CQ bit was checked by dpapad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-08-06 00:51:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[dpapad, 2016-08-06 00:54:20]

Description was changed from

==========
MD Settings: Tune searching algorithm to skip certain subpages.

 - Introducing a new CSS attribute 'skip-search' to mark elements that should be
   ignored.
 - Modifying searching algorithm, to skip such elements, both when searching but
   also when triggering a forced rendering.
 - Modify <settings-animated-pages> to carry over the 'skip-search' attribute
   from the template to the <settings-subpage> stamped instance.
 - Remove settings-subpage noAssociatedControl member variable since it is not
   necessary anymore.

BUG=635017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Tune searching algorithm to skip certain subpages.

 - Introducing a new CSS attribute 'skip-search' to mark elements that should be
   ignored.
 - Modifying searching algorithm, to skip such elements, both when searching but
   also when triggering a forced rendering.
 - Modify <settings-animated-pages> to carry over the 'skip-search' attribute
   from the <template is="dom-if"> to the <settings-subpage> stamped instance.
 - Remove settings-subpage noAssociatedControl member variable since it is not
   necessary anymore.

BUG=635017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-08-06 00:54:49]

Description was changed from

==========
MD Settings: Tune searching algorithm to skip certain subpages.

 - Introducing a new CSS attribute 'skip-search' to mark elements that should be
   ignored.
 - Modifying searching algorithm, to skip such elements, both when searching but
   also when triggering a forced rendering.
 - Modify <settings-animated-pages> to carry over the 'skip-search' attribute
   from the <template is="dom-if"> to the <settings-subpage> stamped instance.
 - Remove settings-subpage noAssociatedControl member variable since it is not
   necessary anymore.

BUG=635017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Tune searching algorithm to skip certain subpages.

 - Introducing a new CSS attribute 'skip-search' to mark elements that should be
   ignored.
 - Modifying searching algorithm, to skip such elements, both when searching but
   also when triggering a forced rendering.
 - Modify <settings-animated-pages> to carry over the 'skip-search' attribute
   from the <template is="dom-if"> to the <settings-subpage> stamped instance.
 - Remove settings-subpage's noAssociatedControl member variable since it is not
   necessary anymore.

BUG=635017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

[dpapad, 2016-08-06 01:13:50]

dpapad@chromium.org changed reviewers:
+ dbeam@chromium.org

[Dan Beam, 2016-08-06 01:50:28]

can we name it no-search instead?

skip-search might be ambiguous as to whether to continue and just pass over that
specific node.  this is what TreeWalker does on it's SKIP* param:
https://developer.mozilla.org/en-US/docs/Web/API/NodeFilter#Methods

https://codereview.chromium.org/2217403002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2217403002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:150:
if (template.hasAttribute('skip-search')) {
no curlies

https://codereview.chromium.org/2217403002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:150:
if (template.hasAttribute('skip-search')) {
i guess this isn't a bad idea, but maybe just repeating the attribute in the
subpage in the HTML would be more intuitive

[commit-bot: I haz the power, 2016-08-06 02:13:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-08-06 02:13:54]

Dry run: This issue passed the CQ dry run.

[dpapad, 2016-08-08 17:27:01]

Renamed to no-search.

https://codereview.chromium.org/2217403002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2217403002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:150:
if (template.hasAttribute('skip-search')) {
On 2016/08/06 at 01:50:27, Dan Beam wrote:
> i guess this isn't a bad idea, but maybe just repeating the attribute in the
subpage in the HTML would be more intuitive

Carrying over the 'no-search' attribute once programmatically, seems less
redundant and therefore less error-prone than copying it over manually in
multiple occasions. 

How about adding a comment here explaining why this attribute is copied instead?

[Dan Beam, 2016-08-08 17:42:56]

https://codereview.chromium.org/2217403002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2217403002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:150:
if (template.hasAttribute('skip-search')) {
On 2016/08/08 17:27:01, dpapad wrote:
> On 2016/08/06 at 01:50:27, Dan Beam wrote:
> > i guess this isn't a bad idea, but maybe just repeating the attribute in the
> subpage in the HTML would be more intuitive
> 
> Carrying over the 'no-search' attribute once programmatically, seems less
> redundant and therefore less error-prone than copying it over manually in
> multiple occasions. 
> 
> How about adding a comment here explaining why this attribute is copied
instead?

that's fine

https://codereview.chromium.org/2217403002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/printing_page/printing_page.html (right):

https://codereview.chromium.org/2217403002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/printing_page.html:46: restamp
no-search>
unwrap

https://codereview.chromium.org/2217403002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/privacy_page/privacy_page.html (right):

https://codereview.chromium.org/2217403002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/privacy_page/privacy_page.html:196: no-search>
unwrap all of these that need it

https://codereview.chromium.org/2217403002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/search_settings.js (right):

https://codereview.chromium.org/2217403002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_settings.js:132: (node.hasAttribute &&
node.hasAttribute(SKIP_SEARCH_CSS_ATTRIBUTE))) {
realistically, isn't every |root| an Element?  if so: hasAttribute() is
guaranteed to exist.  where is this check necessary?

[dpapad, 2016-08-08 18:15:21]

https://codereview.chromium.org/2217403002/diff/60001/chrome/browser/resource...
File chrome/browser/resources/settings/settings_page/settings_animated_pages.js
(right):

https://codereview.chromium.org/2217403002/diff/60001/chrome/browser/resource...
chrome/browser/resources/settings/settings_page/settings_animated_pages.js:150:
if (template.hasAttribute('skip-search')) {
> > How about adding a comment here explaining why this attribute is copied
instead?
> 
> that's fine

Done.

https://codereview.chromium.org/2217403002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/printing_page/printing_page.html (right):

https://codereview.chromium.org/2217403002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/printing_page/printing_page.html:46: restamp
no-search>
On 2016/08/08 at 17:42:56, Dan Beam wrote:
> unwrap

Done (here and elsewhere).

https://codereview.chromium.org/2217403002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/search_settings.js (right):

https://codereview.chromium.org/2217403002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_settings.js:132: (node.hasAttribute &&
node.hasAttribute(SKIP_SEARCH_CSS_ATTRIBUTE))) {
On 2016/08/08 at 17:42:56, Dan Beam wrote:
> realistically, isn't every |root| an Element?  if so: hasAttribute() is
guaranteed to exist.  where is this check necessary?

hasAttribute is called on |node| not |root| here. This check is needed because
there are two kinds of |node| values that don't have that method, a text node
(Node.TEXT_NODE) and a "shadow-root" node (see http://imgur.com/a/KR1c3, where I
added a console.log(!!node.hasAttribute) line).

[Dan Beam, 2016-08-08 18:50:47]

https://codereview.chromium.org/2217403002/diff/80001/chrome/browser/resource...
File chrome/browser/resources/settings/search_settings.js (right):

https://codereview.chromium.org/2217403002/diff/80001/chrome/browser/resource...
chrome/browser/resources/settings/search_settings.js:132: (node.hasAttribute &&
node.hasAttribute(SKIP_SEARCH_CSS_ATTRIBUTE))) {
On 2016/08/08 18:15:20, dpapad wrote:
> On 2016/08/08 at 17:42:56, Dan Beam wrote:
> > realistically, isn't every |root| an Element?  if so: hasAttribute() is
> guaranteed to exist.  where is this check necessary?
> 
> hasAttribute is called on |node| not |root| here. This check is needed because
> there are two kinds of |node| values that don't have that method, a text node
> (Node.TEXT_NODE) and a "shadow-root" node (see http://imgur.com/a/KR1c3, where
I
> added a console.log(!!node.hasAttribute) line).

ah, i see, text nodes.  yeah, i understand now.  lgtm

[dpapad, 2016-08-08 19:35:18]

The CQ bit was checked by dpapad@chromium.org

[commit-bot: I haz the power, 2016-08-08 19:35:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-08-08 20:24:39]

Committed patchset #3 (id:100001)

[commit-bot: I haz the power, 2016-08-08 20:28:48]

Description was changed from

==========
MD Settings: Tune searching algorithm to skip certain subpages.

 - Introducing a new CSS attribute 'skip-search' to mark elements that should be
   ignored.
 - Modifying searching algorithm, to skip such elements, both when searching but
   also when triggering a forced rendering.
 - Modify <settings-animated-pages> to carry over the 'skip-search' attribute
   from the <template is="dom-if"> to the <settings-subpage> stamped instance.
 - Remove settings-subpage's noAssociatedControl member variable since it is not
   necessary anymore.

BUG=635017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
==========

to

==========
MD Settings: Tune searching algorithm to skip certain subpages.

 - Introducing a new CSS attribute 'skip-search' to mark elements that should be
   ignored.
 - Modifying searching algorithm, to skip such elements, both when searching but
   also when triggering a forced rendering.
 - Modify <settings-animated-pages> to carry over the 'skip-search' attribute
   from the <template is="dom-if"> to the <settings-subpage> stamped instance.
 - Remove settings-subpage's noAssociatedControl member variable since it is not
   necessary anymore.

BUG=635017
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

Committed: https://crrev.com/3a94b6f16b9117a502adda26eddd53ef95832c27
Cr-Commit-Position: refs/heads/master@{#410432}
==========

[commit-bot: I haz the power, 2016-08-08 20:28:49]

Patchset 3 (id:??) landed as
https://crrev.com/3a94b6f16b9117a502adda26eddd53ef95832c27
Cr-Commit-Position: refs/heads/master@{#410432}