Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1324263003:
Blimp: create MessageDispatcher class. (Closed)
DescriptionBlimp: create MessageDispatcher class.
This class routes incoming BlimpMessages to registered handlers based on the messages' types.
R=wez@chromium.org,nyquist@chromium.org
BUG=525766
Committed: https://crrev.com/1b78a36c7edf36f97214600b83e715291146c01c
Cr-Commit-Position: refs/heads/master@{#353924}
Patch Set 1 #Patch Set 2 : Use ostream message for error message #Patch Set 3 : Add unit tests; refactor BUILD.gns #
Total comments: 4
Patch Set 4 : Restore BUILD files, disable compositor code that won't compile. #Patch Set 5 : Addressed nyquist's feedback #
Total comments: 6
Patch Set 6 : Add Blimp common unittests to buildbot waterfalls #
Total comments: 2
Patch Set 7 : Added test entry to tryserver.chromium.linux.json #Patch Set 8 : Remove testonly annotation #
Total comments: 6
Patch Set 9 : Remove tabs from multiplexing key #Patch Set 10 : Resync with master #
Total comments: 9
Patch Set 11 : Fix tryserver entries. #
Total comments: 4
Patch Set 12 : Addressed dpranke's feedback #
Total comments: 17
Patch Set 13 : Addressed wez/nyquist feedback #
Total comments: 9
Patch Set 14 : Addressed wez's feedback. #
Total comments: 21
Patch Set 15 : Addressed wez's feedback #Patch Set 16 : Resync with master #Patch Set 17 : Update buildbot targets #Patch Set 18 : Resync with master. #Patch Set 19 : Remove build rules from common #Patch Set 20 : Add GYP "_run" compatibility rule. #Patch Set 21 : Resolve naming collision for "net" #
Messages
Total messages: 63 (15 generated)
Will add unit tests to this CL if the overall code LGTY.
Depends on CL #1310103003 ("Initial commit of Blimp remote protos")
Added unit tests. Also refactored the BUILDs to isolate the components better - doing so allows me to build and test code in "net" (for example) under Linux without the build running into unmet Android UI dependencies.
Impeding patch! After talking with nyquist@, I decided to create another CL to separate the dispatcher code from the build refactor.
https://codereview.chromium.org/1324263003/diff/40001/blimp/common/net/messag... File blimp/common/net/message_dispatcher.cc (right): https://codereview.chromium.org/1324263003/diff/40001/blimp/common/net/messag... blimp/common/net/message_dispatcher.cc:14: std::string BlimpMessageToString(const BlimpMessage& message) { Nit: Could we make it clear that this is meant to be used for debugging, and not for serialization by changing its name somewhat? https://codereview.chromium.org/1324263003/diff/40001/blimp/common/net/messag... blimp/common/net/message_dispatcher.cc:29: if (feature_handler_map_.find(type) == feature_handler_map_.end()) { Nit: Should we log an error if there already exists a handler? Seems like a helpful error message should we ever do that by mistake. If you end up adding that, following the pattern of early return like below would be handy.
https://codereview.chromium.org/1324263003/diff/40001/blimp/common/net/messag... File blimp/common/net/message_dispatcher.cc (right): https://codereview.chromium.org/1324263003/diff/40001/blimp/common/net/messag... blimp/common/net/message_dispatcher.cc:14: std::string BlimpMessageToString(const BlimpMessage& message) { On 2015/09/08 17:30:12, nyquist wrote: > Nit: Could we make it clear that this is meant to be used for debugging, and not > for serialization by changing its name somewhat? Done. https://codereview.chromium.org/1324263003/diff/40001/blimp/common/net/messag... blimp/common/net/message_dispatcher.cc:29: if (feature_handler_map_.find(type) == feature_handler_map_.end()) { On 2015/09/08 17:30:12, nyquist wrote: > Nit: Should we log an error if there already exists a handler? Seems like a > helpful error message should we ever do that by mistake. If you end up adding > that, following the pattern of early return like below would be handy. Done.
OK, I undid the BUILD refactoring and just commented out the sections of the compositor code that were breaking the build. Let's get things running on the buildbots soon. :)
https://codereview.chromium.org/1324263003/diff/80001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1324263003/diff/80001/BUILD.gn#newcode620 BUILD.gn:620: "//blimp:unit_tests", Could you add this to: //testing/buildbot/chromium.linux.json under 'Android GN' and 'Linux GN'? And then also add this to the BUG= line: 525766
https://codereview.chromium.org/1324263003/diff/80001/blimp/common/compositor... File blimp/common/compositor/blimp_layer_tree_settings.cc (right): https://codereview.chromium.org/1324263003/diff/80001/blimp/common/compositor... blimp/common/compositor/blimp_layer_tree_settings.cc:242: NOTIMPLEMENTED() << "Unresolved dependency TODO for native_theme."; This should be unnecessary now.
https://codereview.chromium.org/1324263003/diff/80001/blimp/BUILD.gn File blimp/BUILD.gn (right): https://codereview.chromium.org/1324263003/diff/80001/blimp/BUILD.gn#newcode6 blimp/BUILD.gn:6: group("unit_tests") { Could this be blimp_tests ? Or if you want to be super-fancy, add another group for that, but that feels like overkill.
https://codereview.chromium.org/1324263003/diff/80001/BUILD.gn File BUILD.gn (right): https://codereview.chromium.org/1324263003/diff/80001/BUILD.gn#newcode620 BUILD.gn:620: "//blimp:unit_tests", On 2015/09/10 00:18:48, nyquist wrote: > Could you add this to: > //testing/buildbot/chromium.linux.json > > under 'Android GN' and 'Linux GN'? > > And then also add this to the BUG= line: > 525766 Done From what I can tell, I can only add test executable targets to the Buildbot JSON, not group targets, so I did just that. https://codereview.chromium.org/1324263003/diff/80001/blimp/BUILD.gn File blimp/BUILD.gn (right): https://codereview.chromium.org/1324263003/diff/80001/blimp/BUILD.gn#newcode6 blimp/BUILD.gn:6: group("unit_tests") { On 2015/09/10 17:24:28, nyquist wrote: > Could this be blimp_tests ? > > Or if you want to be super-fancy, add another group for that, but that feels > like overkill. Done. https://codereview.chromium.org/1324263003/diff/80001/blimp/common/compositor... File blimp/common/compositor/blimp_layer_tree_settings.cc (right): https://codereview.chromium.org/1324263003/diff/80001/blimp/common/compositor... blimp/common/compositor/blimp_layer_tree_settings.cc:242: NOTIMPLEMENTED() << "Unresolved dependency TODO for native_theme."; On 2015/09/10 03:47:33, nyquist wrote: > This should be unnecessary now. Done.
https://codereview.chromium.org/1324263003/diff/100001/testing/buildbot/chrom... File testing/buildbot/chromium.linux.json (right): https://codereview.chromium.org/1324263003/diff/100001/testing/buildbot/chrom... testing/buildbot/chromium.linux.json:11: "test": "blimp_common_unit_tests" Could we add these lines to tryserver.chromium.linux.json as well?
blimp lgtm
Done. Will hold off until LGTWez. https://codereview.chromium.org/1324263003/diff/100001/testing/buildbot/chrom... File testing/buildbot/chromium.linux.json (right): https://codereview.chromium.org/1324263003/diff/100001/testing/buildbot/chrom... testing/buildbot/chromium.linux.json:11: "test": "blimp_common_unit_tests" On 2015/09/10 18:19:39, nyquist wrote: > Could we add these lines to tryserver.chromium.linux.json as well? Done.
https://codereview.chromium.org/1324263003/diff/140001/blimp/BUILD.gn File blimp/BUILD.gn (right): https://codereview.chromium.org/1324263003/diff/140001/blimp/BUILD.gn#newcode17 blimp/BUILD.gn:17: "common:blimp_common_unit_tests", I see now that the style of this is different than in the group above. Should we be consistent with //blimp/... prefix for everything that's not local?
https://codereview.chromium.org/1324263003/diff/140001/testing/buildbot/chrom... File testing/buildbot/chromium.linux.json (right): https://codereview.chromium.org/1324263003/diff/140001/testing/buildbot/chrom... testing/buildbot/chromium.linux.json:275: "test": "blimp_common_unit_tests" Could you add to Linux GN (dbg) as well? https://codereview.chromium.org/1324263003/diff/140001/testing/buildbot/tryse... File testing/buildbot/tryserver.chromium.linux.json (right): https://codereview.chromium.org/1324263003/diff/140001/testing/buildbot/tryse... testing/buildbot/tryserver.chromium.linux.json:162: "blimp", Could you add to linux_chromium_gn_dbg as well? (same for blimp_common_unit_tests below)
Also removed tab ID from multiplexing key. Starting simple. We can add it later if we need it. :) https://codereview.chromium.org/1324263003/diff/140001/blimp/BUILD.gn File blimp/BUILD.gn (right): https://codereview.chromium.org/1324263003/diff/140001/blimp/BUILD.gn#newcode17 blimp/BUILD.gn:17: "common:blimp_common_unit_tests", On 2015/09/10 20:12:09, nyquist wrote: > I see now that the style of this is different than in the group above. Should we > be consistent with //blimp/... prefix for everything that's not local? Done. https://codereview.chromium.org/1324263003/diff/140001/testing/buildbot/chrom... File testing/buildbot/chromium.linux.json (right): https://codereview.chromium.org/1324263003/diff/140001/testing/buildbot/chrom... testing/buildbot/chromium.linux.json:275: "test": "blimp_common_unit_tests" On 2015/09/10 21:56:47, nyquist wrote: > Could you add to Linux GN (dbg) as well? Done. https://codereview.chromium.org/1324263003/diff/140001/testing/buildbot/tryse... File testing/buildbot/tryserver.chromium.linux.json (right): https://codereview.chromium.org/1324263003/diff/140001/testing/buildbot/tryse... testing/buildbot/tryserver.chromium.linux.json:162: "blimp", On 2015/09/10 21:56:48, nyquist wrote: > Could you add to linux_chromium_gn_dbg as well? (same for > blimp_common_unit_tests below) Done.
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/1324263003/#ps180001 (title: "Resync with master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324263003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324263003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
kmarshall@chromium.org changed reviewers: + dpranke@chromium.org
https://codereview.chromium.org/1324263003/diff/180001/testing/buildbot/tryse... File testing/buildbot/tryserver.chromium.linux.json (right): https://codereview.chromium.org/1324263003/diff/180001/testing/buildbot/tryse... testing/buildbot/tryserver.chromium.linux.json:186: "test": "blimp_common_unit_tests" I think you can move this to the real Android Tests bots now (both release and debug)
Dirk: friendly ping for buildbot configs.
https://chromiumcodereview.appspot.com/1324263003/diff/180001/testing/buildbo... File testing/buildbot/tryserver.chromium.linux.json (right): https://chromiumcodereview.appspot.com/1324263003/diff/180001/testing/buildbo... testing/buildbot/tryserver.chromium.linux.json:186: "test": "blimp_common_unit_tests" On 2015/09/25 22:21:29, nyquist wrote: > I think you can move this to the real Android Tests bots now (both release and > debug) What are the names of the bots?
https://chromiumcodereview.appspot.com/1324263003/diff/180001/testing/buildbo... File testing/buildbot/tryserver.chromium.linux.json (right): https://chromiumcodereview.appspot.com/1324263003/diff/180001/testing/buildbo... testing/buildbot/tryserver.chromium.linux.json:186: "test": "blimp_common_unit_tests" On 2015/10/01 00:02:36, Kevin M wrote: > On 2015/09/25 22:21:29, nyquist wrote: > > I think you can move this to the real Android Tests bots now (both release and > > debug) > > What are the names of the bots? See here: https://codereview.chromium.org/1365823002/ Android GN Android GN (dbg) android_chromium_gn_compile_dbg android_chromium_gn_compile_rel Basically, add to both tryservers and normal servers. Preferrably both for linux and android on both. The main linux bots now run GN by default, so no need to use the GN-specific bots for linux.
sorry for the delay, somehow I didn't see this patch until I was pinged. https://chromiumcodereview.appspot.com/1324263003/diff/180001/BUILD.gn File BUILD.gn (right): https://chromiumcodereview.appspot.com/1324263003/diff/180001/BUILD.gn#newcod... BUILD.gn:619: "//blimp", weren't we going to delete the "//blimp" line, here? https://chromiumcodereview.appspot.com/1324263003/diff/180001/blimp/common/pr... File blimp/common/proto/BUILD.gn (right): https://chromiumcodereview.appspot.com/1324263003/diff/180001/blimp/common/pr... blimp/common/proto/BUILD.gn:7: static_library("proto") { can this just be a group()? why is it a static_library? https://chromiumcodereview.appspot.com/1324263003/diff/180001/testing/buildbo... File testing/buildbot/chromium.linux.json (right): https://chromiumcodereview.appspot.com/1324263003/diff/180001/testing/buildbo... testing/buildbot/chromium.linux.json:278: "test": "blimp_common_unit_tests" We should not be adding anything to the "Linux GN" bots, because they're going away. Adding them to "Linux Tests" (which you're doing) is sufficient. https://chromiumcodereview.appspot.com/1324263003/diff/180001/testing/buildbo... File testing/buildbot/tryserver.chromium.linux.json (right): https://chromiumcodereview.appspot.com/1324263003/diff/180001/testing/buildbo... testing/buildbot/tryserver.chromium.linux.json:186: "test": "blimp_common_unit_tests" On 2015/10/01 00:07:13, nyquist wrote: > On 2015/10/01 00:02:36, Kevin M wrote: > > On 2015/09/25 22:21:29, nyquist wrote: > > > I think you can move this to the real Android Tests bots now (both release > and > > > debug) > > > > What are the names of the bots? > > See here: https://codereview.chromium.org/1365823002/ > > Android GN > Android GN (dbg) > android_chromium_gn_compile_dbg > android_chromium_gn_compile_rel > > Basically, add to both tryservers and normal servers. Preferrably both for linux > and android on both. > > The main linux bots now run GN by default, so no need to use the GN-specific > bots for linux. Right, do not add these lines. The linux_chromium_gn_rel bots are going away, because they're now redundant with the linux_chromium_rel_ng bots. (You don't need to add entries for linux_chromium_rel_ng; it automatically uses the entries in "Linux Tests" on chromium.linux).
https://chromiumcodereview.appspot.com/1324263003/diff/180001/testing/buildbo... File testing/buildbot/tryserver.chromium.linux.json (right): https://chromiumcodereview.appspot.com/1324263003/diff/180001/testing/buildbo... testing/buildbot/tryserver.chromium.linux.json:186: "test": "blimp_common_unit_tests" On 2015/10/01 00:14:03, Dirk Pranke wrote: > On 2015/10/01 00:07:13, nyquist wrote: > > On 2015/10/01 00:02:36, Kevin M wrote: > > > On 2015/09/25 22:21:29, nyquist wrote: > > > > I think you can move this to the real Android Tests bots now (both release > > and > > > > debug) > > > > > > What are the names of the bots? > > > > See here: https://codereview.chromium.org/1365823002/ > > > > Android GN > > Android GN (dbg) > > android_chromium_gn_compile_dbg > > android_chromium_gn_compile_rel > > > > Basically, add to both tryservers and normal servers. Preferrably both for > linux > > and android on both. > > > > The main linux bots now run GN by default, so no need to use the GN-specific > > bots for linux. > > Right, do not add these lines. The linux_chromium_gn_rel bots are going away, > because they're now redundant with the linux_chromium_rel_ng bots. (You don't > need to add entries for linux_chromium_rel_ng; it automatically uses the entries > in "Linux Tests" on chromium.linux). OK, done. nyquist@: It doesn't appear that Android GN (dbg) has any gtest targets at the moment? It seems odd that I'd be creating a new test list for it.
https://chromiumcodereview.appspot.com/1324263003/diff/180001/testing/buildbo... File testing/buildbot/tryserver.chromium.linux.json (right): https://chromiumcodereview.appspot.com/1324263003/diff/180001/testing/buildbo... testing/buildbot/tryserver.chromium.linux.json:186: "test": "blimp_common_unit_tests" On 2015/10/01 00:23:01, Kevin M wrote: > On 2015/10/01 00:14:03, Dirk Pranke wrote: > > On 2015/10/01 00:07:13, nyquist wrote: > > > On 2015/10/01 00:02:36, Kevin M wrote: > > > > On 2015/09/25 22:21:29, nyquist wrote: > > > > > I think you can move this to the real Android Tests bots now (both > release > > > and > > > > > debug) > > > > > > > > What are the names of the bots? > > > > > > See here: https://codereview.chromium.org/1365823002/ > > > > > > Android GN > > > Android GN (dbg) > > > android_chromium_gn_compile_dbg > > > android_chromium_gn_compile_rel > > > > > > Basically, add to both tryservers and normal servers. Preferrably both for > > linux > > > and android on both. > > > > > > The main linux bots now run GN by default, so no need to use the GN-specific > > > bots for linux. > > > > Right, do not add these lines. The linux_chromium_gn_rel bots are going away, > > because they're now redundant with the linux_chromium_rel_ng bots. (You don't > > need to add entries for linux_chromium_rel_ng; it automatically uses the > entries > > in "Linux Tests" on chromium.linux). > > OK, done. > > nyquist@: It doesn't appear that Android GN (dbg) has any gtest targets at the > moment? It seems odd that I'd be creating a new test list for it. That's super odd. 'Android GN', does in fact have tests: http://build.chromium.org/p/chromium.linux/builders/Android%20GN/builds/30846 I don't feel strongly either way I guess.
On 2015/10/01 00:26:54, nyquist wrote: > https://chromiumcodereview.appspot.com/1324263003/diff/180001/testing/buildbo... > File testing/buildbot/tryserver.chromium.linux.json (right): > > https://chromiumcodereview.appspot.com/1324263003/diff/180001/testing/buildbo... > testing/buildbot/tryserver.chromium.linux.json:186: "test": > "blimp_common_unit_tests" > On 2015/10/01 00:23:01, Kevin M wrote: > > On 2015/10/01 00:14:03, Dirk Pranke wrote: > > > On 2015/10/01 00:07:13, nyquist wrote: > > > > On 2015/10/01 00:02:36, Kevin M wrote: > > > > > On 2015/09/25 22:21:29, nyquist wrote: > > > > > > I think you can move this to the real Android Tests bots now (both > > release > > > > and > > > > > > debug) > > > > > > > > > > What are the names of the bots? > > > > > > > > See here: https://codereview.chromium.org/1365823002/ > > > > > > > > Android GN > > > > Android GN (dbg) > > > > android_chromium_gn_compile_dbg > > > > android_chromium_gn_compile_rel > > > > > > > > Basically, add to both tryservers and normal servers. Preferrably both for > > > linux > > > > and android on both. > > > > > > > > The main linux bots now run GN by default, so no need to use the > GN-specific > > > > bots for linux. > > > > > > Right, do not add these lines. The linux_chromium_gn_rel bots are going > away, > > > because they're now redundant with the linux_chromium_rel_ng bots. (You > don't > > > need to add entries for linux_chromium_rel_ng; it automatically uses the > > entries > > > in "Linux Tests" on chromium.linux). > > > > OK, done. > > > > nyquist@: It doesn't appear that Android GN (dbg) has any gtest targets at the > > moment? It seems odd that I'd be creating a new test list for it. > > That's super odd. 'Android GN', does in fact have tests: > http://build.chromium.org/p/chromium.linux/builders/Android%20GN/builds/30846 > Android GN does actually have devices attached. Android GN (dbg) does not. We should file a ticket to replace the existing bot w/ one that does have devices.
https://chromiumcodereview.appspot.com/1324263003/diff/200001/blimp/common/pr... File blimp/common/proto/BUILD.gn (right): https://chromiumcodereview.appspot.com/1324263003/diff/200001/blimp/common/pr... blimp/common/proto/BUILD.gn:7: static_library("proto") { my earlier question about why this isn't a group() still stands. https://chromiumcodereview.appspot.com/1324263003/diff/200001/testing/buildbo... File testing/buildbot/chromium.linux.json (right): https://chromiumcodereview.appspot.com/1324263003/diff/200001/testing/buildbo... testing/buildbot/chromium.linux.json:277: "test": "blimp_common_unit_tests" don't add this entry, as per my earlier feedback.
https://codereview.chromium.org/1324263003/diff/200001/blimp/common/proto/BUI... File blimp/common/proto/BUILD.gn (right): https://codereview.chromium.org/1324263003/diff/200001/blimp/common/proto/BUI... blimp/common/proto/BUILD.gn:7: static_library("proto") { On 2015/10/01 00:50:48, Dirk Pranke wrote: > my earlier question about why this isn't a group() still stands. Sorry, I didn't catch that comment. No reason why not. Made it a group. https://codereview.chromium.org/1324263003/diff/200001/testing/buildbot/chrom... File testing/buildbot/chromium.linux.json (right): https://codereview.chromium.org/1324263003/diff/200001/testing/buildbot/chrom... testing/buildbot/chromium.linux.json:277: "test": "blimp_common_unit_tests" On 2015/10/01 00:50:48, Dirk Pranke wrote: > don't add this entry, as per my earlier feedback. Done.
https://codereview.chromium.org/1324263003/diff/220001/blimp/BUILD.gn File blimp/BUILD.gn (right): https://codereview.chromium.org/1324263003/diff/220001/blimp/BUILD.gn#newcode27 blimp/BUILD.gn:27: "//blimp/common:blimp_common_unit_tests", Remind me why this is only for android again?
lgtm w/ nyquist's question addressed.
Late Show review.... https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... File blimp/common/net/message_dispatcher.cc (right): https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.cc:23: DCHECK(thread_checker_.CalledOnValidThread()); nit: Doesn't thread-checker do this for you, and before the std::map dtor, since it's the final member of the class? https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.cc:31: LOG(ERROR) << "Handler already registered for type=" << type << "."; Replace this with a DCHECK; it's a coding-time error to add >1 https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.cc:40: LOG(ERROR) << "No registered handler for " There's no real point logging this - who will see it? :P We should have a UMA stat for this case and DCHECK on it; in Release builds we should probably disconnect the client. https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.cc:45: DCHECK(handler_iter->second); This DCHECK should be on |handler| when it's passed into AddHandler, since that's the path through which a nullptr handler would have reached here. https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.cc:47: LOG(WARNING) << BlimpMessageToDebugString(message) See above re logging vs DCHECK & disconnect. https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... File blimp/common/net/message_dispatcher.h (right): https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.h:18: class BLIMP_COMMON_EXPORT MessageDispatcher { nit: BlimpMessageDispatcher, for consistency w/ BlimpMessage? https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.h:23: virtual bool Validate(const BlimpMessage& message) const = 0; What's the value in keeping this and OnMessage separate; surely you can just have OnMessage return |true| if it handles the message, and let the handler implementation break validation into a separate sub-routine if necessary? https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.h:29: virtual void OnMessage(const BlimpMessage& message) = 0; nit: OnBlimpMessage?
https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... File blimp/common/net/message_dispatcher.cc (right): https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.cc:38: auto handler_iter = feature_handler_map_.find(message.type()); Note that for the small numbers of message-types we'll be dealing with, a map lookup may actually involve sufficient work (e.g. in terms of memory accesses) to be slower than a simple linear if..elseif... sequence of tests.
https://codereview.chromium.org/1324263003/diff/220001/blimp/BUILD.gn File blimp/BUILD.gn (right): https://codereview.chromium.org/1324263003/diff/220001/blimp/BUILD.gn#newcode27 blimp/BUILD.gn:27: "//blimp/common:blimp_common_unit_tests", On 2015/10/01 21:44:08, nyquist wrote: > Remind me why this is only for android again? Done. https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... File blimp/common/net/message_dispatcher.cc (right): https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.cc:23: DCHECK(thread_checker_.CalledOnValidThread()); On 2015/10/02 01:10:29, Wez wrote: > nit: Doesn't thread-checker do this for you, and before the std::map dtor, since > it's the final member of the class? I guess it would, neato! https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.cc:31: LOG(ERROR) << "Handler already registered for type=" << type << "."; On 2015/10/02 01:10:29, Wez wrote: > Replace this with a DCHECK; it's a coding-time error to add >1 Done. https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.cc:38: auto handler_iter = feature_handler_map_.find(message.type()); On 2015/10/02 01:12:09, Wez wrote: > Note that for the small numbers of message-types we'll be dealing with, a map > lookup may actually involve sufficient work (e.g. in terms of memory accesses) > to be slower than a simple linear if..elseif... sequence of tests. How about this, SmallMap? Linear comparisons across a contiguous array; should be fast for small numbers of things. It cuts down on code duplication vs. adding conditionals and fields for every feature type. https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.cc:40: LOG(ERROR) << "No registered handler for " On 2015/10/02 01:10:29, Wez wrote: > There's no real point logging this - who will see it? :P > > We should have a UMA stat for this case and DCHECK on it; in Release builds we > should probably disconnect the client. OK. Added a bool return value so the session/channel can take the appropriate action. https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.cc:45: DCHECK(handler_iter->second); On 2015/10/02 01:10:29, Wez wrote: > This DCHECK should be on |handler| when it's passed into AddHandler, since > that's the path through which a nullptr handler would have reached here. Done https://codereview.chromium.org/1324263003/diff/220001/blimp/common/net/messa... blimp/common/net/message_dispatcher.cc:47: LOG(WARNING) << BlimpMessageToDebugString(message) On 2015/10/02 01:10:29, Wez wrote: > See above re logging vs DCHECK & disconnect. Done.
https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... File blimp/common/net/message_dispatcher.h (right): https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... blimp/common/net/message_dispatcher.h:19: class BLIMP_COMMON_EXPORT MessageDispatcher { Since this dispatches BlimpMessages, let's call it BlimpMessageDispatcher? https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... blimp/common/net/message_dispatcher.h:24: virtual bool Validate(const BlimpMessage& message) const = 0; As previously noted, what's the value of the Validate()/OnMessage() split? https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... blimp/common/net/message_dispatcher.h:53: bool Dispatch(const BlimpMessage& message) const; Why do we want this to have a separate message-handling API to the handlers themselves? If we have a BlimpMessageHandler interface then MessageDispatcher just becomes a de-multiplexing handler, and that in turn means that the BlimpMessagePump can take any arbitrary BlimpMessageHandler and pump BlimpMessages to it. Suggest making an explicit comment that calling code should typically treat Dispatch errors as fatal, and e.g. disconnect the peer. https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... File blimp/common/net/message_dispatcher_unittest.cc (right): https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... blimp/common/net/message_dispatcher_unittest.cc:45: dispatcher.Dispatch(compositor_msg); Verify that the Dispatcher returns an error in this case.
https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... File blimp/common/net/message_dispatcher.h (right): https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... blimp/common/net/message_dispatcher.h:19: class BLIMP_COMMON_EXPORT MessageDispatcher { On 2015/10/07 00:20:15, Wez wrote: > Since this dispatches BlimpMessages, let's call it BlimpMessageDispatcher? Done. https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... blimp/common/net/message_dispatcher.h:24: virtual bool Validate(const BlimpMessage& message) const = 0; On 2015/10/07 00:20:15, Wez wrote: > As previously noted, what's the value of the Validate()/OnMessage() split? I guess it doesn't make a lot of sense to validate it at this layer - the delegates can report errors in whichever way suits them. I'll have OnMessage return a bool. https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... blimp/common/net/message_dispatcher.h:53: bool Dispatch(const BlimpMessage& message) const; On 2015/10/07 00:20:15, Wez wrote: > Why do we want this to have a separate message-handling API to the handlers > themselves? > > If we have a BlimpMessageHandler interface then MessageDispatcher just becomes a > de-multiplexing handler, and that in turn means that the BlimpMessagePump can > take any arbitrary BlimpMessageHandler and pump BlimpMessages to it. > > Suggest making an explicit comment that calling code should typically treat > Dispatch errors as fatal, and e.g. disconnect the peer. Good idea, done. See comments on BlimpMessageReceiver regarding disconnection policy.
https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... File blimp/common/net/message_dispatcher.h (right): https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... blimp/common/net/message_dispatcher.h:19: class BLIMP_COMMON_EXPORT MessageDispatcher { On 2015/10/07 00:20:15, Wez wrote: > Since this dispatches BlimpMessages, let's call it BlimpMessageDispatcher? Done. https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... blimp/common/net/message_dispatcher.h:24: virtual bool Validate(const BlimpMessage& message) const = 0; On 2015/10/07 00:20:15, Wez wrote: > As previously noted, what's the value of the Validate()/OnMessage() split? I guess it doesn't make a lot of sense to validate it at this layer - the delegates can report errors in whichever way suits them. I'll have OnMessage return a bool. https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... blimp/common/net/message_dispatcher.h:53: bool Dispatch(const BlimpMessage& message) const; On 2015/10/07 00:20:15, Wez wrote: > Why do we want this to have a separate message-handling API to the handlers > themselves? > > If we have a BlimpMessageHandler interface then MessageDispatcher just becomes a > de-multiplexing handler, and that in turn means that the BlimpMessagePump can > take any arbitrary BlimpMessageHandler and pump BlimpMessages to it. > > Suggest making an explicit comment that calling code should typically treat > Dispatch errors as fatal, and e.g. disconnect the peer. Good idea, done. See comments on BlimpMessageReceiver regarding disconnection policy.
https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... File blimp/common/net/message_dispatcher.h (right): https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... blimp/common/net/message_dispatcher.h:24: virtual bool Validate(const BlimpMessage& message) const = 0; On 2015/10/07 at 00:58:51, Kevin M wrote: > On 2015/10/07 00:20:15, Wez wrote: > > As previously noted, what's the value of the Validate()/OnMessage() split? > > I guess it doesn't make a lot of sense to validate it at this layer - the delegates can report errors in whichever way suits them. > > I'll have OnMessage return a bool. bool or net::Error? net::Error means you have to pick an error-code but it also means that we can have the message pump log something vaguely helpful if a handler balks at a message. https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... File blimp/net/blimp_message_dispatcher.cc (right): https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher.cc:31: DCHECK(false) << "Handler already registered for type=" << type << "."; DLOG(FATAL) https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher.cc:41: << BlimpMessageToDebugString(message) << "."; As previously noted, this indicates a coding error somewhere, or malicious client, so it's not clear that logging anything here, at least in release builds, is useful. I think it'd be better to just have the OnMessage() return-code be a net::Error and try to return something sensible - WDYT? Otherwise, maybe make this a DVLOG or DLOG(FATAL) https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... File blimp/net/blimp_message_dispatcher.h (right): https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher.h:26: // with the type |type|. nit: Suggest "... of the specified |type|." https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher.h:30: // of |this|. nit: Strictly |handler| only need be valid when OnMessage() is called. https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher.h:38: // message. This API is part of the BlimpMessageReceiver interface - no need to re-document it here. Suggest instead just a one line comment "// BlimpMessageReceiver implementation.". As regards behaviour in case of no handler registered, we should be returning an error code, not logging an error. You can probably document the no-receiver behaviour on the AddReceiver API. https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher.h:44: base::ThreadChecker thread_checker_; nit: This seems overkill - I don't think there's any reason we'd expect the caller to get confused about which thread this class is being called on? https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... File blimp/net/blimp_message_dispatcher_unittest.cc (right): https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher_unittest.cc:44: .WillOnce(Return(false)); Note that with gMock it's not safe, in general, to mingle EXPECT_CALL() and calls that might trigger the mock objects to be touched - you need to set up the expectations up-front and then run the test case. https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... File blimp/net/blimp_message_receiver.h (right): https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_receiver.h:12: // Abstract interface for classes that can receive BlimpMessages. nit: I can see it's abstract; suggest just "Interface implemented by components that process BlimpMessages." https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_receiver.h:21: // connection to be terminated. nit: Make clear that the caller needs to terminate the connection - "should cause the connection to be terminated" could mean that this class returning false is expected to implicitly terminate it, but might not. Also, this comment seems unnecessarily verbose - e.g. it mentions threading, which has absolutely nothing to do with anything else mentioned in this header! Suggest just: "Processes the BlimpMessage, or returns false on error. Callers should e.g. terminate the connection from which |message| was received." https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_receiver.h:22: virtual bool OnMessage(const BlimpMessage& message) = 0; nit: OnBlimpMessage, for consistency, and 'cos OnMessage is super generic sounding?
https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... File blimp/common/net/message_dispatcher.h (right): https://codereview.chromium.org/1324263003/diff/240001/blimp/common/net/messa... blimp/common/net/message_dispatcher.h:24: virtual bool Validate(const BlimpMessage& message) const = 0; On 2015/10/09 23:15:34, Wez wrote: > On 2015/10/07 at 00:58:51, Kevin M wrote: > > On 2015/10/07 00:20:15, Wez wrote: > > > As previously noted, what's the value of the Validate()/OnMessage() split? > > > > I guess it doesn't make a lot of sense to validate it at this layer - the > delegates can report errors in whichever way suits them. > > > > I'll have OnMessage return a bool. > > bool or net::Error? net::Error means you have to pick an error-code but it also > means that we can have the message pump log something vaguely helpful if a > handler balks at a message. Done. https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... File blimp/net/blimp_message_dispatcher.cc (right): https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher.cc:31: DCHECK(false) << "Handler already registered for type=" << type << "."; On 2015/10/09 23:15:34, Wez wrote: > DLOG(FATAL) Done. https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher.cc:41: << BlimpMessageToDebugString(message) << "."; On 2015/10/09 23:15:34, Wez wrote: > As previously noted, this indicates a coding error somewhere, or malicious > client, so it's not clear that logging anything here, at least in release > builds, is useful. I think it'd be better to just have the OnMessage() > return-code be a net::Error and try to return something sensible - WDYT? > Otherwise, maybe make this a DVLOG or DLOG(FATAL) Done. https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... File blimp/net/blimp_message_dispatcher.h (right): https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher.h:26: // with the type |type|. On 2015/10/09 23:15:34, Wez wrote: > nit: Suggest "... of the specified |type|." Done. https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher.h:30: // of |this|. On 2015/10/09 23:15:34, Wez wrote: > nit: Strictly |handler| only need be valid when OnMessage() is called. Done. https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher.h:38: // message. On 2015/10/09 23:15:34, Wez wrote: > This API is part of the BlimpMessageReceiver interface - no need to re-document > it here. Suggest instead just a one line comment "// BlimpMessageReceiver > implementation.". > > As regards behaviour in case of no handler registered, we should be returning an > error code, not logging an error. You can probably document the no-receiver > behaviour on the AddReceiver API. Done. https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher.h:44: base::ThreadChecker thread_checker_; On 2015/10/09 23:15:34, Wez wrote: > nit: This seems overkill - I don't think there's any reason we'd expect the > caller to get confused about which thread this class is being called on? OK, I'm still figuring out the right degree of anal-retentiveness to employ for these sorts of things. https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... File blimp/net/blimp_message_dispatcher_unittest.cc (right): https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher_unittest.cc:44: .WillOnce(Return(false)); On 2015/10/09 23:15:34, Wez wrote: > Note that with gMock it's not safe, in general, to mingle EXPECT_CALL() and > calls that might trigger the mock objects to be touched - you need to set up the > expectations up-front and then run the test case. Done. https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... File blimp/net/blimp_message_receiver.h (right): https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_receiver.h:12: // Abstract interface for classes that can receive BlimpMessages. On 2015/10/09 23:15:34, Wez wrote: > nit: I can see it's abstract; suggest just "Interface implemented by components > that process BlimpMessages." Done. https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_receiver.h:21: // connection to be terminated. On 2015/10/09 23:15:35, Wez wrote: > nit: Make clear that the caller needs to terminate the connection - "should > cause the connection to be terminated" could mean that this class returning > false is expected to implicitly terminate it, but might not. > > Also, this comment seems unnecessarily verbose - e.g. it mentions threading, > which has absolutely nothing to do with anything else mentioned in this header! > Suggest just: > > "Processes the BlimpMessage, or returns false on error. Callers should e.g. > terminate the connection from which |message| was received." Done. https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_receiver.h:22: virtual bool OnMessage(const BlimpMessage& message) = 0; On 2015/10/09 23:15:34, Wez wrote: > nit: OnBlimpMessage, for consistency, and 'cos OnMessage is super generic > sounding? Done.
lgtm https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... File blimp/net/blimp_message_dispatcher.h (right): https://codereview.chromium.org/1324263003/diff/260001/blimp/net/blimp_messag... blimp/net/blimp_message_dispatcher.h:44: base::ThreadChecker thread_checker_; On 2015/10/10 at 00:42:34, Kevin M wrote: > On 2015/10/09 23:15:34, Wez wrote: > > nit: This seems overkill - I don't think there's any reason we'd expect the > > caller to get confused about which thread this class is being called on? > > OK, I'm still figuring out the right degree of anal-retentiveness to employ for these sorts of things. Yeah, it's a fine line!
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nyquist@chromium.org, dpranke@chromium.org, wez@chromium.org Link to the patchset: https://codereview.chromium.org/1324263003/#ps300001 (title: "Resync with master")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324263003/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324263003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, wez@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/1324263003/#ps340001 (title: "Resync with master.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324263003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324263003/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_chromium_gn_compile_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, wez@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/1324263003/#ps360001 (title: "Remove build rules from common")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324263003/360001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324263003/360001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by kmarshall@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dpranke@chromium.org, wez@chromium.org, nyquist@chromium.org Link to the patchset: https://codereview.chromium.org/1324263003/#ps400001 (title: "Resolve naming collision for "net"")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1324263003/400001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1324263003/400001
Message was sent while issue was closed.
Committed patchset #21 (id:400001)
Message was sent while issue was closed.
Patchset 21 (id:??) landed as https://crrev.com/1b78a36c7edf36f97214600b83e715291146c01c Cr-Commit-Position: refs/heads/master@{#353924} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
