Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(230)

Issue 39443002: settings: Add async system salt retrieval logic in DeviceOAuth2TokenServiceFactory (Closed)

Created:
7 years, 2 months ago by satorux1
Modified:
7 years, 1 month ago
CC:
chromium-reviews, stevenjb+watch_chromium.org, davemoore+watch_chromium.org, oshima+watch_chromium.org, nkostylev+watch_chromium.org
Visibility:
Public.

Description

settings: Add async system salt retrieval logic in DeviceOAuth2TokenServiceFactory Along the way, remove GetCachedSystemSalt() from SystemSaltGetter. BUG=309959, 306547 TEST=none R=hashimoto@chromium.org, pastarmovj@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=230957

Patch Set 1 : #

Total comments: 1

Patch Set 2 : add unit test #

Total comments: 8

Patch Set 3 : address comments #

Total comments: 2

Patch Set 4 : fix device_cloud_policy_manager_chromeos_unittest.cc #

Patch Set 5 : address comments #

Total comments: 25

Patch Set 6 : rebase #

Patch Set 7 : address comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+282 lines, -65 lines) Patch
M chrome/browser/chromeos/policy/device_cloud_policy_manager_chromeos_unittest.cc View 1 2 3 1 chunk +2 lines, -3 lines 0 comments Download
M chrome/browser/chromeos/settings/device_oauth2_token_service_factory.h View 1 2 3 4 5 6 2 chunks +25 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc View 1 2 3 4 5 6 5 chunks +65 lines, -28 lines 0 comments Download
A chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc View 1 2 3 4 5 6 1 chunk +165 lines, -0 lines 0 comments Download
M chrome/browser/chromeos/settings/token_encryptor.h View 1 2 3 4 5 6 3 chunks +5 lines, -8 lines 0 comments Download
M chrome/browser/chromeos/settings/token_encryptor.cc View 1 2 3 4 5 6 4 chunks +9 lines, -16 lines 0 comments Download
M chrome/chrome_tests_unit.gypi View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download
M chromeos/cryptohome/system_salt_getter.h View 1 chunk +0 lines, -5 lines 0 comments Download
M chromeos/cryptohome/system_salt_getter.cc View 1 chunk +0 lines, -4 lines 0 comments Download
M chromeos/dbus/fake_cryptohome_client.h View 1 2 3 4 5 6 2 chunks +8 lines, -0 lines 0 comments Download
M chromeos/dbus/fake_cryptohome_client.cc View 1 2 2 chunks +2 lines, -1 line 0 comments Download

Messages

