Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2252093002:
Add support for Omaha cohorts to the component updater. (Closed)
|
Created:
4 years, 4 months ago by waffles Modified:
4 years, 4 months ago Reviewers:
Sorin Jianu CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for Omaha cohorts to the component updater.
BUG=638633, 639569
Committed: https://crrev.com/f43eb2fd3de79a4ae91b49fae79c9c66d850612f
Cr-Commit-Position: refs/heads/master@{#413809}
Patch Set 1 #Patch Set 2 : Don't serialize the attrs when they are empty. #
Total comments: 12
Patch Set 3 : Through #11 #Patch Set 4 : Using unique_ptrs instead of booleans. #
Total comments: 12
Patch Set 5 : Through #13 #
Total comments: 4
Patch Set 6 : Through #16 #Patch Set 7 : Unittest compilation fix #Patch Set 8 : Fix unit test #
Messages
Total messages: 37 (26 generated)
The CQ bit was checked by waffles@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...
waffles@chromium.org changed reviewers: + sorin@chromium.org
Sorin, PTAL.
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 waffles@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.
Small fry, thank you! https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... File components/update_client/persisted_data.cc (right): https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... components/update_client/persisted_data.cc:83: DCHECK_EQ(std::string::npos, id.find('.')); If the dcheck is a pre-condition for the argument we can move it closer to the top of the function. Then, we could declare cohort closer to where it is used. https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... components/update_client/persisted_data.cc:91: DCHECK(thread_checker_.CalledOnValidThread()); Is there a way to reduce dublication of code in these functions? https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... File components/update_client/update_checker.cc (right): https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... components/update_client/update_checker.cc:94: const std::string& cohort = metadata->GetCohort(item->id); This is a small deal and mostly a matter of personal preference. The code is correct as it is written. However, I avoid references to temporaries and I prefer to use named local variables in cases like this. The code is as efficient as the code with references (the temporary is moved into the local variable in one case, and in the other case the temporary is still created and needs to live until the end of the scope for the reference). Why the fuss? Depending what the code does after the call site, I still could end up with dangling references, so personally, I am always suspicious when I see the reference to something that is local. https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... File components/update_client/update_response.cc (right): https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... components/update_client/update_response.cc:285: result->cohort = GetAttribute(app, "cohort"); Would it make sense to dynamically allocate the cohort if it has the attribute and not use the boolean? https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... File components/update_client/update_response.h (right): https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... components/update_client/update_response.h:103: bool set_cohort; are these initialized anywhere? https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... components/update_client/update_response.h:103: bool set_cohort; What is the meaning of the booleans?
https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... File components/update_client/persisted_data.cc (right): https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... components/update_client/persisted_data.cc:83: DCHECK_EQ(std::string::npos, id.find('.')); On 2016/08/18 21:15:15, Sorin Jianu wrote: > If the dcheck is a pre-condition for the argument we can move it closer to the > top of the function. Then, we could declare cohort closer to where it is used. Done. https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... components/update_client/persisted_data.cc:91: DCHECK(thread_checker_.CalledOnValidThread()); On 2016/08/18 21:15:15, Sorin Jianu wrote: > Is there a way to reduce dublication of code in these functions? Done. https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... File components/update_client/update_checker.cc (right): https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... components/update_client/update_checker.cc:94: const std::string& cohort = metadata->GetCohort(item->id); On 2016/08/18 21:15:15, Sorin Jianu wrote: > This is a small deal and mostly a matter of personal preference. The code is > correct as it is written. However, I avoid references to temporaries and I > prefer to use named local variables in cases like this. The code is as efficient > as the code with references (the temporary is moved into the local variable in > one case, and in the other case the temporary is still created and needs to live > until the end of the scope for the reference). > Why the fuss? Depending what the code does after the call site, I still could > end up with dangling references, so personally, I am always suspicious when I > see the reference to something that is local. Done. https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... File components/update_client/update_response.cc (right): https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... components/update_client/update_response.cc:285: result->cohort = GetAttribute(app, "cohort"); On 2016/08/18 21:15:15, Sorin Jianu wrote: > Would it make sense to dynamically allocate the cohort if it has the attribute > and not use the boolean? According to the spec, if the server serializes any attribute, the client is supposed to write that value. If it serializes no attribute, the client is not supposed to alter its value. So consider: Client sends <app cohort="foo" ...> Server responds <app cohort="" ...> Client must set cohort to "". vs. Client sends <app cohort="foo" ...> Server responses <app ...> (no "cohort" attribute). Client stays in cohort "foo". In hindsight this was unnecessary complexity in the spec. Omaha didn't implement it this way, and so the server was changed to always serialize the attribute, echoing back the client's existing value if it doesn't need to change. Once we took this solution we assumed there would never be any race between the client adjusting values (e.g. cohorthint) while there was an in-flight update-check. I would be comfortable adjusting the protocol to state that compatible servers will always send these three attributes, and that clients should clear their cohort/name/hint if the server doesn't send it (with the idea that sending nothing == sending ""). Let me know if you want to go that way. https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... File components/update_client/update_response.h (right): https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... components/update_client/update_response.h:103: bool set_cohort; On 2016/08/18 21:15:15, Sorin Jianu wrote: > What is the meaning of the booleans? Done. https://codereview.chromium.org/2252093002/diff/20001/components/update_clien... components/update_client/update_response.h:103: bool set_cohort; On 2016/08/18 21:15:15, Sorin Jianu wrote: > are these initialized anywhere? Done.
no worries. https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... File components/update_client/persisted_data.cc (right): https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... components/update_client/persisted_data.cc:39: int result = fallback; we could move the declaration closer to where the variable is used. https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... components/update_client/persisted_data.cc:47: return result; Do you like this rewrite? int PersistedData::GetInt(const std::string& id, const std::string& key, int fallback) const { DCHECK(thread_checker_.CalledOnValidThread()); // We assume ids do not contain '.' characters. DCHECK_EQ(std::string::npos, id.find('.')); if (!pref_service_) return fallback; const base::DictionaryValue* dict = pref_service_->GetDictionary(kPersistedDataPreference); if (!dict) return fallback; int result(0); return dict->GetInteger( base::StringPrintf("apps.%s.%s", id.c_str(), key.c_str()), &result) ? result : fallback; } https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... components/update_client/persisted_data.cc:57: std::string result; same as above. https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... components/update_client/persisted_data.cc:74: if (!result.empty()) { {} not needed or we could use ?; https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... File components/update_client/update_checker.cc (right): https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... components/update_client/update_checker.cc:215: for (auto result : update_response.results().list) { could be useful to auto& https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... File components/update_client/update_response.cc (right): https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... components/update_client/update_response.cc:43: UpdateResponse::Result& UpdateResponse::Result::operator=(const Result& other) { oh, no.
Description was changed from ========== Add support for Omaha cohorts to the component updater. BUG=638633 ========== to ========== Add support for Omaha cohorts to the component updater. BUG=638633,639569 ==========
Thanks! PTAL. I tried to get clever with setting things in the map, but maybe it's overly complex and better to just unroll the loop. https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... File components/update_client/persisted_data.cc (right): https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... components/update_client/persisted_data.cc:39: int result = fallback; On 2016/08/22 19:59:33, Sorin Jianu wrote: > we could move the declaration closer to where the variable is used. Done. https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... components/update_client/persisted_data.cc:47: return result; On 2016/08/22 19:59:33, Sorin Jianu wrote: > Do you like this rewrite? > > int PersistedData::GetInt(const std::string& id, > const std::string& key, > int fallback) const { > DCHECK(thread_checker_.CalledOnValidThread()); > // We assume ids do not contain '.' characters. > DCHECK_EQ(std::string::npos, id.find('.')); > if (!pref_service_) > return fallback; > const base::DictionaryValue* dict = > pref_service_->GetDictionary(kPersistedDataPreference); > if (!dict) > return fallback; > int result(0); > return dict->GetInteger( > base::StringPrintf("apps.%s.%s", id.c_str(), key.c_str()), &result) > ? result : fallback; > } Done. https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... components/update_client/persisted_data.cc:57: std::string result; On 2016/08/22 19:59:33, Sorin Jianu wrote: > same as above. Done. https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... components/update_client/persisted_data.cc:74: if (!result.empty()) { On 2016/08/22 19:59:33, Sorin Jianu wrote: > {} not needed or we could use ?; Done. https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... File components/update_client/update_checker.cc (right): https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... components/update_client/update_checker.cc:215: for (auto result : update_response.results().list) { On 2016/08/22 19:59:34, Sorin Jianu wrote: > could be useful to auto& Done. https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... File components/update_client/update_response.cc (right): https://codereview.chromium.org/2252093002/diff/60001/components/update_clien... components/update_client/update_response.cc:43: UpdateResponse::Result& UpdateResponse::Result::operator=(const Result& other) { On 2016/08/22 19:59:34, Sorin Jianu wrote: > oh, no. Yeah. :( Tried another approach using a map.
lgtm Thank you! https://codereview.chromium.org/2252093002/diff/80001/components/update_clien... File components/update_client/update_response.cc (right): https://codereview.chromium.org/2252093002/diff/80001/components/update_clien... components/update_client/update_response.cc:106: return std::unique_ptr<std::string>(new std::string( Can we use MakeUnique or WrapUniquento make this somehow more readable? https://codereview.chromium.org/2252093002/diff/80001/components/update_clien... File components/update_client/update_response.h (right): https://codereview.chromium.org/2252093002/diff/80001/components/update_clien... components/update_client/update_response.h:110: static const char* kCohort; can we use static const char kCohort[] ?
The CQ bit was checked by waffles@chromium.org to run a CQ dry run
https://codereview.chromium.org/2252093002/diff/80001/components/update_clien... File components/update_client/update_response.cc (right): https://codereview.chromium.org/2252093002/diff/80001/components/update_clien... components/update_client/update_response.cc:106: return std::unique_ptr<std::string>(new std::string( On 2016/08/23 00:20:03, Sorin Jianu wrote: > Can we use MakeUnique or WrapUniquento make this somehow more readable? Done. https://codereview.chromium.org/2252093002/diff/80001/components/update_clien... File components/update_client/update_response.h (right): https://codereview.chromium.org/2252093002/diff/80001/components/update_clien... components/update_client/update_response.h:110: static const char* kCohort; On 2016/08/23 00:20:03, Sorin Jianu wrote: > can we use static const char kCohort[] ? Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm <3
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) 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 waffles@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 waffles@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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by waffles@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sorin@chromium.org Link to the patchset: https://codereview.chromium.org/2252093002/#ps140001 (title: "Fix unit test")
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 ========== Add support for Omaha cohorts to the component updater. BUG=638633,639569 ========== to ========== Add support for Omaha cohorts to the component updater. BUG=638633,639569 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Add support for Omaha cohorts to the component updater. BUG=638633,639569 ========== to ========== Add support for Omaha cohorts to the component updater. BUG=638633,639569 Committed: https://crrev.com/f43eb2fd3de79a4ae91b49fae79c9c66d850612f Cr-Commit-Position: refs/heads/master@{#413809} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/f43eb2fd3de79a4ae91b49fae79c9c66d850612f Cr-Commit-Position: refs/heads/master@{#413809} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
