Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2844383006:
Turn ArcSupportHost from Observer model to Delegate (Closed)
|
Created:
3 years, 7 months ago by victorhsieh Modified:
3 years, 6 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, oshima+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, victorhsieh+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionTurn ArcSupportHost from Observer model to Delegate
ArcSupportHost's subscribers have some overlap of responsibility, and
there are several places to skip the work in case it is supposed to be
done in the other subscriber.
Instead, this change turns the observer model into several delegates
for better responsibility split.
TEST=unit_test --gtest_filter=Arc*
TEST=browser_test --gtest_filer=Arc*
TEST=(disable silent sign-in) accept ToS, click sign-in and unplug
ethernet cable. See feedback button. Click retry, click sign-in,
See Play Store.
TEST=(disable silent sign-in) accept ToS, force logout from Chrome OS.
Re-login. See the window pops up with sign-in button.
TEST=git cl try
BUG=698418
Review-Url: https://codereview.chromium.org/2844383006
Cr-Commit-Position: refs/heads/master@{#477376}
Committed: https://chromium.googlesource.com/chromium/src/+/63a1da923764a69409907f8135d0b4bbc56f0a03
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebase #Patch Set 3 : addressed some comments #Patch Set 4 : Turn ArcSupportHost from Observer model to Delegate #
Total comments: 20
Patch Set 5 : addressed comments #
Total comments: 34
Patch Set 6 : Turn ArcSupportHost from Observer model to Delegate #Patch Set 7 : rebase #Patch Set 8 : Turn ArcSupportHost from Observer model to Delegate #Patch Set 9 : rebase for real #Patch Set 10 : Turn ArcSupportHost from Observer model to Delegate #
Total comments: 2
Patch Set 11 : git cl try #
Total comments: 14
Patch Set 12 : address Luis' comments #
Total comments: 2
Patch Set 13 : renamed to OnRetryClicked #Patch Set 14 : Turn ArcSupportHost from Observer model to Delegate #Patch Set 15 : rebase #
Total comments: 6
Patch Set 16 : DCHECK #Patch Set 17 : -DCHECK #Patch Set 18 : rebase #Messages
Total messages: 84 (56 generated)
victorhsieh@google.com changed reviewers: + hidehiko@chromium.org, khmel@chromium.org, victorhsieh@google.com
PTA overview L. I just made it compiled and passed most tests. I'd like to check to see if this is generally a good change or not, and if there is any red flag. I will polish the change if we agreed to move toward this direction.
On 2017/04/28 23:54:28, victorhsieh0 wrote: > PTA overview L. > > I just made it compiled and passed most tests. I'd like to check to see if this > is generally a good change or not, and if there is any red flag. I will polish > the change if we agreed to move toward this direction. Conceptually, I like the idea, and there's no red flag from my point of view. Some high level comments. - I'd appreciate more documents :-). Specifically, retry / error handling event dispatching strategy should be clearly documented. - s/Tos/TermsOfService/ for consistency? - Some no-longer-needed observer related code is still remaining. (Maybe this is your "polish" target?). - Nice if you can add tests.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
no red flags from me too :) just left some high-level naming bikeshed comments, but will wait until after the polish for the rest. https://codereview.chromium.org/2844383006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2844383006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_support_host.h:106: void SetAuthHandler(AuthHandler* handler); Probably a naming nit, but typically Handlers are chained: https://en.wikipedia.org/wiki/Chain-of-responsibility_pattern , https://cs.chromium.org/chromium/src/chrome/service/service_ipc_server.h?l=66 These all look like Delegates to me. https://codereview.chromium.org/2844383006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_support_host.h:107: void UnsetAuthHandler(); Maybe remove all the Unset*() and just call SetXxxYyy(nullptr)? https://codereview.chromium.org/2844383006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_support_host.h:201: ErrorHandler* error_handler_ = nullptr; // not owned Should you DCHECK if any of these are non-nullptr on desruction? That'll signal that there was a race / destruction order SNAFU.
The CQ bit was checked by victorhsieh@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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) chromium_presubmit on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?)) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, build has not started yet; builder either lacks capacity or does not exist (misspelled?))
The CQ bit was checked by victorhsieh@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 ========== WIP: Turn ArcSupportHost from Observer model to Handler ArcSupportHost's subscribers have some overlap of responsibility, and there are several places to skip the work in case it is supposed to be done in the other subscriber. Instead, this change turns the observer model into several handlers for better responsibility split. TEST=TODO BUG=698418 ========== to ========== Turn ArcSupportHost from Observer model to Delegate ArcSupportHost's subscribers have some overlap of responsibility, and there are several places to skip the work in case it is supposed to be done in the other subscriber. Instead, this change turns the observer model into several delegates for better responsibility split. TEST=unit_test --gtest_filter=Arc* TEST=browser_test --gtest_filer=Arc* TEST=git cl try BUG=698418 ==========
The CQ bit was checked by victorhsieh@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...
PTAL https://codereview.chromium.org/2844383006/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2844383006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_support_host.h:106: void SetAuthHandler(AuthHandler* handler); On 2017/05/01 16:45:56, Luis Héctor Chávez wrote: > Probably a naming nit, but typically Handlers are chained: > https://en.wikipedia.org/wiki/Chain-of-responsibility_pattern , > https://cs.chromium.org/chromium/src/chrome/service/service_ipc_server.h?l=66 > > These all look like Delegates to me. Done. https://codereview.chromium.org/2844383006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_support_host.h:107: void UnsetAuthHandler(); On 2017/05/01 16:45:56, Luis Héctor Chávez wrote: > Maybe remove all the Unset*() and just call SetXxxYyy(nullptr)? Done. https://codereview.chromium.org/2844383006/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_support_host.h:201: ErrorHandler* error_handler_ = nullptr; // not owned On 2017/05/01 16:45:56, Luis Héctor Chávez wrote: > Should you DCHECK if any of these are non-nullptr on desruction? That'll signal > that there was a race / destruction order SNAFU. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:968: // ERROR_WITH_FEEDBACK is set in OnSignInFailed(). In the case, stopping This comment seems to be out of sync with the code now. Maybe just omit the ERROR_WITH_FEEDBACK bit. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:977: // For second or later boot, either ERROR_WITH_FEEDBACK case or ACTIVE This, too. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:47: // Delegate to handle manual authentication related events. The life cycle of All the "The life cycle of the instance..." comments are maybe unnecessary, since they are responsible for their own lifetime. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:80: virtual void OnTermsRetry() = 0; OnTermsRetried? OnTermsRetryClicked? to be consistent with the OnObjectVerbed pattern in the rest of the method names. Same in L61. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:83: class ErrorDelegate { Comment? At least mention that this is the "fallback" delegate that will be invoked only if there is no more specific delegate. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:91: virtual void OnOptInAborted() {} Any reason why this Delegate has implementations, while the other two only have pure virtual methods? https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host_unittest.cc (right): https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:21: class ArcSupportHostTest : public testing::Test { Can this class (and the rest of the tests) be in the anonymous namespace? Adding `using namespace arc;` is fine if you want to avoid explicitly adding the `arc::` namespace. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:47: class MockAuthDelegate : public ArcSupportHost::AuthDelegate { Can these three classes be in the anonymous namespace? They don't seem to need to be nested within ArcSupportHostTest. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:92: support_host()->SetAuthDelegate(nullptr); (optional) Maybe add an RAII ScopedAuthDelegate? Not sure if the tradeoff is worth it. Same for the other Delegates. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:162: EXPECT_CALL(error_delegate, OnOptInAborted()); Is there an easy way to express that you expect something to NOT be called?
PTAL https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:968: // ERROR_WITH_FEEDBACK is set in OnSignInFailed(). In the case, stopping On 2017/05/12 15:41:30, Luis Héctor Chávez wrote: > This comment seems to be out of sync with the code now. Maybe just omit the > ERROR_WITH_FEEDBACK bit. Done. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:977: // For second or later boot, either ERROR_WITH_FEEDBACK case or ACTIVE On 2017/05/12 15:41:30, Luis Héctor Chávez wrote: > This, too. Done. Please let me know if there is something worth to comment about, e.g. if it should still happen in certain condition. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:47: // Delegate to handle manual authentication related events. The life cycle of On 2017/05/12 15:41:30, Luis Héctor Chávez wrote: > All the "The life cycle of the instance..." comments are maybe unnecessary, > since they are responsible for their own lifetime. Removed, but add extra comment in the relevant code, which implicitly relies on the pointer availability. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:80: virtual void OnTermsRetry() = 0; On 2017/05/12 15:41:31, Luis Héctor Chávez wrote: > OnTermsRetried? OnTermsRetryClicked? to be consistent with the OnObjectVerbed > pattern in the rest of the method names. Same in L61. Done. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:83: class ErrorDelegate { On 2017/05/12 15:41:31, Luis Héctor Chávez wrote: > Comment? At least mention that this is the "fallback" delegate that will be > invoked only if there is no more specific delegate. Done. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:91: virtual void OnOptInAborted() {} On 2017/05/12 15:41:31, Luis Héctor Chávez wrote: > Any reason why this Delegate has implementations, while the other two only have > pure virtual methods? Done. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host_unittest.cc (right): https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:21: class ArcSupportHostTest : public testing::Test { On 2017/05/12 15:41:31, Luis Héctor Chávez wrote: > Can this class (and the rest of the tests) be in the anonymous namespace? Adding > `using namespace arc;` is fine if you want to avoid explicitly adding the > `arc::` namespace. Done. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:47: class MockAuthDelegate : public ArcSupportHost::AuthDelegate { On 2017/05/12 15:41:31, Luis Héctor Chávez wrote: > Can these three classes be in the anonymous namespace? They don't seem to need > to be nested within ArcSupportHostTest. Done. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:92: support_host()->SetAuthDelegate(nullptr); On 2017/05/12 15:41:31, Luis Héctor Chávez wrote: > (optional) Maybe add an RAII ScopedAuthDelegate? Not sure if the tradeoff is > worth it. Same for the other Delegates. Done. https://codereview.chromium.org/2844383006/diff/60001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:162: EXPECT_CALL(error_delegate, OnOptInAborted()); On 2017/05/12 15:41:31, Luis Héctor Chávez wrote: > Is there an easy way to express that you expect something to NOT be called? .Times(0). But AuthDelegate does not have the related function in this case. Added some .Times(0) in some other cases.
try is not running on last PS? Could you run it with your next PS? https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:963: DCHECK(!terms_of_service_negotiator_); Could you verify that this is not a part of auth flow case, too? https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:967: if (arc_session_runner_->IsStopped()) { The condition looks negated? Unfortunately, it is difficult to make testing automated now, but could you kindly test the scenarios manually, too? (And update TEST= line in description?). https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:192: auth_delegate_ = delegate; Maybe. if (delegate) DCHECK(!tos_delegate_); (or, DCHECK(!delegate || !tos_delegate_); with comment?) https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:197: tos_delegate_ = delegate; Ditto. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:554: DCHECK(auth_delegate_); Could you move DCHECK to L552? On anytime this event is triggered, auth_delegate_ should be set, IIUC? https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:571: DCHECK(tos_delegate_); Ditto. Move to L563? https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:89: // the ARC support window is closed (except when when the terms of service nit: s/when when/when/ https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host_unittest.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:47: class DelegateRAII { Can this be merged into ArcSupportHostTest? Actual RAII can be done in TearDown(), instead. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:127: EXPECT_CALL(*auth_delegate, OnAuthSucceeded(Eq("fake_code"))); IIUC, even if OnAuthSucceeded is not called, the test would pass? At least StrictMock is needed? # Or, alternatively how about getting rid of gmock, because it is very difficult to set up and to write right expectation precisely, although the syntax can look simple at quick glance, so the maintenance cost sounds very expensive, according to my experience... https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:164: EXPECT_CALL(tos_delegate, OnTermsAgreed(_, _, _)); Can args be checked, too? https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:167: support_host()->SetTermsOfServiceDelegate(nullptr); Unnecessary, too? https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:201: support_host()->SetTermsOfServiceDelegate(nullptr); Ditto.
Description was changed from
==========
Turn ArcSupportHost from Observer model to Delegate
ArcSupportHost's subscribers have some overlap of responsibility, and
there are several places to skip the work in case it is supposed to be
done in the other subscriber.
Instead, this change turns the observer model into several delegates
for better responsibility split.
TEST=unit_test --gtest_filter=Arc*
TEST=browser_test --gtest_filer=Arc*
TEST=git cl try
BUG=698418
==========
to
==========
Turn ArcSupportHost from Observer model to Delegate
ArcSupportHost's subscribers have some overlap of responsibility, and
there are several places to skip the work in case it is supposed to be
done in the other subscriber.
Instead, this change turns the observer model into several delegates
for better responsibility split.
TEST=unit_test --gtest_filter=Arc*
TEST=browser_test --gtest_filer=Arc*
TEST=(disable silent sign-in) accept ToS, click sign-in and unplug
ethernet cable. See feedback button. Click retry, click sign-in,
See Play Store.
TEST=(disable silent sign-in) accept ToS, force logout from Chrome OS.
Re-login. See the window pops up with sign-in button.
TEST=git cl try
BUG=698418
==========
The CQ bit was checked by victorhsieh@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...
PTAL https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:963: DCHECK(!terms_of_service_negotiator_); On 2017/05/15 09:05:44, hidehiko wrote: > Could you verify that this is not a part of auth flow case, too? Any suggestion on how to check this? Auth is mostly not involved in ASM now, except perhaps the final onProvisioningFinished. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:967: if (arc_session_runner_->IsStopped()) { On 2017/05/15 09:05:44, hidehiko wrote: > The condition looks negated? > > Unfortunately, it is difficult to make testing automated now, but could you > kindly test the scenarios manually, too? (And update TEST= line in > description?). Done. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:192: auth_delegate_ = delegate; On 2017/05/15 09:05:44, hidehiko wrote: > Maybe. > > if (delegate) > DCHECK(!tos_delegate_); > > (or, DCHECK(!delegate || !tos_delegate_); with comment?) Done. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:197: tos_delegate_ = delegate; On 2017/05/15 09:05:44, hidehiko wrote: > Ditto. Done. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:554: DCHECK(auth_delegate_); On 2017/05/15 09:05:44, hidehiko wrote: > Could you move DCHECK to L552? > On anytime this event is triggered, auth_delegate_ should be set, IIUC? Done. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.cc:571: DCHECK(tos_delegate_); On 2017/05/15 09:05:44, hidehiko wrote: > Ditto. Move to L563? Done. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host.h:89: // the ARC support window is closed (except when when the terms of service On 2017/05/15 09:05:44, hidehiko wrote: > nit: s/when when/when/ Done. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host_unittest.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:47: class DelegateRAII { On 2017/05/15 09:05:44, hidehiko wrote: > Can this be merged into ArcSupportHostTest? > Actual RAII can be done in TearDown(), instead. Done. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:127: EXPECT_CALL(*auth_delegate, OnAuthSucceeded(Eq("fake_code"))); On 2017/05/15 09:05:44, hidehiko wrote: > IIUC, even if OnAuthSucceeded is not called, the test would pass? > At least StrictMock is needed? Did you mean uninterested call? IIUC this call must be called as expected, but if OnAuthFailed is called, it will still pass. StrictMock can prevent that if desired. > > # Or, alternatively how about getting rid of gmock, because it is very difficult > to set up and to write right expectation precisely, although the syntax can look > simple at quick glance, so the maintenance cost sounds very expensive, according > to my experience... Any alternatives? :) Otherwise, I'd prefer gmock as it seems to be commonly used already. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:164: EXPECT_CALL(tos_delegate, OnTermsAgreed(_, _, _)); On 2017/05/15 09:05:44, hidehiko wrote: > Can args be checked, too? Those parameters (whether the user agree to show metrics, enabling B&R, location) don't look relevant to the test. So I'd not add expectation here. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:167: support_host()->SetTermsOfServiceDelegate(nullptr); On 2017/05/15 09:05:44, hidehiko wrote: > Unnecessary, too? Done. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:201: support_host()->SetTermsOfServiceDelegate(nullptr); On 2017/05/15 09:05:44, hidehiko wrote: > Ditto. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
ping?
Oops, sorry for delay. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:963: DCHECK(!terms_of_service_negotiator_); On 2017/05/15 22:31:53, victorhsieh0 wrote: > On 2017/05/15 09:05:44, hidehiko wrote: > > Could you verify that this is not a part of auth flow case, too? > > Any suggestion on how to check this? Auth is mostly not involved in ASM now, > except perhaps the final onProvisioningFinished. Three scenarios, I'm worried about. 1) - Enable ARC with manual auth flag. - And let manual auth fail in ArcSupport app. Then click retry. 2) - Enable ARC with manual auth flag. - Let auth fail in ARC container. - Then click retry. 3) - Enable ARC, and wait for PlayStore. Maybe install random app. - Disable ARC, which should trigger data removal. - Immediately after disabling (before actual stop), re-enable ARC. - I think you'd see error page (This behavior itself is a bug, and WIP, though). Then click retry. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host_unittest.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:127: EXPECT_CALL(*auth_delegate, OnAuthSucceeded(Eq("fake_code"))); On 2017/05/15 22:31:54, victorhsieh0 wrote: > On 2017/05/15 09:05:44, hidehiko wrote: > > IIUC, even if OnAuthSucceeded is not called, the test would pass? > > At least StrictMock is needed? > Did you mean uninterested call? IIUC this call must be called as expected, but > if OnAuthFailed is called, it will still pass. StrictMock can prevent that if > desired. > If gmock is continued to be used, StrictMock sounds better. > > > > # Or, alternatively how about getting rid of gmock, because it is very > difficult > > to set up and to write right expectation precisely, although the syntax can > look > > simple at quick glance, so the maintenance cost sounds very expensive, > according > > to my experience... > Any alternatives? :) Otherwise, I'd prefer gmock as it seems to be commonly > used already. If we can keep this size, I'm ok with gmock. However, if there is a possibility to extend the test, hand crafted fake (not mock) implementation sounds better, IMHO? https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:164: EXPECT_CALL(tos_delegate, OnTermsAgreed(_, _, _)); On 2017/05/15 22:31:54, victorhsieh0 wrote: > On 2017/05/15 09:05:44, hidehiko wrote: > > Can args be checked, too? > > Those parameters (whether the user agree to show metrics, enabling B&R, > location) don't look relevant to the test. So I'd not add expectation here. WDY mean? My understanding is that the preference value is passed here, on agreement, and it is a part of function's spec?
The CQ bit was checked by victorhsieh@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: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by victorhsieh@chromium.org to run a CQ dry run
The CQ bit was unchecked by victorhsieh@google.com
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 checked by victorhsieh@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.
PTAL https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:963: DCHECK(!terms_of_service_negotiator_); On 2017/05/25 12:05:52, hidehiko wrote: > On 2017/05/15 22:31:53, victorhsieh0 wrote: > > On 2017/05/15 09:05:44, hidehiko wrote: > > > Could you verify that this is not a part of auth flow case, too? > > > > Any suggestion on how to check this? Auth is mostly not involved in ASM now, > > except perhaps the final onProvisioningFinished. > > Three scenarios, I'm worried about. Done. It's a bit hacky but please let me know if you see a better way. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_support_host_unittest.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:127: EXPECT_CALL(*auth_delegate, OnAuthSucceeded(Eq("fake_code"))); On 2017/05/25 12:05:52, hidehiko wrote: > If gmock is continued to be used, StrictMock sounds better. Done > If we can keep this size, I'm ok with gmock. However, if there is a possibility > to extend the test, hand crafted fake (not mock) implementation sounds better, > IMHO? I really hope we won't have to grow the responsibility of this class. To me, I don't see a problem using gmock at this moment. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:164: EXPECT_CALL(tos_delegate, OnTermsAgreed(_, _, _)); On 2017/05/25 12:05:52, hidehiko wrote: > On 2017/05/15 22:31:54, victorhsieh0 wrote: > > On 2017/05/15 09:05:44, hidehiko wrote: > > > Can args be checked, too? > > > > Those parameters (whether the user agree to show metrics, enabling B&R, > > location) don't look relevant to the test. So I'd not add expectation here. > > WDY mean? My understanding is that the preference value is passed here, on > agreement, and it is a part of function's spec? Oh, I missed the point. Done.
https://codereview.chromium.org/2844383006/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2844383006/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:551: DCHECK(error_delegate_); What if OptIn is closed during the progress. In this case we should not stop ARC. ARC is stopped only in case OptIn UI closed on Start or Error page. Is this true with new code?
The CQ bit was checked by victorhsieh@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.
PTAL (PS11's subject is just an accident) https://codereview.chromium.org/2844383006/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2844383006/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:551: DCHECK(error_delegate_); On 2017/05/26 19:43:46, khmel wrote: > What if OptIn is closed during the progress. > In this case we should not stop ARC. > ARC is stopped only in case OptIn UI closed on Start or Error page. > Is this true with new code? Done. Renamed the function as discussed offline.
https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:966: DCHECK(!support_host_->HasAuthDelegateForTest()); you shouldn't use ...ForTest() methods outside of tests. either move this DCHECK to ArcSupportHost or rename the method to HasAuthDelegate(). https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:970: if (!arc_session_runner_->IsStopped()) { nit: maybe invert the order so that the positive case comes before. https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:557: if (message.GetString(kCode, &code)) { nit: maybe convert to guard clause pattern? if (!message.GetString(kCode, &code)) { NOTREACHED(); return; } auth_delegate_->OnAuthSucceded(code); same with L579 https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:50: virtual ~AuthDelegate() = default; nit: make the dtors protected (per the ARC chromium style). https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:116: bool HasAuthDelegateForTest() { return auth_delegate_ != nullptr; } nit: you can make this const https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host_unittest.cc (right): https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:124: EXPECT_CALL(*auth_delegate, OnAuthSucceeded(Eq("fake_code"))); nit: maybe make this literal a constexpr char[] here in this method. https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/extensions/fake_arc_support.cc (right): https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/extensions/fake_arc_support.cc:27: if (native_message_host_) { nit: use guard clause pattern if (!native_message_host_) return; // rest also, maybe consider refactoring these two lines so that both Close() and ~FakeArcSupport() can call them to avoid the repetition.
The CQ bit was checked by victorhsieh@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.
PTAL https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:966: DCHECK(!support_host_->HasAuthDelegateForTest()); On 2017/05/30 16:06:27, Luis Héctor Chávez wrote: > you shouldn't use ...ForTest() methods outside of tests. either move this DCHECK > to ArcSupportHost or rename the method to HasAuthDelegate(). Done. https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:970: if (!arc_session_runner_->IsStopped()) { On 2017/05/30 16:06:27, Luis Héctor Chávez wrote: > nit: maybe invert the order so that the positive case comes before. Done. https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.cc (right): https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.cc:557: if (message.GetString(kCode, &code)) { On 2017/05/30 16:06:27, Luis Héctor Chávez wrote: > nit: maybe convert to guard clause pattern? > > if (!message.GetString(kCode, &code)) { > NOTREACHED(); > return; > } > auth_delegate_->OnAuthSucceded(code); > > same with L579 Done. https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:50: virtual ~AuthDelegate() = default; On 2017/05/30 16:06:27, Luis Héctor Chávez wrote: > nit: make the dtors protected (per the ARC chromium style). Done. https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:116: bool HasAuthDelegateForTest() { return auth_delegate_ != nullptr; } On 2017/05/30 16:06:27, Luis Héctor Chávez wrote: > nit: you can make this const Done. https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host_unittest.cc (right): https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host_unittest.cc:124: EXPECT_CALL(*auth_delegate, OnAuthSucceeded(Eq("fake_code"))); On 2017/05/30 16:06:27, Luis Héctor Chávez wrote: > nit: maybe make this literal a constexpr char[] here in this method. Done. https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/extensions/fake_arc_support.cc (right): https://codereview.chromium.org/2844383006/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/extensions/fake_arc_support.cc:27: if (native_message_host_) { On 2017/05/30 16:06:27, Luis Héctor Chávez wrote: > nit: use guard clause pattern > > if (!native_message_host_) > return; > // rest > > also, maybe consider refactoring these two lines so that both Close() and > ~FakeArcSupport() can call them to avoid the repetition. Done.
Sorry for delay. (Back from sick leave...) https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:963: DCHECK(!terms_of_service_negotiator_); On 2017/05/26 19:38:41, victorhsieh0 wrote: > On 2017/05/25 12:05:52, hidehiko wrote: > > On 2017/05/15 22:31:53, victorhsieh0 wrote: > > > On 2017/05/15 09:05:44, hidehiko wrote: > > > > Could you verify that this is not a part of auth flow case, too? > > > > > > Any suggestion on how to check this? Auth is mostly not involved in ASM > now, > > > except perhaps the final onProvisioningFinished. > > > > Three scenarios, I'm worried about. > Done. It's a bit hacky but please let me know if you see a better way. Thank you for manual check. Could you double check if third case is covered. In that case, error is reported L665 at latest PS. So, tos negotiator is not created yet, and this function is called, IIUC. Then, there is no way to restart ToS negotiation? https://codereview.chromium.org/2844383006/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2844383006/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:88: // Called when the window is closed but only after terms accepted. If the This comment looks not true? If an error happens before starting ToS negotiation, and then error page is closed, this is called even before ToS agreement.
Sorry for delay. (Back from sick leave...) https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:963: DCHECK(!terms_of_service_negotiator_); On 2017/05/26 19:38:41, victorhsieh0 wrote: > On 2017/05/25 12:05:52, hidehiko wrote: > > On 2017/05/15 22:31:53, victorhsieh0 wrote: > > > On 2017/05/15 09:05:44, hidehiko wrote: > > > > Could you verify that this is not a part of auth flow case, too? > > > > > > Any suggestion on how to check this? Auth is mostly not involved in ASM > now, > > > except perhaps the final onProvisioningFinished. > > > > Three scenarios, I'm worried about. > Done. It's a bit hacky but please let me know if you see a better way. Thank you for manual check. Could you double check if third case is covered. In that case, error is reported L665 at latest PS. So, tos negotiator is not created yet, and this function is called, IIUC. Then, there is no way to restart ToS negotiation? https://codereview.chromium.org/2844383006/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2844383006/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:88: // Called when the window is closed but only after terms accepted. If the This comment looks not true? If an error happens before starting ToS negotiation, and then error page is closed, this is called even before ToS agreement.
The CQ bit was checked by victorhsieh@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.
https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:963: DCHECK(!terms_of_service_negotiator_); On 2017/05/31 17:17:01, hidehiko wrote: > On 2017/05/26 19:38:41, victorhsieh0 wrote: > > On 2017/05/25 12:05:52, hidehiko wrote: > > > On 2017/05/15 22:31:53, victorhsieh0 wrote: > > > > On 2017/05/15 09:05:44, hidehiko wrote: > > > > > Could you verify that this is not a part of auth flow case, too? > > > > > > > > Any suggestion on how to check this? Auth is mostly not involved in ASM > > now, > > > > except perhaps the final onProvisioningFinished. > > > > > > Three scenarios, I'm worried about. > > Done. It's a bit hacky but please let me know if you see a better way. > > Thank you for manual check. > Could you double check if third case is covered. > > In that case, error is reported L665 at latest PS. So, tos negotiator is not > created yet, and this function is called, IIUC. > Then, there is no way to restart ToS negotiation? I see the problem now. How would you suggest to fix this? I see your TODO at L665. Is it ok to also check enable_requests_ at L665 as a workaround, until you remove L665? if (!arc_session_runner_->IsStopped() && !enable_requested_) { Or we may have to add back the origianl prefs::kArcTermsAccepted condition, and not restart the instance as a special case. https://codereview.chromium.org/2844383006/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2844383006/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:88: // Called when the window is closed but only after terms accepted. If the On 2017/05/31 17:17:01, hidehiko wrote: > This comment looks not true? > If an error happens before starting ToS negotiation, and then error page is > closed, this is called even before ToS agreement. Done, I didn't consider that case. Now I just call this function OnWindowClosed...
https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:963: DCHECK(!terms_of_service_negotiator_); On 2017/05/31 20:49:28, victorhsieh0 wrote: > On 2017/05/31 17:17:01, hidehiko wrote: > > On 2017/05/26 19:38:41, victorhsieh0 wrote: > > > On 2017/05/25 12:05:52, hidehiko wrote: > > > > On 2017/05/15 22:31:53, victorhsieh0 wrote: > > > > > On 2017/05/15 09:05:44, hidehiko wrote: > > > > > > Could you verify that this is not a part of auth flow case, too? > > > > > > > > > > Any suggestion on how to check this? Auth is mostly not involved in ASM > > > now, > > > > > except perhaps the final onProvisioningFinished. > > > > > > > > Three scenarios, I'm worried about. > > > Done. It's a bit hacky but please let me know if you see a better way. > > > > Thank you for manual check. > > Could you double check if third case is covered. > > > > In that case, error is reported L665 at latest PS. So, tos negotiator is not > > created yet, and this function is called, IIUC. > > Then, there is no way to restart ToS negotiation? > > I see the problem now. How would you suggest to fix this? I see your TODO at > L665. Is it ok to also check enable_requests_ at L665 as a workaround, until > you remove L665? > > if (!arc_session_runner_->IsStopped() && !enable_requested_) { > > Or we may have to add back the origianl prefs::kArcTermsAccepted condition, and > not restart the instance as a special case. I think kArcTermsAccepted sounds safer.
The CQ bit was checked by victorhsieh@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: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) linux_chromium_headless_rel on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by victorhsieh@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...
Done + rebased. https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/80001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager.cc:963: DCHECK(!terms_of_service_negotiator_); > I think kArcTermsAccepted sounds safer. Done. Reverted some changes here. Also added a TODO to you as we may be able to remove this case later, but let me know if it's not the case.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Thanks! lgtm. Defer to Luis.
lgtm https://codereview.chromium.org/2844383006/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2844383006/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:72: // Called when the user rejects the terms of service or close the page. nit: closes https://codereview.chromium.org/2844383006/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:83: // Delegate to handle general error events. Note that some of the callback nit: extra space
lgtm
lgtm
lgtm sorry for the delay :( https://codereview.chromium.org/2844383006/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:981: DCHECK_EQ(State::ACTIVE, state_); Maybe for a follow-up change, but do we have any stronger guarantees about the value of |state_| in this function? At the very least should also be able to add it to the branch in L986 IIUC. Can we also add it (or a similar DCHECK) to L971?
The CQ bit was checked by victorhsieh@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...
Thanks. https://codereview.chromium.org/2844383006/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_session_manager.cc (right): https://codereview.chromium.org/2844383006/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_session_manager.cc:981: DCHECK_EQ(State::ACTIVE, state_); On 2017/06/06 17:51:17, Luis Héctor Chávez wrote: > Maybe for a follow-up change, but do we have any stronger guarantees about the > value of |state_| in this function? At the very least should also be able to add > it to the branch in L986 IIUC. > > Can we also add it (or a similar DCHECK) to L971? OK, I'll do this as a follow-up change. https://codereview.chromium.org/2844383006/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_support_host.h (right): https://codereview.chromium.org/2844383006/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:72: // Called when the user rejects the terms of service or close the page. On 2017/06/02 17:02:40, khmel wrote: > nit: closes Done. https://codereview.chromium.org/2844383006/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_support_host.h:83: // Delegate to handle general error events. Note that some of the callback On 2017/06/02 17:02:41, khmel wrote: > nit: extra space Done.
The CQ bit was unchecked by victorhsieh@google.com
The CQ bit was checked by victorhsieh@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from hidehiko@chromium.org, lhchavez@chromium.org, khmel@chromium.org Link to the patchset: https://codereview.chromium.org/2844383006/#ps340001 (title: "rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 340001, "attempt_start_ts": 1496773644402210,
"parent_rev": "c5bee0bea1528a475e4a00515f06f39c08af395c", "commit_rev":
"63a1da923764a69409907f8135d0b4bbc56f0a03"}
Message was sent while issue was closed.
Description was changed from
==========
Turn ArcSupportHost from Observer model to Delegate
ArcSupportHost's subscribers have some overlap of responsibility, and
there are several places to skip the work in case it is supposed to be
done in the other subscriber.
Instead, this change turns the observer model into several delegates
for better responsibility split.
TEST=unit_test --gtest_filter=Arc*
TEST=browser_test --gtest_filer=Arc*
TEST=(disable silent sign-in) accept ToS, click sign-in and unplug
ethernet cable. See feedback button. Click retry, click sign-in,
See Play Store.
TEST=(disable silent sign-in) accept ToS, force logout from Chrome OS.
Re-login. See the window pops up with sign-in button.
TEST=git cl try
BUG=698418
==========
to
==========
Turn ArcSupportHost from Observer model to Delegate
ArcSupportHost's subscribers have some overlap of responsibility, and
there are several places to skip the work in case it is supposed to be
done in the other subscriber.
Instead, this change turns the observer model into several delegates
for better responsibility split.
TEST=unit_test --gtest_filter=Arc*
TEST=browser_test --gtest_filer=Arc*
TEST=(disable silent sign-in) accept ToS, click sign-in and unplug
ethernet cable. See feedback button. Click retry, click sign-in,
See Play Store.
TEST=(disable silent sign-in) accept ToS, force logout from Chrome OS.
Re-login. See the window pops up with sign-in button.
TEST=git cl try
BUG=698418
Review-Url: https://codereview.chromium.org/2844383006
Cr-Commit-Position: refs/heads/master@{#477376}
Committed:
https://chromium.googlesource.com/chromium/src/+/63a1da923764a69409907f8135d0...
==========
Message was sent while issue was closed.
Committed patchset #18 (id:340001) as https://chromium.googlesource.com/chromium/src/+/63a1da923764a69409907f8135d0... |
