Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1622663002:
WebUI: Replace cr.sendWithCallback with cr.sendWithPromise. (Closed)
DescriptionWebUI: Replacing cr.sendWithCallback with cr.sendWithPromise.
BUG=580633
Committed: https://crrev.com/09d947a9ce90972275ff96009396a2c166a5b5b8
Cr-Commit-Position: refs/heads/master@{#371448}
Patch Set 1 #Patch Set 2 : Migrating caller + cleanups. #
Total comments: 12
Patch Set 3 : Addressing comments #Patch Set 4 : Remove usage of Map for now. #
Total comments: 14
Patch Set 5 : Nits. #
Total comments: 6
Patch Set 6 : Addressing comments. #Patch Set 7 : Addressing comments: Removing new line before anonymous function. #
Total comments: 2
Messages
Total messages: 20 (7 generated)
Description was changed from ========== WebUI: Replace cr.sendWithCallback with cr.sendWithPromise, WIP. BUG= ========== to ========== WebUI: Replace cr.sendWithCallback with cr.sendWithPromise, WIP. BUG=580633 ==========
Patchset #2 (id:20001) has been deleted
Description was changed from ========== WebUI: Replace cr.sendWithCallback with cr.sendWithPromise, WIP. BUG=580633 ========== to ========== WebUI: Replacing cr.sendWithCallback with cr.sendWithPromise. BUG=580633 ==========
dpapad@chromium.org changed reviewers: + dbeam@chromium.org
https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/cr_test.html (left): https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/cr_test.html:282: function testSendWithCallback_PassesJSArgs() { Interesting fact. Both this function and following function had the exact same name, which means only the latter was ever executed during testing.
enjoy reverting back to not using Map! https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/cr_test.html (right): https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/cr_test.html:247: x, z, 'Should return a different object after clearing for testing'); nit: revert https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/cr_test.html:278: window['cr']['webUIListenerCallback']('fullscreen-enabled', true); we don't do renaming, also why can't this just be cr.webuiListenerCallback('fullscreen-enabled', true)? https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/webui_resource_async_browsertest.js (right): https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/webui_resource_async_browsertest.js:23: browsePreload: DUMMY_URL, where does this magic come from? https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/webui_resource_async_browsertest.js:61: window['cr']['webUIResponse'].apply(null, globalCallbackArgs); why can't this just be cr.webUIResponse.apply(cr, globalCallbackArgs);? https://codereview.chromium.org/1622663002/diff/40001/ui/webui/resources/js/c... File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/1622663002/diff/40001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:320: * @type {!Map<string, !Function>} pretty sure this breaks safari, which is why jhawkins@ reverted it to Object<> after also trying Map 6 month ago https://codereview.chromium.org/1228183002 https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje...
https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/cr_test.html (right): https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/cr_test.html:247: x, z, 'Should return a different object after clearing for testing'); On 2016/01/23 at 02:48:51, Dan Beam wrote: > nit: revert Reverting, but previous version violates styleguide. Argument lines must start on the same column (JS styleguide inherits this from C++ styleguide AFAIK). https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/cr_test.html:278: window['cr']['webUIListenerCallback']('fullscreen-enabled', true); On 2016/01/23 at 02:48:51, Dan Beam wrote: > we don't do renaming, also why can't this just be cr.webuiListenerCallback('fullscreen-enabled', true)? Done. No good reason (I was just trying to remove the executeFunctionByName, did not realize this can be even simpler). https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/webui_resource_async_browsertest.js (right): https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/webui_resource_async_browsertest.js:23: browsePreload: DUMMY_URL, On 2016/01/23 at 02:48:51, Dan Beam wrote: > where does this magic come from? https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/data/w... https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/webui_resource_async_browsertest.js:61: window['cr']['webUIResponse'].apply(null, globalCallbackArgs); On 2016/01/23 at 02:48:51, Dan Beam wrote: > why can't this just be cr.webUIResponse.apply(cr, globalCallbackArgs);? Done. https://codereview.chromium.org/1622663002/diff/40001/ui/webui/resources/js/c... File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/1622663002/diff/40001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:320: * @type {!Map<string, !Function>} On 2016/01/23 at 02:48:51, Dan Beam wrote: > pretty sure this breaks safari, which is why jhawkins@ reverted it to Object<> after also trying Map 6 month ago > > https://codereview.chromium.org/1228183002 > > https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Obje... Reverted. I guess by "Safari" you mean Webkit-based browsers (chrome on IOS)? I would like to understand a bit more about this, since I was planning to convert more places in our JS code to use Map where possible, so questions. - What version of Safari/Webkit do we care about? According to https://kangax.github.io/compat-table/es6/, IOS8/9 have most parts of Map implemented already. - Are we missing the trybot that would catch this error? https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:358: var id = methodName + '_' + createUid(); FYI, added an '_' symbol between methodName and the uid, because previous code had a corner case that would break it. Imagine the following scenario. Two chrome.send() message names exist 'foo1' and 'foo'. sendWithPromise('foo1') // inserts key 'foo11' to the map object. Now calling sendWithPromise('foo') 10 times also inserts key 'foo11' to the map (possibly overwriting the entry that corresponds to message name 'foo1'. New code would not have a problem since it would insert foo_11 instead.
looks pretty good to me https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... File chrome/test/data/webui/cr_test.html (right): https://codereview.chromium.org/1622663002/diff/40001/chrome/test/data/webui/... chrome/test/data/webui/cr_test.html:247: x, z, 'Should return a different object after clearing for testing'); On 2016/01/25 18:34:22, dpapad wrote: > On 2016/01/23 at 02:48:51, Dan Beam wrote: > > nit: revert > > Reverting, but previous version violates styleguide. Argument lines must start > on the same column (JS styleguide inherits this from C++ styleguide AFAIK). nah, it doesn't https://engdoc.corp.google.com/eng/doc/javascriptguide.xml?cl=head#Code_forma... Function arguments """When the function call is itself indented, you're free to start the 4-space indent relative to the beginning of the original statement or relative to the beginning of the current function call.""" https://codereview.chromium.org/1622663002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/webui_resource_async_browsertest.js (right): https://codereview.chromium.org/1622663002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/webui_resource_async_browsertest.js:40: var expectedChromeSendMessageName = 'echoMessage'; nit: CHROME_SEND_NAME https://codereview.chromium.org/1622663002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/webui_resource_async_browsertest.js:62: }); whenChromeSendCalled(CHROME_SEND_NAME).then(function(args) { ... }); https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:340: // If no response arguments were passed from C++, resolving the Promise resolve https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:341: // without any arguments, else wraping all response arguments in an array wrap https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:344: resolverFn(responseArgs.length == 0 ? undefined : responseArgs); why not just resolve with an empty array? https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:358: var id = methodName + '_' + createUid(); On 2016/01/25 18:34:22, dpapad wrote: > FYI, added an '_' symbol between methodName and the uid, because previous code > had a corner case that would break it. Imagine the following scenario. > > Two chrome.send() message names exist 'foo1' and 'foo'. > > sendWithPromise('foo1') // inserts key 'foo11' to the map object. > Now calling sendWithPromise('foo') 10 times also inserts key 'foo11' to the map > (possibly overwriting the entry that corresponds to message name 'foo1'. New > code would not have a problem since it would insert > foo_11 instead. Acknowledged.
https://codereview.chromium.org/1622663002/diff/80001/chrome/test/data/webui/... File chrome/test/data/webui/webui_resource_async_browsertest.js (right): https://codereview.chromium.org/1622663002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/webui_resource_async_browsertest.js:40: var expectedChromeSendMessageName = 'echoMessage'; On 2016/01/25 at 18:48:37, Dan Beam wrote: > nit: CHROME_SEND_NAME Done. https://codereview.chromium.org/1622663002/diff/80001/chrome/test/data/webui/... chrome/test/data/webui/webui_resource_async_browsertest.js:62: }); On 2016/01/25 at 18:48:37, Dan Beam wrote: > whenChromeSendCalled(CHROME_SEND_NAME).then(function(args) { > ... > }); Done. https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:340: // If no response arguments were passed from C++, resolving the Promise On 2016/01/25 at 18:48:37, Dan Beam wrote: > resolve Done. https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:341: // without any arguments, else wraping all response arguments in an array On 2016/01/25 at 18:48:37, Dan Beam wrote: > wrap Done. https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:344: resolverFn(responseArgs.length == 0 ? undefined : responseArgs); On 2016/01/25 at 18:48:37, Dan Beam wrote: > why not just resolve with an empty array? Partly to avoid creating empty arrays unnecessarily and partly because I thought it makes it clearer that no arguments were passed from C++. Another alternative I am considering is to restrict C++ calls of webUIResponse() to only pass the callback ID plus a single argument (primitive value or DictionaryValue/ListValue) such that we don't need to wrap multiple response aguments to an Array ourselves here. I think that is better since it is the most flexible. As of now even a primitive (say boolean) arrives at the caller within an array of length 1, see change in appearance_page.js.
https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:344: resolverFn(responseArgs.length == 0 ? undefined : responseArgs); On 2016/01/25 19:10:36, dpapad wrote: > On 2016/01/25 at 18:48:37, Dan Beam wrote: > > why not just resolve with an empty array? > > Partly to avoid creating empty arrays unnecessarily responseArgs is already created > and partly because I thought > it makes it clearer that no arguments were passed from C++. ehhhhh, I think it's more annoying to have to check the type of args. counting on it always being the same type (at least the wrapper) is less error prone, IMO > > Another alternative I am considering is to restrict C++ calls of webUIResponse() > to only pass the callback ID plus a single argument (primitive value or > DictionaryValue/ListValue) such that we don't need to wrap multiple response > aguments to an Array ourselves here. I think that is better since it is the most > flexible. As of now even a primitive (say boolean) arrives at the caller within > an array of length 1, see change in appearance_page.js. arguments <-> list manipulation is pretty common in JS, so I don't think one is preferred to the other. chrome.send() could easily be made to do chrome.send('msg', param1, param2) instead of chrome.send('msg', [param1, param2]), but it's not really a game changer https://codereview.chromium.org/1622663002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/1622663002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/appearance_page/appearance_page.js:130: }.bind(this)); cr.sendWithPromise('getResetThemeEnabled').then(function(response) { this.setResetThemeEnabled(response[0]); }.bind(this)); would save a line https://codereview.chromium.org/1622663002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/webui_resource_async_browsertest.js (right): https://codereview.chromium.org/1622663002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/webui_resource_async_browsertest.js:59: function(args) { please use the minimum, reasonable amount of lines so people can see more code at once
https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/1622663002/diff/80001/ui/webui/resources/js/c... ui/webui/resources/js/cr.js:344: resolverFn(responseArgs.length == 0 ? undefined : responseArgs); Changed it such that the array is always forwarded to the Promise. It is far more common to send multiple arguments in the request, than receive multiple arguments in the response. Wrapping arguments passed from C++ (which is the response) in an array seems unnecessary when in most cases C++ has already pre-packaged the response to an array or object such that only one argument is passed back. Either way, I think we can refine this point in a follow up CL if necessary. On 2016/01/25 at 23:13:38, Dan Beam wrote: > On 2016/01/25 19:10:36, dpapad wrote: > > On 2016/01/25 at 18:48:37, Dan Beam wrote: > > > why not just resolve with an empty array? > > > > Partly to avoid creating empty arrays unnecessarily > > responseArgs is already created > > > and partly because I thought > > it makes it clearer that no arguments were passed from C++. > > ehhhhh, I think it's more annoying to have to check the type of args. counting on it always being the same type (at least the wrapper) is less error prone, IMO > > > > > Another alternative I am considering is to restrict C++ calls of webUIResponse() > > to only pass the callback ID plus a single argument (primitive value or > > DictionaryValue/ListValue) such that we don't need to wrap multiple response > > aguments to an Array ourselves here. I think that is better since it is the most > > flexible. As of now even a primitive (say boolean) arrives at the caller within > > an array of length 1, see change in appearance_page.js. > > arguments <-> list manipulation is pretty common in JS, so I don't think one is preferred to the other. chrome.send() could easily be made to do chrome.send('msg', param1, param2) instead of chrome.send('msg', [param1, param2]), but it's not really a game changer https://codereview.chromium.org/1622663002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/1622663002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/appearance_page/appearance_page.js:130: }.bind(this)); On 2016/01/25 at 23:13:38, Dan Beam wrote: > cr.sendWithPromise('getResetThemeEnabled').then(function(response) { > this.setResetThemeEnabled(response[0]); > }.bind(this)); > > would save a line Breaking line before anonymous function is fairly common. Sometimes is required, because it allows adding @param annotations, and sometimes it is just for readability. I don't think there is a hard rule in the styleguide about one or the other, so can we consider this (and similar comment in webui_resource_async_browsertest.js) an optional nit? https://codereview.chromium.org/1622663002/diff/100001/chrome/test/data/webui... File chrome/test/data/webui/webui_resource_async_browsertest.js (right): https://codereview.chromium.org/1622663002/diff/100001/chrome/test/data/webui... chrome/test/data/webui/webui_resource_async_browsertest.js:59: function(args) { On 2016/01/25 at 23:13:38, Dan Beam wrote: > please use the minimum, reasonable amount of lines so people can see more code at once See response to similar comment in appearance_page.js.
https://codereview.chromium.org/1622663002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/1622663002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/appearance_page/appearance_page.js:130: }.bind(this)); On 2016/01/25 23:54:44, dpapad wrote: > On 2016/01/25 at 23:13:38, Dan Beam wrote: > > cr.sendWithPromise('getResetThemeEnabled').then(function(response) { > > this.setResetThemeEnabled(response[0]); > > }.bind(this)); > > > > would save a line > > Breaking line before anonymous function is fairly common. Sometimes is required, > because it allows adding @param annotations, and sometimes it is just for > readability. I don't think there is a hard rule in the styleguide about one or > the other, so can we consider this (and similar comment in > webui_resource_async_browsertest.js) an optional nit? n o
Patchset #7 (id:140001) has been deleted
https://codereview.chromium.org/1622663002/diff/100001/chrome/browser/resourc... File chrome/browser/resources/settings/appearance_page/appearance_page.js (right): https://codereview.chromium.org/1622663002/diff/100001/chrome/browser/resourc... chrome/browser/resources/settings/appearance_page/appearance_page.js:130: }.bind(this)); Done. Summarizing lengthy IM discussion: Changing this to unblock this CL. Personally I find that readability is improved when I can visually locate the start/end of a function faster, and that happens when \n is before "function". On 2016/01/25 at 23:56:43, Dan Beam wrote: > On 2016/01/25 23:54:44, dpapad wrote: > > On 2016/01/25 at 23:13:38, Dan Beam wrote: > > > cr.sendWithPromise('getResetThemeEnabled').then(function(response) { > > > this.setResetThemeEnabled(response[0]); > > > }.bind(this)); > > > > > > would save a line > > > > Breaking line before anonymous function is fairly common. Sometimes is required, > > because it allows adding @param annotations, and sometimes it is just for > > readability. I don't think there is a hard rule in the styleguide about one or > > the other, so can we consider this (and similar comment in > > webui_resource_async_browsertest.js) an optional nit? > > n > o
lgtm with either style, you can do what you want until we have an objective tie breaker https://codereview.chromium.org/1622663002/diff/160001/chrome/test/data/webui... File chrome/test/data/webui/webui_resource_async_browsertest.js (right): https://codereview.chromium.org/1622663002/diff/160001/chrome/test/data/webui... chrome/test/data/webui/webui_resource_async_browsertest.js:68: assertEquals(['foo', 'bar'].join(), response.join()); in this case I don't think it's suitable for me to object to this, even if it's not my personal style. it's hard for you to know when I'm *not* objecting to things that I would write differently. assume I'm staying reserved most of the time ;) https://codereview.chromium.org/1622663002/diff/160001/ui/webui/resources/js/... File ui/webui/resources/js/cr.js (right): https://codereview.chromium.org/1622663002/diff/160001/ui/webui/resources/js/... ui/webui/resources/js/cr.js:342: resolverFn(Array.prototype.slice.call(arguments, 1)); this seems saner and simpler, double win
The CQ bit was checked by dpapad@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1622663002/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1622663002/160001
Message was sent while issue was closed.
Committed patchset #7 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== WebUI: Replacing cr.sendWithCallback with cr.sendWithPromise. BUG=580633 ========== to ========== WebUI: Replacing cr.sendWithCallback with cr.sendWithPromise. BUG=580633 Committed: https://crrev.com/09d947a9ce90972275ff96009396a2c166a5b5b8 Cr-Commit-Position: refs/heads/master@{#371448} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/09d947a9ce90972275ff96009396a2c166a5b5b8 Cr-Commit-Position: refs/heads/master@{#371448} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
