Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2082793003:
MD Settings: First iteration of searching within settings. (Closed)
|
Created:
4 years, 6 months ago by dpapad Modified:
4 years, 5 months ago CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, dschuyler, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@cr_search_migration0 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: First iteration of searching within settings.
This CL includes the core logic of the searching mechanism
- Traversing the DOM tree (taking into account Shadow DOM).
- Finding and highlighting matches (using familiar yellow rectangle
for now).
- Showing/hiding <settings-section> instances depending on search
outcome.
- Un-highlighting matches and restoring DOM structure.
- Navigating to the default page (chrome://md-settings), when a search
is triggered from any other page/sub-page.
- Force rendering of the "advanced" page and update search results.
Sub-pages that are not rendered are currently not searched. This will be
addressed in follow up CLs along with multiple other issues.
BUG=608535
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/e18c3a8d876d941206fc600dea2681dab2d290d8
Cr-Commit-Position: refs/heads/master@{#404222}
Patch Set 1 : Nits. #
Total comments: 11
Patch Set 2 : Address comment. #Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : Fix error when searching from About page. #
Total comments: 3
Patch Set 5 : Recursive version #
Total comments: 19
Patch Set 6 : Address comments. #Patch Set 7 : Fix subtlety. #Patch Set 8 : Use deep #
Total comments: 10
Patch Set 9 : Address comments. #
Total comments: 43
Patch Set 10 : Addressing comments. #
Total comments: 2
Patch Set 11 : Address comments. #Messages
Total messages: 65 (30 generated)
Description was changed from ========== WIP, hookup searching algorithm. BUG=608535 ========== to ========== WIP, hookup searching algorithm. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #4 (id:60001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
Patchset #1 (id:40001) has been deleted
Patchset #1 (id:80001) has been deleted
Description was changed from ========== WIP, hookup searching algorithm. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: First iteration of searching within settings. This CL includes the core logic of the searching mechanism - Traversing the complex DOM tree (taking into account Shadow DOM). - Finding and highlighting matches (using familiar yellow rectangle for now). - Un-highlighting matches and restoring DOM structure. - Navigating to the default page (chrome://md-settings), when a search is triggered from any other page/sub-page. - Force render the "advanced" page and update search results. Sub-pages that are not rendered are currently not searched. This will be addressed in follow up CLs along with multiple other issues. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: First iteration of searching within settings. This CL includes the core logic of the searching mechanism - Traversing the complex DOM tree (taking into account Shadow DOM). - Finding and highlighting matches (using familiar yellow rectangle for now). - Un-highlighting matches and restoring DOM structure. - Navigating to the default page (chrome://md-settings), when a search is triggered from any other page/sub-page. - Force render the "advanced" page and update search results. Sub-pages that are not rendered are currently not searched. This will be addressed in follow up CLs along with multiple other issues. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: First iteration of searching within settings. This CL includes the core logic of the searching mechanism - Traversing the complex DOM tree (taking into account Shadow DOM, via the introduction of a ShadowDomTreeWalker utility class). - Finding and highlighting matches (using familiar yellow rectangle for now). - Un-highlighting matches and restoring DOM structure. - Navigating to the default page (chrome://md-settings), when a search is triggered from any other page/sub-page. - Force render the "advanced" page and update search results. Sub-pages that are not rendered are currently not searched. This will be addressed in follow up CLs along with multiple other issues. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:100001) has been deleted
Patchset #1 (id:120001) has been deleted
Patchset #1 (id:140001) has been deleted
Patchset #1 (id:160001) has been deleted
Patchset #1 (id:180001) has been deleted
Patchset #1 (id:200001) has been deleted
Patchset #1 (id:220001) has been deleted
Description was changed from ========== MD Settings: First iteration of searching within settings. This CL includes the core logic of the searching mechanism - Traversing the complex DOM tree (taking into account Shadow DOM, via the introduction of a ShadowDomTreeWalker utility class). - Finding and highlighting matches (using familiar yellow rectangle for now). - Un-highlighting matches and restoring DOM structure. - Navigating to the default page (chrome://md-settings), when a search is triggered from any other page/sub-page. - Force render the "advanced" page and update search results. Sub-pages that are not rendered are currently not searched. This will be addressed in follow up CLs along with multiple other issues. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: First iteration of searching within settings. This CL includes the core logic of the searching mechanism - Traversing the DOM tree (taking into account Shadow DOM, via the introduction of a ShadowDomTreeWalker utility class, which uses native TreeWalker under the cover). - Finding and highlighting matches (using familiar yellow rectangle for now). - Un-highlighting matches and restoring DOM structure. - Navigating to the default page (chrome://md-settings), when a search is triggered from any other page/sub-page. - Force render the "advanced" page and update search results. Sub-pages that are not rendered are currently not searched. This will be addressed in follow up CLs along with multiple other issues. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: First iteration of searching within settings. This CL includes the core logic of the searching mechanism - Traversing the DOM tree (taking into account Shadow DOM, via the introduction of a ShadowDomTreeWalker utility class, which uses native TreeWalker under the cover). - Finding and highlighting matches (using familiar yellow rectangle for now). - Un-highlighting matches and restoring DOM structure. - Navigating to the default page (chrome://md-settings), when a search is triggered from any other page/sub-page. - Force render the "advanced" page and update search results. Sub-pages that are not rendered are currently not searched. This will be addressed in follow up CLs along with multiple other issues. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: First iteration of searching within settings. This CL includes the core logic of the searching mechanism - Traversing the DOM tree (taking into account Shadow DOM, via the introduction of a ShadowDomTreeWalker utility class, which uses native TreeWalker under the cover). - Finding and highlighting matches (using familiar yellow rectangle for now). - Un-highlighting matches and restoring DOM structure. - Navigating to the default page (chrome://md-settings), when a search is triggered from any other page/sub-page. - Force rendering of the "advanced" page and update search results. Sub-pages that are not rendered are currently not searched. This will be addressed in follow up CLs along with multiple other issues. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: First iteration of searching within settings. This CL includes the core logic of the searching mechanism - Traversing the DOM tree (taking into account Shadow DOM, via the introduction of a ShadowDomTreeWalker utility class, which uses native TreeWalker under the cover). - Finding and highlighting matches (using familiar yellow rectangle for now). - Un-highlighting matches and restoring DOM structure. - Navigating to the default page (chrome://md-settings), when a search is triggered from any other page/sub-page. - Force rendering of the "advanced" page and update search results. Sub-pages that are not rendered are currently not searched. This will be addressed in follow up CLs along with multiple other issues. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: First iteration of searching within settings. This CL includes the core logic of the searching mechanism - Traversing the DOM tree (taking into account Shadow DOM, via the introduction of a ShadowDomTreeWalker utility class, which uses native TreeWalker under the cover). - Finding and highlighting matches (using familiar yellow rectangle for now). - Showing/hiding <settings-section> instances depending on search outcome. - Un-highlighting matches and restoring DOM structure. - Navigating to the default page (chrome://md-settings), when a search is triggered from any other page/sub-page. - Force rendering of the "advanced" page and update search results. Sub-pages that are not rendered are currently not searched. This will be addressed in follow up CLs along with multiple other issues. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
Thought about writing tests, but thought it might be better to wait until this part of the codebase reaches a more stable status. So, no tests yet, but the goal is to add tests before considering searching completed.
https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.js (right): https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.js:43: this.fire('advanced-page-ready'); i'm not sure this event is necessary. ready is called synchronously when this element is created. this element is created (asynchronously) when the dom-if becomes truthy. so, you could use observe DOM changes in settings_main.js: https://www.polymer-project.org/1.0/docs/devguide/local-dom#observe-nodes or just schedule an async task. https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:46: value: new PromiseResolver(), i know this is sort of a singleton, but for the sake of testability, set this in created() https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:145: this.showAdvancedPage_ = true; without using a PromiseResolver: var searchAdvancedPage = function() { settings.search(query, assert(this.$$('settings-advanced-page'))); }.bind(this); if (!this.showAdvancedPage_) { this.showAdvancedPage_ = true; setTimeout(searchAdvancedPage); } else { searchAdvancedPage(); }
https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.js (right): https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.js:43: this.fire('advanced-page-ready'); On 2016/06/27 at 23:00:04, michaelpg wrote: > i'm not sure this event is necessary. ready is called synchronously when this element is created. this element is created (asynchronously) when the dom-if becomes truthy. > > so, you could use observe DOM changes in settings_main.js: https://www.polymer-project.org/1.0/docs/devguide/local-dom#observe-nodes > > or just schedule an async task. I tried using observeNodes() (after your suggestion), without success. First I get a call for an empty text node during startup, then my callback is never called when the advanced page is rendered. Besides those difficulties, observing the DOM to catch an event that occurs only one time for the lifetime of the page seems unnecessary. Also by "just schedule an async task", do you mean calling this.async() right after flipping the dom-if boolean? I tried this also, but it ends up not finding any results in the "Advanced" page, whereas the event + PromiseResolver always searches at the right time (results are found in the advanced page). https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:46: value: new PromiseResolver(), On 2016/06/27 at 23:00:04, michaelpg wrote: > i know this is sort of a singleton, but for the sake of testability, set this in created() Done. https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:145: this.showAdvancedPage_ = true; On 2016/06/27 at 23:00:04, michaelpg wrote: > without using a PromiseResolver: > > var searchAdvancedPage = function() { > settings.search(query, assert(this.$$('settings-advanced-page'))); > }.bind(this); > > if (!this.showAdvancedPage_) { > this.showAdvancedPage_ = true; > setTimeout(searchAdvancedPage); > } else { > searchAdvancedPage(); > } What is the benefit of not using PromiseResolver here? The proposed code snippet is not entirely equivalent in behavior. The following line (146), guarantees that even if the advanced page is already rendered (via a previous search, or maybe the user navigated manually), the code is yielding to the message loop before performing a blocking search operation. The proposed snippet only yields if the advanced page was not already rendered. Also using setTimeout() instead of Promises feels a bit hackier than using a Promise.
will get to this soon
michaelpg@chromium.org changed reviewers: + michaelpg@chromium.org
https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.js (right): https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.js:43: this.fire('advanced-page-ready'); On 2016/06/27 23:37:26, dpapad wrote: > On 2016/06/27 at 23:00:04, michaelpg wrote: > > i'm not sure this event is necessary. ready is called synchronously when this > element is created. this element is created (asynchronously) when the dom-if > becomes truthy. > > > > so, you could use observe DOM changes in settings_main.js: > https://www.polymer-project.org/1.0/docs/devguide/local-dom#observe-nodes > > > > or just schedule an async task. > > I tried using observeNodes() (after your suggestion), without success. First I > get a call for an empty text node during startup, then my callback is never > called when the advanced page is rendered. Besides those difficulties, observing > the DOM to catch an event that occurs only one time for the lifetime of the page > seems unnecessary. you could try adding a listener for the dom-if's dom-change event: https://www.polymer-project.org/1.0/docs/api/dom-if#event-dom-change and then removing the listener when it fires > > Also by "just schedule an async task", do you mean calling this.async() right > after flipping the dom-if boolean? I tried this also, but it ends up not finding > any results in the "Advanced" page, whereas the event + PromiseResolver always > searches at the right time (results are found in the advanced page). Yep, that's what I meant. As a sanity check, you can try calling render() on the dom-if which should force it to be created (and ready) immediately. Maybe there are other async things going on in wherever the results are? If so, can you guarantee all of those are always ready by the time the PromiseResolver is resolved, or is that just what you've observed so far? https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:145: this.showAdvancedPage_ = true; On 2016/06/27 23:37:27, dpapad wrote: > On 2016/06/27 at 23:00:04, michaelpg wrote: > > without using a PromiseResolver: > > > > var searchAdvancedPage = function() { > > settings.search(query, assert(this.$$('settings-advanced-page'))); > > }.bind(this); > > > > if (!this.showAdvancedPage_) { > > this.showAdvancedPage_ = true; > > setTimeout(searchAdvancedPage); > > } else { > > searchAdvancedPage(); > > } > > What is the benefit of not using PromiseResolver here? PromiseResolver is sugar for creating a Promise when you need one but don't have one. I don't think we need one so I don't think we need to add sugar for one. > > The proposed code snippet is not entirely equivalent in behavior. The following > line (146), guarantees that even if the advanced page is already rendered (via a > previous search, or maybe the user navigated manually), the code is yielding to > the message loop before performing a blocking search operation. The proposed > snippet only yields if the advanced page was not already rendered. Oh. Add a comment describing that intention, then. (To be clear, the desired behavior is to search the basic page synchronously, then in a separate task, search the advanced page, even if the advanced page is already visible?) > Also using > setTimeout() instead of Promises feels a bit hackier than using a Promise. okay: this.showAdvancedPage_ = true; this.async(function() { settings.search(query, assert(this.$$('settings-advanced-page'))); }); and you can delete 11 LOC for firing an event *at a known time* to resolve a promise. All that does is schedule a microtask, the above does the same thing without unnecessary indirection. (Or use setTimeout if you actually want to schedule a new task for the next event loop.) By the way, line 139 synchronously enables the basic page's dom-if but doesn't wait to perform the search on line 142. Why don't you need a whenBasicPageReady_ construct? Does this work if you trigger it from the About page without having loaded the Basic page first? I guess it may work because showBasicPage_ is initialized to true. I wonder why we do that.
https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/advanced_page/advanced_page.js (right): https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/advanced_page/advanced_page.js:43: this.fire('advanced-page-ready'); Removed this event. https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:145: this.showAdvancedPage_ = true; On 2016/06/29 at 01:18:57, michaelpg wrote: > On 2016/06/27 23:37:27, dpapad wrote: > > On 2016/06/27 at 23:00:04, michaelpg wrote: > > > without using a PromiseResolver: > > > > > > var searchAdvancedPage = function() { > > > settings.search(query, assert(this.$$('settings-advanced-page'))); > > > }.bind(this); > > > > > > if (!this.showAdvancedPage_) { > > > this.showAdvancedPage_ = true; > > > setTimeout(searchAdvancedPage); > > > } else { > > > searchAdvancedPage(); > > > } > > > > What is the benefit of not using PromiseResolver here? > > PromiseResolver is sugar for creating a Promise when you need one but don't have > one. I don't think we need one so I don't think we need to add sugar for one. > > > > > The proposed code snippet is not entirely equivalent in behavior. The following > > line (146), guarantees that even if the advanced page is already rendered (via a > > previous search, or maybe the user navigated manually), the code is yielding to > > the message loop before performing a blocking search operation. The proposed > > snippet only yields if the advanced page was not already rendered. > > Oh. Add a comment describing that intention, then. (To be clear, the desired > behavior is to search the basic page synchronously, then in a separate task, > search the advanced page, even if the advanced page is already visible?) Yes, added a comment for the current intention. Keep in mind that the argument about asynchronicity here will be obsolete very soon, as I am asyncifying search internals in follow up CLs anyway (even for the basic page). > > > Also using > > setTimeout() instead of Promises feels a bit hackier than using a Promise. > > okay: > > this.showAdvancedPage_ = true; > this.async(function() { > settings.search(query, assert(this.$$('settings-advanced-page'))); > }); Changed in favor of setTimeout(). For the record, using this.async() is not equivalent. The advanced page contents have a series of dom-ifs, and when using this.async() search happens before the underlying dom-if have been rendered (for example before settings-private-page ready). When I use the promise (or setTimeout) all the underlying dom-ifs have been rendered. It can be easily showcased with a few console.logs, but the bottom line is that this.async() and using a Promise here are not behaviorally equivalent. > > and you can delete 11 LOC for firing an event *at a known time* to resolve a > promise. All that does is schedule a microtask, the above does the same thing > without unnecessary indirection. (Or use setTimeout if you actually want to > schedule a new task for the next event loop.) As stated above, changed to setTimeout. > > By the way, line 139 synchronously enables the basic page's dom-if but doesn't > wait to perform the search on line 142. Why don't you need a whenBasicPageReady_ > construct? Does this work if you trigger it from the About page without having > loaded the Basic page first? > > I guess it may work because showBasicPage_ is initialized to true. I wonder > why we do that. The basic page is always rendered, even if you navigate directly to chrome://md-settings/advanced. So I don't think that the dom-if around the basic page and showBasicPage_ have any reason for existence anymore.
https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2082793003/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:145: this.showAdvancedPage_ = true; On 2016/06/29 18:20:12, dpapad wrote: > On 2016/06/29 at 01:18:57, michaelpg wrote: > > On 2016/06/27 23:37:27, dpapad wrote: > > > On 2016/06/27 at 23:00:04, michaelpg wrote: > > > > without using a PromiseResolver: > > > > > > > > var searchAdvancedPage = function() { > > > > settings.search(query, assert(this.$$('settings-advanced-page'))); > > > > }.bind(this); > > > > > > > > if (!this.showAdvancedPage_) { > > > > this.showAdvancedPage_ = true; > > > > setTimeout(searchAdvancedPage); > > > > } else { > > > > searchAdvancedPage(); > > > > } > > > > > > What is the benefit of not using PromiseResolver here? > > > > PromiseResolver is sugar for creating a Promise when you need one but don't > have > > one. I don't think we need one so I don't think we need to add sugar for one. > > > > > > > > The proposed code snippet is not entirely equivalent in behavior. The > following > > > line (146), guarantees that even if the advanced page is already rendered > (via a > > > previous search, or maybe the user navigated manually), the code is yielding > to > > > the message loop before performing a blocking search operation. The proposed > > > snippet only yields if the advanced page was not already rendered. > > > > Oh. Add a comment describing that intention, then. (To be clear, the desired > > behavior is to search the basic page synchronously, then in a separate task, > > search the advanced page, even if the advanced page is already visible?) > > Yes, added a comment for the current intention. Keep in mind that the argument > about > asynchronicity here will be obsolete very soon, as I am asyncifying search > internals > in follow up CLs anyway (even for the basic page). > > > > > > Also using > > > setTimeout() instead of Promises feels a bit hackier than using a Promise. > > > > okay: > > > > this.showAdvancedPage_ = true; > > this.async(function() { > > settings.search(query, assert(this.$$('settings-advanced-page'))); > > }); > > Changed in favor of setTimeout(). > > For the record, using this.async() is not equivalent. The advanced page contents > have a series of dom-ifs, and when using this.async() > search happens before the underlying dom-if have been rendered (for example > before settings-private-page ready). When I use the promise (or setTimeout) all > the underlying dom-ifs have been rendered. It can be easily showcased with a few > console.logs, but the bottom line is that this.async() and using a Promise here > are not behaviorally equivalent. Makes sense. Polymer queues effects like dom-if's rendering and schedules a microtask to process the queue. A Promise will exec after the entire queue has been flushed (assuming, I think, that you've created the Promise before Polymer created the MutationObserver it uses to schedule its own microtasks, but some of that may be platform-dependent -- hooray for undefined and underspecified behavior!) This also presumes the template rendering does not trigger more templates which themselves rely on Promises to complete, which maybe we do (e.g. for initializing languages). Ultimately there's enough asynchronicity in Settings (e.g., talking to the browser process) that you'd be hard-pressed to find the perfect time to run a search. > > > > > and you can delete 11 LOC for firing an event *at a known time* to resolve a > > promise. All that does is schedule a microtask, the above does the same thing > > without unnecessary indirection. (Or use setTimeout if you actually want to > > schedule a new task for the next event loop.) > As stated above, changed to setTimeout. > > > > > By the way, line 139 synchronously enables the basic page's dom-if but doesn't > > wait to perform the search on line 142. Why don't you need a > whenBasicPageReady_ > > construct? Does this work if you trigger it from the About page without having > > loaded the Basic page first? > > > > I guess it may work because showBasicPage_ is initialized to true. I wonder > > why we do that. > > The basic page is always rendered, even if you navigate directly to > chrome://md-settings/advanced. So I don't think that the dom-if around the basic > page and showBasicPage_ have any reason for existence anymore. The basic page is rendered even when you navigate to the About page? Why?
> The basic page is rendered even when you navigate to the About page? Why? Eh, I did not think about the About page, only had Advanced in my mind. I am not handling the case of navigating directly to chrome://md-settings/help and then triggering a search. I prefer not to handle this yet, because I am not even sure if we should be able to search from the About page. I'll make a note to address it eventually though.
On 2016/06/29 at 20:34:41, dpapad wrote: > > The basic page is rendered even when you navigate to the About page? Why? > > Eh, I did not think about the About page, only had Advanced in my mind. I am not handling the case of navigating directly to chrome://md-settings/help and then triggering a search. I prefer not to handle this yet, because I am not even sure if we should be able to search from the About page. I'll make a note to address it eventually though. Fixed this to not throw a runtime exception when searching from the About page. The code no longer assumes that the basic page is already rendered.
FYI, the next CL that makes searching be non-blocking is at https://codereview.chromium.org/2103133007 (still WIP, functional but needs code cleanup). I am only mentioning it here, because I think it makes it more obvious of what is included in the "first iteration" vs "second iteration". 1st iteration adds all the core logic for searching rendered DOM. 2nd iteration makes it non-blocking and adds logic for forced rendering of subpages (followed by a search within those pages).
https://codereview.chromium.org/2082793003/diff/280001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:193: if (!!node.shadowRoot) equivalent to if (node.shadowRoot) https://codereview.chromium.org/2082793003/diff/280001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:248: this.walkers_.splice(0, 1); nit: this.walkers_.shift() https://codereview.chromium.org/2082793003/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:228: node, NodeFilter.SHOW_ALL, this.nodeFilter_, false)); according to esprehn@, if we're doing SHOW_ALL or SHOW_ELEMENT | SHOW_TEXT (it's a bitmask, btw), we're not really gaining much by using a walker -- just adding memory overhead in my testing, I could not produce a slower recursive, pure JS + DOM version: https://gist.github.com/danbeam/83a60be46c72ae57dd9bcc18bddc0e50 I put this in settings.js so it wasn't being eval()'d from the console (I've heard v8 wont optimize that), and run it in quite a few different configurations (NUM_TESTS = 1 so less JIT optimizations, NUM_TESTS=100, NUM_TESTS=1 but with a ton of independently scheduled setTimeout()s, etc.). I'm really tempted to just write recursive or iterative JS + DOM if it simplifies things.
https://codereview.chromium.org/2082793003/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:228: node, NodeFilter.SHOW_ALL, this.nodeFilter_, false)); On 2016/06/30 at 16:33:17, Dan Beam wrote: > according to esprehn@, if we're doing SHOW_ALL or SHOW_ELEMENT | SHOW_TEXT (it's a bitmask, btw), we're not really gaining much by using a walker -- just adding memory overhead > > in my testing, I could not produce a slower recursive, pure JS + DOM version: > https://gist.github.com/danbeam/83a60be46c72ae57dd9bcc18bddc0e50 > > I put this in settings.js so it wasn't being eval()'d from the console (I've heard v8 wont optimize that), and run it in quite a few different configurations (NUM_TESTS = 1 so less JIT optimizations, NUM_TESTS=100, NUM_TESTS=1 but with a ton of independently scheduled setTimeout()s, etc.). > > I'm really tempted to just write recursive or iterative JS + DOM if it simplifies things. Whether it simplifies things, is questionable, until we figure out how can the recursive version be asyncified. I have already done this at https://codereview.chromium.org/2103133007 for the iterative (Walker) version. It is not clear to me if recursive approach simplifies things, and I would need probably another day to try CL 2103133007 with recursive version. Let me know if you think this is time well spent.
Patchset #5 (id:320001) has been deleted
Patchset #5 (id:340001) has been deleted
Description was changed from ========== MD Settings: First iteration of searching within settings. This CL includes the core logic of the searching mechanism - Traversing the DOM tree (taking into account Shadow DOM, via the introduction of a ShadowDomTreeWalker utility class, which uses native TreeWalker under the cover). - Finding and highlighting matches (using familiar yellow rectangle for now). - Showing/hiding <settings-section> instances depending on search outcome. - Un-highlighting matches and restoring DOM structure. - Navigating to the default page (chrome://md-settings), when a search is triggered from any other page/sub-page. - Force rendering of the "advanced" page and update search results. Sub-pages that are not rendered are currently not searched. This will be addressed in follow up CLs along with multiple other issues. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: First iteration of searching within settings. This CL includes the core logic of the searching mechanism - Traversing the DOM tree (taking into account Shadow DOM). - Finding and highlighting matches (using familiar yellow rectangle for now). - Showing/hiding <settings-section> instances depending on search outcome. - Un-highlighting matches and restoring DOM structure. - Navigating to the default page (chrome://md-settings), when a search is triggered from any other page/sub-page. - Force rendering of the "advanced" page and update search results. Sub-pages that are not rendered are currently not searched. This will be addressed in follow up CLs along with multiple other issues. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Updated DOM traversal algorithm per discussion. PTAL. https://codereview.chromium.org/2082793003/diff/300001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/300001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:228: node, NodeFilter.SHOW_ALL, this.nodeFilter_, false)); > Let me know if you think this is time well spent. I've updated this CL to traverse the DOM in a pure JS recursive manner, removed ShadowDomTreeWalker helper class. https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:49: var walker = document.createTreeWalker( I am still using TreeWalker for removing highlights here, because it seems faster than the recursive version. I kept the recursive version around if you want to verify timings, just replace the contents of this function with http://pastebin.com/jrWF82q4.
this is looking better and I'm closer to understanding the reasoning behind previous and current choices :) https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:19: IGNORED_ELEMENTS = new Set([ nit: var IGNORED_ELEMENTS https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:49: var walker = document.createTreeWalker( On 2016/06/30 21:27:44, dpapad wrote: > I am still using TreeWalker for removing highlights here, because it seems > faster than the recursive version. I kept the recursive version around if you > want to verify timings, just replace the contents of this function with > http://pastebin.com/jrWF82q4. can we use queryEffectiveChildren('.search-highlight-hit, .search-hightlight-wrapper') instead? https://www.polymer-project.org/1.0/docs/devguide/local-dom#light-dom-children https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:134: if (IGNORED_ELEMENTS.has(node.tagName)) esprehn@ recommended just doing: function ignoreNode(node) { var nodeName = node.nodeName; return nodeName == 'CONTENT' || nodeName == 'CR-EVENTS' ...; } https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:141: if (regExp.test(textContent)) { it's not clear to me whether we should be using regexp or indexOf() but in this case I'm fairly sure that using regexp makes the code simpler (and I can't prove the performance diff) https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:143: applyHighlightUi_(node, textContent.split(regExp)); ah, if we use regex we don't have to make as many case-converted copies. that's an advantage https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:150: doSearch(childNodes[i]); esprehn said using https://developer.mozilla.org/en-US/docs/Web/API/Node/nextSibling might be faster, i.e. var child = node.firstChild; while (child !== null) { doSearch(child); child = child.nextSibling; } https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:170: parent.host : parent.parentNode; worth using domHost? https://www.polymer-project.org/1.0/docs/api/Polymer.Base#property-domHost https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:184: sections[i].style.display = visible ? '' : 'none'; can we use [hidden] instead? https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:198: var searchText = text.trim().toLowerCase().replace(SANITIZE_REGEX, '\\$&'); why toLowerCase() if we're using 'i'?
https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:19: IGNORED_ELEMENTS = new Set([ On 2016/07/01 at 00:54:55, Dan Beam wrote: > nit: var IGNORED_ELEMENTS Oops, done. https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:49: var walker = document.createTreeWalker( On 2016/07/01 at 00:54:55, Dan Beam wrote: > On 2016/06/30 21:27:44, dpapad wrote: > > I am still using TreeWalker for removing highlights here, because it seems > > faster than the recursive version. I kept the recursive version around if you > > want to verify timings, just replace the contents of this function with > > http://pastebin.com/jrWF82q4. > > can we use queryEffectiveChildren('.search-highlight-hit, .search-hightlight-wrapper') instead? > https://www.polymer-project.org/1.0/docs/devguide/local-dom#light-dom-children search-highlight-hit and search-hightlight-wrapper classes are only needed inside removeHighlightUi_() method, not here. Here we need to start a new search every time a shadoRoot is encountered, so I don't fully understand your suggestion. In general, I'd like to avoid using Polymer helper functions in favor of native functions, since Polymer helpers mostly provide a cross-compatibility layer for Shady and Shadow DOM, which we don't need. https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:134: if (IGNORED_ELEMENTS.has(node.tagName)) On 2016/07/01 at 00:54:55, Dan Beam wrote: > esprehn@ recommended just doing: > > function ignoreNode(node) { > var nodeName = node.nodeName; > return nodeName == 'CONTENT' || nodeName == 'CR-EVENTS' ...; > } Looking up whether an item exists in a Set is advertized as O(1). The compound OR statement seems like a linear search. Am I missing something? Perhaps this is a question for @esprehn. I would like to know more before removing the Set usage here. https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:143: applyHighlightUi_(node, textContent.split(regExp)); On 2016/07/01 at 00:54:55, Dan Beam wrote: > ah, if we use regex we don't have to make as many case-converted copies. that's an advantage True. https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:150: doSearch(childNodes[i]); On 2016/07/01 at 00:54:55, Dan Beam wrote: > esprehn said using https://developer.mozilla.org/en-US/docs/Web/API/Node/nextSibling might be faster, i.e. > > var child = node.firstChild; > while (child !== null) { > doSearch(child); > child = child.nextSibling; > } Done. There was a subtlety here though. If there is a hit |child| is being removed from the DOM inside doSearch(), and is being replaced by a new node. Therefore, calling nextSibling after |child| has been removed from the DOM is not correct (always returns null). But, since TEXT_NODE nodes never have children, and we only replace TEXT_NODE nodes in the DOM, calling nextSibling after doSearch() is fine. I added an early return statement few lines above, since for TEXT_NODES we don't even need to get here. https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:170: parent.host : parent.parentNode; On 2016/07/01 at 00:54:55, Dan Beam wrote: > worth using domHost? > https://www.polymer-project.org/1.0/docs/api/Polymer.Base#property-domHost No. The reason is that domHost includes a few Polymer function calls that are completely unnecessary, see [1] and [2]. [1] https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... [2] https://cs.chromium.org/chromium/src/third_party/polymer/v1_0/components-chro... (needed for Shady DOM only). https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:184: sections[i].style.display = visible ? '' : 'none'; On 2016/07/01 at 00:54:55, Dan Beam wrote: > can we use [hidden] instead? Done. https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:198: var searchText = text.trim().toLowerCase().replace(SANITIZE_REGEX, '\\$&'); On 2016/07/01 at 00:54:55, Dan Beam wrote: > why toLowerCase() if we're using 'i'? Removed.
https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/360001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:150: doSearch(childNodes[i]); On 2016/07/01 at 18:08:43, dpapad wrote: > On 2016/07/01 at 00:54:55, Dan Beam wrote: > > esprehn said using https://developer.mozilla.org/en-US/docs/Web/API/Node/nextSibling might be faster, i.e. > > > > var child = node.firstChild; > > while (child !== null) { > > doSearch(child); > > child = child.nextSibling; > > } > > Done. There was a subtlety here though. If there is a hit |child| is being removed from the DOM inside doSearch(), and is being replaced by a new node. Therefore, calling nextSibling after |child| has been removed from the DOM is not correct (always returns null). But, since TEXT_NODE nodes never have children, and we only replace TEXT_NODE nodes in the DOM, calling nextSibling after doSearch() is fine. I added an early return statement few lines above, since for TEXT_NODES we don't even need to get here. Fixed subtlety at patchset 7. TEXT_NODE still have siblings, so my reasoning above was partially wrong (need to call nextSibling before doSearch()).
Patchset #8 (id:420001) has been deleted
Patchset #8 (id:440001) has been deleted
Per our discussion, updated the removeHighligts_() method to not be recursive and use /deep/ instead (it is slightly faster as expected).
lgtm https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:41: function findAndRemoveHighligts_(node) { fix typo https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:75: function applyHighlightUi_(node, tokens) { i don't think you need 2 verbs here, i.e. just highlight (don't need apply as well) https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:127: while (child !== null) { nit: put a comment here about how doSearch() may affect the results of nextSibling https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:137: this.currentRoute = {page: 'basic', section: '', subpage: [], url: ''}; nit: can you just omit the URL? and maybe add a route for search?
also, i don't really know a great way, but maybe figure out a good testing story for this?
Regarding tests, see my first comment in this CL, https://codereview.chromium.org/2082793003#msg20. https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:41: function findAndRemoveHighligts_(node) { On 2016/07/01 at 21:32:34, Dan Beam wrote: > fix typo Done. https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:75: function applyHighlightUi_(node, tokens) { On 2016/07/01 at 21:32:35, Dan Beam wrote: > i don't think you need 2 verbs here, i.e. just highlight (don't need apply as well) Done. https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:127: while (child !== null) { On 2016/07/01 at 21:32:35, Dan Beam wrote: > nit: put a comment here about how doSearch() may affect the results of nextSibling Done. https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:137: this.currentRoute = {page: 'basic', section: '', subpage: [], url: ''}; On 2016/07/01 at 21:32:35, Dan Beam wrote: > nit: can you just omit the URL? and maybe add a route for search? The 'url' field is needed only for the Compiler. I prefer not to deal with routing in this CL, but the goal is to add routing for searching in follow up CL (after the core logic is completed).
https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:137: this.currentRoute = {page: 'basic', section: '', subpage: [], url: ''}; On 2016/07/01 21:59:42, dpapad wrote: > On 2016/07/01 at 21:32:35, Dan Beam wrote: > > nit: can you just omit the URL? and maybe add a route for search? > > The 'url' field is needed only for the Compiler. I prefer not to deal with > routing in this CL, but the goal is to add routing for searching in follow up CL > (after the core logic is completed). fun fact: it's only required if you: a) depend settings_router and actually get the @typedef (otherwise it's silently ignored, oh yay!) b) land before my CL to make it optional ;)[1] [1] https://codereview.chromium.org/2115133002
either way, still lgtm
https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_main/settings_main.js (right): https://codereview.chromium.org/2082793003/diff/460001/chrome/browser/resourc... chrome/browser/resources/settings/settings_main/settings_main.js:137: this.currentRoute = {page: 'basic', section: '', subpage: [], url: ''}; On 2016/07/01 22:41:01, Dan Beam wrote: > On 2016/07/01 21:59:42, dpapad wrote: > > On 2016/07/01 at 21:32:35, Dan Beam wrote: > > > nit: can you just omit the URL? and maybe add a route for search? > > > > The 'url' field is needed only for the Compiler. I prefer not to deal with > > routing in this CL, but the goal is to add routing for searching in follow up > CL > > (after the core logic is completed). > > fun fact: it's only required if you: > a) depend settings_router and actually get the @typedef (otherwise it's silently > ignored, oh yay!) > b) land before my CL to make it optional ;)[1] > > [1] https://codereview.chromium.org/2115133002 or dschuyler@'s CL that makes url: (string|undefined), that'd also help ;) https://codereview.chromium.org/2111223002/
@michaelpg: Ping
mostly questions. overall it looks good, and it's way more concise than I thought it would be, given the nature of the problem -- looks like a lot of thought went into it! https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:19: var IGNORED_ELEMENTS = new Set([ why were these chosen? who is responsible for keeping this up-to-date? what about: paper-progress paper-spinner paper-tooltip https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:22: 'IMG', add DOM-MODULE, or comment that the only dom-module in the root document is in <body> which we don't search https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:24: 'PAPER-ICON-BUTTON', do we need iron-list in here? https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:44: for (var i = 0; i < wrappers.length; i++) { optional nit: for (var wrapper of wrappers) { https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:48: for (var j = 0; j < hitElements.length; j++) { optional nit: for (var hitElement of hitElements) https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:56: if (wrapper.previousSibling) optional performance suggestion: only set textContent once var text = wrapper.textContent; if () text = ' ' + text; if () text += ' '; wrapper.textContent = text; https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:57: wrapper.textContent = ' ' + wrapper.textContent; why add spaces (how do you know there were spaces before) https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:61: wrapper.parentElement.insertBefore( is this the same as: wrapper.parentElement.replaceChild(wrapper.firstChild, wrapper); it calls remove() for you, and seems more idiomatic to me https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:70: * param {!Node} node The text node to be highlighted. |node| ends up being @param (surprised closure didn't complain about this) https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:71: * removed from the DOM tree. 4-space indent https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:72: * @param {!Array<string>} tokens The string tokens that did not match. I don't quite get this -- tokens that did *not* match? does that have something to do with the odd/even branch below? https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:80: node.parentNode.insertBefore(wrapper, node); replaceChild? https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:88: span.style['background-color'] = 'yellow'; whoa, I had no idea this worked! (I can't find it in the spec though. Use style.backgroundColor please.) https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:155: parent.hidden = false; if |parent| is hidden by "display: none" (e.g. by a dom-if) but still in the DOM, this won't show it (meaning searching within this section was a waste of time). is that ok? https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:166: sections[i].hidden = !visible; same question https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:178: // Generate search text by applying lowercase and escaping any characters how does this lowercase? https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:187: findAndHighlightMatches_(page, new RegExp('(' + searchText + ')', 'i')); oh https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/settings_resources.grd:15: <structure name="IDR_SETTINGS_SEARCH_SETTINGS_JS" alphabetize https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:62: this.$$('cr-toolbar').addEventListener('search-changed', function(e) { is it possible to type something in the search field before <settings-ui> is ready?
https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:19: var IGNORED_ELEMENTS = new Set([ On 2016/07/04 at 19:23:56, michaelpg wrote: > why were these chosen? These elements do not contain any useful text for search. Pruning the search tree has a measurable impact on performance. >who is responsible for keeping this up-to-date? Whoever owns the "search within settings" feature (which might translate to OWNERS of chrome/browser/resources/settings). > > what about: > paper-progress > paper-spinner > paper-tooltip Could not find any occurrences in our code for paper-progress and paper-tooltip. Added paper-spinner to the set. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:22: 'IMG', On 2016/07/04 at 19:23:55, michaelpg wrote: > add DOM-MODULE, or comment that the only dom-module in the root document is in <body> which we don't search Added a comment. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:24: 'PAPER-ICON-BUTTON', On 2016/07/04 at 19:23:55, michaelpg wrote: > do we need iron-list in here? See following TODO. I am planning to go through various dynamically populated lists (dom-repeat or iron-list) and mark them with an attribute such that search can skip them this way. Perhaps iron-list can always be skipped, so no need for a new attribute, but I prefer to evaluate this separately. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:44: for (var i = 0; i < wrappers.length; i++) { On 2016/07/04 at 19:23:55, michaelpg wrote: > optional nit: for (var wrapper of wrappers) { Done. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:48: for (var j = 0; j < hitElements.length; j++) { On 2016/07/04 at 19:23:55, michaelpg wrote: > optional nit: for (var hitElement of hitElements) Done. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:56: if (wrapper.previousSibling) On 2016/07/04 at 19:23:56, michaelpg wrote: > optional performance suggestion: only set textContent once > > var text = wrapper.textContent; > if () text = ' ' + text; > if () text += ' '; > wrapper.textContent = text; Prefer not to apply this micro-optimization. In most cases there is no previousSibling and nextSibling, so existing version does not need to set the textContent at all. The suggestion optimizes the corner cases at the expense of always having to set wrapper.textContent, even if no spaces where added. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:57: wrapper.textContent = ' ' + wrapper.textContent; On 2016/07/04 at 19:23:56, michaelpg wrote: > why add spaces (how do you know there were spaces before) This is addressing a corner case where a hit exists right next to a link. For example (from the privacy page): Google Chrome may use <a>web services</a> to ... If you search for "use", without this logic after clearing the highlights there would be no space between "use" and the link text. The existence of a previous/nextSibling seems like a good criterion of whether a space existed there before (I have not found any case that breaks this assumption). https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:61: wrapper.parentElement.insertBefore( On 2016/07/04 at 19:23:55, michaelpg wrote: > is this the same as: > > wrapper.parentElement.replaceChild(wrapper.firstChild, wrapper); > > it calls remove() for you, and seems more idiomatic to me Done. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:70: * param {!Node} node The text node to be highlighted. |node| ends up being On 2016/07/04 at 19:23:56, michaelpg wrote: > @param (surprised closure didn't complain about this) Done. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:71: * removed from the DOM tree. On 2016/07/04 at 19:23:55, michaelpg wrote: > 4-space indent Done. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:72: * @param {!Array<string>} tokens The string tokens that did not match. On 2016/07/04 at 19:23:56, michaelpg wrote: > I don't quite get this -- tokens that did *not* match? does that have something to do with the odd/even branch below? Comment was obsolete. Clarified this parameter in the comment. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:80: node.parentNode.insertBefore(wrapper, node); On 2016/07/04 at 19:23:56, michaelpg wrote: > replaceChild? Done. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:155: parent.hidden = false; On 2016/07/04 at 19:23:55, michaelpg wrote: > if |parent| is hidden by "display: none" (e.g. by a dom-if) but still in the DOM, this won't show it (meaning searching within this section was a waste of time). is that ok? Parent can only be a <settings-section> here and I am not aware of any settings sections that are hidden by a "display: none". Remember that all the <settings-section> belonging to the "advanced" page are explicitly dom-ifed to true so that they can be searched (done in <settings-main>). https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:166: sections[i].hidden = !visible; On 2016/07/04 at 19:23:56, michaelpg wrote: > same question Same answer. Have not encountered a <settings-section> that is hidden during searching. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:178: // Generate search text by applying lowercase and escaping any characters On 2016/07/04 at 19:23:56, michaelpg wrote: > how does this lowercase? It does not need to. The regExp constructed below is case insensitive. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/settings_resources.grd:15: <structure name="IDR_SETTINGS_SEARCH_SETTINGS_JS" On 2016/07/04 at 19:23:56, michaelpg wrote: > alphabetize Done, although I think the alphabetized version is messier. Now this entry is wedged between entries of the on_startup_page/ folder. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_ui/settings_ui.js (right): https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/settings_ui/settings_ui.js:62: this.$$('cr-toolbar').addEventListener('search-changed', function(e) { On 2016/07/04 at 19:23:56, michaelpg wrote: > is it possible to type something in the search field before <settings-ui> is ready? The cr-toolbar fires a bogus event during its initialization (seems like a bug but have not investigated further). That bogus event was causing the callback to fire before <settings-ui> was ready, so adding the listener here (instead of adding it in HTML) fixes the issue. On a related note, the fact that Polymer happily calls functions on a Polymer element before that element's ready() method has been called seems like a bug to me, but I am guessing it is WAI.
lgtm, great start https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:19: var IGNORED_ELEMENTS = new Set([ On 2016/07/06 18:24:54, dpapad wrote: > On 2016/07/04 at 19:23:56, michaelpg wrote: > > why were these chosen? > These elements do not contain any useful text for search. Pruning the search > tree has a measurable impact on performance. > > >who is responsible for keeping this up-to-date? > Whoever owns the "search within settings" feature (which might translate to > OWNERS of chrome/browser/resources/settings). > > > > > what about: > > paper-progress > > paper-spinner > > paper-tooltip > > Could not find any occurrences in our code for paper-progress and paper-tooltip. > Added paper-spinner to the set. paper-progress is part of paper-slider; if you open Customize Fonts some paper-progress elements get added to the DOM. Perhaps you also want to add paper-slider, I don't think there's anything we want to match there. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:24: 'PAPER-ICON-BUTTON', On 2016/07/06 18:24:55, dpapad wrote: > On 2016/07/04 at 19:23:55, michaelpg wrote: > > do we need iron-list in here? > > See following TODO. I am planning to go through various dynamically populated > lists (dom-repeat or iron-list) and mark them with an attribute such that search > can skip them this way. Perhaps iron-list can always be skipped, so no need for > a new attribute, but I prefer to evaluate this separately. My fear is that if you don't blacklist iron-list, the set of matches will depend on the scroll position within iron-lists. Unlike dom-repeat which is rendered in its entirety (eventually). It'd be easier for developing and debugging if Search were as deterministic and reproducible as possible, at any step of the implementation. If that's impractical, then it's fine, but it seemed easy to add here. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:178: // Generate search text by applying lowercase and escaping any characters On 2016/07/06 18:24:55, dpapad wrote: > On 2016/07/04 at 19:23:56, michaelpg wrote: > > how does this lowercase? > > It does not need to. The regExp constructed below is case insensitive. Understood but the comment makes the code look wrong. :-) https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... File chrome/browser/resources/settings/settings_resources.grd (right): https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/settings_resources.grd:15: <structure name="IDR_SETTINGS_SEARCH_SETTINGS_JS" On 2016/07/06 18:24:55, dpapad wrote: > On 2016/07/04 at 19:23:56, michaelpg wrote: > > alphabetize > > Done, although I think the alphabetized version is messier. Now this entry is > wedged between entries of the on_startup_page/ folder. ok, looks like the rest of the file is a mess. Feel free to put back here, or maybe near IDR_SETTINGS_SETTINGS_HTML, or leave as is. https://codereview.chromium.org/2082793003/diff/500001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/500001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:92: wrapper.appendChild(span); is there a risk of the new <span> being styled unintentionally by a CSS selector ending in "span"? examples: https://cs.chromium.org/search/?q=file:settings+%5Csspan%5Cs%5C%7B%7C%5Csspan...
https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:19: var IGNORED_ELEMENTS = new Set([ > Perhaps you also want to add paper-slider, I don't think there's anything we want to match there. Done, added paper-slider. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:24: 'PAPER-ICON-BUTTON', > It'd be easier for developing and debugging if Search were as deterministic and reproducible as possible, at any step of the implementation. If that's impractical, then it's fine, but it seemed easy to add here. Added iron-list to the set. After a quick inspection it seems that we use it for dynamic data only. https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:178: // Generate search text by applying lowercase and escaping any characters On 2016/07/06 at 23:41:16, michaelpg wrote: > On 2016/07/06 18:24:55, dpapad wrote: > > On 2016/07/04 at 19:23:56, michaelpg wrote: > > > how does this lowercase? > > > > It does not need to. The regExp constructed below is case insensitive. > > Understood but the comment makes the code look wrong. :-) Updated comment (it was obsolete). https://codereview.chromium.org/2082793003/diff/500001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2082793003/diff/500001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:92: wrapper.appendChild(span); On 2016/07/06 at 23:41:16, michaelpg wrote: > is there a risk of the new <span> being styled unintentionally by a CSS selector ending in "span"? > > examples: https://cs.chromium.org/search/?q=file:settings+%5Csspan%5Cs%5C%7B%7C%5Csspan... Yes, I was actually able to reproduce one such case in the internet page, where the yellow rectangle has an unnecessary 10px margin. The actual list of potential collisions is smaller than the link you provided, see https://cs.chromium.org/search/?q=%5Csspan%5Cs%5C%7B%7C%5Csspan,$+file:%5Esrc.... I'll address those collisions in follow up CL. My priority is to get 1st and 2nd (asyncify search) iterations in, before fixing minor bugs.
The CQ bit was checked by dpapad@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dbeam@chromium.org, michaelpg@chromium.org Link to the patchset: https://codereview.chromium.org/2082793003/#ps520001 (title: "Address comments.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #11 (id:520001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== MD Settings: First iteration of searching within settings. This CL includes the core logic of the searching mechanism - Traversing the DOM tree (taking into account Shadow DOM). - Finding and highlighting matches (using familiar yellow rectangle for now). - Showing/hiding <settings-section> instances depending on search outcome. - Un-highlighting matches and restoring DOM structure. - Navigating to the default page (chrome://md-settings), when a search is triggered from any other page/sub-page. - Force rendering of the "advanced" page and update search results. Sub-pages that are not rendered are currently not searched. This will be addressed in follow up CLs along with multiple other issues. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: First iteration of searching within settings. This CL includes the core logic of the searching mechanism - Traversing the DOM tree (taking into account Shadow DOM). - Finding and highlighting matches (using familiar yellow rectangle for now). - Showing/hiding <settings-section> instances depending on search outcome. - Un-highlighting matches and restoring DOM structure. - Navigating to the default page (chrome://md-settings), when a search is triggered from any other page/sub-page. - Force rendering of the "advanced" page and update search results. Sub-pages that are not rendered are currently not searched. This will be addressed in follow up CLs along with multiple other issues. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/e18c3a8d876d941206fc600dea2681dab2d290d8 Cr-Commit-Position: refs/heads/master@{#404222} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/e18c3a8d876d941206fc600dea2681dab2d290d8 Cr-Commit-Position: refs/heads/master@{#404222}
Message was sent while issue was closed.
On 2016/07/07 17:34:12, dpapad wrote: > https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... > File chrome/browser/resources/settings/search_settings.js (right): > > https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:19: var IGNORED_ELEMENTS = > new Set([ > > Perhaps you also want to add paper-slider, I don't think there's anything we > want to match there. > > Done, added paper-slider. > > https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:24: 'PAPER-ICON-BUTTON', > > It'd be easier for developing and debugging if Search were as deterministic > and reproducible as possible, at any step of the implementation. If that's > impractical, then it's fine, but it seemed easy to add here. > > Added iron-list to the set. After a quick inspection it seems that we use it for > dynamic data only. > > https://codereview.chromium.org/2082793003/diff/480001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:178: // Generate search > text by applying lowercase and escaping any characters > On 2016/07/06 at 23:41:16, michaelpg wrote: > > On 2016/07/06 18:24:55, dpapad wrote: > > > On 2016/07/04 at 19:23:56, michaelpg wrote: > > > > how does this lowercase? > > > > > > It does not need to. The regExp constructed below is case insensitive. > > > > Understood but the comment makes the code look wrong. :-) > > Updated comment (it was obsolete). > > https://codereview.chromium.org/2082793003/diff/500001/chrome/browser/resourc... > File chrome/browser/resources/settings/search_settings.js (right): > > https://codereview.chromium.org/2082793003/diff/500001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:92: > wrapper.appendChild(span); > On 2016/07/06 at 23:41:16, michaelpg wrote: > > is there a risk of the new <span> being styled unintentionally by a CSS > selector ending in "span"? > > > > examples: > https://cs.chromium.org/search/?q=file:settings+%5Csspan%5Cs%5C%7B%7C%5Csspan... > > Yes, I was actually able to reproduce one such case in the internet page, where > the yellow rectangle has an unnecessary 10px margin. The actual list of > potential collisions is smaller than the link you provided, see > https://cs.chromium.org/search/?q=%5Csspan%5Cs%5C%7B%7C%5Csspan,$+file:%5Esrc.... > > I'll address those collisions in follow up CL. My priority is to get 1st and 2nd > (asyncify search) iterations in, before fixing minor bugs. sgtm |
