Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
433283002:
Adding the Copresence RpcHandler and HttpPost helper. (Closed)
|
Created:
6 years, 4 months ago by Charlie Modified:
6 years, 4 months ago Reviewers:
rkc, Daniel Erat, willchan no longer on Chromium, xiyuan, Roger Tawa OOO till Jul 10th, darin (slow to review), brettw, blundell CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@directive-handler Project:
chromium Visibility:
Public. |
DescriptionThis is part of the preliminary introduction of the chrome.copresence API,
which enables the exchange of short messages with nearby devices.
These classes manage all the communication between Chrome and
the Copresence server.
We access the server proto API through Apiary. I deleted
most of deprecated proto fields, since we are no longer using them.
Gone too are the unpublish and unsubscribe all calls, which are
probably going to be removed on the server. The deprecated
namespace field must stay, unfortunately, because we need
some additional configuration for the new flow.
Based on issue 419073002 (adding the directive handler).
R=derat@chromium.org, xiyuan@chromium.org
BUG=365493
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=288540
Patch Set 1 #Patch Set 2 : Rebasing off the correct CL #
Total comments: 99
Patch Set 3 : First round of review fixes #
Total comments: 14
Patch Set 4 : Some small fixes #
Total comments: 2
Patch Set 5 : Final fixes #Patch Set 6 : Fixes for DEPS review #
Total comments: 5
Patch Set 7 : Adding cleanup logic for HttpPost. Also rebased to master. #
Total comments: 2
Patch Set 8 : Removing redundant reset() call. #
Total comments: 16
Patch Set 9 : Removing WeakPtrs and using Shutdown() instead. #Patch Set 10 : More WeakPtr fixes #
Total comments: 11
Patch Set 11 : RpcHandler deletes HttpPosts #
Total comments: 4
Patch Set 12 : Addressing nits #
Total comments: 37
Patch Set 13 : Addressing comments from willchan #Patch Set 14 : Fixing HttpPost deletion #Patch Set 15 : Some last fixes #Patch Set 16 : Merging to head #Patch Set 17 : Reverting whispernet_client.h #Patch Set 18 : Build/merge fixes #Patch Set 19 : Moving proto changes to a different CL #Patch Set 20 : Fixing Chromium breakage #Patch Set 21 : Fixing Chromium and Windows build failures #Patch Set 22 : Fixing RpcHandlerTest.Initialize #
Total comments: 1
Messages
Total messages: 86 (0 generated)
Ping. Dan and Xiyuan, please take a look when you get a chance.
https://codereview.chromium.org/433283002/diff/20001/components/copresence.gypi File components/copresence.gypi (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence.gy... components/copresence.gypi:12: '../chrome/common_constants.gyp:common_constants', Is it allowed to have back dependency on chrome from components? Can we create copresence_switches.cc instead? https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/rpc_handler.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:274: base::Unretained(this), AsWeakPtr()? https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/rpc_handler_unittest.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:52: struct FakeDirectiveHandler : public DirectiveHandler { nit: struct -> class and add "public:" etc since this feels more like a class rather than a collection of data.
taking a look now. (note that you need to explicitly mail changes to reviewers after uploading them to rietveld; the reviewers aren't notified automatically.)
Thanks. I thought maybe the review wasn't sent out for some reason. I added reviewers but didn't see an email myself. On Tue, Aug 5, 2014 at 5:19 PM, <derat@chromium.org> wrote: > taking a look now. (note that you need to explicitly mail changes to > reviewers > after uploading them to rietveld; the reviewers aren't notified > automatically.) > > https://codereview.chromium.org/433283002/ > -- Charlie Kehoe | Software Engineer | ckehoe@google.com To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
here are some initial comments (i didn't get beyond http_post.cc yet but will continue later) https://codereview.chromium.org/433283002/diff/20001/chrome/common/chrome_swi... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/433283002/diff/20001/chrome/common/chrome_swi... chrome/common/chrome_switches.cc:163: // Use this address for calls to the Copresence server (via Apiary). change to: // Address for calls to the Copresence server etc. https://codereview.chromium.org/433283002/diff/20001/chrome/common/chrome_swi... chrome/common/chrome_switches.cc:167: // Use this Apiary tracing token for calls to the Copresence server. change to: // Apiary tracing token etc. https://codereview.chromium.org/433283002/diff/20001/chrome/common/chrome_swi... chrome/common/chrome_switches.cc:790: // Use this address for the ledger (Copresence) server. get rid of "Use this" here and in the next comment too https://codereview.chromium.org/433283002/diff/20001/components/copresence.gypi File components/copresence.gypi (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence.gy... components/copresence.gypi:12: '../chrome/common_constants.gyp:common_constants', On 2014/08/05 21:33:39, xiyuan wrote: > Is it allowed to have back dependency on chrome from components? Can we create > copresence_switches.cc instead? no, this isn't allowed, and yes, dependencies that are shared between chrome/ and something under components/ should live in components/. https://codereview.chromium.org/433283002/diff/20001/components/copresence/BU... File components/copresence/BUILD.gn (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/BU... components/copresence/BUILD.gn:3: # that can be found in the LICENSE file. why the shortened wrapping? ditto for other files https://codereview.chromium.org/433283002/diff/20001/components/copresence/DEPS File components/copresence/DEPS (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/DE... components/copresence/DEPS:3: "+chrome/common", this shouldn't depend on chrome https://codereview.chromium.org/433283002/diff/20001/components/copresence/ha... File components/copresence/handlers/directive_handler.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/ha... components/copresence/handlers/directive_handler.cc:40: DirectiveHandler::DirectiveHandler() { this leaves the object in a state where methods will crash (e.g. AddDirective() appears to depend on audio_handler_ being a valid object). please instead use a stub implementation for tests. https://codereview.chromium.org/433283002/diff/20001/components/copresence/ha... File components/copresence/handlers/directive_handler.h (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/ha... components/copresence/handlers/directive_handler.h:22: // given to it by the client. is your editor wrapping comments at some width less than 80 columns? https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post.cc:31: // Send a request to the Copresence server. put comments like these in the header instead of the implementation https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post.cc:97: delete this; i'm not sure whether it's safe to leak these if they're still running when chrome exits https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post.h:29: * This class handles all Apiary calls to the Copresence server. use c++-style comments (//) https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post.h:46: HttpPost(net::URLRequestContextGetter* url_context_getter, document whether the ownership of this argument is transferred (if so, pass it in a scoped_ptr instead) https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post.h:48: const scoped_ptr<google::protobuf::MessageLite> request_proto, const reference instead of const scoped_ptr? https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/http_post_unittest.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post_unittest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. we don't use the (c) anymore
here are the rest of my comments on patch set 2 https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/http_post_unittest.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post_unittest.cc:7: #include "chrome/common/chrome_switches.h" (mentioned earlier, but this shouldn't depend on chrome/) https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post_unittest.cc:30: make_scoped_refptr(new base::TestSimpleTaskRunner)); indent four spaces beyond previous line, not two https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post_unittest.cc:57: proto_.set_client("test_client"); just do this in the c'tor. you don't need SetUp() here; both it and the c'tor run for every test: https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de... https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post_unittest.cc:62: CommandLine::ForCurrentProcess()->AppendSwitchASCII( could this cause problems for other tests running in the same process? it'd probably be cleaner to make the test explicitly pass this value into the class that uses it instead of modifying the command line. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post_unittest.cc:66: static void TearDownTestCase() {} don't define this if it's empty https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post_unittest.cc:73: int received_response_code_; initialize this in the c'tor so it doesn't contain garbage if it never gets set https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/rpc_handler.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:33: // TODO(rkc): Move this to the wrapper. if this is duplicated now (i know i saw it earlier in rkc's change), please move it to a shared location instead https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:44: const char kReportRequestRpcName[] = "report"; make this be a public static const member so it isn't duplicated in the test https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:48: // Logs the status and returns true if the status was OK. this looks like it only logs the status if it was an error. maybe rename it to LogStatusIfError()? also update this comment to something like: // If |status| is non-OK, logs an error. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:53: std::stringstream log_message; the style guide says not to use streams except for logging (by which i think it's referring to actual LOG lines). how about just condensing all this stuff? LOG(ERROR) << "Copresence error code " << status.code() << (status.message().empty() ? std::string() : std::string(": ") + status.message()); https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:63: void LogStatus(const util::error::Code& code, const std::string& context) { don't overload function names https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:70: bool LogStatus(const ReportResponse& response) { don't overload function names https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:72: if (response.has_manage_messages_response()) { omit curly brackets for single-line statements https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:84: // Request construction nit: add trailing period https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:97: ReportRequest* request) { can this be a const reference? https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:133: return BROADCAST_AND_SCAN; do you need to check whether both broadcast_only and scan_only are false? https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:136: DeviceState* GetDeviceCapabilities( return a scoped_ptr instead so the ownership transfer is clear https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:137: ReportRequest* request) { can this be a const reference? also, unwrap this line https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:223: << "\nRpcHandler::Initialize() must complete successfully " why do you have a newline at the beginning of this string? https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:265: // Directive_handler will be destructed before us, so unretained is safe. s/Directive_handler/|directive_handler_|/ https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:367: // TODO(rkc/ckehoe): Store the token in a valid_token_cache_ with a nit: |valid_token_cache_| https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:368: // short ttl (like 10s) and send it up with every report request. nit: s/ttl/TTL/ https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/rpc_handler.h (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.h:16: #include "components/copresence/rpc/http_post.h" fix sorting https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.h:37: explicit RpcHandler(CopresenceClientDelegate* delegate); document ownership of |delegate| https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.h:41: // Clients must call this and wait for init_done_callback nit: |init_done_callback| (since that's the common style in chrome) https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.h:62: typedef base::Callback<void(net::URLRequestContextGetter*, document what these different fields contain (the string and protobuf especially) https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.h:94: CopresenceClientDelegate* delegate_; document ownership https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/rpc_handler_unittest.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:115: int status_code, const std::string& response) { nit: one argument per line if they don't all fit on the first line https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:166: // Register with server nit: add trailing period here and in following comments https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:297: DCHECK(test_response.SerializeToString(&serialized_proto)); s/DCHECK/ASSERT_TRUE/
https://codereview.chromium.org/433283002/diff/20001/chrome/common/chrome_swi... File chrome/common/chrome_switches.cc (right): https://codereview.chromium.org/433283002/diff/20001/chrome/common/chrome_swi... chrome/common/chrome_switches.cc:163: // Use this address for calls to the Copresence server (via Apiary). On 2014/08/06 00:44:46, Daniel Erat wrote: > change to: > > // Address for calls to the Copresence server etc. Done. https://codereview.chromium.org/433283002/diff/20001/chrome/common/chrome_swi... chrome/common/chrome_switches.cc:167: // Use this Apiary tracing token for calls to the Copresence server. On 2014/08/06 00:44:46, Daniel Erat wrote: > change to: > > // Apiary tracing token etc. Done. https://codereview.chromium.org/433283002/diff/20001/chrome/common/chrome_swi... chrome/common/chrome_switches.cc:790: // Use this address for the ledger (Copresence) server. On 2014/08/06 00:44:46, Daniel Erat wrote: > get rid of "Use this" here and in the next comment too Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence.gypi File components/copresence.gypi (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence.gy... components/copresence.gypi:12: '../chrome/common_constants.gyp:common_constants', On 2014/08/05 21:33:39, xiyuan wrote: > Is it allowed to have back dependency on chrome from components? Can we create > copresence_switches.cc instead? Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/BU... File components/copresence/BUILD.gn (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/BU... components/copresence/BUILD.gn:3: # that can be found in the LICENSE file. I prefer to avoid newlines at awkward points in the sentence. I don't spread across more lines than needed to accomplish this, though. If the last line is short already, I reflow. Do you need me to put it back? On 2014/08/06 00:44:46, Daniel Erat wrote: > why the shortened wrapping? ditto for other files https://codereview.chromium.org/433283002/diff/20001/components/copresence/DEPS File components/copresence/DEPS (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/DE... components/copresence/DEPS:3: "+chrome/common", On 2014/08/06 00:44:46, Daniel Erat wrote: > this shouldn't depend on chrome Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/ha... File components/copresence/handlers/directive_handler.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/ha... components/copresence/handlers/directive_handler.cc:40: DirectiveHandler::DirectiveHandler() { Not sure I understand - FakeDirectiveHandler is the stub implementation, and it still needs a constructor to invoke. I moved all the initialization to an Initialize() method, and added DCHECKs. On 2014/08/06 00:44:46, Daniel Erat wrote: > this leaves the object in a state where methods will crash (e.g. AddDirective() > appears to depend on audio_handler_ being a valid object). please instead use a > stub implementation for tests. https://codereview.chromium.org/433283002/diff/20001/components/copresence/ha... File components/copresence/handlers/directive_handler.h (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/ha... components/copresence/handlers/directive_handler.h:22: // given to it by the client. See previous response. On 2014/08/06 00:44:46, Daniel Erat wrote: > is your editor wrapping comments at some width less than 80 columns? https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post.cc:31: // Send a request to the Copresence server. On 2014/08/06 00:44:46, Daniel Erat wrote: > put comments like these in the header instead of the implementation Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post.cc:97: delete this; Ok. Can you clarify how you'd like us to handle this? Should the HttpPost stay in the scope of the caller, and we pass a WeakPtr to the URLFetcher? On 2014/08/06 00:44:46, Daniel Erat wrote: > i'm not sure whether it's safe to leak these if they're still running when > chrome exits https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post.h:29: * This class handles all Apiary calls to the Copresence server. On 2014/08/06 00:44:47, Daniel Erat wrote: > use c++-style comments (//) Done. Is this in the style guide? I suppose Chromium doesn't use Doxygen? https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post.h:46: HttpPost(net::URLRequestContextGetter* url_context_getter, On 2014/08/06 00:44:47, Daniel Erat wrote: > document whether the ownership of this argument is transferred (if so, pass it > in a scoped_ptr instead) Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post.h:48: const scoped_ptr<google::protobuf::MessageLite> request_proto, On 2014/08/06 00:44:47, Daniel Erat wrote: > const reference instead of const scoped_ptr? Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/http_post_unittest.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post_unittest.cc:1: // Copyright (c) 2014 The Chromium Authors. All rights reserved. On 2014/08/06 00:44:47, Daniel Erat wrote: > we don't use the (c) anymore Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post_unittest.cc:7: #include "chrome/common/chrome_switches.h" On 2014/08/06 16:01:15, Daniel Erat wrote: > (mentioned earlier, but this shouldn't depend on chrome/) Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post_unittest.cc:30: make_scoped_refptr(new base::TestSimpleTaskRunner)); Oops. Done. On 2014/08/06 16:01:15, Daniel Erat wrote: > indent four spaces beyond previous line, not two https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post_unittest.cc:57: proto_.set_client("test_client"); On 2014/08/06 16:01:15, Daniel Erat wrote: > just do this in the c'tor. you don't need SetUp() here; both it and the c'tor > run for every test: > https://code.google.com/p/googletest/wiki/FAQ#Should_I_use_the_constructor/de... Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post_unittest.cc:62: CommandLine::ForCurrentProcess()->AppendSwitchASCII( Makes sense. Done. On 2014/08/06 16:01:16, Daniel Erat wrote: > could this cause problems for other tests running in the same process? it'd > probably be cleaner to make the test explicitly pass this value into the class > that uses it instead of modifying the command line. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post_unittest.cc:66: static void TearDownTestCase() {} For some reason I got errors when I had SetUpTestCase(), but not TearDownTestCase(). Anyway, neither is needed anymore. On 2014/08/06 16:01:16, Daniel Erat wrote: > don't define this if it's empty https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post_unittest.cc:73: int received_response_code_; Done. Sorry, writing Java code has made me sloppy :-) On 2014/08/06 16:01:15, Daniel Erat wrote: > initialize this in the c'tor so it doesn't contain garbage if it never gets set https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/rpc_handler.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:33: // TODO(rkc): Move this to the wrapper. I have his CLs patched in, but a grep of my tree doesn't turn up any other copies. Maybe you're thinking of the inverse function, FromUrlSafe? I do think both ToUrlSafe and FromUrlSafe should be in the same place. Hence the TODO. On 2014/08/06 16:01:17, Daniel Erat wrote: > if this is duplicated now (i know i saw it earlier in rkc's change), please move > it to a shared location instead https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:44: const char kReportRequestRpcName[] = "report"; On 2014/08/06 16:01:16, Daniel Erat wrote: > make this be a public static const member so it isn't duplicated in the test Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:48: // Logs the status and returns true if the status was OK. On 2014/08/06 16:01:16, Daniel Erat wrote: > this looks like it only logs the status if it was an error. maybe rename it to > LogStatusIfError()? also update this comment to something like: > > // If |status| is non-OK, logs an error. Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:53: std::stringstream log_message; Sure. Done. On 2014/08/06 16:01:16, Daniel Erat wrote: > the style guide says not to use streams except for logging (by which i think > it's referring to actual LOG lines). how about just condensing all this stuff? > > LOG(ERROR) << "Copresence error code " << status.code() > << (status.message().empty() ? std::string() : > std::string(": ") + status.message()); https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:63: void LogStatus(const util::error::Code& code, const std::string& context) { On 2014/08/06 16:01:16, Daniel Erat wrote: > don't overload function names Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:70: bool LogStatus(const ReportResponse& response) { On 2014/08/06 16:01:17, Daniel Erat wrote: > don't overload function names Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:72: if (response.has_manage_messages_response()) { On 2014/08/06 16:01:16, Daniel Erat wrote: > omit curly brackets for single-line statements Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:84: // Request construction This is supposed to be a section header. Added a newline after. On 2014/08/06 16:01:16, Daniel Erat wrote: > nit: add trailing period https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:97: ReportRequest* request) { Yes. Done. On 2014/08/06 16:01:17, Daniel Erat wrote: > can this be a const reference? https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:133: return BROADCAST_AND_SCAN; No, this is our default. Added a comment. On 2014/08/06 16:01:16, Daniel Erat wrote: > do you need to check whether both broadcast_only and scan_only are false? https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:136: DeviceState* GetDeviceCapabilities( On 2014/08/06 16:01:16, Daniel Erat wrote: > return a scoped_ptr instead so the ownership transfer is clear Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:137: ReportRequest* request) { On 2014/08/06 16:01:16, Daniel Erat wrote: > can this be a const reference? > > also, unwrap this line Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:223: << "\nRpcHandler::Initialize() must complete successfully " I liked the log message better all on one line. But I suppose this breaks log-grepping. Removed. On 2014/08/06 16:01:17, Daniel Erat wrote: > why do you have a newline at the beginning of this string? https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:265: // Directive_handler will be destructed before us, so unretained is safe. On 2014/08/06 16:01:17, Daniel Erat wrote: > s/Directive_handler/|directive_handler_|/ Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:274: base::Unretained(this), On 2014/08/05 21:33:39, xiyuan wrote: > AsWeakPtr()? Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:367: // TODO(rkc/ckehoe): Store the token in a valid_token_cache_ with a On 2014/08/06 16:01:16, Daniel Erat wrote: > nit: |valid_token_cache_| Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:368: // short ttl (like 10s) and send it up with every report request. On 2014/08/06 16:01:16, Daniel Erat wrote: > nit: s/ttl/TTL/ Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/rpc_handler.h (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.h:16: #include "components/copresence/rpc/http_post.h" On 2014/08/06 16:01:17, Daniel Erat wrote: > fix sorting Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.h:37: explicit RpcHandler(CopresenceClientDelegate* delegate); On 2014/08/06 16:01:17, Daniel Erat wrote: > document ownership of |delegate| Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.h:41: // Clients must call this and wait for init_done_callback On 2014/08/06 16:01:17, Daniel Erat wrote: > nit: |init_done_callback| (since that's the common style in chrome) Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.h:62: typedef base::Callback<void(net::URLRequestContextGetter*, On 2014/08/06 16:01:17, Daniel Erat wrote: > document what these different fields contain (the string and protobuf > especially) Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.h:94: CopresenceClientDelegate* delegate_; On 2014/08/06 16:01:17, Daniel Erat wrote: > document ownership Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/rpc_handler_unittest.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:52: struct FakeDirectiveHandler : public DirectiveHandler { On 2014/08/05 21:33:39, xiyuan wrote: > nit: struct -> class and add "public:" etc since this feels more like a class > rather than a collection of data. Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:115: int status_code, const std::string& response) { On 2014/08/06 16:01:17, Daniel Erat wrote: > nit: one argument per line if they don't all fit on the first line Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:166: // Register with server On 2014/08/06 16:01:17, Daniel Erat wrote: > nit: add trailing period here and in following comments Done. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:297: DCHECK(test_response.SerializeToString(&serialized_proto)); On 2014/08/06 16:01:17, Daniel Erat wrote: > s/DCHECK/ASSERT_TRUE/ Done.
https://codereview.chromium.org/433283002/diff/20001/components/copresence/BU... File components/copresence/BUILD.gn (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/BU... components/copresence/BUILD.gn:3: # that can be found in the LICENSE file. On 2014/08/06 19:32:17, Charlie wrote: > I prefer to avoid newlines at awkward points in the sentence. I don't spread > across more lines than needed to accomplish this, though. If the last line is > short already, I reflow. > > Do you need me to put it back? yes, i think it's better to leave boilerplate comments untouched so they match other files. i'd prefer that you just wrap all comments at 80 columns since it requires less thinking and seems to be what's typically done in chrome, but i don't have strong feelings there. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post.cc:97: delete this; On 2014/08/06 19:32:18, Charlie wrote: > Ok. Can you clarify how you'd like us to handle this? Should the HttpPost stay > in the scope of the caller, and we pass a WeakPtr to the URLFetcher? i'm not entirely sure whether this is a problem, but it might cause failures in the memcheck or asan bots. if you can get away with doing this, it's definitely simpler than the alternative of yeah, passing weak pointers and keeping this class owned by some other class that deletes all of its outstanding requests on destruction while also getting notified when requests finish to avoid double-frees. :-( here's an old change where i had to do something similar and took the more complicated approach: https://chromiumcodereview.appspot.com/10818017 GDataContactsService::DownloadContacts() creates a new DownloadContactsRequest and registers it. OnRequestComplete() is called when the request is done, and ~GDataContactsService() cancels and deletes all of the outstanding requests. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/rpc_handler.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:33: // TODO(rkc): Move this to the wrapper. On 2014/08/06 19:32:19, Charlie wrote: > I have his CLs patched in, but a grep of my tree doesn't turn up any other > copies. Maybe you're thinking of the inverse function, FromUrlSafe? > > I do think both ToUrlSafe and FromUrlSafe should be in the same place. Hence the > TODO. > > On 2014/08/06 16:01:17, Daniel Erat wrote: > > if this is duplicated now (i know i saw it earlier in rkc's change), please > move > > it to a shared location instead > yeah, that's probably what i'm thinking of. https://codereview.chromium.org/433283002/diff/40001/components/copresence/co... File components/copresence/copresence_switches.cc (right): https://codereview.chromium.org/433283002/diff/40001/components/copresence/co... components/copresence/copresence_switches.cc:11: const char kCopresenceServer[] = "copresence-server"; nit: get rid of extra spaces before the equals sign https://codereview.chromium.org/433283002/diff/40001/components/copresence/pu... File components/copresence/public/copresence_client_delegate.h (right): https://codereview.chromium.org/433283002/diff/40001/components/copresence/pu... components/copresence/public/copresence_client_delegate.h:22: enum CopresenceStatus { SUCCESS, FAIL, NONE }; i think that you're just using NONE for testing, right? if so, it'd be better to avoid it so that non-test code doesn't need to handle it in switch statements. https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... components/copresence/rpc/http_post.h:34: // TODO(ckehoe): Template this class to parse the proto received. it'd probably be better to avoid templates if possible here, since i believe they e.g. require defining this class in a header https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... File components/copresence/rpc/rpc_handler_unittest.cc (right): https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:65: virtual void RemoveDirectives(const std::string& op_id) OVERRIDE {} should this scan through directives_? seems a bit strange that AddDirective() adds but this is a no-op. if it's intentionally, maybe rename directives_ to added_directives_ so it's clear what it contains. https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:67: const std::vector<Directive>& GetDirectives() { nit: make this be a simple const accessor and move it after the d'tor: const std::vector<Directive>& directives() const { ... } https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:72: std::vector<Directive> directives; s/directives/directives_/ also, DISALLOW_COPY_AND_ASSIGN(FakeDirectiveHandler) (maybe unnecessary if the base class has it already, but i think it's generally used in chromium regardless)
https://codereview.chromium.org/433283002/diff/20001/components/copresence/BU... File components/copresence/BUILD.gn (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/BU... components/copresence/BUILD.gn:3: # that can be found in the LICENSE file. On 2014/08/06 21:35:17, Daniel Erat wrote: > On 2014/08/06 19:32:17, Charlie wrote: > > I prefer to avoid newlines at awkward points in the sentence. I don't spread > > across more lines than needed to accomplish this, though. If the last line is > > short already, I reflow. > > > > Do you need me to put it back? > > yes, i think it's better to leave boilerplate comments untouched so they match > other files. i'd prefer that you just wrap all comments at 80 columns since it > requires less thinking and seems to be what's typically done in chrome, but i > don't have strong feelings there. Done. Although for the record, an 80-column limit would put the word "found" on the second line also, even with C++ // comments. Maybe it's wrapped at 72, or 75. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/http_post.cc:97: delete this; On 2014/08/06 21:35:17, Daniel Erat wrote: > On 2014/08/06 19:32:18, Charlie wrote: > > Ok. Can you clarify how you'd like us to handle this? Should the HttpPost stay > > in the scope of the caller, and we pass a WeakPtr to the URLFetcher? > > i'm not entirely sure whether this is a problem, but it might cause failures in > the memcheck or asan bots. if you can get away with doing this, it's definitely > simpler than the alternative of yeah, passing weak pointers and keeping this > class owned by some other class that deletes all of its outstanding requests on > destruction while also getting notified when requests finish to avoid > double-frees. :-( > > here's an old change where i had to do something similar and took the more > complicated approach: https://chromiumcodereview.appspot.com/10818017 > > GDataContactsService::DownloadContacts() creates a new DownloadContactsRequest > and registers it. OnRequestComplete() is called when the request is done, and > ~GDataContactsService() cancels and deletes all of the outstanding requests. Thanks. Added a note to the header file. https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... File components/copresence/rpc/rpc_handler.cc (right): https://codereview.chromium.org/433283002/diff/20001/components/copresence/rp... components/copresence/rpc/rpc_handler.cc:33: // TODO(rkc): Move this to the wrapper. On 2014/08/06 21:35:18, Daniel Erat wrote: > On 2014/08/06 19:32:19, Charlie wrote: > > I have his CLs patched in, but a grep of my tree doesn't turn up any other > > copies. Maybe you're thinking of the inverse function, FromUrlSafe? > > > > I do think both ToUrlSafe and FromUrlSafe should be in the same place. Hence > the > > TODO. > > > > On 2014/08/06 16:01:17, Daniel Erat wrote: > > > if this is duplicated now (i know i saw it earlier in rkc's change), please > > move > > > it to a shared location instead > > > > yeah, that's probably what i'm thinking of. Acknowledged. https://codereview.chromium.org/433283002/diff/40001/components/copresence/co... File components/copresence/copresence_switches.cc (right): https://codereview.chromium.org/433283002/diff/40001/components/copresence/co... components/copresence/copresence_switches.cc:11: const char kCopresenceServer[] = "copresence-server"; On 2014/08/06 21:35:18, Daniel Erat wrote: > nit: get rid of extra spaces before the equals sign Done. https://codereview.chromium.org/433283002/diff/40001/components/copresence/pu... File components/copresence/public/copresence_client_delegate.h (right): https://codereview.chromium.org/433283002/diff/40001/components/copresence/pu... components/copresence/public/copresence_client_delegate.h:22: enum CopresenceStatus { SUCCESS, FAIL, NONE }; On 2014/08/06 21:35:18, Daniel Erat wrote: > i think that you're just using NONE for testing, right? if so, it'd be better to > avoid it so that non-test code doesn't need to handle it in switch statements. Seems to make the tests clunkier, but ok. https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... components/copresence/rpc/http_post.h:34: // TODO(ckehoe): Template this class to parse the proto received. On 2014/08/06 21:35:18, Daniel Erat wrote: > it'd probably be better to avoid templates if possible here, since i believe > they e.g. require defining this class in a header Can you clarify the cost of moving the implementation to a header? What's at stake besides a millisecond or two of compile time? https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... File components/copresence/rpc/rpc_handler_unittest.cc (right): https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:65: virtual void RemoveDirectives(const std::string& op_id) OVERRIDE {} On 2014/08/06 21:35:18, Daniel Erat wrote: > should this scan through directives_? seems a bit strange that AddDirective() > adds but this is a no-op. if it's intentionally, maybe rename directives_ to > added_directives_ so it's clear what it contains. RemoveDirectives isn't implemented yet in the real DirectiveHandler either. Adding a parallel TODO. https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:67: const std::vector<Directive>& GetDirectives() { On 2014/08/06 21:35:18, Daniel Erat wrote: > nit: make this be a simple const accessor and move it after the d'tor: > > const std::vector<Directive>& directives() const { ... } Done. https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:72: std::vector<Directive> directives; On 2014/08/06 21:35:18, Daniel Erat wrote: > s/directives/directives_/ > > also, DISALLOW_COPY_AND_ASSIGN(FakeDirectiveHandler) (maybe unnecessary if the > base class has it already, but i think it's generally used in chromium > regardless) Done.
lgtm with a nit i forgot to answer it earlier, but chromium uses c++-style comments exclusively, as far as i'm aware (other than /* param */ comments within argument lists in function calls). i think that all of the google3 c++ code that i can remember looking at also uses c++ comments. https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... components/copresence/rpc/http_post.h:34: // TODO(ckehoe): Template this class to parse the proto received. On 2014/08/06 22:36:23, Charlie wrote: > On 2014/08/06 21:35:18, Daniel Erat wrote: > > it'd probably be better to avoid templates if possible here, since i believe > > they e.g. require defining this class in a header > > Can you clarify the cost of moving the implementation to a header? What's at > stake besides a millisecond or two of compile time? - bigger binaries since the implementation is being duplicated in callers - hard-to-read interface in general, i think that there's a push in chromium to avoid templates except in cases where you're writing general code that needs to be able to operate on arbitrary types. https://codereview.chromium.org/433283002/diff/60001/components/copresence/rp... File components/copresence/rpc/rpc_handler_unittest.cc (right): https://codereview.chromium.org/433283002/diff/60001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:57: const std::vector<Directive>& GetDirectives() const { nit: rename this to added_directives() since it's an accessor
As Dan pointed out, we should re-think how the lifetime of HttpPost is managed. I have seen DCHECKs fired when destructing a profile with lingering URLFetcher jobs. This could be a bug load. But I don't want to block the CL. So LGTM.
https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/40001/components/copresence/rp... components/copresence/rpc/http_post.h:34: // TODO(ckehoe): Template this class to parse the proto received. On 2014/08/06 22:45:08, Daniel Erat wrote: > On 2014/08/06 22:36:23, Charlie wrote: > > On 2014/08/06 21:35:18, Daniel Erat wrote: > > > it'd probably be better to avoid templates if possible here, since i believe > > > they e.g. require defining this class in a header > > > > Can you clarify the cost of moving the implementation to a header? What's at > > stake besides a millisecond or two of compile time? > > - bigger binaries since the implementation is being duplicated in callers > - hard-to-read interface > > in general, i think that there's a push in chromium to avoid templates except in > cases where you're writing general code that needs to be able to operate on > arbitrary types. Makes sense. You mean the implementation is duplicated per set of template parameters, right? I hope the compiler/linker dedupes vector<int>, for example, instead of keeping one copy per .o file. Actually this need may go away, since we're talking about getting rid of the RegisterDevice rpc. https://codereview.chromium.org/433283002/diff/60001/components/copresence/rp... File components/copresence/rpc/rpc_handler_unittest.cc (right): https://codereview.chromium.org/433283002/diff/60001/components/copresence/rp... components/copresence/rpc/rpc_handler_unittest.cc:57: const std::vector<Directive>& GetDirectives() const { On 2014/08/06 22:45:08, Daniel Erat wrote: > nit: rename this to added_directives() since it's an accessor Done.
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/433283002/80001
+rogerta for googleapis OWNERS review. Sorry for the TODO. This is a time-sensitive CL targeted for M38. +willchan for net OWNERS review.
https://codereview.chromium.org/433283002/diff/100001/components/copresence/r... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/100001/components/copresence/r... components/copresence/rpc/http_post.h:54: // TODO(ckehoe): Self-deletion is a possible source of memory leaks. During profile shutdown, all the network objects that the URLRequestContextGetter points to will be deleted. If the callback gets invoked during shutdown, we now have use after frees, which can be a security hole. I don't think you can leave this as a TODO.
https://codereview.chromium.org/433283002/diff/100001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/100001/components/copresence/r... components/copresence/rpc/http_post.cc:39: // TODO(ckehoe): Replace this with an app-specific key To clarify, we may still need the Chrome API key if the app doesn't provide its own. https://codereview.chromium.org/433283002/diff/100001/components/copresence/r... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/100001/components/copresence/r... components/copresence/rpc/http_post.h:54: // TODO(ckehoe): Self-deletion is a possible source of memory leaks. On 2014/08/06 23:57:39, willchan wrote: > During profile shutdown, all the network objects that the > URLRequestContextGetter points to will be deleted. If the callback gets invoked > during shutdown, we now have use after frees, which can be a security hole. I > don't think you can leave this as a TODO. Ok. Before I write the code, can you confirm if this solution will work? 1. Don't have HttpPost self-delete. Instead, it is owned by (the persistent) RpcHandler, which is the only one who will care about HTTP responses anyway. 2. Pass the HttpPost as a WeakPtr<net::URLFetcherDelegate>. Then if the callback is invoked during shutdown, it will just be ignored.
https://codereview.chromium.org/433283002/diff/100001/components/copresence/r... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/100001/components/copresence/r... components/copresence/rpc/http_post.h:54: // TODO(ckehoe): Self-deletion is a possible source of memory leaks. On 2014/08/07 00:08:24, Charlie wrote: > On 2014/08/06 23:57:39, willchan wrote: > > During profile shutdown, all the network objects that the > > URLRequestContextGetter points to will be deleted. If the callback gets > invoked > > during shutdown, we now have use after frees, which can be a security hole. I > > don't think you can leave this as a TODO. > > Ok. Before I write the code, can you confirm if this solution will work? > > 1. Don't have HttpPost self-delete. Instead, it is owned by (the persistent) > RpcHandler, which is the only one who will care about HTTP responses anyway. > 2. Pass the HttpPost as a WeakPtr<net::URLFetcherDelegate>. Then if the callback > is invoked during shutdown, it will just be ignored. Sorry, I just got started with the review. But I saw that and immediately sent a comment. I don't know how this code fits in, so I need to read up more before I can advise you how best to fix this. But basically, you need to make sure that when profiles and the browser process shutdown, they cancel all URLFetchers that they own. I don't know what owns this network request, so I need to go read this component. It'll take awhile.
https://codereview.chromium.org/433283002/diff/100001/components/copresence/r... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/100001/components/copresence/r... components/copresence/rpc/http_post.h:54: // TODO(ckehoe): Self-deletion is a possible source of memory leaks. On 2014/08/07 00:08:24, Charlie wrote: > On 2014/08/06 23:57:39, willchan wrote: > > During profile shutdown, all the network objects that the > > URLRequestContextGetter points to will be deleted. If the callback gets > invoked > > during shutdown, we now have use after frees, which can be a security hole. I > > don't think you can leave this as a TODO. > > Ok. Before I write the code, can you confirm if this solution will work? > > 1. Don't have HttpPost self-delete. Instead, it is owned by (the persistent) > RpcHandler, which is the only one who will care about HTTP responses anyway. > 2. Pass the HttpPost as a WeakPtr<net::URLFetcherDelegate>. Then if the callback > is invoked during shutdown, it will just be ignored. I would assume this is what you'd do? 1.) Keep a vector of HTTP posts created by the rpc handler in the handler that creates it. 2.) Rename DisconnectFromWhispernet() to Shutdown(). This method will get called before a profile destructs. In the Shutdown method, disconnect from whispernet as usual, and also delete all our http posts. Once deleted, we don't have to worry about the callback. I am not sure why the HttpPost needs to be a WeakPtr.
Ok, how's this?
Nifty. lgtm to me, still need Will to approve though.
https://codereview.chromium.org/433283002/diff/120001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/120001/components/copresence/r... components/copresence/rpc/http_post.cc:74: url_fetcher_.reset(); why do you need to explicitly reset this? it's a scoped_ptr, so it should already be destroyed when you delete the HttpPost
https://codereview.chromium.org/433283002/diff/120001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/120001/components/copresence/r... components/copresence/rpc/http_post.cc:74: url_fetcher_.reset(); On 2014/08/07 17:55:22, Daniel Erat wrote: > why do you need to explicitly reset this? it's a scoped_ptr, so it should > already be destroyed when you delete the HttpPost Good point. Replacing with a comment.
https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... components/copresence/rpc/http_post.cc:74: // This deletes the url_fetcher_, which aborts the request. Nit: The comment is not necessary :)
https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... components/copresence/rpc/http_post.cc:74: // This deletes the url_fetcher_, which aborts the request. On 2014/08/07 18:01:04, Rahul Chaturvedi wrote: > Nit: The comment is not necessary :) Technically no comments are necessary unless the style guide explicitly requires them. But this is clearer. If it were really so obvious, we would all have seen the problem before Will pointed it out.
https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... components/copresence/rpc/http_post.cc:74: // This deletes the url_fetcher_, which aborts the request. On 2014/08/07 18:05:01, Charlie wrote: > On 2014/08/07 18:01:04, Rahul Chaturvedi wrote: > > Nit: The comment is not necessary :) > > Technically no comments are necessary unless the style guide explicitly requires > them. But this is clearer. If it were really so obvious, we would all have seen > the problem before Will pointed it out. It's a nit so feel free to address it how you want.
Hi guys, I don't see a problem adding a dep on google_apis for access to the api keys. On a different note, should any of this code actually live in google_apis? Seems like it makes sense for copresence to be a component, but are there low-level accesses to google apis that could be placed in a common location like google_apis?
On 2014/08/07 19:53:18, Roger Tawa (build sheriff) wrote: > Hi guys, > > I don't see a problem adding a dep on google_apis for access to the api keys. > > On a different note, should any of this code actually live in google_apis? > Seems like it makes sense for copresence to be a component, but are there > low-level accesses to google apis that could be placed in a common location like > google_apis? I believe all our code is specific to copresence and not reusable by any other component in Chrome.
lgtm
I'm still going through this, but here's a first pass. https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... components/copresence/rpc/http_post.cc:74: // This deletes the url_fetcher_, which aborts the request. On 2014/08/07 19:25:06, Rahul Chaturvedi wrote: > On 2014/08/07 18:05:01, Charlie wrote: > > On 2014/08/07 18:01:04, Rahul Chaturvedi wrote: > > > Nit: The comment is not necessary :) > > > > Technically no comments are necessary unless the style guide explicitly > requires > > them. But this is clearer. If it were really so obvious, we would all have > seen > > the problem before Will pointed it out. > > It's a nit so feel free to address it how you want. It's a little odd that HttpPost::Cancel() does `delete this;` Why not just have the owner (the RpcHandler?) delete the HttpPost? https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... components/copresence/rpc/http_post.h:35: public base::SupportsWeakPtr<HttpPost> { Can you explain why this inherits from SupportsWeakPtr? That is generally discouraged because we like to have explicit ownership. If someone else is holding a WeakPtr<Foo>, it is more difficult to understand the lifetime of Foo. If you need it internally for base::Bind(), I recommend using WeakPtrFactory instead, since that don't expose WeakPtrs outside the class, which helps consumers of this class understand the lifetime contract. https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... File components/copresence/rpc/rpc_handler.h (right): https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... components/copresence/rpc/rpc_handler.h:31: class RpcHandler : public base::SupportsWeakPtr<RpcHandler> { My same comment about base::SupportsWeakPtr applies here too.
I looked for the hook into the browser. This component seems to stand alone (cs.chromium.org can't find any usage of it). I can't figure out what owns this component. Is there a design doc for this so I can read through to understand? Is the BrowserProcess object supposed to own this? Is the Profile? What thread is this supposed to run on? URLRequestContextGetter is typically acquired on the UI thread, but needs to be used on the IO thread. I don't see any posting across threads. It makes me wonder if this code has a bunch of race conditions. https://codereview.chromium.org/433283002/diff/140001/components/copresence/c... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/433283002/diff/140001/components/copresence/c... components/copresence/copresence_client.cc:31: AsWeakPtr(), Why is AsWeakPtr() used here? Doesn't CopresenceClient own rpc_handler_? I assume so, since it uses a scoped_ptr<RpcHandler>. If I'm correct, then why is a weak pointer necessary? Whenever the CopresenceClient gets deleted, it will delete RpcHandler. Using WeakPtrs like this just makes users ask more questions about the lifetime of the various objects. What owns what? https://codereview.chromium.org/433283002/diff/140001/components/copresence/c... components/copresence/copresence_client.cc:46: delegate_->GetWhispernetClient()->Shutdown(); Who's the delegate of CopresenceClient? Who is calling CopresenceClient::Shutdown()? Are they the same entity? If not, what's the lifetime relationship between them? Why is there a Shutdown() in the first place? Why not just delete CopresenceClient? https://codereview.chromium.org/433283002/diff/140001/components/copresence/p... File components/copresence/public/copresence_client_delegate.h (right): https://codereview.chromium.org/433283002/diff/140001/components/copresence/p... components/copresence/public/copresence_client_delegate.h:37: virtual net::URLRequestContextGetter* GetRequestContext() const = 0; What's the lifetime of this URLRequestContextGetter supposed to be? How does this work during shutdown when the URLRequestContextGetter is getting destroyed?
On 2014/08/07 20:42:43, willchan wrote: > I looked for the hook into the browser. This component seems to stand alone > (http://cs.chromium.org can't find any usage of it). I can't figure out what owns this > component. Is there a design doc for this so I can read through to understand? > Is the BrowserProcess object supposed to own this? Is the Profile? What thread > is this supposed to run on? URLRequestContextGetter is typically acquired on the > UI thread, but needs to be used on the IO thread. I don't see any posting across > threads. It makes me wonder if this code has a bunch of race conditions. The RpcHandler is owned by the CopresenceClient which is owned by CopresenceService. CopresenceService is a browser context keyed service (you can see its implementation at https://codereview.chromium.org/444513005/) I think we should be fine with just destructing the client in CopresenceService::Shutdown(). I'll go ahead and make that change to the CL that contains that code. So, CopresenceService overrides the Shutdown method to disconnect/cleanup objects. CopresenceService::Shutdown() deletes CopresenceClient which delete the RpcHandler, which will cancel any remaining HTTPPosts (Charlie is sending out a patch that does this). This will ensure that before the profile dies, all our url requests will be canceled/deleted, hence I believe we should be safe in our usage of UrlRequestContextGetter. Anything I may be missing here?
Thanks for the pointers. That helps immensely.
On 2014/08/07 21:40:28, willchan wrote: > Thanks for the pointers. That helps immensely. Uploaded a patch to https://codereview.chromium.org/444513005/ to now destruct the CopresenceClient on Shutdown() getting called (and prevents its recreation in case an API tries to use it).
Sorry if I missed any comments. Codereview seems not to be tracking them. https://codereview.chromium.org/433283002/diff/140001/components/copresence/c... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/433283002/diff/140001/components/copresence/c... components/copresence/copresence_client.cc:31: AsWeakPtr(), On 2014/08/07 20:42:43, willchan wrote: > Why is AsWeakPtr() used here? Doesn't CopresenceClient own rpc_handler_? I > assume so, since it uses a scoped_ptr<RpcHandler>. If I'm correct, then why is a > weak pointer necessary? Whenever the CopresenceClient gets deleted, it will > delete RpcHandler. > > Using WeakPtrs like this just makes users ask more questions about the lifetime > of the various objects. What owns what? Removed. https://codereview.chromium.org/433283002/diff/140001/components/copresence/c... components/copresence/copresence_client.cc:46: delegate_->GetWhispernetClient()->Shutdown(); On 2014/08/07 20:42:43, willchan wrote: > Who's the delegate of CopresenceClient? Who is calling > CopresenceClient::Shutdown()? Are they the same entity? If not, what's the > lifetime relationship between them? Why is there a Shutdown() in the first > place? Why not just delete CopresenceClient? Addressed by Rahul? https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... components/copresence/rpc/http_post.cc:74: // This deletes the url_fetcher_, which aborts the request. On 2014/08/07 20:27:05, willchan wrote: > On 2014/08/07 19:25:06, Rahul Chaturvedi wrote: > > On 2014/08/07 18:05:01, Charlie wrote: > > > On 2014/08/07 18:01:04, Rahul Chaturvedi wrote: > > > > Nit: The comment is not necessary :) > > > > > > Technically no comments are necessary unless the style guide explicitly > > requires > > > them. But this is clearer. If it were really so obvious, we would all have > > seen > > > the problem before Will pointed it out. > > > > It's a nit so feel free to address it how you want. > > It's a little odd that HttpPost::Cancel() does `delete this;` > > Why not just have the owner (the RpcHandler?) delete the HttpPost? Notice that the HttpPost destructor is private. If we're counting on this object deleting itself, we want to have explicit control over deletion. Having this method in the API forces deletion to be explicit - i.e. the HttpPost cannot just be allowed to go out of scope. https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... components/copresence/rpc/http_post.h:35: public base::SupportsWeakPtr<HttpPost> { You're right, WeakPtr isn't needed. It actually isn't used at all (anymore). On 2014/08/07 20:27:05, willchan wrote: > Can you explain why this inherits from SupportsWeakPtr? That is generally > discouraged because we like to have explicit ownership. If someone else is > holding a WeakPtr<Foo>, it is more difficult to understand the lifetime of Foo. > If you need it internally for base::Bind(), I recommend using WeakPtrFactory > instead, since that don't expose WeakPtrs outside the class, which helps > consumers of this class understand the lifetime contract. https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... File components/copresence/rpc/rpc_handler.h (right): https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... components/copresence/rpc/rpc_handler.h:31: class RpcHandler : public base::SupportsWeakPtr<RpcHandler> { On 2014/08/07 20:27:05, willchan wrote: > My same comment about base::SupportsWeakPtr applies here too. Done. Using base::Unretained(this) instead, and revoking the pointers on shutdown/destruction.
https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... components/copresence/rpc/http_post.cc:74: // This deletes the url_fetcher_, which aborts the request. On 2014/08/07 22:40:58, Charlie wrote: > On 2014/08/07 20:27:05, willchan wrote: > > On 2014/08/07 19:25:06, Rahul Chaturvedi wrote: > > > On 2014/08/07 18:05:01, Charlie wrote: > > > > On 2014/08/07 18:01:04, Rahul Chaturvedi wrote: > > > > > Nit: The comment is not necessary :) > > > > > > > > Technically no comments are necessary unless the style guide explicitly > > > requires > > > > them. But this is clearer. If it were really so obvious, we would all have > > > seen > > > > the problem before Will pointed it out. > > > > > > It's a nit so feel free to address it how you want. > > > > It's a little odd that HttpPost::Cancel() does `delete this;` > > > > Why not just have the owner (the RpcHandler?) delete the HttpPost? > > Notice that the HttpPost destructor is private. If we're counting on this object > deleting itself, we want to have explicit control over deletion. Having this > method in the API forces deletion to be explicit - i.e. the HttpPost cannot just > be allowed to go out of scope. > But isn't this weird? This sounds like: class Foo { private: void MyPrivateFunction() { ... } public: void SecretAccessToMyPrivateFunction() { MyPrivateFunction(); } }; Replace MyPrivateFunction with the destructor, and SecretAccessToMyPrivateFunction() with Cancel(). Generally speaking, if Foo is able to delete Bar, then Foo probably owns Bar. And if you want to keep ownership clean, then you probably want Foo to be the only thing that deletes Bar. And not to allow Bar to delete itself.
https://codereview.chromium.org/433283002/diff/180001/components/copresence/c... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/433283002/diff/180001/components/copresence/c... components/copresence/copresence_client.cc:46: void CopresenceClient::Shutdown() { Get rid of Shutdown. We aren't using it anymore from the Copresence service, we now directly just delete the client. Also, don't bother calling WhispernetClient()::Shutdown either. We delete the whispernet client also on Shutdown (after the CopresenceClient is deleted).
https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/140001/components/copresence/r... components/copresence/rpc/http_post.cc:74: // This deletes the url_fetcher_, which aborts the request. On 2014/08/07 22:47:52, willchan wrote: > On 2014/08/07 22:40:58, Charlie wrote: > > On 2014/08/07 20:27:05, willchan wrote: > > > On 2014/08/07 19:25:06, Rahul Chaturvedi wrote: > > > > On 2014/08/07 18:05:01, Charlie wrote: > > > > > On 2014/08/07 18:01:04, Rahul Chaturvedi wrote: > > > > > > Nit: The comment is not necessary :) > > > > > > > > > > Technically no comments are necessary unless the style guide explicitly > > > > requires > > > > > them. But this is clearer. If it were really so obvious, we would all > have > > > > seen > > > > > the problem before Will pointed it out. > > > > > > > > It's a nit so feel free to address it how you want. > > > > > > It's a little odd that HttpPost::Cancel() does `delete this;` > > > > > > Why not just have the owner (the RpcHandler?) delete the HttpPost? > > > > Notice that the HttpPost destructor is private. If we're counting on this > object > > deleting itself, we want to have explicit control over deletion. Having this > > method in the API forces deletion to be explicit - i.e. the HttpPost cannot > just > > be allowed to go out of scope. > > > > But isn't this weird? This sounds like: > > class Foo { > private: > void MyPrivateFunction() { ... } > > public: > void SecretAccessToMyPrivateFunction() { MyPrivateFunction(); } > }; > > Replace MyPrivateFunction with the destructor, and > SecretAccessToMyPrivateFunction() with Cancel(). > > Generally speaking, if Foo is able to delete Bar, then Foo probably owns Bar. > And if you want to keep ownership clean, then you probably want Foo to be the > only thing that deletes Bar. And not to allow Bar to delete itself. I guess that makes sense. If HttpPost deletes itself, it should be the only thing that deletes itself. And if not, someone else should have the sole responsibility. Fixed. https://codereview.chromium.org/433283002/diff/180001/components/copresence/c... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/433283002/diff/180001/components/copresence/c... components/copresence/copresence_client.cc:46: void CopresenceClient::Shutdown() { On 2014/08/07 22:49:51, Rahul Chaturvedi wrote: > Get rid of Shutdown. We aren't using it anymore from the Copresence service, we > now directly just delete the client. Also, don't bother calling > WhispernetClient()::Shutdown either. We delete the whispernet client also on > Shutdown (after the CopresenceClient is deleted). Done. Except we still need to notify WhispernetClient somehow that we are disappearing. Relying on someone else to do it isn't safe.
lgtm https://codereview.chromium.org/433283002/diff/200001/components/copresence/r... File components/copresence/rpc/rpc_handler.cc (right): https://codereview.chromium.org/433283002/diff/200001/components/copresence/r... components/copresence/rpc/rpc_handler.cc:217: // On delete, this request will be cancelled. nit: s/delete/destruction https://codereview.chromium.org/433283002/diff/200001/components/copresence/r... components/copresence/rpc/rpc_handler.cc:242: // On delete, this request will be cancelled. nit: ditto
https://codereview.chromium.org/433283002/diff/200001/components/copresence/r... File components/copresence/rpc/rpc_handler.cc (right): https://codereview.chromium.org/433283002/diff/200001/components/copresence/r... components/copresence/rpc/rpc_handler.cc:217: // On delete, this request will be cancelled. On 2014/08/07 23:41:18, Rahul Chaturvedi wrote: > nit: s/delete/destruction Done. https://codereview.chromium.org/433283002/diff/200001/components/copresence/r... components/copresence/rpc/rpc_handler.cc:242: // On delete, this request will be cancelled. On 2014/08/07 23:41:18, Rahul Chaturvedi wrote: > nit: ditto Done.
https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... components/copresence/rpc/http_post.cc:12: #include "content/public/browser/browser_context.h" Where is this used? https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... components/copresence/rpc/http_post.cc:45: CommandLine* command_line = CommandLine::ForCurrentProcess(); Only my personal preference (so you should feel free to ignore): Please pass this in instead of accessing CommandLine. CommandLine is a global variable. Generally speaking, it's bad to directly access global variables instead of passing in the input. I'm a firm believer in only accessing command line flags fairly close to main(), and then passing in the flag values to the relevant code. https://codereview.chromium.org/433283002/diff/220001/components/copresence/c... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/c... components/copresence/copresence_client.cc:38: // The WhispernetClient must discard this on Shutdown. I like that you got rid of the AsWeakPtr(). It's interesting to see that the WhispernetClient must discard this on Shutdown. Is this always true? Do they always have such a tight coupling? If so, maybe it's best to expose a single public API that encapsulates both of them, rather than let the embedder screw this up. What do you think? https://codereview.chromium.org/433283002/diff/220001/components/copresence/h... File components/copresence/handlers/directive_handler.h (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/h... components/copresence/handlers/directive_handler.h:29: // This function must be called before any others. If it's not safe to use this object before Initialize() is called, why not combine this code with the constructor? https://codereview.chromium.org/433283002/diff/220001/components/copresence/h... components/copresence/handlers/directive_handler.h:35: virtual void AddDirective(const copresence::Directive& directive); Why are these virtual? https://codereview.chromium.org/433283002/diff/220001/components/copresence/p... File components/copresence/public/copresence_client.h (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/p... components/copresence/public/copresence_client.h:45: virtual ~CopresenceClient(); Why is this virtual? https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/http_post.cc:7: #include <google/protobuf/message_lite.h> Sometimes we use the system protobufs and sometimes we use the protobufs in third_party. You should make sure this works in both. See https://code.google.com/p/chromium/codesearch#chromium/src/dbus/message.cc&q=... for an example. https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/http_post.cc:56: DCHECK(request_proto.SerializeToString(&request_data)); Isn't this a bug? You're only going to serialize this proto into |request_data| in debug mode? https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/http_post.cc:83: LOG(ERROR) << "Couldn't contact the Copresence server at " This is a WARNING. There are plenty of valid reasons not to be able to reach the Copresence server. https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/http_post.h:50: const ResponseCallback& response_callback); This response callback doesn't make sense to me. It's redundant. This sounds like. Mom: Son, I'm dropping you off at school. Here's a cell phone so you can text me to let me know when to pick you up. Remember to mention who you are in the text message. Son: Why do I have to do that mom? You're the one giving me this cell phone and it has a unique phone number. Why do I have to say who I am? Mom: Because I said so. Mom is the owner of the HttpPost. The HttpPost is the Son. It doesn't make sense that the ResponseCallback (the cell phone) needs to take the HttpPost* as a parameter. The HttpPost owner is creating the HttpPost::ResponseCallback. Create a callback that is already bound to the appropriate HttpPost*. https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... File components/copresence/rpc/http_post_unittest.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/http_post_unittest.cc:5: #include "base/command_line.h" Why is this here? https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/http_post_unittest.cc:8: #include "components/copresence/rpc/http_post.h" This goes first, as per style guide.
https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... components/copresence/rpc/http_post.cc:45: CommandLine* command_line = CommandLine::ForCurrentProcess(); On 2014/08/08 01:05:58, willchan wrote: > Only my personal preference (so you should feel free to ignore): > Please pass this in instead of accessing CommandLine. CommandLine is a global > variable. Generally speaking, it's bad to directly access global variables > instead of passing in the input. I'm a firm believer in only accessing command > line flags fairly close to main(), and then passing in the flag values to the > relevant code. We'd have to access this global object 'somewhere'. I don't see anywhere this can go where the decrease in the distance from main will be more than an epsilon. https://codereview.chromium.org/433283002/diff/220001/components/copresence/c... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/c... components/copresence/copresence_client.cc:38: // The WhispernetClient must discard this on Shutdown. On 2014/08/08 01:05:58, willchan wrote: > I like that you got rid of the AsWeakPtr(). It's interesting to see that the > WhispernetClient must discard this on Shutdown. Is this always true? Do they > always have such a tight coupling? If so, maybe it's best to expose a single > public API that encapsulates both of them, rather than let the embedder screw > this up. What do you think? So this is actually a bug. If the whispernet client takes longer to initialize than the lifetime of CopresenceClient (for example, in case the whispernet init is stalled during which we are done with the CopresenceClient and destruct it), this will be a user-after-free. I actually thought we were getting rid of the weak pointer only in the RPC handler, I didn't realize we removed them from here too. I believe a WeakPtr is a better solution here than imposing lifetime requirements on the Whispernet client. This is one of the few cases that a WeakPtr is actually good for (when you are given a object that you do not have the control on the lifetime of). I would recommend that this code keep using WeakPtrs. This 'can' be done without it by specifically adding code to the whispernet client to abort an initialization but the resulting code will be uglier than simply using a WeakPtr here. (btw, I am not completely sure what you mean by a single public API? If you're talking about combining the RPC handler and the whispernet client, that is not feasible since the whispernet client is created in the browser since it is dependent on it; the RPC client has no reason to live in the browser.) https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/http_post.cc:56: DCHECK(request_proto.SerializeToString(&request_data)); On 2014/08/08 01:05:59, willchan wrote: > Isn't this a bug? You're only going to serialize this proto into |request_data| > in debug mode? This is a bug.
https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... components/copresence/rpc/http_post.cc:45: CommandLine* command_line = CommandLine::ForCurrentProcess(); On 2014/08/08 01:29:07, Rahul Chaturvedi wrote: > On 2014/08/08 01:05:58, willchan wrote: > > Only my personal preference (so you should feel free to ignore): > > Please pass this in instead of accessing CommandLine. CommandLine is a global > > variable. Generally speaking, it's bad to directly access global variables > > instead of passing in the input. I'm a firm believer in only accessing command > > line flags fairly close to main(), and then passing in the flag values to the > > relevant code. > > We'd have to access this global object 'somewhere'. I don't see anywhere this > can go where the decrease in the distance from main will be more than an > epsilon. Replying just to your point, but again, it's just a personal preference, so consider it non-blocking for this review. I think even an epsilon is important. For example, let's say we have a bunch of factory employees. The manager tells them that lunch time begins when the sun is at its highest point in the sky. Now, the sun and the sky is part of the global environment. Do the workers really care about the sun and the sky here? No, they just need to know when it's time for lunch. And if the sky is cloudy, it's silly for all of them not to get lunch. Instead of having a global dependency, the manager should just tell the workers when lunch time is, or scope it to something like a clock. This also makes it more testable, since managers can change when they announce lunch time, or can change the time on the clock. It's much easier to do that, rather than change the position of the sun in the sky. It's true, it might be a small epsilon. Maybe the managers themselves decide lunch time based on the position of the sun in the sky. But at least all the workers don't have to. That's still a win. Enforcing good encapsulation boundaries and explicitly passing dependencies is good code hygiene and we should encourage this. https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... components/copresence/rpc/http_post.cc:47: url = net::AppendQueryParameter( I don't see a unit test for this. Can you add one? https://codereview.chromium.org/433283002/diff/220001/components/copresence/c... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/c... components/copresence/copresence_client.cc:38: // The WhispernetClient must discard this on Shutdown. On 2014/08/08 01:29:07, Rahul Chaturvedi wrote: > On 2014/08/08 01:05:58, willchan wrote: > > I like that you got rid of the AsWeakPtr(). It's interesting to see that the > > WhispernetClient must discard this on Shutdown. Is this always true? Do they > > always have such a tight coupling? If so, maybe it's best to expose a single > > public API that encapsulates both of them, rather than let the embedder screw > > this up. What do you think? > > So this is actually a bug. If the whispernet client takes longer to initialize > than the lifetime of CopresenceClient (for example, in case the whispernet init > is stalled during which we are done with the CopresenceClient and destruct it), > this will be a user-after-free. > > I actually thought we were getting rid of the weak pointer only in the RPC > handler, I didn't realize we removed them from here too. I believe a WeakPtr is > a better solution here than imposing lifetime requirements on the Whispernet > client. > > This is one of the few cases that a WeakPtr is actually good for (when you are > given a object that you do not have the control on the lifetime of). I would > recommend that this code keep using WeakPtrs. This 'can' be done without it by > specifically adding code to the whispernet client to abort an initialization but > the resulting code will be uglier than simply using a WeakPtr here. > > > (btw, I am not completely sure what you mean by a single public API? If you're > talking about combining the RPC handler and the whispernet client, that is not > feasible since the whispernet client is created in the browser since it is > dependent on it; the RPC client has no reason to live in the browser.) I chatted with rkc@ offline about this, but I can't review this without understanding the expected lifetime of the ChromeWhispernetClient. Please link me to that code. https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... File components/copresence/rpc/rpc_handler_unittest.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/rpc_handler_unittest.cc:16: #include "components/copresence/rpc/rpc_handler.h" This should go first, as per style guide.
I'm going to cut this review short because I have no time to review this in more detail. The net/ usage looks OK to me. Once my existing concerns are addressed, I'm fine with the approving this CL. There are still a number of outstanding issues though, and I'm still waiting on those. One of them is the use of WeakPtr<CopresenceClient> in a callback handed to the WhispernetClient. I'd prefer not to do this. As far as I can tell, the current layering is CopresenceClient uses the WhispernetClient. Currently, they have the same lifetime, but it's possible that WhispernetClient may have a longer lifetime later on. Either CopresenceClient will need to always provide a bunch of callbacks bound with WeakPtr<CopresenceClient> to WhispernetClient, or CopresenceClient needs to explicitly notify WhispernetClient when the CopresenceClient is going away, so the WhispernetClient can nuke the callbacks. Without looking at the code in more detail to understand the architecture, my inkling is to prefer the latter.
On 2014/08/08 20:55:53, willchan wrote: > I'm going to cut this review short because I have no time to review this in more > detail. The net/ usage looks OK to me. Once my existing concerns are addressed, > I'm fine with the approving this CL. > > There are still a number of outstanding issues though, and I'm still waiting on > those. One of them is the use of WeakPtr<CopresenceClient> in a callback handed > to the WhispernetClient. I'd prefer not to do this. As far as I can tell, the > current layering is CopresenceClient uses the WhispernetClient. Currently, they > have the same lifetime, but it's possible that WhispernetClient may have a > longer lifetime later on. Either CopresenceClient will need to always provide a > bunch of callbacks bound with WeakPtr<CopresenceClient> to WhispernetClient, or > CopresenceClient needs to explicitly notify WhispernetClient when the > CopresenceClient is going away, so the WhispernetClient can nuke the callbacks. > Without looking at the code in more detail to understand the architecture, my > inkling is to prefer the latter. We actually do do the latter, except for the initialize callback. It seems heavyweight to have a CancelInitialize() call in the WhispernetClient interface when we could just use WeakPtr instead. That said: Rahul, is there a reason the WhispernetClient can't be passed into CopresenceClient, and owned by it? Then I think we wouldn't have this problem. I do think it's cleaner to have the WhispernetClient as part of CopresenceClientDelegate. But I also don't think anyone besides CopresenceClient needs to use WhispernetClient.
Adding Colin to this CL, although not in a blocking fashion. Just wanted him to take a look at this component since the directory structure is different from most other components.
https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... File components/copresence/rpc/rpc_handler.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/rpc_handler.cc:280: base::Unretained(this))); It looks like the |this| here gets passed to WhispernetClient. If y'all do eventually change it so the WhispernetClient can outlive the CopresenceClient, then seems like a bug. How does this callback get disconnected? https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/rpc_handler.cc:483: if (whispernet_client) { Hm, why would this ever be NULL?
+darin for third_party/protobuf review. https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... components/copresence/rpc/http_post.cc:12: #include "content/public/browser/browser_context.h" On 2014/08/08 01:05:58, willchan wrote: > Where is this used? It's outdated. Deleted. https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... components/copresence/rpc/http_post.cc:45: CommandLine* command_line = CommandLine::ForCurrentProcess(); On 2014/08/08 18:01:20, willchan wrote: > On 2014/08/08 01:29:07, Rahul Chaturvedi wrote: > > On 2014/08/08 01:05:58, willchan wrote: > > > Only my personal preference (so you should feel free to ignore): > > > Please pass this in instead of accessing CommandLine. CommandLine is a > global > > > variable. Generally speaking, it's bad to directly access global variables > > > instead of passing in the input. I'm a firm believer in only accessing > command > > > line flags fairly close to main(), and then passing in the flag values to > the > > > relevant code. > > > > We'd have to access this global object 'somewhere'. I don't see anywhere this > > can go where the decrease in the distance from main will be more than an > > epsilon. > > Replying just to your point, but again, it's just a personal preference, so > consider it non-blocking for this review. I think even an epsilon is important. > For example, let's say we have a bunch of factory employees. The manager tells > them that lunch time begins when the sun is at its highest point in the sky. > > Now, the sun and the sky is part of the global environment. Do the workers > really care about the sun and the sky here? No, they just need to know when it's > time for lunch. And if the sky is cloudy, it's silly for all of them not to get > lunch. Instead of having a global dependency, the manager should just tell the > workers when lunch time is, or scope it to something like a clock. This also > makes it more testable, since managers can change when they announce lunch time, > or can change the time on the clock. It's much easier to do that, rather than > change the position of the sun in the sky. > > It's true, it might be a small epsilon. Maybe the managers themselves decide > lunch time based on the position of the sun in the sky. But at least all the > workers don't have to. That's still a win. Enforcing good encapsulation > boundaries and explicitly passing dependencies is good code hygiene and we > should encourage this. I don't get your argument. Why should main() "own" the flags? This flag doesn't, and shouldn't, concern anyone but copresence. Keeping it localized here seems like better encapsulation to me. There will always be global dependencies. Most of Chrome can't function without the base:: and net:: packages. What we don't want to do is to store state in them when that state should belong to us (think strtok vs. strtok_r). But we aren't changing the flag values. The user happens to have just one command line on which to put flags, and this is the way we pull out our piece. All of that said: you wanted a test for this, so it has to be injectable. So RpcHandler will inject it (from the command line). There's your epsilon. https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... components/copresence/rpc/http_post.cc:47: url = net::AppendQueryParameter( On 2014/08/08 18:01:20, willchan wrote: > I don't see a unit test for this. Can you add one? Done. https://codereview.chromium.org/433283002/diff/220001/components/copresence/c... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/c... components/copresence/copresence_client.cc:38: // The WhispernetClient must discard this on Shutdown. On 2014/08/08 01:29:07, Rahul Chaturvedi wrote: > On 2014/08/08 01:05:58, willchan wrote: > > I like that you got rid of the AsWeakPtr(). It's interesting to see that the > > WhispernetClient must discard this on Shutdown. Is this always true? Do they > > always have such a tight coupling? If so, maybe it's best to expose a single > > public API that encapsulates both of them, rather than let the embedder screw > > this up. What do you think? > > So this is actually a bug. If the whispernet client takes longer to initialize > than the lifetime of CopresenceClient (for example, in case the whispernet init > is stalled during which we are done with the CopresenceClient and destruct it), > this will be a user-after-free. > > I actually thought we were getting rid of the weak pointer only in the RPC > handler, I didn't realize we removed them from here too. I believe a WeakPtr is > a better solution here than imposing lifetime requirements on the Whispernet > client. > > This is one of the few cases that a WeakPtr is actually good for (when you are > given a object that you do not have the control on the lifetime of). I would > recommend that this code keep using WeakPtrs. This 'can' be done without it by > specifically adding code to the whispernet client to abort an initialization but > the resulting code will be uglier than simply using a WeakPtr here. > > > (btw, I am not completely sure what you mean by a single public API? If you're > talking about combining the RPC handler and the whispernet client, that is not > feasible since the whispernet client is created in the browser since it is > dependent on it; the RPC client has no reason to live in the browser.) Putting just this one back to AsWeakPtr(), and getting rid of the WhispernetClient::Shutdown() call. In theory we could instead add a CancelInitialize() method to cancel the callback, but that seems unnecessary when WeakPtr already solves the problem. https://codereview.chromium.org/433283002/diff/220001/components/copresence/h... File components/copresence/handlers/directive_handler.h (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/h... components/copresence/handlers/directive_handler.h:29: // This function must be called before any others. On 2014/08/08 01:05:59, willchan wrote: > If it's not safe to use this object before Initialize() is called, why not > combine this code with the constructor? We explicitly don't want these to be constructor parameters so that subclassing is simplified. https://codereview.chromium.org/433283002/diff/220001/components/copresence/h... components/copresence/handlers/directive_handler.h:35: virtual void AddDirective(const copresence::Directive& directive); On 2014/08/08 01:05:58, willchan wrote: > Why are these virtual? So the FakeDirectiveHandler can override them. See rpc_handler_unittest.cc. https://codereview.chromium.org/433283002/diff/220001/components/copresence/p... File components/copresence/public/copresence_client.h (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/p... components/copresence/public/copresence_client.h:45: virtual ~CopresenceClient(); On 2014/08/08 01:05:59, willchan wrote: > Why is this virtual? Oudated? WeakPtr is back. https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/http_post.cc:7: #include <google/protobuf/message_lite.h> On 2014/08/08 01:05:59, willchan wrote: > Sometimes we use the system protobufs and sometimes we use the protobufs in > third_party. You should make sure this works in both. See > https://code.google.com/p/chromium/codesearch#chromium/src/dbus/message.cc&q=... > for an example. Ok, but why? That seems like a mess. https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/http_post.cc:56: DCHECK(request_proto.SerializeToString(&request_data)); On 2014/08/08 01:29:07, Rahul Chaturvedi wrote: > On 2014/08/08 01:05:59, willchan wrote: > > Isn't this a bug? You're only going to serialize this proto into > |request_data| > > in debug mode? > > This is a bug. Fixed. https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/http_post.cc:83: LOG(ERROR) << "Couldn't contact the Copresence server at " On 2014/08/08 01:05:59, willchan wrote: > This is a WARNING. There are plenty of valid reasons not to be able to reach the > Copresence server. Done. https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/http_post.h:50: const ResponseCallback& response_callback); On 2014/08/08 01:05:59, willchan wrote: > This response callback doesn't make sense to me. It's redundant. This sounds > like. > > Mom: Son, I'm dropping you off at school. Here's a cell phone so you can text me > to let me know when to pick you up. Remember to mention who you are in the text > message. > Son: Why do I have to do that mom? You're the one giving me this cell phone and > it has a unique phone number. Why do I have to say who I am? > Mom: Because I said so. > > Mom is the owner of the HttpPost. The HttpPost is the Son. It doesn't make sense > that the ResponseCallback (the cell phone) needs to take the HttpPost* as a > parameter. The HttpPost owner is creating the HttpPost::ResponseCallback. Create > a callback that is already bound to the appropriate HttpPost*. Good idea. Fixed. Though I would have gotten it faster without the story :-) https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... File components/copresence/rpc/http_post_unittest.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/http_post_unittest.cc:5: #include "base/command_line.h" On 2014/08/08 01:05:59, willchan wrote: > Why is this here? Outdated. Removed. https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/http_post_unittest.cc:8: #include "components/copresence/rpc/http_post.h" On 2014/08/08 01:05:59, willchan wrote: > This goes first, as per style guide. Done. https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... File components/copresence/rpc/rpc_handler.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/rpc_handler.cc:483: if (whispernet_client) { On 2014/08/08 21:45:13, willchan wrote: > Hm, why would this ever be NULL? In a test, where we don't want to have a whispernet client. https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... File components/copresence/rpc/rpc_handler_unittest.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/rpc_handler_unittest.cc:16: #include "components/copresence/rpc/rpc_handler.h" On 2014/08/08 18:01:20, willchan wrote: > This should go first, as per style guide. I wondered about that. Fixed.
https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... File components/copresence/rpc/rpc_handler.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/rpc_handler.cc:280: base::Unretained(this))); On 2014/08/08 21:45:13, willchan wrote: > It looks like the |this| here gets passed to WhispernetClient. If y'all do > eventually change it so the WhispernetClient can outlive the CopresenceClient, > then seems like a bug. How does this callback get disconnected? |this| is getting passed to directive_handler_, not WhispernetClient.
https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... File components/copresence/rpc/http_post.h (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/http_post.h:50: const ResponseCallback& response_callback); On 2014/08/08 21:57:53, Charlie wrote: > On 2014/08/08 01:05:59, willchan wrote: > > This response callback doesn't make sense to me. It's redundant. This sounds > > like. > > > > Mom: Son, I'm dropping you off at school. Here's a cell phone so you can text > me > > to let me know when to pick you up. Remember to mention who you are in the > text > > message. > > Son: Why do I have to do that mom? You're the one giving me this cell phone > and > > it has a unique phone number. Why do I have to say who I am? > > Mom: Because I said so. > > > > Mom is the owner of the HttpPost. The HttpPost is the Son. It doesn't make > sense > > that the ResponseCallback (the cell phone) needs to take the HttpPost* as a > > parameter. The HttpPost owner is creating the HttpPost::ResponseCallback. > Create > > a callback that is already bound to the appropriate HttpPost*. > > Good idea. Fixed. > > Though I would have gotten it faster without the story :-) Turns out I did this one wrong after all. Stay tuned for a real fix.
https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... File components/copresence/rpc/http_post.cc (right): https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... components/copresence/rpc/http_post.cc:45: CommandLine* command_line = CommandLine::ForCurrentProcess(); On 2014/08/08 21:57:52, Charlie wrote: > On 2014/08/08 18:01:20, willchan wrote: > > On 2014/08/08 01:29:07, Rahul Chaturvedi wrote: > > > On 2014/08/08 01:05:58, willchan wrote: > > > > Only my personal preference (so you should feel free to ignore): > > > > Please pass this in instead of accessing CommandLine. CommandLine is a > > global > > > > variable. Generally speaking, it's bad to directly access global variables > > > > instead of passing in the input. I'm a firm believer in only accessing > > command > > > > line flags fairly close to main(), and then passing in the flag values to > > the > > > > relevant code. > > > > > > We'd have to access this global object 'somewhere'. I don't see anywhere > this > > > can go where the decrease in the distance from main will be more than an > > > epsilon. > > > > Replying just to your point, but again, it's just a personal preference, so > > consider it non-blocking for this review. I think even an epsilon is > important. > > For example, let's say we have a bunch of factory employees. The manager tells > > them that lunch time begins when the sun is at its highest point in the sky. > > > > Now, the sun and the sky is part of the global environment. Do the workers > > really care about the sun and the sky here? No, they just need to know when > it's > > time for lunch. And if the sky is cloudy, it's silly for all of them not to > get > > lunch. Instead of having a global dependency, the manager should just tell the > > workers when lunch time is, or scope it to something like a clock. This also > > makes it more testable, since managers can change when they announce lunch > time, > > or can change the time on the clock. It's much easier to do that, rather than > > change the position of the sun in the sky. > > > > It's true, it might be a small epsilon. Maybe the managers themselves decide > > lunch time based on the position of the sun in the sky. But at least all the > > workers don't have to. That's still a win. Enforcing good encapsulation > > boundaries and explicitly passing dependencies is good code hygiene and we > > should encourage this. > > I don't get your argument. Why should main() "own" the flags? This flag doesn't, > and shouldn't, concern anyone but copresence. Keeping it localized here seems > like better encapsulation to me. But the point is that it's not localized :) The concept of a command line is an application level concept, shared by the entire application. For example, what if your code is embedded in a non-browser application that uses command line flags different, perhaps via libcef (https://code.google.com/p/chromiumembedded/)? Or what if we want to let an enterprise group policy override your setting? Or what if we wanted to configure this via Finch? Basically, you're saying that your configuration comes from page 25 line 10 of this magic book of instructions. If anyone wants to configure you, they should go find this magic book (the base::CommandLine object), open it up to page 25 (the name of the flag), and rewrite line 10 (which is where the flag value is stored). And I'm saying, why do we need a whole book just to give you a single instruction? Recall the whole multiprofile and componentization project. Folks thought for the longest time that they would always have access to a single Profile object, and always pulled their data out from that global Profile*. Well, it turns out in some new Chromium ports, we may have multiple profiles, and sometimes we might not have Profiles at all. So we had to have a massive refactoring project to get rid of Profile* usage across large chunks of the code base. Lesson learned: stop reading from global variables (like the Profile* returned by GetDefaultProfile()) with large scopes. You should explicitly pass your parameters. > > There will always be global dependencies. Most of Chrome can't function without > the base:: and net:: packages. What we don't want to do is to store state in > them when that state should belong to us (think strtok vs. strtok_r). But we > aren't changing the flag values. The user happens to have just one command line > on which to put flags, and this is the way we pull out our piece. > > All of that said: you wanted a test for this, so it has to be injectable. So > RpcHandler will inject it (from the command line). There's your epsilon. That's definitely an improvement, and I think it still can be improved :) And as I said before, this is non-blocking, so go ahead and ignore it. But I hope that you will see why this is suboptimal.
On 2014/08/08 22:31:01, willchan wrote: > https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... > File components/copresence/rpc/http_post.cc (right): > > https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... > components/copresence/rpc/http_post.cc:45: CommandLine* command_line = > CommandLine::ForCurrentProcess(); > On 2014/08/08 21:57:52, Charlie wrote: > > On 2014/08/08 18:01:20, willchan wrote: > > > On 2014/08/08 01:29:07, Rahul Chaturvedi wrote: > > > > On 2014/08/08 01:05:58, willchan wrote: > > > > > Only my personal preference (so you should feel free to ignore): > > > > > Please pass this in instead of accessing CommandLine. CommandLine is a > > > global > > > > > variable. Generally speaking, it's bad to directly access global > variables > > > > > instead of passing in the input. I'm a firm believer in only accessing > > > command > > > > > line flags fairly close to main(), and then passing in the flag values > to > > > the > > > > > relevant code. > > > > > > > > We'd have to access this global object 'somewhere'. I don't see anywhere > > this > > > > can go where the decrease in the distance from main will be more than an > > > > epsilon. > > > > > > Replying just to your point, but again, it's just a personal preference, so > > > consider it non-blocking for this review. I think even an epsilon is > > important. > > > For example, let's say we have a bunch of factory employees. The manager > tells > > > them that lunch time begins when the sun is at its highest point in the sky. > > > > > > Now, the sun and the sky is part of the global environment. Do the workers > > > really care about the sun and the sky here? No, they just need to know when > > it's > > > time for lunch. And if the sky is cloudy, it's silly for all of them not to > > get > > > lunch. Instead of having a global dependency, the manager should just tell > the > > > workers when lunch time is, or scope it to something like a clock. This also > > > makes it more testable, since managers can change when they announce lunch > > time, > > > or can change the time on the clock. It's much easier to do that, rather > than > > > change the position of the sun in the sky. > > > > > > It's true, it might be a small epsilon. Maybe the managers themselves decide > > > lunch time based on the position of the sun in the sky. But at least all the > > > workers don't have to. That's still a win. Enforcing good encapsulation > > > boundaries and explicitly passing dependencies is good code hygiene and we > > > should encourage this. > > > > I don't get your argument. Why should main() "own" the flags? This flag > doesn't, > > and shouldn't, concern anyone but copresence. Keeping it localized here seems > > like better encapsulation to me. > > But the point is that it's not localized :) The concept of a command line is an > application level concept, shared by the entire application. For example, what > if your code is embedded in a non-browser application that uses command line > flags different, perhaps via libcef > (https://code.google.com/p/chromiumembedded/) Or what if we want to let an > enterprise group policy override your setting? Or what if we wanted to configure > this via Finch? > > Basically, you're saying that your configuration comes from page 25 line 10 of > this magic book of instructions. If anyone wants to configure you, they should > go find this magic book (the base::CommandLine object), open it up to page 25 > (the name of the flag), and rewrite line 10 (which is where the flag value is > stored). And I'm saying, why do we need a whole book just to give you a single > instruction? > > Recall the whole multiprofile and componentization project. Folks thought for > the longest time that they would always have access to a single Profile object, > and always pulled their data out from that global Profile*. Well, it turns out > in some new Chromium ports, we may have multiple profiles, and sometimes we > might not have Profiles at all. So we had to have a massive refactoring project > to get rid of Profile* usage across large chunks of the code base. > > Lesson learned: stop reading from global variables (like the Profile* returned > by GetDefaultProfile()) with large scopes. You should explicitly pass your > parameters. > > > > > There will always be global dependencies. Most of Chrome can't function > without > > the base:: and net:: packages. What we don't want to do is to store state in > > them when that state should belong to us (think strtok vs. strtok_r). But we > > aren't changing the flag values. The user happens to have just one command > line > > on which to put flags, and this is the way we pull out our piece. > > > > All of that said: you wanted a test for this, so it has to be injectable. So > > RpcHandler will inject it (from the command line). There's your epsilon. > > That's definitely an improvement, and I think it still can be improved :) And as > I said before, this is non-blocking, so go ahead and ignore it. But I hope that > you will see why this is suboptimal. Actually, you may be interested to know that a chrome://copresence page is on our todo list, where all of these settings will be available. And then all of our flags will get deleted. Tada!
On 2014/08/08 22:33:41, Charlie wrote: > On 2014/08/08 22:31:01, willchan wrote: > > > https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... > > File components/copresence/rpc/http_post.cc (right): > > > > > https://codereview.chromium.org/433283002/diff/180001/components/copresence/r... > > components/copresence/rpc/http_post.cc:45: CommandLine* command_line = > > CommandLine::ForCurrentProcess(); > > On 2014/08/08 21:57:52, Charlie wrote: > > > On 2014/08/08 18:01:20, willchan wrote: > > > > On 2014/08/08 01:29:07, Rahul Chaturvedi wrote: > > > > > On 2014/08/08 01:05:58, willchan wrote: > > > > > > Only my personal preference (so you should feel free to ignore): > > > > > > Please pass this in instead of accessing CommandLine. CommandLine is a > > > > global > > > > > > variable. Generally speaking, it's bad to directly access global > > variables > > > > > > instead of passing in the input. I'm a firm believer in only accessing > > > > command > > > > > > line flags fairly close to main(), and then passing in the flag values > > to > > > > the > > > > > > relevant code. > > > > > > > > > > We'd have to access this global object 'somewhere'. I don't see anywhere > > > this > > > > > can go where the decrease in the distance from main will be more than an > > > > > epsilon. > > > > > > > > Replying just to your point, but again, it's just a personal preference, > so > > > > consider it non-blocking for this review. I think even an epsilon is > > > important. > > > > For example, let's say we have a bunch of factory employees. The manager > > tells > > > > them that lunch time begins when the sun is at its highest point in the > sky. > > > > > > > > Now, the sun and the sky is part of the global environment. Do the workers > > > > really care about the sun and the sky here? No, they just need to know > when > > > it's > > > > time for lunch. And if the sky is cloudy, it's silly for all of them not > to > > > get > > > > lunch. Instead of having a global dependency, the manager should just tell > > the > > > > workers when lunch time is, or scope it to something like a clock. This > also > > > > makes it more testable, since managers can change when they announce lunch > > > time, > > > > or can change the time on the clock. It's much easier to do that, rather > > than > > > > change the position of the sun in the sky. > > > > > > > > It's true, it might be a small epsilon. Maybe the managers themselves > decide > > > > lunch time based on the position of the sun in the sky. But at least all > the > > > > workers don't have to. That's still a win. Enforcing good encapsulation > > > > boundaries and explicitly passing dependencies is good code hygiene and we > > > > should encourage this. > > > > > > I don't get your argument. Why should main() "own" the flags? This flag > > doesn't, > > > and shouldn't, concern anyone but copresence. Keeping it localized here > seems > > > like better encapsulation to me. > > > > But the point is that it's not localized :) The concept of a command line is > an > > application level concept, shared by the entire application. For example, what > > if your code is embedded in a non-browser application that uses command line > > flags different, perhaps via libcef > > (https://code.google.com/p/chromiumembedded/) Or what if we want to let an > > enterprise group policy override your setting? Or what if we wanted to > configure > > this via Finch? > > > > Basically, you're saying that your configuration comes from page 25 line 10 of > > this magic book of instructions. If anyone wants to configure you, they should > > go find this magic book (the base::CommandLine object), open it up to page 25 > > (the name of the flag), and rewrite line 10 (which is where the flag value is > > stored). And I'm saying, why do we need a whole book just to give you a single > > instruction? > > > > Recall the whole multiprofile and componentization project. Folks thought for > > the longest time that they would always have access to a single Profile > object, > > and always pulled their data out from that global Profile*. Well, it turns out > > in some new Chromium ports, we may have multiple profiles, and sometimes we > > might not have Profiles at all. So we had to have a massive refactoring > project > > to get rid of Profile* usage across large chunks of the code base. > > > > Lesson learned: stop reading from global variables (like the Profile* returned > > by GetDefaultProfile()) with large scopes. You should explicitly pass your > > parameters. > > > > > > > > There will always be global dependencies. Most of Chrome can't function > > without > > > the base:: and net:: packages. What we don't want to do is to store state in > > > them when that state should belong to us (think strtok vs. strtok_r). But we > > > aren't changing the flag values. The user happens to have just one command > > line > > > on which to put flags, and this is the way we pull out our piece. > > > > > > All of that said: you wanted a test for this, so it has to be injectable. So > > > RpcHandler will inject it (from the command line). There's your epsilon. > > > > That's definitely an improvement, and I think it still can be improved :) And > as > > I said before, this is non-blocking, so go ahead and ignore it. But I hope > that > > you will see why this is suboptimal. > > Actually, you may be interested to know that a chrome://copresence page is on > our todo list, where all of these settings will be available. And then all of > our flags will get deleted. Tada! Great!
https://codereview.chromium.org/433283002/diff/220001/components/copresence/c... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/c... components/copresence/copresence_client.cc:38: // The WhispernetClient must discard this on Shutdown. On 2014/08/08 21:57:52, Charlie wrote: > On 2014/08/08 01:29:07, Rahul Chaturvedi wrote: > > On 2014/08/08 01:05:58, willchan wrote: > > > I like that you got rid of the AsWeakPtr(). It's interesting to see that the > > > WhispernetClient must discard this on Shutdown. Is this always true? Do they > > > always have such a tight coupling? If so, maybe it's best to expose a single > > > public API that encapsulates both of them, rather than let the embedder > screw > > > this up. What do you think? > > > > So this is actually a bug. If the whispernet client takes longer to initialize > > than the lifetime of CopresenceClient (for example, in case the whispernet > init > > is stalled during which we are done with the CopresenceClient and destruct > it), > > this will be a user-after-free. > > > > I actually thought we were getting rid of the weak pointer only in the RPC > > handler, I didn't realize we removed them from here too. I believe a WeakPtr > is > > a better solution here than imposing lifetime requirements on the Whispernet > > client. > > > > This is one of the few cases that a WeakPtr is actually good for (when you are > > given a object that you do not have the control on the lifetime of). I would > > recommend that this code keep using WeakPtrs. This 'can' be done without it by > > specifically adding code to the whispernet client to abort an initialization > but > > the resulting code will be uglier than simply using a WeakPtr here. > > > > > > (btw, I am not completely sure what you mean by a single public API? If you're > > talking about combining the RPC handler and the whispernet client, that is not > > feasible since the whispernet client is created in the browser since it is > > dependent on it; the RPC client has no reason to live in the browser.) > > Putting just this one back to AsWeakPtr(), and getting rid of the > WhispernetClient::Shutdown() call. In theory we could instead add a > CancelInitialize() method to cancel the callback, but that seems unnecessary > when WeakPtr already solves the problem. Please don't use SupportsWeakPtr(), there's reasonably strong consensus that it's a bad idea and we should kill it -https://groups.google.com/a/chromium.org/d/msg/chromium-dev/N6w_CkKEwvE/evs2pRDD0loJ. If you must use WeakPtrs, use a WeakPtrFactory instead. https://codereview.chromium.org/433283002/diff/220001/components/copresence/h... File components/copresence/handlers/directive_handler.h (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/h... components/copresence/handlers/directive_handler.h:29: // This function must be called before any others. On 2014/08/08 21:57:52, Charlie wrote: > On 2014/08/08 01:05:59, willchan wrote: > > If it's not safe to use this object before Initialize() is called, why not > > combine this code with the constructor? > > We explicitly don't want these to be constructor parameters so that subclassing > is simplified. Can you explain further? Can't subclasses just call the parent constructor? https://codereview.chromium.org/433283002/diff/220001/components/copresence/h... components/copresence/handlers/directive_handler.h:35: virtual void AddDirective(const copresence::Directive& directive); On 2014/08/08 21:57:52, Charlie wrote: > On 2014/08/08 01:05:58, willchan wrote: > > Why are these virtual? > > So the FakeDirectiveHandler can override them. See rpc_handler_unittest.cc. Got it, that makes sense. My personal suggestion, which I admit is very debatable, is if you're going to override for testing purposes, to make the base class an abstract interface. Otherwise, it becomes a bit difficult to understand how your test subclass will interact with existing behavior in the production class. In other words: 1) DirectiveHandler - abstract interface 2) DirectiveHandlerImpl - has all the production implementation 3) MockDirectiveHandler - lets you mock out the DirectiveHandler dependency https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... File components/copresence/rpc/rpc_handler.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/r... components/copresence/rpc/rpc_handler.cc:280: base::Unretained(this))); On 2014/08/08 22:00:37, Rahul Chaturvedi wrote: > On 2014/08/08 21:45:13, willchan wrote: > > It looks like the |this| here gets passed to WhispernetClient. If y'all do > > eventually change it so the WhispernetClient can outlive the CopresenceClient, > > then seems like a bug. How does this callback get disconnected? > > |this| is getting passed to directive_handler_, not WhispernetClient. Indeed, thanks!
+brettw for third_party/protobuf review
Removing myself as blocking for this CL, since the net:: usage LGTM. That said, I still have concerns about other parts of the code, and will stream in more comments later on when I have time. Cheers.
On 2014/08/08 23:31:30, willchan wrote: > Removing myself as blocking for this CL, since the net:: usage LGTM. That said, > I still have concerns about other parts of the code, and will stream in more > comments later on when I have time. > > Cheers. Thanks Will!
https://codereview.chromium.org/433283002/diff/220001/components/copresence/c... File components/copresence/copresence_client.cc (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/c... components/copresence/copresence_client.cc:38: // The WhispernetClient must discard this on Shutdown. On 2014/08/08 22:53:33, willchan wrote: > On 2014/08/08 21:57:52, Charlie wrote: > > On 2014/08/08 01:29:07, Rahul Chaturvedi wrote: > > > On 2014/08/08 01:05:58, willchan wrote: > > > > I like that you got rid of the AsWeakPtr(). It's interesting to see that > the > > > > WhispernetClient must discard this on Shutdown. Is this always true? Do > they > > > > always have such a tight coupling? If so, maybe it's best to expose a > single > > > > public API that encapsulates both of them, rather than let the embedder > > screw > > > > this up. What do you think? > > > > > > So this is actually a bug. If the whispernet client takes longer to > initialize > > > than the lifetime of CopresenceClient (for example, in case the whispernet > > init > > > is stalled during which we are done with the CopresenceClient and destruct > > it), > > > this will be a user-after-free. > > > > > > I actually thought we were getting rid of the weak pointer only in the RPC > > > handler, I didn't realize we removed them from here too. I believe a WeakPtr > > is > > > a better solution here than imposing lifetime requirements on the Whispernet > > > client. > > > > > > This is one of the few cases that a WeakPtr is actually good for (when you > are > > > given a object that you do not have the control on the lifetime of). I would > > > recommend that this code keep using WeakPtrs. This 'can' be done without it > by > > > specifically adding code to the whispernet client to abort an initialization > > but > > > the resulting code will be uglier than simply using a WeakPtr here. > > > > > > > > > (btw, I am not completely sure what you mean by a single public API? If > you're > > > talking about combining the RPC handler and the whispernet client, that is > not > > > feasible since the whispernet client is created in the browser since it is > > > dependent on it; the RPC client has no reason to live in the browser.) > > > > Putting just this one back to AsWeakPtr(), and getting rid of the > > WhispernetClient::Shutdown() call. In theory we could instead add a > > CancelInitialize() method to cancel the callback, but that seems unnecessary > > when WeakPtr already solves the problem. > > Please don't use SupportsWeakPtr(), there's reasonably strong consensus that > it's a bad idea and we should kill it > -https://groups.google.com/a/chromium.org/d/msg/chromium-dev/N6w_CkKEwvE/evs2pRDD0loJ. > If you must use WeakPtrs, use a WeakPtrFactory instead. Noted. Will fix after this is in. https://codereview.chromium.org/433283002/diff/220001/components/copresence/h... File components/copresence/handlers/directive_handler.h (right): https://codereview.chromium.org/433283002/diff/220001/components/copresence/h... components/copresence/handlers/directive_handler.h:29: // This function must be called before any others. On 2014/08/08 22:53:34, willchan wrote: > On 2014/08/08 21:57:52, Charlie wrote: > > On 2014/08/08 01:05:59, willchan wrote: > > > If it's not safe to use this object before Initialize() is called, why not > > > combine this code with the constructor? > > > > We explicitly don't want these to be constructor parameters so that > subclassing > > is simplified. > > Can you explain further? Can't subclasses just call the parent constructor? Yes, but this is easier than passing in a bunch of empty callbacks. In the client we already have a Create() method instead of a public constructor, which would take the place of Initialize(). Added a TODO to do that here too. https://codereview.chromium.org/433283002/diff/220001/components/copresence/h... components/copresence/handlers/directive_handler.h:35: virtual void AddDirective(const copresence::Directive& directive); On 2014/08/08 22:53:33, willchan wrote: > On 2014/08/08 21:57:52, Charlie wrote: > > On 2014/08/08 01:05:58, willchan wrote: > > > Why are these virtual? > > > > So the FakeDirectiveHandler can override them. See rpc_handler_unittest.cc. > > Got it, that makes sense. > > My personal suggestion, which I admit is very debatable, is if you're going to > override for testing purposes, to make the base class an abstract interface. > Otherwise, it becomes a bit difficult to understand how your test subclass will > interact with existing behavior in the production class. In other words: > 1) DirectiveHandler - abstract interface > 2) DirectiveHandlerImpl - has all the production implementation > 3) MockDirectiveHandler - lets you mock out the DirectiveHandler dependency Not a bad idea. This is also what we do in the client. Added a TODO.
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/433283002/300001
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/433283002/320001
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/433283002/340001
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/433283002/360001
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/433283002/390001
The CQ bit was checked by ckehoe@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/433283002/410001
The CQ bit was unchecked by rkc@chromium.org
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/433283002/410001
The CQ bit was unchecked by rkc@chromium.org
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/433283002/410001
The CQ bit was unchecked by rkc@chromium.org
The CQ bit was checked by rkc@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ckehoe@chromium.org/433283002/410001
Message was sent while issue was closed.
Change committed as 288540
Message was sent while issue was closed.
Is it intended that clients of the copresence component should only access interfaces underneath its public/ directory? https://codereview.chromium.org/433283002/diff/410001/components/copresence/p... File components/copresence/public/copresence_client.h (left): https://codereview.chromium.org/433283002/diff/410001/components/copresence/p... components/copresence/public/copresence_client.h:38: // The CopresenceClient class is the central interface for Copresence The "Client" suffix is typically used in Chromium to indicate an interface that's implemented and injected by the embedder (e.g., ContentClient, AutofillClient, ...). AFAICT, that's not what this class is, but rather it's the chief entrypoint to the component?
Message was sent while issue was closed.
On 2014/08/11 12:42:54, blundell wrote: > Is it intended that clients of the copresence component should only access > interfaces underneath its public/ directory? Yes. > > https://codereview.chromium.org/433283002/diff/410001/components/copresence/p... > File components/copresence/public/copresence_client.h (left): > > https://codereview.chromium.org/433283002/diff/410001/components/copresence/p... > components/copresence/public/copresence_client.h:38: // The CopresenceClient > class is the central interface for Copresence > The "Client" suffix is typically used in Chromium to indicate an interface > that's implemented and injected by the embedder (e.g., ContentClient, > AutofillClient, ...). AFAICT, that's not what this class is, but rather it's the > chief entrypoint to the component? That's right. We called it the client because mostly it just talks to the Copresence server. Do you have some different naming suggestions?
Message was sent while issue was closed.
On 2014/08/11 15:37:28, Charlie wrote: > On 2014/08/11 12:42:54, blundell wrote: > > Is it intended that clients of the copresence component should only access > > interfaces underneath its public/ directory? > > Yes. Nice. Make sure that DEPS are set up correctly to enforce that (i.e., all DEPS files outside of //components/copresence should just have "+components/copresence/public"). > > > > > > https://codereview.chromium.org/433283002/diff/410001/components/copresence/p... > > File components/copresence/public/copresence_client.h (left): > > > > > https://codereview.chromium.org/433283002/diff/410001/components/copresence/p... > > components/copresence/public/copresence_client.h:38: // The CopresenceClient > > class is the central interface for Copresence > > The "Client" suffix is typically used in Chromium to indicate an interface > > that's implemented and injected by the embedder (e.g., ContentClient, > > AutofillClient, ...). AFAICT, that's not what this class is, but rather it's > the > > chief entrypoint to the component? > > That's right. We called it the client because mostly it just talks to the > Copresence server. Do you have some different naming suggestions? CopresenceManager or CopresenceService would be idiomatic names.
Message was sent while issue was closed.
On 2014/08/12 12:51:38, blundell wrote: > On 2014/08/11 15:37:28, Charlie wrote: > > On 2014/08/11 12:42:54, blundell wrote: > > > Is it intended that clients of the copresence component should only access > > > interfaces underneath its public/ directory? > > > > Yes. > > Nice. Make sure that DEPS are set up correctly to enforce that (i.e., all DEPS > files outside of //components/copresence should just have > "+components/copresence/public"). > (meaning, of course, all DEPS files outside of //components/copresence that need to reference copresence at all :) > > > > > > > > > > > https://codereview.chromium.org/433283002/diff/410001/components/copresence/p... > > > File components/copresence/public/copresence_client.h (left): > > > > > > > > > https://codereview.chromium.org/433283002/diff/410001/components/copresence/p... > > > components/copresence/public/copresence_client.h:38: // The CopresenceClient > > > class is the central interface for Copresence > > > The "Client" suffix is typically used in Chromium to indicate an interface > > > that's implemented and injected by the embedder (e.g., ContentClient, > > > AutofillClient, ...). AFAICT, that's not what this class is, but rather it's > > the > > > chief entrypoint to the component? > > > > That's right. We called it the client because mostly it just talks to the > > Copresence server. Do you have some different naming suggestions? > > CopresenceManager or CopresenceService would be idiomatic names.
Message was sent while issue was closed.
On 2014/08/12 12:52:37, blundell wrote: > On 2014/08/12 12:51:38, blundell wrote: > > On 2014/08/11 15:37:28, Charlie wrote: > > > On 2014/08/11 12:42:54, blundell wrote: > > > > Is it intended that clients of the copresence component should only access > > > > interfaces underneath its public/ directory? > > > > > > Yes. > > > > Nice. Make sure that DEPS are set up correctly to enforce that (i.e., all DEPS > > files outside of //components/copresence should just have > > "+components/copresence/public"). > > > > (meaning, of course, all DEPS files outside of //components/copresence that need > to reference copresence at all :) > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/433283002/diff/410001/components/copresence/p... > > > > File components/copresence/public/copresence_client.h (left): > > > > > > > > > > > > > > https://codereview.chromium.org/433283002/diff/410001/components/copresence/p... > > > > components/copresence/public/copresence_client.h:38: // The > CopresenceClient > > > > class is the central interface for Copresence > > > > The "Client" suffix is typically used in Chromium to indicate an interface > > > > that's implemented and injected by the embedder (e.g., ContentClient, > > > > AutofillClient, ...). AFAICT, that's not what this class is, but rather > it's > > > the > > > > chief entrypoint to the component? > > > > > > That's right. We called it the client because mostly it just talks to the > > > Copresence server. Do you have some different naming suggestions? > > > > CopresenceManager or CopresenceService would be idiomatic names. I like CopresenceManager. CopresenceService is actually taken by the copresence browser context keyed service :)
Message was sent while issue was closed.
On 2014/08/12 17:57:34, Rahul Chaturvedi wrote: > On 2014/08/12 12:52:37, blundell wrote: > > On 2014/08/12 12:51:38, blundell wrote: > > > On 2014/08/11 15:37:28, Charlie wrote: > > > > On 2014/08/11 12:42:54, blundell wrote: > > > > > Is it intended that clients of the copresence component should only > access > > > > > interfaces underneath its public/ directory? > > > > > > > > Yes. > > > > > > Nice. Make sure that DEPS are set up correctly to enforce that (i.e., all > DEPS > > > files outside of //components/copresence should just have > > > "+components/copresence/public"). > > > > > > > (meaning, of course, all DEPS files outside of //components/copresence that > need > > to reference copresence at all :) > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/433283002/diff/410001/components/copresence/p... > > > > > File components/copresence/public/copresence_client.h (left): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/433283002/diff/410001/components/copresence/p... > > > > > components/copresence/public/copresence_client.h:38: // The > > CopresenceClient > > > > > class is the central interface for Copresence > > > > > The "Client" suffix is typically used in Chromium to indicate an > interface > > > > > that's implemented and injected by the embedder (e.g., ContentClient, > > > > > AutofillClient, ...). AFAICT, that's not what this class is, but rather > > it's > > > > the > > > > > chief entrypoint to the component? > > > > > > > > That's right. We called it the client because mostly it just talks to the > > > > Copresence server. Do you have some different naming suggestions? > > > > > > CopresenceManager or CopresenceService would be idiomatic names. > > I like CopresenceManager. CopresenceService is actually taken by the copresence > browser context keyed service :) Ok. I'm changing the name as part of some refactoring in another CL: https://codereview.chromium.org/441103002/ |