Total messages: 21 (0 generated)
satorux1
pastarmovj: chrome/browser/chromeos/settings hashimoto: everything pneubeck: FYI, as I followed your suggestion. https://codereview.chromium.org/39443002/diff/10001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc (left): ...
7 years, 2 months ago (2013-10-24 06:25:17 UTC) #1
satorux1
oops. forgot to include the unittest in patch set 1. Patch set 2 includes it.
7 years, 2 months ago (2013-10-24 06:27:04 UTC) #2
hashimoto
https://codereview.chromium.org/39443002/diff/50001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/39443002/diff/50001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc#newcode78 chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:78: DCHECK(!token_service_); Can we handle system_salt.empty() case or emit an ...
7 years, 2 months ago (2013-10-24 06:50:00 UTC) #3
satorux1
https://codereview.chromium.org/39443002/diff/50001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/39443002/diff/50001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc#newcode78 chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:78: DCHECK(!token_service_); On 2013/10/24 06:50:00, hashimoto wrote: > Can we ...
7 years, 2 months ago (2013-10-24 07:33:08 UTC) #4
satorux1
fixed device_cloud_policy_manager_chromeos_unittest.cc in Patch Set 4.
7 years, 2 months ago (2013-10-24 07:40:26 UTC) #5
hashimoto
https://codereview.chromium.org/39443002/diff/50001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/39443002/diff/50001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc#newcode94 chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:94: DCHECK(pending_callbacks_.empty()); On 2013/10/24 07:33:08, satorux1 wrote: > On 2013/10/24 ...
7 years, 2 months ago (2013-10-24 07:49:01 UTC) #6
satorux1
https://codereview.chromium.org/39443002/diff/50001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/39443002/diff/50001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc#newcode94 chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:94: DCHECK(pending_callbacks_.empty()); On 2013/10/24 07:49:01, hashimoto wrote: > On 2013/10/24 ...
7 years, 2 months ago (2013-10-24 08:31:27 UTC) #7
hashimoto
lgtm
7 years, 2 months ago (2013-10-24 08:39:21 UTC) #8
pastarmovj
one suggestion and one nit. https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc#newcode95 chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:95: for (size_t i = ...
7 years, 2 months ago (2013-10-24 09:43:20 UTC) #9
satorux1
https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc#newcode95 chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:95: for (size_t i = 0; i < pending_callbacks_.size(); ++i) ...
7 years, 2 months ago (2013-10-24 10:23:14 UTC) #10
satorux1
https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc#newcode95 chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:95: for (size_t i = 0; i < pending_callbacks_.size(); ++i) ...
7 years, 2 months ago (2013-10-24 10:53:00 UTC) #11
pastarmovj
https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc#newcode95 chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:95: for (size_t i = 0; i < pending_callbacks_.size(); ++i) ...
7 years, 2 months ago (2013-10-24 11:28:48 UTC) #12
satorux1
https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc#newcode95 chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:95: for (size_t i = 0; i < pending_callbacks_.size(); ++i) ...
7 years, 2 months ago (2013-10-24 13:15:52 UTC) #13
pastarmovj
lgtm https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc#newcode95 chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:95: for (size_t i = 0; i < pending_callbacks_.size(); ...
7 years, 2 months ago (2013-10-24 13:21:30 UTC) #14
satorux1
https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc#newcode95 chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:95: for (size_t i = 0; i < pending_callbacks_.size(); ++i) ...
7 years, 2 months ago (2013-10-24 13:21:59 UTC) #15
pneubeck (no reviews)
https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc#newcode95 chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:95: for (size_t i = 0; i < pending_callbacks_.size(); ++i) ...
7 years, 2 months ago (2013-10-24 19:44:12 UTC) #16
satorux1
https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc (right): https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc#newcode95 chrome/browser/chromeos/settings/device_oauth2_token_service_factory.cc:95: for (size_t i = 0; i < pending_callbacks_.size(); ++i) ...
7 years, 1 month ago (2013-10-25 02:36:01 UTC) #17
satorux1
pneubeck@, I've addressed most of your comments, so I'm going to submit this based on ...
7 years, 1 month ago (2013-10-25 05:15:42 UTC) #18
satorux1
Committed patchset #7 manually as r230957 (presubmit successful).
7 years, 1 month ago (2013-10-25 05:16:35 UTC) #19
pneubeck (no reviews)
lgtm https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc File chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc (right): https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc#newcode62 chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc:62: // Test a case where Get() is called ...
7 years, 1 month ago (2013-10-25 07:35:01 UTC) #20
satorux1
7 years, 1 month ago (2013-10-25 07:42:50 UTC) #21
Message was sent while issue was closed.
On 2013/10/25 07:35:01, pneubeck wrote:
> lgtm
> 
>
https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/...
> File
>
chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc
> (right):
> 
>
https://codereview.chromium.org/39443002/diff/390001/chrome/browser/chromeos/...
>
chrome/browser/chromeos/settings/device_oauth2_token_service_factory_unittest.cc:62:
> // Test a case where Get() is called before the factory is initialized.
> On 2013/10/25 02:36:02, satorux1 wrote:
> > On 2013/10/24 19:44:13, pneubeck wrote:
> > > considering how much testing this logic requires, I really hope that we
> don't
> > > have to implement and test this in more places than this one.
> > 
> > I think smaller test cases are often clearer than a jumbo test case that
> covers
> > many things, hence I split test cases here.
> > 
> > 
> > > otherwise, we might consider putting this logic into some template/base
> class
> > > and test it once and for all.
> > 
> > That'd reduce the number of lines, but make tests less easy to follow and
> > change. I think this level of redundancy is acceptable.
> > 
> 
> I think my comment was misleading.
> I meant, if this pattern/logic of "storing callbacks until a shared object is
> asynchronously created" has to be used somewhere outside of
> DeviceOAuth2TokenServiceFactory, then we should consider sharing that code.

Ah I see. I misunderstood your comments. I agree with you that a common library
for solving this problem would be nice, but it may be small enough for
developers to reinvent for particular needs. :)

> 
> Your implementation and tests, I think, are perfectly fine.

Powered by Google App Engine
This is Rietveld 408576698