Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2610013002:
[Devtools] Doctyped Object.js (Closed)
|
Created:
3 years, 11 months ago by allada Modified:
3 years, 11 months ago CC:
chromium-reviews, caseq+blink_chromium.org, lushnikov+blink_chromium.org, pfeldman+blink_chromium.org, apavlov+blink_chromium.org, devtools-reviews_chromium.org, blink-reviews, pfeldman, kozyatinskiy+blink_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Devtools] Doctyped Object.js
This is a code health patch to doctype devtools more with closure
compiler.
R=dgozman
BUG=None
Review-Url: https://codereview.chromium.org/2610013002
Cr-Commit-Position: refs/heads/master@{#442476}
Committed: https://chromium.googlesource.com/chromium/src/+/4376a2a8722814a5f3feaaa5f7b335beb5ad29df
Patch Set 1 #Patch Set 2 : [Devtools] Doctyped Object.js #
Total comments: 7
Patch Set 3 : changes #
Total comments: 11
Patch Set 4 : changes #
Depends on Patchset: Dependent Patchsets: Messages
Total messages: 24 (13 generated)
PTL
https://codereview.chromium.org/2610013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/Object.js (left): https://codereview.chromium.org/2610013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:171: eventInfo.eventTarget.off(eventInfo.eventType, eventInfo.method, eventInfo.receiver); Removing this is incorrect. https://codereview.chromium.org/2610013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/Object.js (right): https://codereview.chromium.org/2610013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:46: this._listeners = /** @type {!Map<symbol, !Array<!Common.Object._listenerCallbackTuple>>} */ (new Map()); I'm surprised you have to cast here. https://codereview.chromium.org/2610013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:108: if (!this._listeners) Let's add a line here var emitableListener = /** @type {function(!Common.Emitable)} */ (listener); to express that T must be emitable. https://codereview.chromium.org/2610013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:123: off(eventType, listener, thisObject) { ditto
Done. I added a new class: "Common.EventTarget.EventDescriptorStruct" because we require a class matching those properties in: "Common.EventTarget.removeEventListeners", and this is the way closure enforces properties to exist on classes/objects. https://codereview.chromium.org/2610013002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/Object.js (right): https://codereview.chromium.org/2610013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:46: this._listeners = /** @type {!Map<symbol, !Array<!Common.Object._listenerCallbackTuple>>} */ (new Map()); On 2017/01/04 18:07:53, dgozman wrote: > I'm surprised you have to cast here. There are no side affects, but closure silently doesn't like it if I don't do it. https://codereview.chromium.org/2610013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:108: if (!this._listeners) On 2017/01/04 18:07:53, dgozman wrote: > Let's add a line here > var emitableListener = /** @type {function(!Common.Emitable)} */ (listener); > to express that T must be emitable. Done. https://codereview.chromium.org/2610013002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:123: off(eventType, listener, thisObject) { On 2017/01/04 18:07:53, dgozman wrote: > ditto Done.
PTaL https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/Object.js (right): https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:101: * @param {function(new:Common.Emitable, ...)} eventType Nice little trick here to enforce T to be a Common.Emitable :-)
allada@chromium.org changed reviewers: + caseq@chromium.org, luoe@chromium.org
PTL
The CQ bit was checked by allada@chromium.org to run a CQ dry run
Dry run: 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
Dry run: This issue passed the CQ dry run.
lgtm with comments https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/Object.js (right): https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:101: * @param {function(new:Common.Emitable, ...)} eventType On 2017/01/04 22:03:54, allada wrote: > Nice little trick here to enforce T to be a Common.Emitable :-) Acknowledged. https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:111: this._listeners.set(eventType, []); This looks like the same case as line 49. Should they both be typed? https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:151: Common.Emitable = function() {}; Nit: Since emitter is spelled with 2 "t"s, I think this should be changed "Emittable".
https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/Object.js (right): https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:31: /** @type {(!Map<symbol, !Array<!Common.Object._listenerCallbackTuple>>|undefined)} */ Should be symbol|Emitable https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:46: this._listeners = /** @type {!Map<symbol, !Array<!Common.Object._listenerCallbackTuple>>} */ (new Map()); Let's not type here - should be enough in the constructor? https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:111: this._listeners.set(eventType, []); On 2017/01/09 19:07:00, luoe wrote: > This looks like the same case as line 49. Should they both be typed? I'd say we should not type them both. In fact, looks like closure didn't catch us using Common.Emitable as a symbol. Does it make sense to even type things now?
The CQ bit was checked by allada@chromium.org to run a CQ dry run
PTaL https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/devtools/front_end/common/Object.js (right): https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:31: /** @type {(!Map<symbol, !Array<!Common.Object._listenerCallbackTuple>>|undefined)} */ On 2017/01/09 21:46:18, dgozman wrote: > Should be symbol|Emitable Done. https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:46: this._listeners = /** @type {!Map<symbol, !Array<!Common.Object._listenerCallbackTuple>>} */ (new Map()); On 2017/01/09 21:46:18, dgozman wrote: > Let's not type here - should be enough in the constructor? Done. https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:111: this._listeners.set(eventType, []); On 2017/01/09 21:46:18, dgozman wrote: > On 2017/01/09 19:07:00, luoe wrote: > > This looks like the same case as line 49. Should they both be typed? > > I'd say we should not type them both. In fact, looks like closure didn't catch > us using Common.Emitable as a symbol. Does it make sense to even type things > now? Done. https://codereview.chromium.org/2610013002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/devtools/front_end/common/Object.js:151: Common.Emitable = function() {}; On 2017/01/09 19:07:00, luoe wrote: > Nit: Since emitter is spelled with 2 "t"s, I think this should be changed > "Emittable". Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by allada@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from luoe@chromium.org Link to the patchset: https://codereview.chromium.org/2610013002/#ps60001 (title: "changes")
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": 60001, "attempt_start_ts": 1484019229520910,
"parent_rev": "05f1d2ae71ddb73d9bab16ca5b1c29f1c5bffa71", "commit_rev":
"4376a2a8722814a5f3feaaa5f7b335beb5ad29df"}
Message was sent while issue was closed.
Description was changed from ========== [Devtools] Doctyped Object.js This is a code health patch to doctype devtools more with closure compiler. R=dgozman BUG=None ========== to ========== [Devtools] Doctyped Object.js This is a code health patch to doctype devtools more with closure compiler. R=dgozman BUG=None Review-Url: https://codereview.chromium.org/2610013002 Cr-Commit-Position: refs/heads/master@{#442476} Committed: https://chromium.googlesource.com/chromium/src/+/4376a2a8722814a5f3feaaa5f7b3... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/4376a2a8722814a5f3feaaa5f7b3... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
