Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2284663003:
DevTools: Use a Map for modelListeners in TargetManager (Closed)
|
Created:
4 years, 3 months ago by alph Modified:
4 years, 3 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. |
DescriptionDevTools: Use a Map for modelListeners in TargetManager
The Object used to store the listeners before does not support iteration
over Symbol keys.
BUG=641468
Committed: https://crrev.com/895cd721262c4fcf86ea35e5a3fdb92bb8b85fdd
Cr-Commit-Position: refs/heads/master@{#414816}
Patch Set 1 #
Total comments: 9
Messages
Total messages: 13 (6 generated)
The CQ bit was checked by alph@chromium.org to run a CQ dry run
alph@chromium.org changed reviewers: + dgozman@chromium.org, lushnikov@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm, thank you! https://codereview.chromium.org/2284663003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right): https://codereview.chromium.org/2284663003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:19: /** @type {!Map<symbol, !Array<{modelClass: !Function, thisObject: (!Object|undefined), listener: function(!WebInspector.Event)}>>} */ technically, this should be (symbol|string) https://codereview.chromium.org/2284663003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:118: * @param {symbol} eventType (symbol|string) as well https://codereview.chromium.org/2284663003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:136: * @param {symbol} eventType ditto https://codereview.chromium.org/2284663003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:260: for (var pair of this._modelListeners) { nit: this probably never compiles - there are no tuples in closure
https://codereview.chromium.org/2284663003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right): https://codereview.chromium.org/2284663003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:19: /** @type {!Map<symbol, !Array<{modelClass: !Function, thisObject: (!Object|undefined), listener: function(!WebInspector.Event)}>>} */ On 2016/08/26 19:34:45, lushnikov wrote: > technically, this should be (symbol|string) I hope we don't have strings anymore! Let's change to symbol everywhere.
Patchset #2 (id:20001) has been deleted
https://codereview.chromium.org/2284663003/diff/1/third_party/WebKit/Source/d... File third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js (right): https://codereview.chromium.org/2284663003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:19: /** @type {!Map<symbol, !Array<{modelClass: !Function, thisObject: (!Object|undefined), listener: function(!WebInspector.Event)}>>} */ On 2016/08/26 20:52:49, dgozman wrote: > On 2016/08/26 19:34:45, lushnikov wrote: > > technically, this should be (symbol|string) > > I hope we don't have strings anymore! Let's change to symbol everywhere. I'll keep it just symbol and make sure there are no strings in a separate patch. https://codereview.chromium.org/2284663003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:118: * @param {symbol} eventType On 2016/08/26 19:34:45, lushnikov wrote: > (symbol|string) as well ditto https://codereview.chromium.org/2284663003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:136: * @param {symbol} eventType On 2016/08/26 19:34:45, lushnikov wrote: > ditto ditto https://codereview.chromium.org/2284663003/diff/1/third_party/WebKit/Source/d... third_party/WebKit/Source/devtools/front_end/sdk/TargetManager.js:260: for (var pair of this._modelListeners) { On 2016/08/26 19:34:45, lushnikov wrote: > nit: this probably never compiles - there are no tuples in closure yeah. it seems to treat them as having value type.
The CQ bit was checked by alph@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #1 (id:1)
Message was sent while issue was closed.
Description was changed from ========== DevTools: Use a Map for modelListeners in TargetManager The Object used to store the listeners before does not support iteration over Symbol keys. BUG=641468 ========== to ========== DevTools: Use a Map for modelListeners in TargetManager The Object used to store the listeners before does not support iteration over Symbol keys. BUG=641468 Committed: https://crrev.com/895cd721262c4fcf86ea35e5a3fdb92bb8b85fdd Cr-Commit-Position: refs/heads/master@{#414816} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/895cd721262c4fcf86ea35e5a3fdb92bb8b85fdd Cr-Commit-Position: refs/heads/master@{#414816} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
