Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2940233002:
WebUI: Allow using ES6 classes in the styleguide. (Closed)
|
Created:
3 years, 6 months ago by dpapad Modified:
3 years, 6 months ago 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 Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionWebUI: Allow using ES6 classes in the styleguide.
As part of the same CL, I am converting search_settings.js to
use ES6 classes, to ensure that Closure compiler correctly handles it.
BUG=671426
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
Review-Url: https://codereview.chromium.org/2940233002
Cr-Commit-Position: refs/heads/master@{#480663}
Committed: https://chromium.googlesource.com/chromium/src/+/f2fb4313701e313ca6aa74682ac627d07e9bd172
Patch Set 1 #
Total comments: 2
Patch Set 2 : Remove static #Patch Set 3 : More #Patch Set 4 : Link to thread. #
Total comments: 2
Patch Set 5 : Add IOS note #
Total comments: 2
Patch Set 6 : Address comments. #Patch Set 7 : Resolve conflicts. #
Messages
Total messages: 32 (13 generated)
Description was changed from ========== MD Settings: Convert search_settings.js to use ES6 classes. BUG=671426 ========== to ========== MD Settings: Convert search_settings.js to use ES6 classes. BUG=671426 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
tbreisacher@chromium.org changed reviewers: + tbreisacher@chromium.org
https://codereview.chromium.org/2940233002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2940233002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/search_settings.js:554: SearchRequest.SANITIZE_REGEX_ = /[-[\]{}()*+?.,\\^$|#\s]/g; nit: Since this isn't Java, you don't have to make everything a property of a class. I would recommend making this just a local variable: let SANITIZE_REGEX = ... and then it's "private" automatically just because it's only accessible in this function
I'll update the ES6 styleguide to allow ES6 classes, and as part of this CL and will re-ping. https://codereview.chromium.org/2940233002/diff/1/chrome/browser/resources/se... File chrome/browser/resources/settings/search_settings.js (right): https://codereview.chromium.org/2940233002/diff/1/chrome/browser/resources/se... chrome/browser/resources/settings/search_settings.js:554: SearchRequest.SANITIZE_REGEX_ = /[-[\]{}()*+?.,\\^$|#\s]/g; On 2017/06/15 at 22:48:37, Tyler Breisacher (Chromium) wrote: > nit: Since this isn't Java, you don't have to make everything a property of a class. I would recommend making this just a local variable: > > let SANITIZE_REGEX = ... > > and then it's "private" automatically just because it's only accessible in this function Good suggestion, done. I used 'var' for now, since I prefer not to mix var with let. I plan to convert these once let/const have been whitelisted in our ES6 styleguide.
Description was changed from ========== MD Settings: Convert search_settings.js to use ES6 classes. BUG=671426 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Allow using ES6 classes in the styleguide. As part of the same CL, I am converting search_settings.js to use ES6 classes, to ensure that Closure compiler correctly handles it. BUG=671426 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ==========
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
maybe link to the chromium-dev@ discussion on this topic? is there anywhere class doesn't work? iOS9?
https://codereview.chromium.org/2940233002/diff/60001/docs/es6_chromium.md File docs/es6_chromium.md (right): https://codereview.chromium.org/2940233002/diff/60001/docs/es6_chromium.md#ne... docs/es6_chromium.md:219: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/S1h-0m2ohOw/jyaiM... nvm, i found this
@eugenebut: Do you know if ES6 classes work everywhere where Chrome runs on IOS? https://codereview.chromium.org/2940233002/diff/60001/docs/es6_chromium.md File docs/es6_chromium.md (right): https://codereview.chromium.org/2940233002/diff/60001/docs/es6_chromium.md#ne... docs/es6_chromium.md:219: https://groups.google.com/a/chromium.org/d/msg/chromium-dev/S1h-0m2ohOw/jyaiM... On 2017/06/16 at 00:43:41, Dan Beam wrote: > nvm, i found this Ack.
On 2017/06/16 00:52:01, dpapad wrote: > @eugenebut: Do you know if ES6 classes work everywhere where Chrome runs on IOS? http://kangax.github.io/compat-table/es6/#test-class
On 2017/06/16 at 01:00:36, dbeam wrote: > On 2017/06/16 00:52:01, dpapad wrote: > > @eugenebut: Do you know if ES6 classes work everywhere where Chrome runs on IOS? > > http://kangax.github.io/compat-table/es6/#test-class Added note about iOS9 and ES6 classes.
On 2017/06/16 01:33:43, dpapad wrote: > On 2017/06/16 at 01:00:36, dbeam wrote: > > On 2017/06/16 00:52:01, dpapad wrote: > > > @eugenebut: Do you know if ES6 classes work everywhere where Chrome runs on > IOS? > > > > http://kangax.github.io/compat-table/es6/#test-class > > Added note about iOS9 and ES6 classes. Thanks for the note. Do you know if iOS10 fully supports ES6 classes? If so, then we can add a TODO(crbug.com/bug) to remove this comment once iOS 9 support is dropped.
phistuck@gmail.com changed reviewers: + phistuck@gmail.com
https://codereview.chromium.org/2940233002/diff/80001/docs/es6_chromium.md File docs/es6_chromium.md (right): https://codereview.chromium.org/2940233002/diff/80001/docs/es6_chromium.md#ne... docs/es6_chromium.md:221: **Note**: => does not work in iOS9. Don't use it in code that runs on Chrome for Remove "=>". (Or replace with "classes")
On 2017/06/16 at 03:44:10, eugenebut wrote: > On 2017/06/16 01:33:43, dpapad wrote: > > On 2017/06/16 at 01:00:36, dbeam wrote: > > > On 2017/06/16 00:52:01, dpapad wrote: > > > > @eugenebut: Do you know if ES6 classes work everywhere where Chrome runs on > > IOS? > > > > > > http://kangax.github.io/compat-table/es6/#test-class > > > > Added note about iOS9 and ES6 classes. > Thanks for the note. Do you know if iOS10 fully supports ES6 classes? If so, then we can add a TODO(crbug.com/bug) to remove this comment once iOS 9 support is dropped. Added note. According to http://kangax.github.io/compat-table/es6/#test-class, ES6 classes are fully supported on iOS10.
https://codereview.chromium.org/2940233002/diff/80001/docs/es6_chromium.md File docs/es6_chromium.md (right): https://codereview.chromium.org/2940233002/diff/80001/docs/es6_chromium.md#ne... docs/es6_chromium.md:221: **Note**: => does not work in iOS9. Don't use it in code that runs on Chrome for On 2017/06/16 at 14:53:09, PhistucK wrote: > Remove "=>". > (Or replace with "classes") Done.
lgtm
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...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for chrome/browser/resources/settings/search_settings.js:
While running git apply --index -3 -p1;
error: patch failed: chrome/browser/resources/settings/search_settings.js:365
Falling back to three-way merge...
Applied patch to 'chrome/browser/resources/settings/search_settings.js' with
conflicts.
U chrome/browser/resources/settings/search_settings.js
Patch: chrome/browser/resources/settings/search_settings.js
Index: chrome/browser/resources/settings/search_settings.js
diff --git a/chrome/browser/resources/settings/search_settings.js
b/chrome/browser/resources/settings/search_settings.js
index
ad541dd43c290f012d82080607789645da34f083..d1884bb6fb6cbaa2599548c15ee9519202cc826b
100644
--- a/chrome/browser/resources/settings/search_settings.js
+++ b/chrome/browser/resources/settings/search_settings.js
@@ -260,45 +260,43 @@ cr.define('settings', function() {
highlightAssociatedControl_(associatedControl, rawQuery);
}
- /**
- * @constructor
- *
- * @param {!settings.SearchRequest} request
- * @param {!Node} node
- */
- function Task(request, node) {
- /** @protected {!settings.SearchRequest} */
- this.request = request;
+ /** @abstract */
+ class Task {
+ /**
+ * @param {!settings.SearchRequest} request
+ * @param {!Node} node
+ */
+ constructor(request, node) {
+ /** @protected {!settings.SearchRequest} */
+ this.request = request;
- /** @protected {!Node} */
- this.node = node;
- }
+ /** @protected {!Node} */
+ this.node = node;
+ }
- Task.prototype = {
/**
* @abstract
* @return {!Promise}
*/
- exec: function() {},
- };
-
- /**
- * A task that takes a <template is="dom-if">...</template> node
corresponding
- * to a setting subpage and renders it. A SearchAndHighlightTask is posted
for
- * the newly rendered subtree, once rendering is done.
- * @constructor
- * @extends {Task}
- *
- * @param {!settings.SearchRequest} request
- * @param {!Node} node
- */
- function RenderTask(request, node) {
- Task.call(this, request, node);
+ exec() {}
}
- RenderTask.prototype = {
+ class RenderTask extends Task {
+ /**
+ * A task that takes a <template is="dom-if">...</template> node
+ * corresponding to a setting subpage and renders it. A
+ * SearchAndHighlightTask is posted for the newly rendered subtree, once
+ * rendering is done.
+ *
+ * @param {!settings.SearchRequest} request
+ * @param {!Node} node
+ */
+ constructor(request, node) {
+ super(request, node);
+ }
+
/** @override */
- exec: function() {
+ exec() {
var routePath = this.node.getAttribute('route-path');
var subpageTemplate =
this.node['_content'].querySelector('settings-subpage');
@@ -318,43 +316,37 @@ cr.define('settings', function() {
resolve();
}.bind(this));
}.bind(this));
- },
- };
-
- /**
- * @constructor
- * @extends {Task}
- *
- * @param {!settings.SearchRequest} request
- * @param {!Node} node
- */
- function SearchAndHighlightTask(request, node) {
- Task.call(this, request, node);
+ }
}
- SearchAndHighlightTask.prototype = {
+ class SearchAndHighlightTask extends Task {
+ /**
+ * @param {!settings.SearchRequest} request
+ * @param {!Node} node
+ */
+ constructor(request, node) {
+ super(request, node);
+ }
+
/** @override */
- exec: function() {
+ exec() {
var foundMatches = findAndHighlightMatches_(this.request, this.node);
this.request.updateMatches(foundMatches);
return Promise.resolve();
- },
- };
-
- /**
- * @constructor
- * @extends {Task}
- *
- * @param {!settings.SearchRequest} request
- * @param {!Node} page
- */
- function TopLevelSearchTask(request, page) {
- Task.call(this, request, page);
+ }
}
- TopLevelSearchTask.prototype = {
+ class TopLevelSearchTask extends Task {
+ /**
+ * @param {!settings.SearchRequest} request
+ * @param {!Node} page
+ */
+ constructor(request, page) {
+ super(request, page);
+ }
+
/** @override */
- exec: function() {
+ exec() {
findAndRemoveHighlights_(this.node);
var shouldSearch = this.request.regExp !== null;
@@ -365,92 +357,89 @@ cr.define('settings', function() {
}
return Promise.resolve();
- },
+ }
/**
* @param {boolean} visible
* @private
*/
- setSectionsVisibility_: function(visible) {
+ setSectionsVisibility_(visible) {
var sections = this.node.querySelectorAll('settings-section');
for (var i = 0; i < sections.length; i++)
sections[i].hiddenBySearch = !visible;
- },
- };
-
- /**
- * @constructor
- * @param {!settings.SearchRequest} request
- */
- function TaskQueue(request) {
- /** @private {!settings.SearchRequest} */
- this.request_ = request;
-
- /**
- * @private {{
- * high: !Array<!Task>,
- * middle: !Array<!Task>,
- * low: !Array<!Task>
- * }}
- */
- this.queues_;
- this.reset();
-
- /** @private {?Function} */
- this.onEmptyCallback_ = null;
-
- /**
- * Whether a task is currently running.
- * @private {boolean}
- */
- this.running_ = false;
+ }
}
- TaskQueue.prototype = {
+ class TaskQueue {
+ /** @param {!settings.SearchRequest} request */
+ constructor(request) {
+ /** @private {!settings.SearchRequest} */
+ this.request_ = request;
+
+ /**
+ * @private {{
+ * high: !Array<!Task>,
+ * middle: !Array<!Task>,
+ * low: !Array<!Task>
+ * }}
+ */
+ this.queues_;
+ this.reset();
+
+ /** @private {?Function} */
+ this.onEmptyCallback_ = null;
+
+ /**
+ * Whether a task is currently running.
+ * @private {boolean}
+ */
+ this.running_ = false;
+ }
+
/** Drops all tasks. */
- reset: function() {
+ reset() {
this.queues_ = {high: [], middle: [], low: []};
- },
+ }
/** @param {!TopLevelSearchTask} task */
- addTopLevelSearchTask: function(task) {
+ addTopLevelSearchTask(task) {
this.queues_.high.push(task);
this.consumePending_();
- },
+ }
/** @param {!SearchAndHighlightTask} task */
- addSearchAndHighlightTask: function(task) {
+ addSearchAndHighlightTask(task) {
this.queues_.middle.push(task);
this.consumePending_();
- },
+ }
/** @param {!RenderTask} task */
- addRenderTask: function(task) {
+ addRenderTask(task) {
this.queues_.low.push(task);
this.consumePending_();
- },
+ }
/**
* Registers a callback to be called every time the queue becomes empty.
* @param {function():void} onEmptyCallback
*/
- onEmpty: function(onEmptyCallback) {
+ onEmpty(onEmptyCallback) {
this.onEmptyCallback_ = onEmptyCallback;
- },
+ }
/**
* @return {!Task|undefined}
* @private
*/
- popNextTask_: function() {
+ popNextTask_() {
return this.queues_.high.shift() ||
this.queues_.middle.shift() ||
this.queues_.low.shift();
- },
+ }
/** @private */
- consumePending_: function() {
+ consumePending_() {
if (this.running_)
return;
@@ -476,125 +465,116 @@ cr.define('settings', function() {
}.bind(this));
return;
}
- },
- };
-
- /**
- * @constructor
- *
- * @param {string} rawQuery
- * @param {!HTMLElement} root
- */
- var SearchRequest = function(rawQuery, root) {
- /** @private {string} */
- this.rawQuery_ = rawQuery;
-
- /** @private {!HTMLElement} */
- this.root_ = root;
-
- /** @type {?RegExp} */
- this.regExp = this.generateRegExp_();
+ }
+ }
+ class SearchRequest {
/**
- * Whether this request was canceled before completing.
- * @type {boolean}
+ * @param {string} rawQuery
+ * @param {!HTMLElement} root
*/
- this.canceled = false;
+ constructor(rawQuery, root) {
+ /** @private {string} */
+ this.rawQuery_ = rawQuery;
- /** @private {boolean} */
- this.foundMatches_ = false;
+ /** @private {!HTMLElement} */
+ this.root_ = root;
- /** @type {!PromiseResolver} */
- this.resolver = new PromiseResolver();
+ /** @type {?RegExp} */
+ this.regExp = this.generateRegExp_();
- /** @private {!TaskQueue} */
- this.queue_ = new TaskQueue(this);
- this.queue_.onEmpty(function() {
- this.resolver.resolve(this);
- }.bind(this));
- };
+ /**
+ * Whether this request was canceled before completing.
+ * @type {boolean}
+ */
+ this.canceled = false;
+
+ /** @private {boolean} */
+ this.foundMatches_ = false;
- /** @private {!RegExp} */
- SearchRequest.SANITIZE_REGEX_ = /[-[\]{}()*+?.,\\^$|#\s]/g;
+ /** @type {!PromiseResolver} */
+ this.resolver = new PromiseResolver();
+
+ /** @private {!TaskQueue} */
+ this.queue_ = new TaskQueue(this);
+ this.queue_.onEmpty(function() {
+ this.resolver.resolve(this);
+ }.bind(this));
+ }
- SearchRequest.prototype = {
/**
* Fires this search request.
*/
- start: function() {
+ start() {
this.queue_.addTopLevelSearchTask(
new TopLevelSearchTask(this, this.root_));
- },
+ }
/**
* @return {?RegExp}
* @private
*/
- generateRegExp_: function() {
+ generateRegExp_() {
var regExp = null;
// Generate search text by escaping any characters that would be
// problematic for regular expressions.
- var searchText = this.rawQuery_.trim().replace(
- SearchRequest.SANITIZE_REGEX_, '\\$&');
+ var searchText = this.rawQuery_.trim().replace(SANITIZE_REGEX, …
(message too large)
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/2940233002/#ps120001 (title: "Resolve conflicts.")
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: ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
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...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1497914833834910,
"parent_rev": "fccdd655843ff9deb920c6541a773d63b7c8f670", "commit_rev":
"f2fb4313701e313ca6aa74682ac627d07e9bd172"}
Message was sent while issue was closed.
Description was changed from ========== WebUI: Allow using ES6 classes in the styleguide. As part of the same CL, I am converting search_settings.js to use ES6 classes, to ensure that Closure compiler correctly handles it. BUG=671426 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation ========== to ========== WebUI: Allow using ES6 classes in the styleguide. As part of the same CL, I am converting search_settings.js to use ES6 classes, to ensure that Closure compiler correctly handles it. BUG=671426 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2940233002 Cr-Commit-Position: refs/heads/master@{#480663} Committed: https://chromium.googlesource.com/chromium/src/+/f2fb4313701e313ca6aa74682ac6... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/f2fb4313701e313ca6aa74682ac6... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
