Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2553193002:
Remove explicit singletonness of ArcBridgeService part 4. (Closed)
|
Created:
4 years ago by hidehiko Modified:
4 years ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, alemate+watch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, achuith+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemove explicit singletonness of ArcBridgeService part 4.
This CL gets rid of using ArcBridgeService::Get() in
arc_robot_auto_code_fetcher_browsertest, by getting rid of
depending of ArcAuthService.
Test for AttemptUserExit is moved to arc_session_manager_unittest.
For that purpose, fixed username_hash for ARC Kiosk fake user.
BUG=657687
BUG=b/31079732
TEST=Ran bots.
Committed: https://crrev.com/26b2f7b619d9a83b2151b47ad175ed9e1ecf3ccf
Cr-Commit-Position: refs/heads/master@{#437811}
Patch Set 1 #
Total comments: 7
Patch Set 2 : Address comments #
Total comments: 1
Patch Set 3 : Address comments. #
Total comments: 5
Patch Set 4 : rebase #Patch Set 5 : Address comments. #
Messages
Total messages: 32 (20 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...
hidehiko@chromium.org changed reviewers: + achuith@chromium.org, lhchavez@chromium.org, poromov@chromium.org
This is the last CL to prepare ArcBridgeService::Get(). PTAL. lhchavez@: c/b/c/arc OWNER. poromov@: ARC robot auth expert. achuith@: c/b/c/login OWNER. Thank you for review in advance.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
IIUC you didn't need the previous 3 CLs to achieve this. But anyways it's a good thing to reduce the number of singletons, so I'm okay with that :) https://codereview.chromium.org/2553193002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2553193002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:414: // At the end of test, we leave the dangling pointer |terminated|, I don't understand this comment. You are calling EXPECT_TRUE(terminated) in L422. Given that you set |terminated = false| in L416, you are expecting the callback to be invoked. There is no dangling pointer involved since the callback would not be invoked again. https://codereview.chromium.org/2553193002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc (left): https://codereview.chromium.org/2553193002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc:177: // a result of error of auth code fetching. Great! Having this Run() was kind of annoying :D https://codereview.chromium.org/2553193002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc (right): https://codereview.chromium.org/2553193002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc:140: EXPECT_EQ(kFakeAuthCode, auth_code); nit: maybe have a std::string outside the closure and pass a pointer to it, so you can EXPECT_EQ it after L142, thus ensuring that this fails if the callback is not invoked. https://codereview.chromium.org/2553193002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc:152: [](const std::string& auth_code) { EXPECT_TRUE(auth_code.empty()); })); maybe do something similar here, but also pass a bool to distinguish between the cases where it was never called vs. when it was called with an empty auth code.
The CQ bit was checked by achuith@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...
On 2016/12/06 17:19:26, hidehiko wrote: > This is the last CL to prepare ArcBridgeService::Get(). PTAL. > > lhchavez@: c/b/c/arc OWNER. > poromov@: ARC robot auth expert. > achuith@: c/b/c/login OWNER. c/b/c/login lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Remove explicit singletonness of ArcBridgeService part 4. This CL gets rid of using ArcBridgeService::Get() in arc_robot_auto_code_fetcher_browsertest, by getting rid of depending of ArcAuthService. Test for AttemptUserExit is moved to arc_session_manager_unittest. For that purpose, fixed username_hash for ARC Kiosk fake user. BUG=657687 BUG=b/31079732 TEST=Ran bots. ========== to ========== Remove explicit singletonness of ArcBridgeService part 4. This CL gets rid of using ArcBridgeService::Get() in arc_robot_auto_code_fetcher_browsertest, by getting rid of depending of ArcAuthService. Test for AttemptUserExit is moved to arc_session_manager_unittest. For that purpose, fixed username_hash for ARC Kiosk fake user. BUG=657687 BUG=b/31079732 TEST=Ran bots. ==========
poromov@chromium.org changed reviewers: + peletskyi@chromium.org
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...
PTAL. Note that it turned out that the original robot's test had a flakiness issue. In the last patch it was fixed. (Sorry, I misoperated so that PS3 contains rebase and diff from PS2. The update from PS2 is just robot's browser_test, conceptually). https://codereview.chromium.org/2553193002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2553193002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:414: // At the end of test, we leave the dangling pointer |terminated|, On 2016/12/06 19:08:56, Luis Héctor Chávez wrote: > I don't understand this comment. You are calling EXPECT_TRUE(terminated) in > L422. Given that you set |terminated = false| in L416, you are expecting the > callback to be invoked. There is no dangling pointer involved since the callback > would not be invoked again. The attempt_user_exit_callback_ in ArcSessionManager instance still keeps the pointer to |terminated|, even after the test scenario, e.g. TearDown(), which is a dangling pointer. I just assumed that attempt_user_exit_callback_ will never be called, so that we can TearDown() safely. https://codereview.chromium.org/2553193002/diff/1/chrome/browser/chromeos/arc... File chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc (right): https://codereview.chromium.org/2553193002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc:140: EXPECT_EQ(kFakeAuthCode, auth_code); On 2016/12/06 19:08:56, Luis Héctor Chávez wrote: > nit: maybe have a std::string outside the closure and pass a pointer to it, so > you can EXPECT_EQ it after L142, thus ensuring that this fails if the callback > is not invoked. Done. https://codereview.chromium.org/2553193002/diff/1/chrome/browser/chromeos/arc... chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc:152: [](const std::string& auth_code) { EXPECT_TRUE(auth_code.empty()); })); On 2016/12/06 19:08:56, Luis Héctor Chávez wrote: > maybe do something similar here, but also pass a bool to distinguish between the > cases where it was never called vs. when it was called with an empty auth code. Did similar stuff. Instead of passing bool, initialized the auth_code by non-empty value. https://codereview.chromium.org/2553193002/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc (right): https://codereview.chromium.org/2553193002/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc:164: EXPECT_EQ(std::string(), auth_code); Note: instead of std::string::empty(), use EXPECT_EQ so that, in case of error, the content is output.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm with a couple of nits/requests. https://codereview.chromium.org/2553193002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2553193002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:514: // assuming the callback will never be called. I'm still a bit confused by this. If I understand your reply correctly, this comment should mention that the callback will never be called *again*, is that correct? The callback is expected to be called exactly once by the time the code reaches L521. https://codereview.chromium.org/2553193002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc (right): https://codereview.chromium.org/2553193002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc:137: run_loop.Run(); aww, I liked the fact that you replaced the last Run() with RunUntilIdle() :( Is it possible to bring RunUntilIdle() back? If not (maybe due to hopping between multiple threads), please add a comment as to why it's needed.
Thank you for review. poromov@. Friendly, ping? https://codereview.chromium.org/2553193002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2553193002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:514: // assuming the callback will never be called. On 2016/12/08 17:36:13, Luis Héctor Chávez wrote: > I'm still a bit confused by this. If I understand your reply correctly, this > comment should mention that the callback will never be called *again*, is that > correct? The callback is expected to be called exactly once by the time the code > reaches L521. Oh, I see your point, I believe. Updated the comment. How about this? Or, alternatively, if we really want to ensure it is safe, using CancelableCallback https://cs.chromium.org/chromium/src/base/cancelable_callback.h?l=64 may be an option (but it looks overkill to me, though). https://codereview.chromium.org/2553193002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc (right): https://codereview.chromium.org/2553193002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/auth/arc_robot_auth_code_fetcher_browsertest.cc:137: run_loop.Run(); On 2016/12/08 17:36:13, Luis Héctor Chávez wrote: > aww, I liked the fact that you replaced the last Run() with RunUntilIdle() :( > > Is it possible to bring RunUntilIdle() back? If not (maybe due to hopping > between multiple threads), please add a comment as to why it's needed. Unfortunately, I do not think there is a handy way to do it. It actually posts tasks between threads (UI/IO at least), so using RunUntilIdle() may quit() before the FetchCallback is invoked. Using Run() and Quit() is a common way to deal with such a situation, AFAIK. Added comments.
https://codereview.chromium.org/2553193002/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_session_manager_unittest.cc (right): https://codereview.chromium.org/2553193002/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_session_manager_unittest.cc:514: // assuming the callback will never be called. On 2016/12/08 18:01:00, hidehiko wrote: > On 2016/12/08 17:36:13, Luis Héctor Chávez wrote: > > I'm still a bit confused by this. If I understand your reply correctly, this > > comment should mention that the callback will never be called *again*, is that > > correct? The callback is expected to be called exactly once by the time the > code > > reaches L521. > > Oh, I see your point, I believe. > Updated the comment. How about this? > > Or, alternatively, if we really want to ensure it is safe, > using CancelableCallback > https://cs.chromium.org/chromium/src/base/cancelable_callback.h?l=64 may be an > option (but it looks overkill to me, though). No need, the reworded comment is enough.
lgtm дпеь
On 2016/12/09 22:05:26, Sergey Poromov wrote: > lgtm > > дпеь Sorry, it was "lgtm" is Cyrillic and accidentally it was not removed.
The CQ bit was checked by hidehiko@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from achuith@chromium.org, lhchavez@chromium.org Link to the patchset: https://codereview.chromium.org/2553193002/#ps80001 (title: "Address comments.")
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": 80001, "attempt_start_ts": 1481506073438830,
"parent_rev": "17980cc302404184308ddcdd39de992c8cf731d7", "commit_rev":
"762d80f0b9e2cbb9ffa697a6bce2586d644bd387"}
Message was sent while issue was closed.
Description was changed from ========== Remove explicit singletonness of ArcBridgeService part 4. This CL gets rid of using ArcBridgeService::Get() in arc_robot_auto_code_fetcher_browsertest, by getting rid of depending of ArcAuthService. Test for AttemptUserExit is moved to arc_session_manager_unittest. For that purpose, fixed username_hash for ARC Kiosk fake user. BUG=657687 BUG=b/31079732 TEST=Ran bots. ========== to ========== Remove explicit singletonness of ArcBridgeService part 4. This CL gets rid of using ArcBridgeService::Get() in arc_robot_auto_code_fetcher_browsertest, by getting rid of depending of ArcAuthService. Test for AttemptUserExit is moved to arc_session_manager_unittest. For that purpose, fixed username_hash for ARC Kiosk fake user. BUG=657687 BUG=b/31079732 TEST=Ran bots. Review-Url: https://codereview.chromium.org/2553193002 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Remove explicit singletonness of ArcBridgeService part 4. This CL gets rid of using ArcBridgeService::Get() in arc_robot_auto_code_fetcher_browsertest, by getting rid of depending of ArcAuthService. Test for AttemptUserExit is moved to arc_session_manager_unittest. For that purpose, fixed username_hash for ARC Kiosk fake user. BUG=657687 BUG=b/31079732 TEST=Ran bots. Review-Url: https://codereview.chromium.org/2553193002 ========== to ========== Remove explicit singletonness of ArcBridgeService part 4. This CL gets rid of using ArcBridgeService::Get() in arc_robot_auto_code_fetcher_browsertest, by getting rid of depending of ArcAuthService. Test for AttemptUserExit is moved to arc_session_manager_unittest. For that purpose, fixed username_hash for ARC Kiosk fake user. BUG=657687 BUG=b/31079732 TEST=Ran bots. Committed: https://crrev.com/26b2f7b619d9a83b2151b47ad175ed9e1ecf3ccf Cr-Commit-Position: refs/heads/master@{#437811} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/26b2f7b619d9a83b2151b47ad175ed9e1ecf3ccf Cr-Commit-Position: refs/heads/master@{#437811} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
