Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2103133007:
MD Settings: Second iteration of search within settings. (Closed)
|
Created:
4 years, 5 months ago by dpapad Modified:
4 years, 5 months ago Reviewers:
Dan Beam CC:
arv+watch_chromium.org, chromium-reviews, dbeam+watch-settings_chromium.org, michaelpg+watch-md-settings_chromium.org, michaelpg+watch-md-ui_chromium.org, michaelpg, stevenjb+watch-md-settings_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@cr_search_migration1 Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMD Settings: Second iteration of search within settings.
- Force render parts of the page that should be searched but
are currently not rendered.
- Ensuring that searching code yields often enough to the main
thread to prevent UI freeze.
Specifically
- Adding three types of tasks TopLevelSearchTask, SearchTask and
RenderTask.
- Adding a task manager class, which yields before executing a
task by using window.requestIdleCallback().
- Canceling obsolete tasks (happens when a new search is issued after the
task was posted but before it made it to the front of the queue).
BUG=608535
TEST=Navigate to chrome://md-settings. Search for a term that exists in
an unrendered sub-page, for example "engine". Navigate to the subpage by
clicking "Manage search engines". Notice that matches are magically
highlighted even though this subpage was not rendered when the search was
initiated.
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation
Committed: https://crrev.com/a153ff0d4ebfee359d511c0879ebbce1073e088a
Cr-Commit-Position: refs/heads/master@{#405232}
Patch Set 1 : Nits. #Patch Set 2 : Nit. #Patch Set 3 : Nit. #Patch Set 4 : Nits. #
Total comments: 30
Patch Set 5 : Addressing comments. #
Total comments: 25
Patch Set 6 : Nits. #
Total comments: 6
Patch Set 7 : Addressing more comments. #
Total comments: 40
Patch Set 8 : Addressing comments. #
Total comments: 12
Patch Set 9 : Addressing comments. #
Total comments: 4
Patch Set 10 : Address comments. #Messages
Total messages: 42 (19 generated)
Description was changed from ========== MD Settings: Second iteration of search within settings, WIP. Break down synchronous blocking tasks to smaller pieces such that the searching code yields often enough to the main thread to prevent UI freeze. BUG=608535 ========== to ========== MD Settings: Second iteration of search within settings, WIP. Break down synchronous blocking tasks to smaller pieces such that the searching code yields often enough to the main thread to prevent UI freeze. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Patchset #1 (id:1) has been deleted
Patchset #3 (id:60001) has been deleted
Patchset #2 (id:40001) has been deleted
Description was changed from ========== MD Settings: Second iteration of search within settings, WIP. Break down synchronous blocking tasks to smaller pieces such that the searching code yields often enough to the main thread to prevent UI freeze. BUG=608535 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Second iteration of search within settings, WIP. - Adding logic to force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding a task manager class. - Adding three types of tasks TopLevelSearch, SearchTask and RenderTask. - Yielding between tasks using requestIdleCallback() before executing a task. BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "Manage". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Second iteration of search within settings, WIP. - Adding logic to force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding a task manager class. - Adding three types of tasks TopLevelSearch, SearchTask and RenderTask. - Yielding between tasks using requestIdleCallback() before executing a task. BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "Manage". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Second iteration of search within settings, WIP. - Adding logic to force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding a task manager class. - Adding three types of tasks TopLevelSearch, SearchTask and RenderTask. - Yielding between tasks using requestIdleCallback() before executing a task. BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "Manage". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Second iteration of search within settings, WIP. - Adding logic to force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding a task manager class. - Adding three types of tasks TopLevelSearch, SearchTask and RenderTask. - Yielding between tasks using requestIdleCallback() before executing a task. BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "Manage". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Second iteration of search within settings, WIP. - Adding logic to force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding a task manager class. - Adding three types of tasks TopLevelSearch, SearchTask and RenderTask.- - Allowing only one in-flight task at any given time. - Yielding before executing a task by using window.requestIdleCallback(). - Canceling obsolete tasks (happens when a new search is issued after the task was posted but before it made it to the front of the queue). BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "Manage". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Second iteration of search within settings, WIP. - Adding logic to force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding a task manager class. - Adding three types of tasks TopLevelSearch, SearchTask and RenderTask.- - Allowing only one in-flight task at any given time. - Yielding before executing a task by using window.requestIdleCallback(). - Canceling obsolete tasks (happens when a new search is issued after the task was posted but before it made it to the front of the queue). BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "Manage". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Second iteration of search within settings, WIP. - Adding logic to force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding a task manager class. - Adding three types of tasks TopLevelSearch, SearchTask and RenderTask.- - Allowing only one in-flight task at any given time. - Yielding before executing a task by using window.requestIdleCallback(). - Canceling obsolete tasks (happens when a new search is issued after the task was posted but before it made it to the front of the queue). BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "engine". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
This is ready for review, except for two closure_compilation errors, still being worked on.
Description was changed from ========== MD Settings: Second iteration of search within settings, WIP. - Adding logic to force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding a task manager class. - Adding three types of tasks TopLevelSearch, SearchTask and RenderTask.- - Allowing only one in-flight task at any given time. - Yielding before executing a task by using window.requestIdleCallback(). - Canceling obsolete tasks (happens when a new search is issued after the task was posted but before it made it to the front of the queue). BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "engine". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Second iteration of search within settings. - Adding logic to force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding a task manager class. - Adding three types of tasks TopLevelSearch, SearchTask and RenderTask.- - Allowing only one in-flight task at any given time. - Yielding before executing a task by using window.requestIdleCallback(). - Canceling obsolete tasks (happens when a new search is issued after the task was posted but before it made it to the front of the queue). BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "engine". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Second iteration of search within settings. - Adding logic to force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding a task manager class. - Adding three types of tasks TopLevelSearch, SearchTask and RenderTask.- - Allowing only one in-flight task at any given time. - Yielding before executing a task by using window.requestIdleCallback(). - Canceling obsolete tasks (happens when a new search is issued after the task was posted but before it made it to the front of the queue). BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "engine". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Second iteration of search within settings. - Adding logic to force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding a task manager class. - Adding three types of tasks TopLevelSearchTask, SearchTask and RenderTask. - Allowing only one in-flight task at any given time. - Yielding before executing a task by using window.requestIdleCallback(). - Canceling obsolete tasks (happens when a new search is issued after the task was posted but before it made it to the front of the queue). BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "engine". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Second iteration of search within settings. - Adding logic to force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding a task manager class. - Adding three types of tasks TopLevelSearchTask, SearchTask and RenderTask. - Allowing only one in-flight task at any given time. - Yielding before executing a task by using window.requestIdleCallback(). - Canceling obsolete tasks (happens when a new search is issued after the task was posted but before it made it to the front of the queue). BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "engine". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Second iteration of search within settings. - Force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding a task manager class. - Adding three types of tasks TopLevelSearchTask, SearchTask and RenderTask. - Allowing only one in-flight task at any given time. - Yielding before executing a task by using window.requestIdleCallback(). - Canceling obsolete tasks (happens when a new search is issued after the task was posted but before it made it to the front of the queue). BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "engine". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
Description was changed from ========== MD Settings: Second iteration of search within settings. - Force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding a task manager class. - Adding three types of tasks TopLevelSearchTask, SearchTask and RenderTask. - Allowing only one in-flight task at any given time. - Yielding before executing a task by using window.requestIdleCallback(). - Canceling obsolete tasks (happens when a new search is issued after the task was posted but before it made it to the front of the queue). BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "engine". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Second iteration of search within settings. - Force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding three types of tasks TopLevelSearchTask, SearchTask and RenderTask. - Adding a task manager class, which yields before executing a task by using window.requestIdleCallback(). - Canceling obsolete tasks (happens when a new search is issued after the task was posted but before it made it to the front of the queue). BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "engine". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ==========
just some initial thoughts hope they make sense... https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:13: why \n\n? https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:131: new RenderTask(searchContext, node)); see below about "postExec" var queue = SearchManager.getInstance().queue_; var renderTask = new RenderTask(searchContext, node); renderTask.postExec = function(result) { queue.addTask(new SearchTask(searchContext, result)); }; queue.addTask(renderTask); https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:217: this.searchContext = searchContext; nit: this.context while there's no other context? https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:220: this.node = node; /** @type {function(?):void|undefined} */ this.postExec; https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:247: this.node._content.querySelector('settings-subpage'); _content :( https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:254: }.bind(this)); var subpageTemplate = this.node._content.querySelector('settings-subpage'); subpageTemplate.id = subpageTemplate.id || this.node.getAttribute('name'); assert(!this.node.if); this.node.if = true; var resolver = new PromiseResolver; this.node.parentNode.async(resolver.resolve); return resolver.promise; https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:288: }.bind(this)); findAndHighlightMatches_(this.searchContext, this.node); return Promise.resolve(); https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:327: this.highPriorityQueue_ = []; nit: this.queues_ = {high: [], middle: [], low: []}; https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:336: this.currentActiveTask_ = null; nit: why both current and active? just one or the other? https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:342: if (task instanceof TopLevelSearchTask) i think it'd be nice not to do these instanceof checks (i.e. addHighPriorityTask, addTask(PriorityEnum), if each task actually has its own |priority| member), but I don't know how slow instanceof actually is https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:367: return queue.splice(0, 1)[0]; return this.highPriorityQueue_.shift() || this.middlePriorityQueue_.shift() || this.lowPriorityQueue_.shift(); https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:375: if ((this.currentActiveTask_ = this.popNextTask_()) === null) if (this.currentActiveTask_) return; var manager = SearchManager.getInstance(); for (;;) { this.currentActiveTask_ = this.popNextTask_(); if (!this.currentActiveTask_) return; if (this.currentActiveTask_.context.id != manager.context.id) continue; this.currentActiveTask_.exec().then(function(result) { if (this.currentActiveTask_.postExec) this.currentActiveTask_.postExec(result); this.currentActiveTask_ = null; window.requestIdleCallback(this.consumePending_.bind(this)); }.bind(this)); break; } https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:400: execTask_: function(task) { why do we need this if it's only called once? why do we need to pass |task| if it's the same as |currentActiveTask_|? https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:405: this.addTask(new SearchTask(task.searchContext, result)); it'd be nice if the taskqueue didn't need to know what type of tasks it's running, i.e. we make a postExec member and here we do if (task.postExec) task.postExec(); https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:410: nit: why \n\n?
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:13: On 2016/07/09 at 01:36:07, Dan Beam wrote: > why \n\n? Done. https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:131: new RenderTask(searchContext, node)); On 2016/07/09 at 01:36:07, Dan Beam wrote: > see below about "postExec" > > > var queue = SearchManager.getInstance().queue_; > var renderTask = new RenderTask(searchContext, node); > renderTask.postExec = function(result) { > queue.addTask(new SearchTask(searchContext, result)); > }; > queue.addTask(renderTask); Addressed in a slightly different (I think simpler) way. Specifically I am adding the new SearchTask as part of the RenderTask exec(), and avoided introducing the concept of postTask. https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:217: this.searchContext = searchContext; On 2016/07/09 at 01:36:07, Dan Beam wrote: > nit: this.context while there's no other context? Renamed, s/searchContext/context s/activeSearchContext/activeContext https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:220: this.node = node; On 2016/07/09 at 01:36:07, Dan Beam wrote: > /** @type {function(?):void|undefined} */ > this.postExec; Acknowledged. https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:247: this.node._content.querySelector('settings-subpage'); On 2016/07/09 at 01:36:07, Dan Beam wrote: > _content :( It gets a bit worse (but don't know of a better alternative). I had to use quoted notation node_['_content'], to please the compiler. https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:254: }.bind(this)); On 2016/07/09 at 01:36:07, Dan Beam wrote: > var subpageTemplate = this.node._content.querySelector('settings-subpage'); > subpageTemplate.id = subpageTemplate.id || this.node.getAttribute('name'); > > assert(!this.node.if); > this.node.if = true; > > var resolver = new PromiseResolver; > this.node.parentNode.async(resolver.resolve); > return resolver.promise; Done. As said earlier, I am registering a new Search task inside RenderTask#exec(). I think is fine to make the RenderTask a bit smarter (by having it refer to SearchManager.getInstance().queue_.addTask here). https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:288: }.bind(this)); On 2016/07/09 at 01:36:07, Dan Beam wrote: > findAndHighlightMatches_(this.searchContext, this.node); > return Promise.resolve(); Done. https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:327: this.highPriorityQueue_ = []; On 2016/07/09 at 01:36:07, Dan Beam wrote: > nit: this.queues_ = {high: [], middle: [], low: []}; Done. https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:336: this.currentActiveTask_ = null; On 2016/07/09 at 01:36:07, Dan Beam wrote: > nit: why both current and active? just one or the other? Renamed to activeTask_. https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:342: if (task instanceof TopLevelSearchTask) On 2016/07/09 at 01:36:07, Dan Beam wrote: > i think it'd be nice not to do these instanceof checks (i.e. addHighPriorityTask, addTask(PriorityEnum), if each task actually has its own |priority| member), but I don't know how slow instanceof actually is Removed all instanceof checks in favor of three dedicated methods (addTopLevelSearchTask, addSearchTask, addRenderTask). https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:367: return queue.splice(0, 1)[0]; On 2016/07/09 at 01:36:07, Dan Beam wrote: > return this.highPriorityQueue_.shift() || this.middlePriorityQueue_.shift() || > this.lowPriorityQueue_.shift(); Done. https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:375: if ((this.currentActiveTask_ = this.popNextTask_()) === null) On 2016/07/09 at 01:36:07, Dan Beam wrote: > if (this.currentActiveTask_) > return; > > var manager = SearchManager.getInstance(); > for (;;) { > this.currentActiveTask_ = this.popNextTask_(); > if (!this.currentActiveTask_) > return; > > if (this.currentActiveTask_.context.id != manager.context.id) > continue; > > this.currentActiveTask_.exec().then(function(result) { > if (this.currentActiveTask_.postExec) > this.currentActiveTask_.postExec(result); > this.currentActiveTask_ = null; > window.requestIdleCallback(this.consumePending_.bind(this)); > }.bind(this)); > break; > } Done, with some modifications 1) Using while instead of for. 2) Always calling exec() within requestIdleCallback(). IIUC your suggestion only does that when consumePending_() is called by itself, but not from all the other places where it is called. 3) There is no postExec() stuff, (see other related comments). https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:400: execTask_: function(task) { On 2016/07/09 at 01:36:07, Dan Beam wrote: > why do we need this if it's only called once? > > why do we need to pass |task| if it's the same as |currentActiveTask_|? Removed. This is a left over from a previous version of the code where I allowed multiple tasks to run in parallel (no notion of currentActiveTask), so execTask_() was called with different task objects. https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:405: this.addTask(new SearchTask(task.searchContext, result)); On 2016/07/09 at 01:36:07, Dan Beam wrote: > it'd be nice if the taskqueue didn't need to know what type of tasks it's running, i.e. we make a postExec member and here we do > > if (task.postExec) > task.postExec(); Done, this part has been removed. TaskQueue is unaware of specific types. https://codereview.chromium.org/2103133007/diff/120001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:410: On 2016/07/09 at 01:36:07, Dan Beam wrote: > nit: why \n\n? Removed. Initially added because \n\n makes visual parsing easier (IMO).
https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:118: if (node.tagName != 'TEMPLATE' || if this is truly a node, tagName -> nodeName tagName is on Element https://developer.mozilla.org/en-US/docs/Web/API/Element/tagName https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:119: node.getAttribute('name') === null || !node.hasAttribute('name') ? https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:126: if (node.getAttribute('name').indexOf('site-') != -1) consider caching the result of getAttribute('name') if you don't end up using hasAttribute() (or maybe instead of that) https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:130: new RenderTask(context, node)); i might make sense to move this scheduling lower if (forceRenderNeeded_(context, node)) { SearchManager.getInstance().queue_.addRenderTask(context, node); return; } and drop the side effects / make this method const-er. I know the doc says it might schedule stuff, but I think this would be more generic and expected. https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:262: getRenderedNode_: function(id) { what is the point of making this a separate function? https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:313: findAndHighlightMatches_(this.context, this.node); nit: slightly less duplication var searchActive = this.context.regExp !== null; setSectionsVisibility_(this.node, !searchActive); if (searchActive) findAndHighlightMatches_(this.context, this.node); return Promise.resolve(); https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:347: nit: @param here as well https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:360: this.queues_.low.shift() || null; i don't really see the point of |null vs |undefined, but OK https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:374: if (this.activeTask_.context.id != manager.activeContext_.id) { why can't we drop all tasks when search() is called with new text? that'd simplify this code, right? https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:376: // has been issued since this task was submitted. nit: submitted -> queued https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:380: window.requestIdleCallback(function() { so if this is called with nothing to do, we still wait for an idle callback. why is this better than doing 1 thing and deferring /other/ things like i suggested? does this code imply even the search for something to do might take a while? https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:423: }; why don't we clear the queue right here?
https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:380: window.requestIdleCallback(function() { On 2016/07/11 21:37:05, Dan Beam wrote: > so if this is called with nothing to do, we still wait for an idle callback. > why is this better than doing 1 thing and deferring /other/ things like i > suggested? does this code imply even the search for something to do might take > a while? ignore this if running even 1 task synchronously causes the UI to pause
https://codereview.chromium.org/2103133007/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:332: this.activeTask_ = null; i understand why it's helpful to use a task here, but i might prefer a simple bool named |running_| or something, so that we discourage use of a global I'd far prefer to access tasks in local/private state and not count on globals still being around when we resume https://codereview.chromium.org/2103133007/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:366: if (this.activeTask_) if (this.running_) return; while (1) { var task = this.popNextTask_(); if (!task) { this.running_ = false; // Yes, I understand this is an extra step. return; } ... this.running_ = true; // And so is this. window.requestIdleCallback(function() { // There's no way |task| could change here, unlike this.activeTask_. task.exec().then(this.consumePending_.bind(this)); }.bind(this)); }
Addressed comments. PTAL https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:118: if (node.tagName != 'TEMPLATE' || On 2016/07/11 at 21:37:04, Dan Beam wrote: > if this is truly a node, tagName -> nodeName > > tagName is on Element > https://developer.mozilla.org/en-US/docs/Web/API/Element/tagName Done (here and elsewhere). Wondering why the compiler did not complain. https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:119: node.getAttribute('name') === null || On 2016/07/11 at 21:37:05, Dan Beam wrote: > !node.hasAttribute('name') ? Done. https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:126: if (node.getAttribute('name').indexOf('site-') != -1) On 2016/07/11 at 21:37:05, Dan Beam wrote: > consider caching the result of getAttribute('name') if you don't end up using hasAttribute() (or maybe instead of that) This is meant to be temporary. We should be searching site-settings subpages, so I am reluctant doing anything special here, since this if block should go away soon. https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:130: new RenderTask(context, node)); On 2016/07/11 at 21:37:05, Dan Beam wrote: > i might make sense to move this scheduling lower > > if (forceRenderNeeded_(context, node)) { > SearchManager.getInstance().queue_.addRenderTask(context, node); > return; > } > > and drop the side effects / make this method const-er. > > I know the doc says it might schedule stuff, but I think this would be more generic and expected. Done. https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:262: getRenderedNode_: function(id) { On 2016/07/11 at 21:37:05, Dan Beam wrote: > what is the point of making this a separate function? Inlined. It was more readable IMO. https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:313: findAndHighlightMatches_(this.context, this.node); On 2016/07/11 at 21:37:05, Dan Beam wrote: > nit: slightly less duplication > > var searchActive = this.context.regExp !== null; > setSectionsVisibility_(this.node, !searchActive); > if (searchActive) > findAndHighlightMatches_(this.context, this.node); > > return Promise.resolve(); Done. https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:347: On 2016/07/11 at 21:37:05, Dan Beam wrote: > nit: @param here as well Done. https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:360: this.queues_.low.shift() || null; On 2016/07/11 at 21:37:05, Dan Beam wrote: > i don't really see the point of |null vs |undefined, but OK Removed null. But since you asked explaining below why null was there. I should probably write a document instead of repeating my view on null VS undefined (have explained this in the past). Until I write that doc, the short version is as follows. null in JS indicates programmer's intention, whereas undefined can be intentional or accidental (bug). The latter combined with relaxed truthiness checks (implicit boolean conversions), like "if (foo) {}" instead of "if (foo !== null) {}" can hide bugs that otherwise would blow up immediately if null was used and no implicit boolean conversions. Ways to accidentally introduce undefined values 1) Typos: example window.helo instead of window.hello 2) Forgetting to return a value from a function. 3) Forgetting to pass an argument to a function. 4) Programmer's error about scope (expecting a variable to be in scope where it is not). So to sum up, I prefer having undefined values blow up the code, and use null were appropriate, because it increases the chances that a bug like 1-4 will be caught right away. https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:374: if (this.activeTask_.context.id != manager.activeContext_.id) { On 2016/07/11 at 21:37:05, Dan Beam wrote: > why can't we drop all tasks when search() is called with new text? that'd simplify this code, right? Done, see more detailed comment at line 423. https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:376: // has been issued since this task was submitted. On 2016/07/11 at 21:37:04, Dan Beam wrote: > nit: submitted -> queued Done. https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:380: window.requestIdleCallback(function() { On 2016/07/11 at 21:42:13, Dan Beam wrote: > On 2016/07/11 21:37:05, Dan Beam wrote: > > so if this is called with nothing to do, we still wait for an idle callback. > > why is this better than doing 1 thing and deferring /other/ things like i > > suggested? does this code imply even the search for something to do might take > > a while? > > ignore this if running even 1 task synchronously causes the UI to pause Yes, a single task could potentially freeze the UI. But also as discussed offline, let's always requestIdleCallback before calling exec(). The code is simpler to reason that way IMO, since every call to consumePending_() behaves the same, whether it was a recursive call, or a direct call. The previously proposed version is more complicated to reason about, because recursive consumePending_() calls happen within an idle callback (and therefore call exec() in the same idle callback), but direct calls do not call exec() in an idle callback. This complexity seems unnecessary. https://codereview.chromium.org/2103133007/diff/160001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:423: }; On 2016/07/11 at 21:37:05, Dan Beam wrote: > why don't we clear the queue right here? Done. This made me discover a small bug that I also fixed. After calling requestIdleCallback() and before the idle callback is called by the browser, the activeTask_ might have been obsoleted, so it is necessary to check again the context id before executing.
so if you were to write this code from scratch, and you divided the code up into 3 generally separate tasks searching highlighting rendering i doubt it'd result in as many static, stateless methods that take an argument of search text or node (which are already provided as context to a Task). I get that we wanted to land the sync version and iterate, but I don't really think it helps to /stay/ close to code that doesn't make as much sense any more (all the static methods of current HEAD) in this task-based, 3 class world. wdyt? https://codereview.chromium.org/2103133007/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:332: this.activeTask_ = null; On 2016/07/11 21:48:22, Dan Beam wrote: > i understand why it's helpful to use a task here, but i might prefer a simple > bool named |running_| or something, so that we discourage use of a global > > I'd far prefer to access tasks in local/private state and not count on globals > still being around when we resume ping https://codereview.chromium.org/2103133007/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:366: if (this.activeTask_) On 2016/07/11 21:48:22, Dan Beam wrote: > if (this.running_) > return; > > while (1) { > var task = this.popNextTask_(); > if (!task) { > this.running_ = false; // Yes, I understand this is an extra step. > return; > } > > ... > > this.running_ = true; // And so is this. > window.requestIdleCallback(function() { > // There's no way |task| could change here, unlike this.activeTask_. > task.exec().then(this.consumePending_.bind(this)); that's not to say that we don't need to check its validity (as you discovered ;)) > }.bind(this)); > } ping https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:10: * }} nit: /** @typedef {{id: number, rawQuery: ?string, regExp: ?RegExp}} */ https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:22: var SANITIZE_REGEX = /[-[\]{}()*+?.,\\^$|#\s]/g; can this live inside of SearchManager#search? https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:30: var IGNORED_ELEMENTS = new Set([ this should probably live inside of SearchTask, no? https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:55: function findAndRemoveHighlights_(node) { why is this used from only one place? it looks like you've pulled it out as a static method to reuse it from multiple places https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:88: function highlight_(node, tokens) { why is this used only once? https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:119: node.if) { nit: if (node.nodeName != 'TEMPLATE' || !node.hasAttribute('name') || node.if) return false; https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:128: return true; nit: return node.getAttribute('name').indexOf('site-') != 0; https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:187: function revealParentSection_(node) { why is this used only once? https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:203: function setSectionsVisibility_(page, visible) { why is this used only once? https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:242: RenderTask.prototype = { can you make this clearer that we're rendering dom-ifs in this class name? DomIfRenderer or something? https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:246: this.node['_content'].querySelector('settings-subpage'); why can't you use HTMLTemplateElement#content? https://github.com/google/closure-compiler/blob/f62f12046b063b2fd91ec7a860675... https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:258: new SearchTask(this.context, renderedNode)); I still think it'd be nice if RenderTask didn't need to know about SearchTask, but whatever https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:272: function SearchTask(context, node) { can this be a HighlightMatches task? https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:303: findAndHighlightMatches_(this.context, this.node); how is this part different than starting a SearchTask? https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:353: * @return {?Task} is this type still correct? https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:416: }; nit: fits in 1 line https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:426: if (this.activeContext_.rawQuery != text) { why are we doing anything if text == this.activeContext_.rawQuery? https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:438: }; nit: fits in 1 line?
Addressed comments (I think all of them). PTAL On 2016/07/12 00:08:43, Dan Beam wrote: > so if you were to write this code from scratch, and you divided the code up into > 3 generally separate tasks > > searching > highlighting > rendering > > i doubt it'd result in as many static, stateless methods that take an argument > of search text or node (which are already provided as context to a Task). > > I get that we wanted to land the sync version and iterate, but I don't really > think it helps to /stay/ close to code that doesn't make as much sense any more > (all the static methods of current HEAD) in this task-based, 3 class world. > > wdyt? Having core low-level logic inside top-level private (to search_settings.js scope) functions is very flexible, because this logic can be called from different places (for example TopLevelSearchTask and SearchTask call the same function) while we are still forming the final design. Stateless methods are also appealing to me (well, stateless if we ignore the fact that they modify the DOM), because it helps a lot when trying to understand the code. Having said that (per our discussion), I am fine gradually turning those functions to member methods where it makes sense. The latter requires us to settle on our Task sub-classes breakdown though, which we still have not fully done. Given that we talked about some potential drastic changes (making "search" tasks aware of a deadline), I prefer to keep the flexibility for now, and move those methods once our design is more finalized. > > https://codereview.chromium.org/2103133007/diff/180001/chrome/browser/resourc... > File chrome/browser/resources/settings/search_settings.js (right): > > https://codereview.chromium.org/2103133007/diff/180001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:332: this.activeTask_ = > null; > On 2016/07/11 21:48:22, Dan Beam wrote: > > i understand why it's helpful to use a task here, but i might prefer a simple > > bool named |running_| or something, so that we discourage use of a global > > > > I'd far prefer to access tasks in local/private state and not count on globals > > still being around when we resume > > ping > > https://codereview.chromium.org/2103133007/diff/180001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:366: if (this.activeTask_) > On 2016/07/11 21:48:22, Dan Beam wrote: > > if (this.running_) > > return; > > > > while (1) { > > var task = this.popNextTask_(); > > if (!task) { > > this.running_ = false; // Yes, I understand this is an extra step. > > return; > > } > > > > ... > > > > this.running_ = true; // And so is this. > > window.requestIdleCallback(function() { > > // There's no way |task| could change here, unlike this.activeTask_. > > task.exec().then(this.consumePending_.bind(this)); > > that's not to say that we don't need to check its validity (as you discovered > ;)) > > > }.bind(this)); > > } > > ping > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > File chrome/browser/resources/settings/search_settings.js (right): > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:10: * }} > nit: /** @typedef {{id: number, rawQuery: ?string, regExp: ?RegExp}} */ > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:22: var SANITIZE_REGEX = > /[-[\]{}()*+?.,\\^$|#\s]/g; > can this live inside of SearchManager#search? > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:30: var IGNORED_ELEMENTS = > new Set([ > this should probably live inside of SearchTask, no? > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:55: function > findAndRemoveHighlights_(node) { > why is this used from only one place? it looks like you've pulled it out as a > static method to reuse it from multiple places > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:88: function > highlight_(node, tokens) { > why is this used only once? > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:119: node.if) { > nit: > > if (node.nodeName != 'TEMPLATE' || !node.hasAttribute('name') || node.if) > return false; > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:128: return true; > nit: > > return node.getAttribute('name').indexOf('site-') != 0; > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:187: function > revealParentSection_(node) { > why is this used only once? > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:203: function > setSectionsVisibility_(page, visible) { > why is this used only once? > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:242: RenderTask.prototype = > { > can you make this clearer that we're rendering dom-ifs in this class name? > DomIfRenderer or something? > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:246: > this.node['_content'].querySelector('settings-subpage'); > why can't you use HTMLTemplateElement#content? > > https://github.com/google/closure-compiler/blob/f62f12046b063b2fd91ec7a860675... > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:258: new > SearchTask(this.context, renderedNode)); > I still think it'd be nice if RenderTask didn't need to know about SearchTask, > but whatever > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:272: function > SearchTask(context, node) { > can this be a HighlightMatches task? > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:303: > findAndHighlightMatches_(this.context, this.node); > how is this part different than starting a SearchTask? > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:353: * @return {?Task} > is this type still correct? > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:416: }; > nit: fits in 1 line > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:426: if > (this.activeContext_.rawQuery != text) { > why are we doing anything if text == this.activeContext_.rawQuery? > > https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:438: }; > nit: fits in 1 line?
https://codereview.chromium.org/2103133007/diff/180001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:332: this.activeTask_ = null; On 2016/07/12 at 00:08:41, Dan Beam wrote: > On 2016/07/11 21:48:22, Dan Beam wrote: > > i understand why it's helpful to use a task here, but i might prefer a simple > > bool named |running_| or something, so that we discourage use of a global > > > > I'd far prefer to access tasks in local/private state and not count on globals > > still being around when we resume > > ping Done. https://codereview.chromium.org/2103133007/diff/180001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:366: if (this.activeTask_) > ping Done. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:10: * }} On 2016/07/12 at 00:08:42, Dan Beam wrote: > nit: /** @typedef {{id: number, rawQuery: ?string, regExp: ?RegExp}} */ Done. I don't think this is consistent with other typedefs though (and I am not sure if styleguide is mandating about using as few lines as possible). https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:22: var SANITIZE_REGEX = /[-[\]{}()*+?.,\\^$|#\s]/g; On 2016/07/12 at 00:08:42, Dan Beam wrote: > can this live inside of SearchManager#search? Moved it to SearchManager.SANITIZE_REGEX. Trying to avoid generating a new RegExp object every time search() is called. Prior knowledge of mine suggests that generating RegExp objects is non negligible (requires parsing), have not tested this on a modern JS engine, but seems reasonable to cache the RegExp. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:30: var IGNORED_ELEMENTS = new Set([ On 2016/07/12 at 00:08:42, Dan Beam wrote: > this should probably live inside of SearchTask, no? This is accessed by the top level private function findAndHighLightMatches_, so putting it in SearchTask would be awkward. It could be moved if/when findAndHighLightMatches_ becomes a member function of SearchTask. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:55: function findAndRemoveHighlights_(node) { On 2016/07/12 00:08:43, Dan Beam wrote: > why is this used from only one place? it looks like you've pulled it out as a > static method to reuse it from multiple places As discussed, it is pulled into a function because it does a specific operation that is not trivial, and callers do not need to know the internal details (separation of concerns, encapsulation, abstraction). Added a TODO to potentially move it to a private member method (in a follow up CL). https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:88: function highlight_(node, tokens) { On 2016/07/12 00:08:42, Dan Beam wrote: > why is this used only once? Used only once because it is needed once. Reason it is a separate function: same answer as for findAndRemoveHighlights_. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:119: node.if) { On 2016/07/12 at 00:08:42, Dan Beam wrote: > nit: > > if (node.nodeName != 'TEMPLATE' || !node.hasAttribute('name') || node.if) > return false; Done. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:128: return true; On 2016/07/12 at 00:08:42, Dan Beam wrote: > nit: > > return node.getAttribute('name').indexOf('site-') != 0; Done. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:187: function revealParentSection_(node) { On 2016/07/12 at 00:08:43, Dan Beam wrote: > why is this used only once? It is used only once, because it is needed only once ;> Per discussion, I am not inlining this, because it seems more readable to keep it separately. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:203: function setSectionsVisibility_(page, visible) { On 2016/07/12 00:08:42, Dan Beam wrote: > why is this used only once? Inlined. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:242: RenderTask.prototype = { On 2016/07/12 00:08:42, Dan Beam wrote: > can you make this clearer that we're rendering dom-ifs in this class name? > DomIfRenderer or something? I added a description in the constructor. Given that this is the only "render" task class in this file, I prefer not to reflect more detail in the class name (there is no ambiguity within the context of this file). https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:246: this.node['_content'].querySelector('settings-subpage'); On 2016/07/12 at 00:08:42, Dan Beam wrote: > why can't you use HTMLTemplateElement#content? > > https://github.com/google/closure-compiler/blob/f62f12046b063b2fd91ec7a860675... They are not the same. I don't know the details of why they are not the same (or whether they are supposed to), but |_content| is what works here, |content| does not (tried this). Perhaps @michaelpg knows, since I got this code from https://cs.chromium.org/chromium/src/chrome/browser/resources/settings/settin.... https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:258: new SearchTask(this.context, renderedNode)); On 2016/07/12 at 00:08:42, Dan Beam wrote: > I still think it'd be nice if RenderTask didn't need to know about SearchTask, but whatever Acknowledged. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:272: function SearchTask(context, node) { On 2016/07/12 at 00:08:42, Dan Beam wrote: > can this be a HighlightMatches task? I find the proposed name confusing, for the following reason. "HighlightMatches" task implies in my mind that there is no searching involved. It makes me think that a SearchTask produced the result and a HighlightMatchesTask would take those results and highlight them. This is not the case. This task actually searches a subtree and highlights matches as they are found. If you dislike current name, how about renaming to SubLevelSearchTask, as opposed to TopLevelSearchTask? https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:303: findAndHighlightMatches_(this.context, this.node); On 2016/07/12 at 00:08:42, Dan Beam wrote: > how is this part different than starting a SearchTask? SearchTask and TopLevelSearchTask have a small overlap as you discovered. I thought about removing that overlap, but I decided not to do this. Even though I could simply post a SearchTask here, the task priorities would get more complex. Current model is very simple, where there are three types of task and each of them have a different priority. I would like to avoid having SearchTask instances that can have different priorities. Edit (after our discussion): Let's keep TopLevelSearchTask and SearchTask separate for this iteration, and experiment with the idea of merging them (as well as returning early from a SearchTask if time is up) in follow up CLs. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:353: * @return {?Task} On 2016/07/12 at 00:08:42, Dan Beam wrote: > is this type still correct? Compiler seems to accept it. I am not 100% if the compiler differentiates between {?Task} or {undefined|Task}. This is one more reason why I prefer using null to begin with. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:416: }; On 2016/07/12 at 00:08:42, Dan Beam wrote: > nit: fits in 1 line Done. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:426: if (this.activeContext_.rawQuery != text) { On 2016/07/12 at 00:08:42, Dan Beam wrote: > why are we doing anything if text == this.activeContext_.rawQuery? We are not. <cr-toolbar> guarantees that no event is fired if the text did not change. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:438: }; On 2016/07/12 at 00:08:42, Dan Beam wrote: > nit: fits in 1 line? Done.
Friendly ping.
https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:10: * }} On 2016/07/12 02:20:28, dpapad wrote: > On 2016/07/12 at 00:08:42, Dan Beam wrote: > > nit: /** @typedef {{id: number, rawQuery: ?string, regExp: ?RegExp}} */ > > Done. I don't think this is consistent with other typedefs though (and I am not > sure if styleguide is mandating about using as few lines as possible). blame english for reading from left to right rather than top to bottom as the first axis. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:272: function SearchTask(context, node) { On 2016/07/12 02:20:27, dpapad wrote: > On 2016/07/12 at 00:08:42, Dan Beam wrote: > > can this be a HighlightMatches task? > > I find the proposed name confusing, for the following reason. I find the existing name confusing that it doesn't mention DOM *mutation* will be involved > "HighlightMatches" > task implies in my mind that there is no searching involved. It makes me think > that a SearchTask produced the result and a HighlightMatchesTask would take > those results and highlight them. This is not the case. This task actually > searches a subtree and highlights matches as they are found. > > If you dislike current name, how about renaming to SubLevelSearchTask, as > opposed to TopLevelSearchTask? SearchAndHighlightTask, FindAndHighlightTask? ultimately I do think we may want to batch the times we *mutate* the DOM from when we *access* it https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:7: */ /** ... */ is actually 1 line ;) https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:324: this.queues_.low.length = 0; or just make this impl: this.queues_ = {high: [], middle: [], low: []}; and call reset() from the ctor? https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:346: * @return {?Task} isn't this technically !Task|undefined now? https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:377: if (this.isTaskObsolete_(task)) { when will this happen? https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:394: } function startNextTask() { this.running_ = false; this.consumePending_(); } if (this.isTaskValid_(assert(task)) task.exec().then(startNextTask.bind(this)); else startNextTask.call(this);
https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:422: if (this.activeContext_.rawQuery != text) { i'm still confused: you said that we'll never get another event that triggers settings.search(), so what is this code doing? why is it not just returning if searching for the same text? why is it just adding a TopLevelSearchTask?
lgtm
Thanks. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:10: * }} On 2016/07/13 at 01:26:11, Dan Beam wrote: > On 2016/07/12 02:20:28, dpapad wrote: > > On 2016/07/12 at 00:08:42, Dan Beam wrote: > > > nit: /** @typedef {{id: number, rawQuery: ?string, regExp: ?RegExp}} */ > > > > Done. I don't think this is consistent with other typedefs though (and I am not > > sure if styleguide is mandating about using as few lines as possible). > > blame > english > for > reading > from > left > to > right > rather > than > top > to > bottom > as > the > first > axis. [sarcasm follows] Thanks for the insightful answer, now I know who to blame (apparently Javascript typedef definition === English free form text). I am now wondering why natural language processing is considered a hard problem, should be trivial as my new found knowledge suggests. https://codereview.chromium.org/2103133007/diff/200001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:272: function SearchTask(context, node) { On 2016/07/13 at 01:26:11, Dan Beam wrote: > On 2016/07/12 02:20:27, dpapad wrote: > > On 2016/07/12 at 00:08:42, Dan Beam wrote: > > > can this be a HighlightMatches task? > > > > I find the proposed name confusing, for the following reason. > > I find the existing name confusing that it doesn't mention DOM *mutation* will be involved > > > "HighlightMatches" > > task implies in my mind that there is no searching involved. It makes me think > > that a SearchTask produced the result and a HighlightMatchesTask would take > > those results and highlight them. This is not the case. This task actually > > searches a subtree and highlights matches as they are found. > > > > If you dislike current name, how about renaming to SubLevelSearchTask, as > > opposed to TopLevelSearchTask? > > SearchAndHighlightTask, FindAndHighlightTask? Renamed to SearchAndHighlightTask. Technically with this change, TopLevelSearchTask should also be renamed to TopLevelSearchAndHighlight. > > ultimately I do think we may want to batch the times we *mutate* the DOM from when we *access* it The problem we are trying to solve is to not block the UI. Per my understanding, it does not matter if we separate searching from highlighting to different tasks. What matters is to do work within a fixed time slot, pause and yield to the browser, then continue. Interleaving/Separating search and highlight work seems orthogonal to the overall problem. https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:7: */ On 2016/07/13 at 01:26:12, Dan Beam wrote: > /** ... */ > > is actually 1 line ;) Done. https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:324: this.queues_.low.length = 0; On 2016/07/13 at 01:26:12, Dan Beam wrote: > or just make this impl: > > this.queues_ = {high: [], middle: [], low: []}; > > and call reset() from the ctor? Done. https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:346: * @return {?Task} On 2016/07/13 at 01:26:12, Dan Beam wrote: > isn't this technically !Task|undefined now? Done. As said, either way compiler did not complain. https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:377: if (this.isTaskObsolete_(task)) { On 2016/07/13 at 01:26:12, Dan Beam wrote: > when will this happen? Now that we are clearing the queues immediately, this can't happen. Removed and inlined the check below. https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:394: } On 2016/07/13 at 01:26:12, Dan Beam wrote: > function startNextTask() { > this.running_ = false; > this.consumePending_(); > } > > if (this.isTaskValid_(assert(task)) > task.exec().then(startNextTask.bind(this)); > else > startNextTask.call(this); Done (kept isTaskObsolete() though). https://codereview.chromium.org/2103133007/diff/220001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:422: if (this.activeContext_.rawQuery != text) { On 2016/07/13 at 01:28:18, Dan Beam wrote: > i'm still confused: you said that we'll never get another event that triggers settings.search(), so what is this code doing? why is it not just returning if searching for the same text? why is it just adding a TopLevelSearchTask? search() is called twice for every user initiated search, one for basic-page, one for advanced-page. The latter call does not change the search context, but still needs to execute lines 440-441.
https://codereview.chromium.org/2103133007/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:358: isTaskObsolete_: function(task) { did you mean to use this? https://codereview.chromium.org/2103133007/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:380: SearchManager.getInstance().activeContext_.id) { if you do end up using this, I think it'd be odd to see isTaskObsolete_: function(task) { return task.context.id != SearchManager.getInstance().activeContext_.id; }, if (!this.isTaskObsolete_(task)) because that's basically a triple equals (the first one is the !=, the second two are ! and obsolete is basically !current). isTaskObsolete_: function(task) { return task.context.id == SearchManager.getInstance().activeContext_.id; }, if (this.isTaskValid_(task)) has 0 negations. i don't feel super strongly, but I just don't understand why we're pushing for the former
On 2016/07/13 02:33:14, Dan Beam wrote: > https://codereview.chromium.org/2103133007/diff/240001/chrome/browser/resourc... > File chrome/browser/resources/settings/search_settings.js (right): > > https://codereview.chromium.org/2103133007/diff/240001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:358: isTaskObsolete_: > function(task) { > did you mean to use this? > > https://codereview.chromium.org/2103133007/diff/240001/chrome/browser/resourc... > chrome/browser/resources/settings/search_settings.js:380: > SearchManager.getInstance().activeContext_.id) { > if you do end up using this, I think it'd be odd to see > > isTaskObsolete_: function(task) { > return task.context.id != SearchManager.getInstance().activeContext_.id; > }, > > if (!this.isTaskObsolete_(task)) > > because that's basically a triple equals (the first one is the !=, the second > two are ! and obsolete is basically !current). > > isTaskObsolete_: function(task) { > return task.context.id == SearchManager.getInstance().activeContext_.id; > }, isTaskValid_: function(task) { ** > > if (this.isTaskValid_(task)) > > has 0 negations. > > i don't feel super strongly, but I just don't understand why we're pushing for > the former
https://codereview.chromium.org/2103133007/diff/240001/chrome/browser/resourc... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2103133007/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:358: isTaskObsolete_: function(task) { On 2016/07/13 at 02:33:14, Dan Beam wrote: > did you mean to use this? Deleted. I meant to delete this. I don't need this as a separate function anymore. https://codereview.chromium.org/2103133007/diff/240001/chrome/browser/resourc... chrome/browser/resources/settings/search_settings.js:380: SearchManager.getInstance().activeContext_.id) { On 2016/07/13 at 02:33:14, Dan Beam wrote: > if you do end up using this, I think it'd be odd to see > > isTaskObsolete_: function(task) { > return task.context.id != SearchManager.getInstance().activeContext_.id; > }, > > if (!this.isTaskObsolete_(task)) > > because that's basically a triple equals (the first one is the !=, the second two are ! and obsolete is basically !current). > > isTaskObsolete_: function(task) { > return task.context.id == SearchManager.getInstance().activeContext_.id; > }, > > if (this.isTaskValid_(task)) > > has 0 negations. > > i don't feel super strongly, but I just don't understand why we're pushing for the former Acknowledged. I am not using a separate function anymore.
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 Link to the patchset: https://codereview.chromium.org/2103133007/#ps260001 (title: "Address comments.")
The CQ bit was unchecked by dpapad@chromium.org
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...
Your CL relies on deprecated CQ feature(s): * Specifying master names without "master." prefix is deprecated: tryserver.chromium.linux. For more details, see http://crbug.com/617627.
Message was sent while issue was closed.
Committed patchset #10 (id:260001)
Message was sent while issue was closed.
CQ bit was unchecked.
Message was sent while issue was closed.
Description was changed from ========== MD Settings: Second iteration of search within settings. - Force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding three types of tasks TopLevelSearchTask, SearchTask and RenderTask. - Adding a task manager class, which yields before executing a task by using window.requestIdleCallback(). - Canceling obsolete tasks (happens when a new search is issued after the task was posted but before it made it to the front of the queue). BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "engine". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation ========== to ========== MD Settings: Second iteration of search within settings. - Force render parts of the page that should be searched but are currently not rendered. - Ensuring that searching code yields often enough to the main thread to prevent UI freeze. Specifically - Adding three types of tasks TopLevelSearchTask, SearchTask and RenderTask. - Adding a task manager class, which yields before executing a task by using window.requestIdleCallback(). - Canceling obsolete tasks (happens when a new search is issued after the task was posted but before it made it to the front of the queue). BUG=608535 TEST=Navigate to chrome://md-settings. Search for a term that exists in an unrendered sub-page, for example "engine". Navigate to the subpage by clicking "Manage search engines". Notice that matches are magically highlighted even though this subpage was not rendered when the search was initiated. CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:closure_compilation Committed: https://crrev.com/a153ff0d4ebfee359d511c0879ebbce1073e088a Cr-Commit-Position: refs/heads/master@{#405232} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/a153ff0d4ebfee359d511c0879ebbce1073e088a Cr-Commit-Position: refs/heads/master@{#405232} |
