Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1228183002:
cr.js: Replace usage of ES6 Map with Object. (Closed)
|
Created:
5 years, 5 months ago by James Hawkins Modified:
5 years, 5 months ago Reviewers:
Dan Beam CC:
chromium-reviews, oshima+watch_chromium.org, stuartmorgan, arv (Not doing code reviews) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptioncr.js: Replace usage of ES6 Map with Object.
This is necessary because some platforms (iOS) do not yet support this construct.
BUG=508318
Committed: https://crrev.com/956dfa5415736b20ce1a60bcdd807755fd44c43d
Cr-Commit-Position: refs/heads/master@{#338186}
Patch Set 1 #
Total comments: 5
Patch Set 2 : Fix jsdoc. #
Total comments: 7
Patch Set 3 : Fix jsdoc. #Messages
Total messages: 11 (2 generated)
jhawkins@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1228183002/diff/1/ui/webui/resources/js/cr.js File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/1228183002/diff/1/ui/webui/resources/js/cr.js... ui/webui/resources/js/cr.js:321: * @type {!Map<string, Function>} ^ fix https://codereview.chromium.org/1228183002/diff/1/ui/webui/resources/js/cr.js... ui/webui/resources/js/cr.js:376: for (var i = 0; i < listenerCallbacks.length; i++) { wait, this doesn't make much sense... chrome.send('name', argsThatTakeLonger, callbackForFirstArgs); chrome.send('name', differentArgsThatFinishSooner, callbackForSecondArgs); are both executed when the first 'name' callback returns? with the same result?
https://codereview.chromium.org/1228183002/diff/1/ui/webui/resources/js/cr.js File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/1228183002/diff/1/ui/webui/resources/js/cr.js... ui/webui/resources/js/cr.js:321: * @type {!Map<string, Function>} On 2015/07/09 21:03:27, Dan Beam wrote: > ^ fix Done. https://codereview.chromium.org/1228183002/diff/1/ui/webui/resources/js/cr.js... ui/webui/resources/js/cr.js:376: for (var i = 0; i < listenerCallbacks.length; i++) { On 2015/07/09 21:03:27, Dan Beam wrote: > wait, this doesn't make much sense... > > chrome.send('name', argsThatTakeLonger, callbackForFirstArgs); > chrome.send('name', differentArgsThatFinishSooner, callbackForSecondArgs); > > are both executed when the first 'name' callback returns? with the same result? The code you're commenting is the event listener code, which is different from the chrome.send w/ callback code (above). Is that intentional?
lgtm https://codereview.chromium.org/1228183002/diff/1/ui/webui/resources/js/cr.js File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/1228183002/diff/1/ui/webui/resources/js/cr.js... ui/webui/resources/js/cr.js:376: for (var i = 0; i < listenerCallbacks.length; i++) { On 2015/07/09 22:02:50, James Hawkins wrote: > On 2015/07/09 21:03:27, Dan Beam wrote: > > wait, this doesn't make much sense... > > > > chrome.send('name', argsThatTakeLonger, callbackForFirstArgs); > > chrome.send('name', differentArgsThatFinishSooner, callbackForSecondArgs); > > > > are both executed when the first 'name' callback returns? with the same > result? > > The code you're commenting is the event listener code, which is different from > the chrome.send w/ callback code (above). Is that intentional? ah, no. btw, i just saw the createUid() that handles this in sendWithCallback() https://codereview.chromium.org/1228183002/diff/20001/ui/webui/resources/js/c... File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/1228183002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:321: * @type {!Object<string, Function>} nit: Object<string is redundant (just Object<Function> is fine). there's no different. keys can only be strings in JS https://codereview.chromium.org/1228183002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:323: var chromeSendCallbackMap = Object.create(null); nit: I don't see any Object.prototype.* modification in cs.chromium.org and this should be basically equivalent to {} which is probably far more common (and shorter). https://codereview.chromium.org/1228183002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:363: * @type {!Object<string, Array<Function>>} same nit re: Object<string, -> Object<
https://codereview.chromium.org/1228183002/diff/20001/ui/webui/resources/js/c... File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/1228183002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:321: * @type {!Object<string, Function>} On 2015/07/09 22:11:15, Dan Beam wrote: > nit: Object<string is redundant (just Object<Function> is fine). there's no > different. keys can only be strings in JS Done. https://codereview.chromium.org/1228183002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:323: var chromeSendCallbackMap = Object.create(null); On 2015/07/09 22:11:15, Dan Beam wrote: > nit: I don't see any Object.prototype.* modification in http://cs.chromium.org and this > should be basically equivalent to {} which is probably far more common (and > shorter). According to MDN, {} is equivalent to Object.create(Object.prototype), and inheriting the prototype is what I was avoiding here. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... https://codereview.chromium.org/1228183002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:363: * @type {!Object<string, Array<Function>>} On 2015/07/09 22:11:15, Dan Beam wrote: > same nit re: Object<string, -> Object< Done.
lgtm https://codereview.chromium.org/1228183002/diff/20001/ui/webui/resources/js/c... File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/1228183002/diff/20001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:323: var chromeSendCallbackMap = Object.create(null); On 2015/07/09 22:19:50, James Hawkins wrote: > On 2015/07/09 22:11:15, Dan Beam wrote: > > nit: I don't see any Object.prototype.* modification in http://cs.chromium.org > and this > > should be basically equivalent to {} which is probably far more common (and > > shorter). > > According to MDN, {} is equivalent to Object.create(Object.prototype), and > inheriting the prototype is what I was avoiding here. > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... I get what you're saying, we just don't do that currently (which is why looked through chrome for prototype modification of Object) and we hopefully never will. at this level (in cr.js) it's probably the most likely of anywhere, so it's fine I just think it's unnecessary
The CQ bit was checked by jhawkins@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1228183002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/956dfa5415736b20ce1a60bcdd807755fd44c43d Cr-Commit-Position: refs/heads/master@{#338186} |
