Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2194193002:
Fix ArcBridgeBootstrap race issues. (Closed)
|
Created:
4 years, 4 months ago by hidehiko Modified:
4 years, 2 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, yusukes+watch_chromium.org, yoshiat_google.com, Shuhei Takahashi Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix ArcBridgeBootstrap race issues.
This CL restructs the state management in ArcBridgeBootstrap.
The biggest change is; ArcBridgeBootstrap now handles one
life-cycle of ARC instance. To restart, the instance
needs to be recreated.
With the change, the internal state management is also changed.
Detailed behavior is defined and commented in ArcBridgeBootstrapImpl's
comment. This change identifies the problematic behaviors/race behaviors
in the original code, and fixes them. Mainly three changes here;
- Internal state and "if Stop() is called" are managed separately.
- Mojo connection establishment on WorkerPool thread is now cancellable.
- ArcBridgeBootstrap instance is created per ARC instance, so that stale callbacks would not be called.
With the change, FakeArcBridgeBootstrap needs to be updated, too.
BUG=633258
BUG=b/30236819
TEST=Ran ARC on test device. Trybots.
Committed: https://crrev.com/321d423c67a0fb6ae104d1275a15498dc96cd0e9
Cr-Commit-Position: refs/heads/master@{#422124}
Patch Set 1 #Patch Set 2 : Fix ArcBridgeBootstrap race issues. #Patch Set 3 : Fix ArcBridgeBootstrap race issues. #
Total comments: 24
Patch Set 4 : rebase #Patch Set 5 : Address comments. #Patch Set 6 : rebase #
Total comments: 33
Patch Set 7 : Address comments. #
Total comments: 13
Patch Set 8 : rebase #Patch Set 9 : Address comments #
Total comments: 12
Patch Set 10 : Rebase #Patch Set 11 : Address comments #Patch Set 12 : Rebase. #Messages
Total messages: 60 (38 generated)
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Fix ArcBridgeBootstrap race issues. This CL restructs the state management in ArcBridgeBootstrap. The biggest change is; ArcBridgeBootstrap now handles one life-cycle of ARC instance. To restart, the instance needs to be recreated. With the change, the internal state management is also changed. Detailed behavior is defined and commented in ArcBridgeBootstrapImpl's comment. This change identifies the problematic behaviors/race behaviors in the original code, and fixes them. The ArcBridgeHost member is moved from ArcBridgeServiceImpl to ArcBridgeBootstrapImpl, following its lifetime. With the change, FakeArcBridgeBootstrap needs to be updated, too. BUG=b/30236819 TEST=Ran ARC on test device. Trybots. ========== to ========== Fix ArcBridgeBootstrap race issues. This CL restructs the state management in ArcBridgeBootstrap. The biggest change is; ArcBridgeBootstrap now handles one life-cycle of ARC instance. To restart, the instance needs to be recreated. With the change, the internal state management is also changed. Detailed behavior is defined and commented in ArcBridgeBootstrapImpl's comment. This change identifies the problematic behaviors/race behaviors in the original code, and fixes them. Mainly three changes here; - Internal state and "if Stop() is called" are managed separately. - Mojo connection establishment on WorkerPool thread is now cancellable. - ArcBridgeBootstrap instance is created per ARC instance, so that stale callbacks would not be called. The ArcBridgeHost member is moved from ArcBridgeServiceImpl to ArcBridgeBootstrapImpl, following its lifetime. With the change, FakeArcBridgeBootstrap needs to be updated, too. BUG=b/30236819 TEST=Ran ARC on test device. Trybots. ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hidehiko@chromium.org changed reviewers: + lhchavez@lhchavez.com
PTAL. Note: This CL fixes several race/stuck problems. - Crash before Mojo connection establishment. - Stop() invocation during Mojo connection establishment. - Random Start()/Stop() invocation sequence causes mysterious callback. This is on ArcBridgeBootstrap layer fix only. We also need another ArcBridgeService layer fix, too.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org - lhchavez@lhchavez.com
Adding the right lhchavez :P As discussed offline, is it possible to split this change in two? One that only touches implementation details of ArcBridgeBootstrap (this should be cherry-pickable to M53) and another one with the rest of the cleanup. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_bootstrap.cc (left): https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:436: VLOG(2) << "State: " << static_cast<uint32_t>(state_); We're losing some amount of useful logging :( Is it possible to keep this just for the logging? https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:93: PLOG(ERROR) << "Poll() is failed."; nit: Just "poll()" is fine. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:127: // At any phase, Stop() can be called. It does not immediately stop the nt: s/phase/state/ throughout this comment. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:140: // at the beginning of them. This should work under the assumption that Why is this an assumption? (Or rather, why does this need to be explicitly pointed out? I was under the impression that UI thread work always had to take little time). https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:145: // and return. In a callback, it checks if ARC instance is successfully nit: s/In a callback/In its callback/. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:161: // instace is crashed. ArcInstanceStopped() turns the state into STOPPED nit: s/instace is crashed/instance unexpectedly terminates/. Android itself can still request a reboot/shutdown and it will cause an ArcInstanceStopped() event to be fired. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:177: // In NOT_STARTED or STOPPED state, the instance can be safely destructed. What happens if the instance is destroyed in any other state? e.g. User logs off in the middle of some other state? https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:582: // In case that crash happens during connecting Mojo, we unlock the nit: s/during connecting Mojo/before the Mojo channel is connected/ https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:589: // returns SHUTDOWN. nit: s/returns/return/ https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:595: reason = ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE; Have you actually seen this case happen? We should probably look into it. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_bootstrap.h (right): https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.h:54: // Start() cannot be called twice or more. nit: s/cannot/should not/
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix ArcBridgeBootstrap race issues. This CL restructs the state management in ArcBridgeBootstrap. The biggest change is; ArcBridgeBootstrap now handles one life-cycle of ARC instance. To restart, the instance needs to be recreated. With the change, the internal state management is also changed. Detailed behavior is defined and commented in ArcBridgeBootstrapImpl's comment. This change identifies the problematic behaviors/race behaviors in the original code, and fixes them. Mainly three changes here; - Internal state and "if Stop() is called" are managed separately. - Mojo connection establishment on WorkerPool thread is now cancellable. - ArcBridgeBootstrap instance is created per ARC instance, so that stale callbacks would not be called. The ArcBridgeHost member is moved from ArcBridgeServiceImpl to ArcBridgeBootstrapImpl, following its lifetime. With the change, FakeArcBridgeBootstrap needs to be updated, too. BUG=b/30236819 TEST=Ran ARC on test device. Trybots. ========== to ========== Fix ArcBridgeBootstrap race issues. This CL restructs the state management in ArcBridgeBootstrap. The biggest change is; ArcBridgeBootstrap now handles one life-cycle of ARC instance. To restart, the instance needs to be recreated. With the change, the internal state management is also changed. Detailed behavior is defined and commented in ArcBridgeBootstrapImpl's comment. This change identifies the problematic behaviors/race behaviors in the original code, and fixes them. Mainly three changes here; - Internal state and "if Stop() is called" are managed separately. - Mojo connection establishment on WorkerPool thread is now cancellable. - ArcBridgeBootstrap instance is created per ARC instance, so that stale callbacks would not be called. With the change, FakeArcBridgeBootstrap needs to be updated, too. BUG=633258, b/30236819 TEST=Ran ARC on test device. Trybots. ==========
Description was changed from ========== Fix ArcBridgeBootstrap race issues. This CL restructs the state management in ArcBridgeBootstrap. The biggest change is; ArcBridgeBootstrap now handles one life-cycle of ARC instance. To restart, the instance needs to be recreated. With the change, the internal state management is also changed. Detailed behavior is defined and commented in ArcBridgeBootstrapImpl's comment. This change identifies the problematic behaviors/race behaviors in the original code, and fixes them. Mainly three changes here; - Internal state and "if Stop() is called" are managed separately. - Mojo connection establishment on WorkerPool thread is now cancellable. - ArcBridgeBootstrap instance is created per ARC instance, so that stale callbacks would not be called. With the change, FakeArcBridgeBootstrap needs to be updated, too. BUG=633258, b/30236819 TEST=Ran ARC on test device. Trybots. ========== to ========== Fix ArcBridgeBootstrap race issues. This CL restructs the state management in ArcBridgeBootstrap. The biggest change is; ArcBridgeBootstrap now handles one life-cycle of ARC instance. To restart, the instance needs to be recreated. With the change, the internal state management is also changed. Detailed behavior is defined and commented in ArcBridgeBootstrapImpl's comment. This change identifies the problematic behaviors/race behaviors in the original code, and fixes them. Mainly three changes here; - Internal state and "if Stop() is called" are managed separately. - Mojo connection establishment on WorkerPool thread is now cancellable. - ArcBridgeBootstrap instance is created per ARC instance, so that stale callbacks would not be called. With the change, FakeArcBridgeBootstrap needs to be updated, too. BUG=633258 BUG=b/30236819 TEST=Ran ARC on test device. Trybots. ==========
PTAL. > Adding the right lhchavez :P Ugrrr... Sorry. > As discussed offline, is it possible to split this change in two? One that only touches implementation details of ArcBridgeBootstrap (this should be cherry-pickable to M53) and another one with the rest of the cleanup. Stripped some clean up which can be separately done. Though, TBH, it still looks big to me... WDYT? Note: ABBootstrap recreation, which is the main reason causing some edits of non ABBootstrap classes, is actually needed for fixing race issues. Otherwise, stale callbacks could be called. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_bootstrap.cc (left): https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:436: VLOG(2) << "State: " << static_cast<uint32_t>(state_); On 2016/08/01 17:47:38, Luis Héctor Chávez wrote: > We're losing some amount of useful logging :( Is it possible to keep this just > for the logging? Instead of introducing SetState, added VLOGs for each state transitions. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:93: PLOG(ERROR) << "Poll() is failed."; On 2016/08/01 17:47:38, Luis Héctor Chávez wrote: > nit: Just "poll()" is fine. Done. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:127: // At any phase, Stop() can be called. It does not immediately stop the On 2016/08/01 17:47:38, Luis Héctor Chávez wrote: > nt: s/phase/state/ throughout this comment. Done. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:140: // at the beginning of them. This should work under the assumption that On 2016/08/01 17:47:38, Luis Héctor Chávez wrote: > Why is this an assumption? (Or rather, why does this need to be explicitly > pointed out? I was under the impression that UI thread work always had to take > little time). If the background task is very heavy or blocking forever, the stopping mechanism won't work well, because their callback where |stop_requested_| is checked is called with long delay, or will never be called. Mentioning "blocking" situation may be simpler case? WDYT? https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:145: // and return. In a callback, it checks if ARC instance is successfully On 2016/08/01 17:47:37, Luis Héctor Chávez wrote: > nit: s/In a callback/In its callback/. Done. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:161: // instace is crashed. ArcInstanceStopped() turns the state into STOPPED On 2016/08/01 17:47:37, Luis Héctor Chávez wrote: > nit: s/instace is crashed/instance unexpectedly terminates/. Android itself can > still request a reboot/shutdown and it will cause an ArcInstanceStopped() event > to be fired. Done. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:177: // In NOT_STARTED or STOPPED state, the instance can be safely destructed. On 2016/08/01 17:47:38, Luis Héctor Chávez wrote: > What happens if the instance is destroyed in any other state? e.g. User logs off > in the middle of some other state? It would be enter to mysterious ChromeOS state, but if it is like shutting down of the whole ChromeOS process, it wouldn't be problematic I think. Before, starting instance, some operation may run on WorkerPool. That's fine, if it's in shutdown state. After starting, ARC instance may be remaining. If it is in ChromeOS shutdown, it shouldn't be much problematic. (If there are more Start() request after the destruction, it would be conflicting, though). While waiting for the mojo connection, the WorkerPool will be unblocked because the |accept_cancel_pipe_| should be closed. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:582: // In case that crash happens during connecting Mojo, we unlock the On 2016/08/01 17:47:38, Luis Héctor Chávez wrote: > nit: s/during connecting Mojo/before the Mojo channel is connected/ This could call before starting Mojo channel connection, depending on SessionManager's implementation. So, technically this sounds more suitable to the real? Welcome better phrases :-). https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:589: // returns SHUTDOWN. On 2016/08/01 17:47:38, Luis Héctor Chávez wrote: > nit: s/returns/return/ Done. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:595: reason = ArcBridgeService::StopReason::GENERIC_BOOT_FAILURE; On 2016/08/01 17:47:37, Luis Héctor Chávez wrote: > Have you actually seen this case happen? We should probably look into it. No, practically. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_bootstrap.h (right): https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.h:54: // Start() cannot be called twice or more. On 2016/08/01 17:47:38, Luis Héctor Chávez wrote: > nit: s/cannot/should not/ Done.
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Luis, can we resume the review, now?
On 2016/09/06 06:39:58, hidehiko wrote: > Luis, can we resume the review, now? Friendly ping?
I still need to re-review the doc and come back here after doing so. Sorry for the delay :( https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:582: // In case that crash happens during connecting Mojo, we unlock the On 2016/08/02 11:36:21, hidehiko wrote: > On 2016/08/01 17:47:38, Luis Héctor Chávez wrote: > > nit: s/during connecting Mojo/before the Mojo channel is connected/ > > This could call before starting Mojo channel connection, depending on > SessionManager's implementation. So, technically this sounds more suitable to > the real? Welcome better phrases :-). Right, but for the purposes of resetting |accept_cancel_pipe_| we only care if the stopped signal arrives before the Mojo channel is connected, since afterwards it does not matter. This was mostly a grammar nit, btw :P https://codereview.chromium.org/2194193002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/2194193002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:209: return base::WrapUnique(new FakeArcBridgeBootstrap); prefer base::MakeUnique<FakeArcBridgeBootstrap>(). Same in all other places you build one of these objects. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:67: // Creates a pair of pipes. Returns true on success, otherwise false. nit (pedantic): this actually creates a single pipe :) https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:99: VLOG(0) << "Stop() is called during the ConnectMojo()"; nit: "Stop() was called during ConnectMojo()" https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:135: // Do nothing. Immediately transition to the STOPPED. nit: "the STOPPED state". https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:141: // the main tasks are not blocking. After reading this (and the rest of the code) more carefully, I think I understand what you mean. Nevertheless, "not blocking" sounds very confusingly similar to "non-blocking", which is not the case (otherwise they would not run in the WorkerPool). Maybe "This should work under the assumption that the tasks do not block indefinitely"? https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:219: void OnDiskSpaceObtained(int64_t disk_free_bytes); nit: OnFreeDiskSpaceObtained? https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:244: // When Stop() is called, thsi flag is set. nit: s/thsi/this/ https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.h (right): https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.h:49: void set_delegate(Delegate* delegate) { delegate_ = delegate; } This was added because ArcBridgeBootstrap was created before ArcBridgeService and we needed to inject it afterwards. Now you're inverting the order in which they are created, so you can: a) Remove this. b) Make delegate_ a Delegate* const c) Pass in the Delegate* in the constructor. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:31: class Delegate { Wouldn't this make more sense to be called "Factory" and be in ArcBridgeBootstrap? https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:34: virtual std::unique_ptr<ArcBridgeBootstrap> CreateBootstrap() = 0; this needs a virtual destructor. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:86: Delegate* delegate_ = nullptr; If you make the Delegate a Factory, maybe make this a std::unique_ptr<Factory>. That way we can remove the ArcBridgeBootstrap::Create and instead instantiate an ArcBridgeBootstrap::Factory() as the default value for this. In tests, instead of passing "this" as the factory, it's probably better to replace it with a FakeArcBridgeBootstrap::Factory(). https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_service_unittest.cc (right): https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_unittest.cc:35: ~ArcBridgeTest() override {}; remove the ;? (also, = default?) https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_unittest.cc:77: service_.reset(new ArcBridgeServiceImpl); Any reason why you're not using parentheses? I've seen a couple of bugs caused by omitting them and not being super explicit with all the member initialization. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_unittest.cc:92: // the raw pointer only here. This looks dangerous :S Anyways, is there any chance that the factory pattern can help here? If you construct the factory with the flags, maybe you don't need to access the object later? https://codereview.chromium.org/2194193002/diff/100001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_bootstrap.h (right): https://codereview.chromium.org/2194193002/diff/100001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.h:27: // Follows control Start() behavior for testing various situations. nit: s/Follows/The following/
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thank you for review. PTAL. https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... components/arc/arc_bridge_bootstrap.cc:582: // In case that crash happens during connecting Mojo, we unlock the On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > On 2016/08/02 11:36:21, hidehiko wrote: > > On 2016/08/01 17:47:38, Luis Héctor Chávez wrote: > > > nit: s/during connecting Mojo/before the Mojo channel is connected/ > > > > This could call before starting Mojo channel connection, depending on > > SessionManager's implementation. So, technically this sounds more suitable to > > the real? Welcome better phrases :-). > > Right, but for the purposes of resetting |accept_cancel_pipe_| we only care if > the stopped signal arrives before the Mojo channel is connected, since > afterwards it does not matter. This was mostly a grammar nit, btw :P Makes sense. Done. Thank you. https://codereview.chromium.org/2194193002/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc (right): https://codereview.chromium.org/2194193002/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:209: return base::WrapUnique(new FakeArcBridgeBootstrap); On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > prefer base::MakeUnique<FakeArcBridgeBootstrap>(). Same in all other places you > build one of these objects. Acknowledged. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:67: // Creates a pair of pipes. Returns true on success, otherwise false. On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > nit (pedantic): this actually creates a single pipe :) Done. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:99: VLOG(0) << "Stop() is called during the ConnectMojo()"; On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > nit: "Stop() was called during ConnectMojo()" Done. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:135: // Do nothing. Immediately transition to the STOPPED. On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > nit: "the STOPPED state". Done. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:141: // the main tasks are not blocking. On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > After reading this (and the rest of the code) more carefully, I think I > understand what you mean. Nevertheless, "not blocking" sounds very confusingly > similar to "non-blocking", which is not the case (otherwise they would not run > in the WorkerPool). Maybe "This should work under the assumption that the tasks > do not block indefinitely"? Done. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:219: void OnDiskSpaceObtained(int64_t disk_free_bytes); On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > nit: OnFreeDiskSpaceObtained? Done. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:244: // When Stop() is called, thsi flag is set. On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > nit: s/thsi/this/ Done. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.h (right): https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.h:49: void set_delegate(Delegate* delegate) { delegate_ = delegate; } On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > This was added because ArcBridgeBootstrap was created before ArcBridgeService > and we needed to inject it afterwards. Now you're inverting the order in which > they are created, so you can: > > a) Remove this. > b) Make delegate_ a Delegate* const > c) Pass in the Delegate* in the constructor. Can I keep this as is for now? As I added TODO at L33, I'm planning to move the ArcBridgeHostImpl to this class, so that we can replace delegate by Observer to split the dependency. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:31: class Delegate { On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > Wouldn't this make more sense to be called "Factory" and be in > ArcBridgeBootstrap? Replaced by Factory style with base::Callback. Can we keep the factory in this class? The factory is only for testing with ArcBridgeServiceImpl so that it should be independent from ArcBridgeBootstrap? https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:34: virtual std::unique_ptr<ArcBridgeBootstrap> CreateBootstrap() = 0; On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > this needs a virtual destructor. Acknowledged. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:86: Delegate* delegate_ = nullptr; On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > If you make the Delegate a Factory, maybe make this a std::unique_ptr<Factory>. > That way we can remove the ArcBridgeBootstrap::Create and instead instantiate an > ArcBridgeBootstrap::Factory() as the default value for this. In tests, instead > of passing "this" as the factory, it's probably better to replace it with a > FakeArcBridgeBootstrap::Factory(). Replaced by Callback, and now life-time should be managed as value. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_service_unittest.cc (right): https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_unittest.cc:35: ~ArcBridgeTest() override {}; On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > remove the ;? (also, = default?) Oops. Fixed. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_unittest.cc:77: service_.reset(new ArcBridgeServiceImpl); On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > Any reason why you're not using parentheses? I've seen a couple of bugs caused > by omitting them and not being super explicit with all the member > initialization. For consistency, with variables on stack. As you may know, "T var()" means a function prototype, taking no-args and returning T. "T()" in some context means anonymous function. I'm interested in the problem caused by missing parenthesis. Could you share, just before adding ()s? https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_unittest.cc:92: // the raw pointer only here. On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > This looks dangerous :S > > Anyways, is there any chance that the factory pattern can help here? If you > construct the factory with the flags, maybe you don't need to access the object > later? Ok, now I rewrote it under Factory redesign. https://codereview.chromium.org/2194193002/diff/100001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_bootstrap.h (right): https://codereview.chromium.org/2194193002/diff/100001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.h:27: // Follows control Start() behavior for testing various situations. On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > nit: s/Follows/The following/ Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/08 16:53:41, hidehiko wrote: > Thank you for review. PTAL. > > https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... > File components/arc/arc_bridge_bootstrap.cc (right): > > https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... > components/arc/arc_bridge_bootstrap.cc:582: // In case that crash happens during > connecting Mojo, we unlock the > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > On 2016/08/02 11:36:21, hidehiko wrote: > > > On 2016/08/01 17:47:38, Luis Héctor Chávez wrote: > > > > nit: s/during connecting Mojo/before the Mojo channel is connected/ > > > > > > This could call before starting Mojo channel connection, depending on > > > SessionManager's implementation. So, technically this sounds more suitable > to > > > the real? Welcome better phrases :-). > > > > Right, but for the purposes of resetting |accept_cancel_pipe_| we only care if > > the stopped signal arrives before the Mojo channel is connected, since > > afterwards it does not matter. This was mostly a grammar nit, btw :P > > Makes sense. Done. Thank you. > > https://codereview.chromium.org/2194193002/diff/100001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc (right): > > https://codereview.chromium.org/2194193002/diff/100001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:209: return > base::WrapUnique(new FakeArcBridgeBootstrap); > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > prefer base::MakeUnique<FakeArcBridgeBootstrap>(). Same in all other places > you > > build one of these objects. > > Acknowledged. > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > File components/arc/arc_bridge_bootstrap.cc (right): > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > components/arc/arc_bridge_bootstrap.cc:67: // Creates a pair of pipes. Returns > true on success, otherwise false. > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > nit (pedantic): this actually creates a single pipe :) > > Done. > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > components/arc/arc_bridge_bootstrap.cc:99: VLOG(0) << "Stop() is called during > the ConnectMojo()"; > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > nit: "Stop() was called during ConnectMojo()" > > Done. > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > components/arc/arc_bridge_bootstrap.cc:135: // Do nothing. Immediately > transition to the STOPPED. > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > nit: "the STOPPED state". > > Done. > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > components/arc/arc_bridge_bootstrap.cc:141: // the main tasks are not > blocking. > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > After reading this (and the rest of the code) more carefully, I think I > > understand what you mean. Nevertheless, "not blocking" sounds very confusingly > > similar to "non-blocking", which is not the case (otherwise they would not run > > in the WorkerPool). Maybe "This should work under the assumption that the > tasks > > do not block indefinitely"? > > Done. > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > components/arc/arc_bridge_bootstrap.cc:219: void OnDiskSpaceObtained(int64_t > disk_free_bytes); > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > nit: OnFreeDiskSpaceObtained? > > Done. > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > components/arc/arc_bridge_bootstrap.cc:244: // When Stop() is called, thsi flag > is set. > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > nit: s/thsi/this/ > > Done. > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > File components/arc/arc_bridge_bootstrap.h (right): > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > components/arc/arc_bridge_bootstrap.h:49: void set_delegate(Delegate* delegate) > { delegate_ = delegate; } > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > This was added because ArcBridgeBootstrap was created before ArcBridgeService > > and we needed to inject it afterwards. Now you're inverting the order in which > > they are created, so you can: > > > > a) Remove this. > > b) Make delegate_ a Delegate* const > > c) Pass in the Delegate* in the constructor. > > Can I keep this as is for now? > As I added TODO at L33, I'm planning to move the ArcBridgeHostImpl to this > class, so that we can replace delegate by Observer to split the dependency. > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > File components/arc/arc_bridge_service_impl.h (right): > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > components/arc/arc_bridge_service_impl.h:31: class Delegate { > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > Wouldn't this make more sense to be called "Factory" and be in > > ArcBridgeBootstrap? > > Replaced by Factory style with base::Callback. > Can we keep the factory in this class? The factory is only for testing with > ArcBridgeServiceImpl so that it should be independent from ArcBridgeBootstrap? > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > components/arc/arc_bridge_service_impl.h:34: virtual > std::unique_ptr<ArcBridgeBootstrap> CreateBootstrap() = 0; > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > this needs a virtual destructor. > > Acknowledged. > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > components/arc/arc_bridge_service_impl.h:86: Delegate* delegate_ = nullptr; > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > If you make the Delegate a Factory, maybe make this a > std::unique_ptr<Factory>. > > That way we can remove the ArcBridgeBootstrap::Create and instead instantiate > an > > ArcBridgeBootstrap::Factory() as the default value for this. In tests, instead > > of passing "this" as the factory, it's probably better to replace it with a > > FakeArcBridgeBootstrap::Factory(). > > Replaced by Callback, and now life-time should be managed as value. > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > File components/arc/arc_bridge_service_unittest.cc (right): > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > components/arc/arc_bridge_service_unittest.cc:35: ~ArcBridgeTest() override {}; > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > remove the ;? (also, = default?) > > Oops. Fixed. > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > components/arc/arc_bridge_service_unittest.cc:77: service_.reset(new > ArcBridgeServiceImpl); > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > Any reason why you're not using parentheses? I've seen a couple of bugs caused > > by omitting them and not being super explicit with all the member > > initialization. > > For consistency, with variables on stack. As you may know, "T var()" means a > function prototype, taking no-args and returning T. "T()" in some context means > anonymous function. > > I'm interested in the problem caused by missing parenthesis. Could you share, > just before adding ()s? > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > components/arc/arc_bridge_service_unittest.cc:92: // the raw pointer only here. > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > This looks dangerous :S > > > > Anyways, is there any chance that the factory pattern can help here? If you > > construct the factory with the flags, maybe you don't need to access the > object > > later? > > Ok, now I rewrote it under Factory redesign. > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/test/fa... > File components/arc/test/fake_arc_bridge_bootstrap.h (right): > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/test/fa... > components/arc/test/fake_arc_bridge_bootstrap.h:27: // Follows control Start() > behavior for testing various situations. > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > nit: s/Follows/The following/ > > Done. friendly ping?
On 2016/09/12 04:30:59, hidehiko wrote: > On 2016/09/08 16:53:41, hidehiko wrote: > > Thank you for review. PTAL. > > > > > https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... > > File components/arc/arc_bridge_bootstrap.cc (right): > > > > > https://codereview.chromium.org/2194193002/diff/40001/components/arc/arc_brid... > > components/arc/arc_bridge_bootstrap.cc:582: // In case that crash happens > during > > connecting Mojo, we unlock the > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > On 2016/08/02 11:36:21, hidehiko wrote: > > > > On 2016/08/01 17:47:38, Luis Héctor Chávez wrote: > > > > > nit: s/during connecting Mojo/before the Mojo channel is connected/ > > > > > > > > This could call before starting Mojo channel connection, depending on > > > > SessionManager's implementation. So, technically this sounds more suitable > > to > > > > the real? Welcome better phrases :-). > > > > > > Right, but for the purposes of resetting |accept_cancel_pipe_| we only care > if > > > the stopped signal arrives before the Mojo channel is connected, since > > > afterwards it does not matter. This was mostly a grammar nit, btw :P > > > > Makes sense. Done. Thank you. > > > > > https://codereview.chromium.org/2194193002/diff/100001/chrome/browser/chromeo... > > File chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc (right): > > > > > https://codereview.chromium.org/2194193002/diff/100001/chrome/browser/chromeo... > > chrome/browser/chromeos/arc/arc_auth_service_browsertest.cc:209: return > > base::WrapUnique(new FakeArcBridgeBootstrap); > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > prefer base::MakeUnique<FakeArcBridgeBootstrap>(). Same in all other places > > you > > > build one of these objects. > > > > Acknowledged. > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > File components/arc/arc_bridge_bootstrap.cc (right): > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > components/arc/arc_bridge_bootstrap.cc:67: // Creates a pair of pipes. Returns > > true on success, otherwise false. > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > nit (pedantic): this actually creates a single pipe :) > > > > Done. > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > components/arc/arc_bridge_bootstrap.cc:99: VLOG(0) << "Stop() is called during > > the ConnectMojo()"; > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > nit: "Stop() was called during ConnectMojo()" > > > > Done. > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > components/arc/arc_bridge_bootstrap.cc:135: // Do nothing. Immediately > > transition to the STOPPED. > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > nit: "the STOPPED state". > > > > Done. > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > components/arc/arc_bridge_bootstrap.cc:141: // the main tasks are not > > blocking. > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > After reading this (and the rest of the code) more carefully, I think I > > > understand what you mean. Nevertheless, "not blocking" sounds very > confusingly > > > similar to "non-blocking", which is not the case (otherwise they would not > run > > > in the WorkerPool). Maybe "This should work under the assumption that the > > tasks > > > do not block indefinitely"? > > > > Done. > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > components/arc/arc_bridge_bootstrap.cc:219: void OnDiskSpaceObtained(int64_t > > disk_free_bytes); > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > nit: OnFreeDiskSpaceObtained? > > > > Done. > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > components/arc/arc_bridge_bootstrap.cc:244: // When Stop() is called, thsi > flag > > is set. > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > nit: s/thsi/this/ > > > > Done. > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > File components/arc/arc_bridge_bootstrap.h (right): > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > components/arc/arc_bridge_bootstrap.h:49: void set_delegate(Delegate* > delegate) > > { delegate_ = delegate; } > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > This was added because ArcBridgeBootstrap was created before > ArcBridgeService > > > and we needed to inject it afterwards. Now you're inverting the order in > which > > > they are created, so you can: > > > > > > a) Remove this. > > > b) Make delegate_ a Delegate* const > > > c) Pass in the Delegate* in the constructor. > > > > Can I keep this as is for now? > > As I added TODO at L33, I'm planning to move the ArcBridgeHostImpl to this > > class, so that we can replace delegate by Observer to split the dependency. > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > File components/arc/arc_bridge_service_impl.h (right): > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > components/arc/arc_bridge_service_impl.h:31: class Delegate { > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > Wouldn't this make more sense to be called "Factory" and be in > > > ArcBridgeBootstrap? > > > > Replaced by Factory style with base::Callback. > > Can we keep the factory in this class? The factory is only for testing with > > ArcBridgeServiceImpl so that it should be independent from ArcBridgeBootstrap? > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > components/arc/arc_bridge_service_impl.h:34: virtual > > std::unique_ptr<ArcBridgeBootstrap> CreateBootstrap() = 0; > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > this needs a virtual destructor. > > > > Acknowledged. > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > components/arc/arc_bridge_service_impl.h:86: Delegate* delegate_ = nullptr; > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > If you make the Delegate a Factory, maybe make this a > > std::unique_ptr<Factory>. > > > That way we can remove the ArcBridgeBootstrap::Create and instead > instantiate > > an > > > ArcBridgeBootstrap::Factory() as the default value for this. In tests, > instead > > > of passing "this" as the factory, it's probably better to replace it with a > > > FakeArcBridgeBootstrap::Factory(). > > > > Replaced by Callback, and now life-time should be managed as value. > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > File components/arc/arc_bridge_service_unittest.cc (right): > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > components/arc/arc_bridge_service_unittest.cc:35: ~ArcBridgeTest() override > {}; > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > remove the ;? (also, = default?) > > > > Oops. Fixed. > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > components/arc/arc_bridge_service_unittest.cc:77: service_.reset(new > > ArcBridgeServiceImpl); > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > Any reason why you're not using parentheses? I've seen a couple of bugs > caused > > > by omitting them and not being super explicit with all the member > > > initialization. > > > > For consistency, with variables on stack. As you may know, "T var()" means a > > function prototype, taking no-args and returning T. "T()" in some context > means > > anonymous function. > > > > I'm interested in the problem caused by missing parenthesis. Could you share, > > just before adding ()s? > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > > components/arc/arc_bridge_service_unittest.cc:92: // the raw pointer only > here. > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > This looks dangerous :S > > > > > > Anyways, is there any chance that the factory pattern can help here? If you > > > construct the factory with the flags, maybe you don't need to access the > > object > > > later? > > > > Ok, now I rewrote it under Factory redesign. > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/test/fa... > > File components/arc/test/fake_arc_bridge_bootstrap.h (right): > > > > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/test/fa... > > components/arc/test/fake_arc_bridge_bootstrap.h:27: // Follows control Start() > > behavior for testing various situations. > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > nit: s/Follows/The following/ > > > > Done. > > friendly ping? ping?
sorry for the delay :'( https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:31: class Delegate { On 2016/09/08 16:53:41, hidehiko wrote: > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > Wouldn't this make more sense to be called "Factory" and be in > > ArcBridgeBootstrap? > > Replaced by Factory style with base::Callback. > Can we keep the factory in this class? The factory is only for testing with > ArcBridgeServiceImpl so that it should be independent from ArcBridgeBootstrap? Sure, after the refactor and in callback-style it makes more sense to have it here. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_service_unittest.cc (right): https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_unittest.cc:77: service_.reset(new ArcBridgeServiceImpl); On 2016/09/08 16:53:41, hidehiko wrote: > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > Any reason why you're not using parentheses? I've seen a couple of bugs caused > > by omitting them and not being super explicit with all the member > > initialization. > > For consistency, with variables on stack. As you may know, "T var()" means a > function prototype, taking no-args and returning T. "T()" in some context means > anonymous function. > > I'm interested in the problem caused by missing parenthesis. Could you share, > just before adding ()s? There was a recent discussion about this in chromium-dev when doing the WrapUnique -> MakeUnique migration: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/iQgMedVA8-k/dis... There was another similar-but-not-quite discussion about it here, where side effects of one such initialization were non-obvious: https://groups.google.com/a/chromium.org/d/topic/chromium-dev/xH1-ac_nDCY/dis... I know none of these apply here, but I had to go look at the constructor to make sure. In any case, something that came off the first thread I shared is that the C++ Dos and Don'ts were updated to reflect this decision: https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a... "Don't make future maintainers guess whether you left off the '()' on purpose. [...] If you're intentionally leaving off the "()" as an optimisation, please leave a comment." so, please leave a comment if you absolutely want to leave the parens out, but I encourage to add them since there is no optimization IIUC. In this context it's impossible for the most vexing parse to kick in here since it's prefixed by 'new', but in that case I would have suggested using C++11's uniform initialization. https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:252: std::unique_ptr<mojom::ArcBridgeHost> arc_bridge_host_; This seems like it's not used at the moment. Right now, you only call .reset() on it. https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:297: void ArcBridgeBootstrapImpl::Stop() { nit: can we move this down so that the sequence of steps in the most common case (successful case) flow one after another? https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:319: // and stopping procedure will be run there. There is one code path where this is not true: https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m... is called after the main UI thread MessageLoop stops processing messages. Any callback posted in this codepath won't get run. The only one in this block where it matters is STARTING_INSTANCE, which would leak the instance. Fortunately, session_manager will clean up after us :) but it's probably still good to mention in a comment because it took me a while to find. Nevertheless, on debug mode we will crash because the destructor will be called without the callback getting called, so we will be in the wrong state and DCHECK. How should we handle this case? This will be the most common case since it's the case where everything went smoothly and ARC was stopped due to the user logging out, so we'll crash every logout :( https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:129: VLOG(0) << "ARC stopped: " << static_cast<int>(stop_reason); Can we have an operator<< for StopReason? That'll make the logs easier to read without having to correlate what version was being run. https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_service_unittest.cc (right): https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_service_unittest.cc:61: new FakeArcBridgeBootstrap); same comment about default/value initialization. In this case you can use auto bootstrap = base::MakeUnique<FakeArcBridgeBootstrap>() with absolutely no penalty. Same below. https://codereview.chromium.org/2194193002/diff/120001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2194193002/diff/120001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:54: std::unique_ptr<ArcBridgeBootstrap> FakeArcBridgeBootstrap::Create() { nit: //static.
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you for review. PTAL. https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... File components/arc/arc_bridge_service_unittest.cc (right): https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... components/arc/arc_bridge_service_unittest.cc:77: service_.reset(new ArcBridgeServiceImpl); On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > On 2016/09/08 16:53:41, hidehiko wrote: > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > Any reason why you're not using parentheses? I've seen a couple of bugs > caused > > > by omitting them and not being super explicit with all the member > > > initialization. > > > > For consistency, with variables on stack. As you may know, "T var()" means a > > function prototype, taking no-args and returning T. "T()" in some context > means > > anonymous function. > > > > I'm interested in the problem caused by missing parenthesis. Could you share, > > just before adding ()s? > > There was a recent discussion about this in chromium-dev when doing the > WrapUnique -> MakeUnique migration: > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/iQgMedVA8-k/dis... > > There was another similar-but-not-quite discussion about it here, where side > effects of one such initialization were non-obvious: > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/xH1-ac_nDCY/dis... > > I know none of these apply here, but I had to go look at the constructor to make > sure. > > In any case, something that came off the first thread I shared is that the C++ > Dos and Don'ts were updated to reflect this decision: > > https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a... > > "Don't make future maintainers guess whether you left off the '()' on purpose. > [...] If you're intentionally leaving off the "()" as an optimisation, please > leave a comment." > > so, please leave a comment if you absolutely want to leave the parens out, but I > encourage to add them since there is no optimization IIUC. In this context it's > impossible for the most vexing parse to kick in here since it's prefixed by > 'new', but in that case I would have suggested using C++11's uniform > initialization. I see. Thank you for explanation and references. I used () in this CL for consistency. We can replace it by {} as you suggested anytime we want later, I think. https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:252: std::unique_ptr<mojom::ArcBridgeHost> arc_bridge_host_; On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > This seems like it's not used at the moment. Right now, you only call .reset() > on it. Oops. Removed. I'll revive it in a following CL. https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:297: void ArcBridgeBootstrapImpl::Stop() { On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > nit: can we move this down so that the sequence of steps in the most common case > (successful case) flow one after another? Sure, done. https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:319: // and stopping procedure will be run there. On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > There is one code path where this is not true: > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m... > is called after the main UI thread MessageLoop stops processing messages. Any > callback posted in this codepath won't get run. > > The only one in this block where it matters is STARTING_INSTANCE, which would > leak the instance. Fortunately, session_manager will clean up after us :) but > it's probably still good to mention in a comment because it took me a while to > find. > > Nevertheless, on debug mode we will crash because the destructor will be called > without the callback getting called, so we will be in the wrong state and > DCHECK. How should we handle this case? This will be the most common case since > it's the case where everything went smoothly and ARC was stopped due to the user > logging out, so we'll crash every logout :( Good catch. I just temporarily removed the DCHECK in dtor in this CL, and added comments, for short-term solution. I'd like to introduce Shutdown() for Chrome shutdown procedure because of the different background. - On shutdown, we cannot rely on message loop. - On shutdown, we do not need to worry about the case that Chrome tries to start ARC again. with those assumptions, we can simplify the code a bit, I think, but it would need farther refactoring. As long as this CL is not a regression, I'd like to move forward with it, and to address Shutdown() in a separate CL. WDYT? https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.cc (right): https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.cc:129: VLOG(0) << "ARC stopped: " << static_cast<int>(stop_reason); On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > Can we have an operator<< for StopReason? That'll make the logs easier to read > without having to correlate what version was being run. Done. https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_service_unittest.cc (right): https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_service_unittest.cc:61: new FakeArcBridgeBootstrap); On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > same comment about default/value initialization. In this case you can use auto > bootstrap = base::MakeUnique<FakeArcBridgeBootstrap>() with absolutely no > penalty. Same below. Done. Replaced "unique_ptr + new" by "auto + MakeUnique" for all occurrence in this CL. https://codereview.chromium.org/2194193002/diff/120001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2194193002/diff/120001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:54: std::unique_ptr<ArcBridgeBootstrap> FakeArcBridgeBootstrap::Create() { On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > nit: //static. Done.
On 2016/09/20 13:35:20, hidehiko wrote: > Thank you for review. PTAL. > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > File components/arc/arc_bridge_service_unittest.cc (right): > > https://codereview.chromium.org/2194193002/diff/100001/components/arc/arc_bri... > components/arc/arc_bridge_service_unittest.cc:77: service_.reset(new > ArcBridgeServiceImpl); > On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > > On 2016/09/08 16:53:41, hidehiko wrote: > > > On 2016/09/07 23:38:32, Luis Héctor Chávez wrote: > > > > Any reason why you're not using parentheses? I've seen a couple of bugs > > caused > > > > by omitting them and not being super explicit with all the member > > > > initialization. > > > > > > For consistency, with variables on stack. As you may know, "T var()" means a > > > function prototype, taking no-args and returning T. "T()" in some context > > means > > > anonymous function. > > > > > > I'm interested in the problem caused by missing parenthesis. Could you > share, > > > just before adding ()s? > > > > There was a recent discussion about this in chromium-dev when doing the > > WrapUnique -> MakeUnique migration: > > > > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/iQgMedVA8-k/dis... > > > > There was another similar-but-not-quite discussion about it here, where side > > effects of one such initialization were non-obvious: > > > > > https://groups.google.com/a/chromium.org/d/topic/chromium-dev/xH1-ac_nDCY/dis... > > > > I know none of these apply here, but I had to go look at the constructor to > make > > sure. > > > > In any case, something that came off the first thread I shared is that the C++ > > Dos and Don'ts were updated to reflect this decision: > > > > > https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a... > > > > "Don't make future maintainers guess whether you left off the '()' on purpose. > > [...] If you're intentionally leaving off the "()" as an optimisation, please > > leave a comment." > > > > so, please leave a comment if you absolutely want to leave the parens out, but > I > > encourage to add them since there is no optimization IIUC. In this context > it's > > impossible for the most vexing parse to kick in here since it's prefixed by > > 'new', but in that case I would have suggested using C++11's uniform > > initialization. > > I see. Thank you for explanation and references. > I used () in this CL for consistency. We can replace it by {} as you suggested > anytime we want later, I think. > > https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... > File components/arc/arc_bridge_bootstrap.cc (right): > > https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... > components/arc/arc_bridge_bootstrap.cc:252: > std::unique_ptr<mojom::ArcBridgeHost> arc_bridge_host_; > On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > > This seems like it's not used at the moment. Right now, you only call .reset() > > on it. > > Oops. Removed. I'll revive it in a following CL. > > https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... > components/arc/arc_bridge_bootstrap.cc:297: void ArcBridgeBootstrapImpl::Stop() > { > On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > > nit: can we move this down so that the sequence of steps in the most common > case > > (successful case) flow one after another? > > Sure, done. > > https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... > components/arc/arc_bridge_bootstrap.cc:319: // and stopping procedure will be > run there. > On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > > There is one code path where this is not true: > > > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m... > > is called after the main UI thread MessageLoop stops processing messages. Any > > callback posted in this codepath won't get run. > > > > The only one in this block where it matters is STARTING_INSTANCE, which would > > leak the instance. Fortunately, session_manager will clean up after us :) but > > it's probably still good to mention in a comment because it took me a while to > > find. > > > > Nevertheless, on debug mode we will crash because the destructor will be > called > > without the callback getting called, so we will be in the wrong state and > > DCHECK. How should we handle this case? This will be the most common case > since > > it's the case where everything went smoothly and ARC was stopped due to the > user > > logging out, so we'll crash every logout :( > > Good catch. > I just temporarily removed the DCHECK in dtor in this CL, and added comments, > for short-term solution. > > I'd like to introduce Shutdown() for Chrome shutdown procedure because of the > different background. > > - On shutdown, we cannot rely on message loop. > - On shutdown, we do not need to worry about the case that Chrome tries to start > ARC again. > > with those assumptions, we can simplify the code a bit, I think, but it would > need farther refactoring. As long as this CL is not a regression, I'd like to > move forward with it, and to address Shutdown() in a separate CL. > WDYT? > > https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... > File components/arc/arc_bridge_service_impl.cc (right): > > https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... > components/arc/arc_bridge_service_impl.cc:129: VLOG(0) << "ARC stopped: " << > static_cast<int>(stop_reason); > On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > > Can we have an operator<< for StopReason? That'll make the logs easier to read > > without having to correlate what version was being run. > > Done. > > https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... > File components/arc/arc_bridge_service_unittest.cc (right): > > https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... > components/arc/arc_bridge_service_unittest.cc:61: new FakeArcBridgeBootstrap); > On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > > same comment about default/value initialization. In this case you can use auto > > bootstrap = base::MakeUnique<FakeArcBridgeBootstrap>() with absolutely no > > penalty. Same below. > > Done. Replaced "unique_ptr + new" by "auto + MakeUnique" for all occurrence in > this CL. > > https://codereview.chromium.org/2194193002/diff/120001/components/arc/test/fa... > File components/arc/test/fake_arc_bridge_bootstrap.cc (right): > > https://codereview.chromium.org/2194193002/diff/120001/components/arc/test/fa... > components/arc/test/fake_arc_bridge_bootstrap.cc:54: > std::unique_ptr<ArcBridgeBootstrap> FakeArcBridgeBootstrap::Create() { > On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > > nit: //static. > > Done. ping?
Sorry for the delay, again :( https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2194193002/diff/120001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:319: // and stopping procedure will be run there. On 2016/09/20 13:35:19, hidehiko wrote: > On 2016/09/15 23:08:49, Luis Héctor Chávez wrote: > > There is one code path where this is not true: > > > > > https://cs.chromium.org/chromium/src/chrome/browser/chromeos/chrome_browser_m... > > is called after the main UI thread MessageLoop stops processing messages. Any > > callback posted in this codepath won't get run. > > > > The only one in this block where it matters is STARTING_INSTANCE, which would > > leak the instance. Fortunately, session_manager will clean up after us :) but > > it's probably still good to mention in a comment because it took me a while to > > find. > > > > Nevertheless, on debug mode we will crash because the destructor will be > called > > without the callback getting called, so we will be in the wrong state and > > DCHECK. How should we handle this case? This will be the most common case > since > > it's the case where everything went smoothly and ARC was stopped due to the > user > > logging out, so we'll crash every logout :( > > Good catch. > I just temporarily removed the DCHECK in dtor in this CL, and added comments, > for short-term solution. > > I'd like to introduce Shutdown() for Chrome shutdown procedure because of the > different background. > > - On shutdown, we cannot rely on message loop. > - On shutdown, we do not need to worry about the case that Chrome tries to start > ARC again. > > with those assumptions, we can simplify the code a bit, I think, but it would > need farther refactoring. As long as this CL is not a regression, I'd like to > move forward with it, and to address Shutdown() in a separate CL. > WDYT? SGTM and is in-line with other Chrome components. https://codereview.chromium.org/2194193002/diff/160001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2194193002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:98: VLOG(0) << "Stop() was called during ConnectMojo()"; Why is this VLOG(0)? This seems to be of the same importance as the rest of the VLOG(1) statements. https://codereview.chromium.org/2194193002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:222: // DBus callbacks. nit: this is just one callback. https://codereview.chromium.org/2194193002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:444: VLOG(0) << "Mojo connecting is cancelled."; nit: "Mojo connection was cancelled" or "ConnectMojo...". https://codereview.chromium.org/2194193002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:586: VLOG(2) << "Notified that ARC instance is stopped " nit: Maybe this should be VLOG(1). All the other OnStopped calls have VLOG(1) (or P/LOG(ERROR)). https://codereview.chromium.org/2194193002/diff/160001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/2194193002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:46: factory_ = factory; nit: maybe add a DCHECK(!factory.is_null()) to enforce the comment. https://codereview.chromium.org/2194193002/diff/160001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_bootstrap.cc (left): https://codereview.chromium.org/2194193002/diff/160001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:7: #include <utility> Why was this removed? You're still calling std::move. (Just to double check, can you call git cl lint? I swear I fixed that a few months ago... but let me know if it does not give you an IWYU warning.)
The CQ bit was checked by hidehiko@chromium.org to run a CQ dry run
hidehiko@chromium.org changed reviewers: + bartfab@chromium.org
PTAL, Luis. bartfab@, could you take a look as a c/b/policy OWNER? https://codereview.chromium.org/2194193002/diff/160001/components/arc/arc_bri... File components/arc/arc_bridge_bootstrap.cc (right): https://codereview.chromium.org/2194193002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:98: VLOG(0) << "Stop() was called during ConnectMojo()"; On 2016/09/23 05:13:38, Luis Héctor Chávez wrote: > Why is this VLOG(0)? This seems to be of the same importance as the rest of the > VLOG(1) statements. No special reasons. Fixed. https://codereview.chromium.org/2194193002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:222: // DBus callbacks. On 2016/09/23 05:13:38, Luis Héctor Chávez wrote: > nit: this is just one callback. Done. https://codereview.chromium.org/2194193002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:444: VLOG(0) << "Mojo connecting is cancelled."; On 2016/09/23 05:13:38, Luis Héctor Chávez wrote: > nit: "Mojo connection was cancelled" or "ConnectMojo...". Done. https://codereview.chromium.org/2194193002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_bootstrap.cc:586: VLOG(2) << "Notified that ARC instance is stopped " On 2016/09/23 05:13:38, Luis Héctor Chávez wrote: > nit: Maybe this should be VLOG(1). All the other OnStopped calls have VLOG(1) > (or P/LOG(ERROR)). I was thinking this is a part of control-flow message, like ones logged with VLOG(2) above, so I aligned, but I'm ok with either VLOG(1). https://codereview.chromium.org/2194193002/diff/160001/components/arc/arc_bri... File components/arc/arc_bridge_service_impl.h (right): https://codereview.chromium.org/2194193002/diff/160001/components/arc/arc_bri... components/arc/arc_bridge_service_impl.h:46: factory_ = factory; On 2016/09/23 05:13:38, Luis Héctor Chávez wrote: > nit: maybe add a DCHECK(!factory.is_null()) to enforce the comment. Done. Moved to .cc. https://codereview.chromium.org/2194193002/diff/160001/components/arc/test/fa... File components/arc/test/fake_arc_bridge_bootstrap.cc (left): https://codereview.chromium.org/2194193002/diff/160001/components/arc/test/fa... components/arc/test/fake_arc_bridge_bootstrap.cc:7: #include <utility> On 2016/09/23 05:13:38, Luis Héctor Chávez wrote: > Why was this removed? You're still calling std::move. > > (Just to double check, can you call git cl lint? I swear I fixed that a few > months ago... but let me know if it does not give you an IWYU warning.) I removed std::move etc. at all in earlier PS so that I removed the include directive, too. But, later I revived some of them to reduce the changing scope. I just overlooked to restore include at that time... I ran "git cl lint" for this CL, and no error in components/arc. (Note: policy_browsertest.cc has, due to some long macro line. It's unrelated to this, so I just ignored it).
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
lgtm
On 2016/09/27 01:46:12, Luis Héctor Chávez wrote: > lgtm Thank you for review, Luis. Friendly ping, bartfab@.
chrome/browser/policy/policy_browsertest.cc LGTM
chrome/browser/policy/policy_browsertest.cc LGTM
Thank you for review!
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bartfab@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2194193002/#ps220001 (title: "Rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix ArcBridgeBootstrap race issues. This CL restructs the state management in ArcBridgeBootstrap. The biggest change is; ArcBridgeBootstrap now handles one life-cycle of ARC instance. To restart, the instance needs to be recreated. With the change, the internal state management is also changed. Detailed behavior is defined and commented in ArcBridgeBootstrapImpl's comment. This change identifies the problematic behaviors/race behaviors in the original code, and fixes them. Mainly three changes here; - Internal state and "if Stop() is called" are managed separately. - Mojo connection establishment on WorkerPool thread is now cancellable. - ArcBridgeBootstrap instance is created per ARC instance, so that stale callbacks would not be called. With the change, FakeArcBridgeBootstrap needs to be updated, too. BUG=633258 BUG=b/30236819 TEST=Ran ARC on test device. Trybots. ========== to ========== Fix ArcBridgeBootstrap race issues. This CL restructs the state management in ArcBridgeBootstrap. The biggest change is; ArcBridgeBootstrap now handles one life-cycle of ARC instance. To restart, the instance needs to be recreated. With the change, the internal state management is also changed. Detailed behavior is defined and commented in ArcBridgeBootstrapImpl's comment. This change identifies the problematic behaviors/race behaviors in the original code, and fixes them. Mainly three changes here; - Internal state and "if Stop() is called" are managed separately. - Mojo connection establishment on WorkerPool thread is now cancellable. - ArcBridgeBootstrap instance is created per ARC instance, so that stale callbacks would not be called. With the change, FakeArcBridgeBootstrap needs to be updated, too. BUG=633258 BUG=b/30236819 TEST=Ran ARC on test device. Trybots. ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001)
Message was sent while issue was closed.
Description was changed from ========== Fix ArcBridgeBootstrap race issues. This CL restructs the state management in ArcBridgeBootstrap. The biggest change is; ArcBridgeBootstrap now handles one life-cycle of ARC instance. To restart, the instance needs to be recreated. With the change, the internal state management is also changed. Detailed behavior is defined and commented in ArcBridgeBootstrapImpl's comment. This change identifies the problematic behaviors/race behaviors in the original code, and fixes them. Mainly three changes here; - Internal state and "if Stop() is called" are managed separately. - Mojo connection establishment on WorkerPool thread is now cancellable. - ArcBridgeBootstrap instance is created per ARC instance, so that stale callbacks would not be called. With the change, FakeArcBridgeBootstrap needs to be updated, too. BUG=633258 BUG=b/30236819 TEST=Ran ARC on test device. Trybots. ========== to ========== Fix ArcBridgeBootstrap race issues. This CL restructs the state management in ArcBridgeBootstrap. The biggest change is; ArcBridgeBootstrap now handles one life-cycle of ARC instance. To restart, the instance needs to be recreated. With the change, the internal state management is also changed. Detailed behavior is defined and commented in ArcBridgeBootstrapImpl's comment. This change identifies the problematic behaviors/race behaviors in the original code, and fixes them. Mainly three changes here; - Internal state and "if Stop() is called" are managed separately. - Mojo connection establishment on WorkerPool thread is now cancellable. - ArcBridgeBootstrap instance is created per ARC instance, so that stale callbacks would not be called. With the change, FakeArcBridgeBootstrap needs to be updated, too. BUG=633258 BUG=b/30236819 TEST=Ran ARC on test device. Trybots. Committed: https://crrev.com/321d423c67a0fb6ae104d1275a15498dc96cd0e9 Cr-Commit-Position: refs/heads/master@{#422124} ==========
Message was sent while issue was closed.
Patchset 12 (id:??) landed as https://crrev.com/321d423c67a0fb6ae104d1275a15498dc96cd0e9 Cr-Commit-Position: refs/heads/master@{#422124} |
