Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2640023007:
Adds PreviewsType version mechanism for clearing blacklist entries. (Closed)
|
Created:
3 years, 11 months ago by dougarnett Modified:
3 years, 10 months ago CC:
chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdds PreviewsType version mechanism for clearing blacklist entries.
Adds new table to the Previews opt-out store (PreviewsOptOutStoreSQL)
for holding the current version of PreviewsType's used on that client
with clean-up code to delete black list entries for a PreviewsType
when the version of that PreviewsType changes.
BUG=680620
Review-Url: https://codereview.chromium.org/2640023007
Cr-Commit-Position: refs/heads/master@{#450754}
Committed: https://chromium.googlesource.com/chromium/src/+/5e57dedf90886a800035abc8e70048266d1dd110
Patch Set 1 #
Total comments: 12
Patch Set 2 : Addressed tbansal's comments #
Total comments: 1
Patch Set 3 : Removed std::move #
Total comments: 10
Patch Set 4 : Addressed Ryan's comments except no tests yet. #Patch Set 5 : Missed a change in last upload #
Total comments: 2
Patch Set 6 : Fixed return signature of new method #Patch Set 7 : Added sql store test #Patch Set 8 : Cleaned up commented include #
Total comments: 20
Patch Set 9 : Addressed Ryan's comments (except switch nit) #Patch Set 10 : Reworked to also clear blacklist for previews types that are no longer enabled. #Patch Set 11 : Few nits #
Total comments: 7
Patch Set 12 : Addressed Ryan recent comments. #
Messages
Total messages: 66 (44 generated)
The CQ bit was checked by dougarnett@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...
dougarnett@chromium.org changed reviewers: + ryansturm@chromium.org
Ryan, I'm looking for your initial look at approach and code. I haven't yet turned on this code nor written tests nor added version to field trial params but expect there is enough here to give initial feedback on.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
dougarnett@chromium.org changed reviewers: + tbansal@chromium.org
Tarun, I wonder if you can give this an initial look as to approach. Ryan wasn't able to get to it before leaving for a few days and he recommended you.
looks good to me. Just a bunch of nits. https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... components/previews/core/previews_experiments.h:64: // Caller assumes ownership of returned vector. In Chromium, if a function returns raw ptr, the default is to assume that the callee has the ownership. Two possible alternate solutions: (i) return unique_ptr, (ii) take the raw ptr as an argument to the method. https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... components/previews/core/previews_opt_out_store_sql.cc:225: void CheckAndReconcileEnabledPreviewsVersionWithDataBase( May be add a function comment. https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... components/previews/core/previews_opt_out_store_sql.cc:229: "SELECT type version FROM " PREVIEWS_TYPE_VERSION_TABLE_NAME; missing comma s/type/type,/ https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... components/previews/core/previews_opt_out_store_sql.cc:233: std::unique_ptr<std::map<PreviewsType, int>> stored_versions( This could be just a regular map..right? instead of one wrapped with unique_ptr? https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... components/previews/core/previews_opt_out_store_sql.cc:244: for (std::vector<std::pair<PreviewsType, int>>::iterator it = Can this be a const_iterator? https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... components/previews/core/previews_opt_out_store_sql.cc:254: ClearBlacklistForTypeInDataBase(db, runner, type); IIUC, DCHECK_GE(current_version, stored_it->second); ?
The CQ bit was checked by dougarnett@chromium.org to run a CQ dry run
https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... components/previews/core/previews_experiments.h:64: // Caller assumes ownership of returned vector. On 2017/01/23 20:06:11, tbansal1 wrote: > In Chromium, if a function returns raw ptr, the default is to assume that the > callee has the ownership. Two possible alternate solutions: (i) return > unique_ptr, (ii) take the raw ptr as an argument to the method. Done. https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... components/previews/core/previews_opt_out_store_sql.cc:225: void CheckAndReconcileEnabledPreviewsVersionWithDataBase( On 2017/01/23 20:06:11, tbansal1 wrote: > May be add a function comment. Done. https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... components/previews/core/previews_opt_out_store_sql.cc:229: "SELECT type version FROM " PREVIEWS_TYPE_VERSION_TABLE_NAME; On 2017/01/23 20:06:11, tbansal1 wrote: > missing comma > s/type/type,/ Done. https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... components/previews/core/previews_opt_out_store_sql.cc:233: std::unique_ptr<std::map<PreviewsType, int>> stored_versions( On 2017/01/23 20:06:11, tbansal1 wrote: > This could be just a regular map..right? instead of one wrapped with unique_ptr? Done. https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... components/previews/core/previews_opt_out_store_sql.cc:244: for (std::vector<std::pair<PreviewsType, int>>::iterator it = On 2017/01/23 20:06:11, tbansal1 wrote: > Can this be a const_iterator? Done. https://codereview.chromium.org/2640023007/diff/1/components/previews/core/pr... components/previews/core/previews_opt_out_store_sql.cc:254: ClearBlacklistForTypeInDataBase(db, runner, type); On 2017/01/23 20:06:11, tbansal1 wrote: > IIUC, > DCHECK_GE(current_version, stored_it->second); > ? sg
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-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by dougarnett@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.
lgtm % nit but wait for ryansturm@. I am not too sure about the testing framework for the sqlite (mocking or something else). https://codereview.chromium.org/2640023007/diff/20001/components/previews/cor... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2640023007/diff/20001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:251: int current_version = it->second; s/current_version/new_version/
This general approach looks good. I added a few nits while I was looking through, but I wasn't super thorough since this isn't tested yet, and I'd like it to be. The new SQL looks good. Let me know if you need any help/pointers on testing or field trials (or testing field trials). I would be hesitant to land this without tests (and it would be much better with field trial for Offline previews). https://codereview.chromium.org/2640023007/diff/40001/components/previews/cor... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2640023007/diff/40001/components/previews/cor... components/previews/core/previews_experiments.cc:188: // TODO(dougarnett): Add finch control of version for previews types. I'd add the offline previews field trial support in this CL with something like "Offline.Version":"x" and default to 0 like you are doing. I think generally, it could follow the pattern: "<preview_name>.Version" as the parameter name. WDYT? If you want to push this off to a later CL, add a bug to the TODO. https://codereview.chromium.org/2640023007/diff/40001/components/previews/cor... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2640023007/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:60: // Table names use a macro instead of a const, so the can be used inline in s/the/they/ https://codereview.chromium.org/2640023007/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:186: scoped_refptr<base::SingleThreadTaskRunner> runner, You don't use |runner| for a bunch of these. https://codereview.chromium.org/2640023007/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:205: db->GetCachedStatement(SQL_FROM_HERE, kSqlInsert)); GetUniqueStatement instead since this will only be called once per profile start-up. https://codereview.chromium.org/2640023007/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:240: int version = statement.ColumnInt(1); nit: You don't need the local variable for version or PreviewsType, but if you think it helps with readability, I won't complain.
The CQ bit was checked by dougarnett@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 checked by dougarnett@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...
Addressed comments except haven't written tests yet. https://codereview.chromium.org/2640023007/diff/40001/components/previews/cor... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2640023007/diff/40001/components/previews/cor... components/previews/core/previews_experiments.cc:188: // TODO(dougarnett): Add finch control of version for previews types. On 2017/01/25 23:17:53, Ryan Sturm wrote: > I'd add the offline previews field trial support in this CL with something like > "Offline.Version":"x" and default to 0 like you are doing. > > I think generally, it could follow the pattern: > > "<preview_name>.Version" as the parameter name. WDYT? > > If you want to push this off to a later CL, add a bug to the TODO. Add param but followed current naming style. Please check that style. Let men know if we can and you think we should reconcile the param name "show_offline_pages" with the PreviewsType::OFFLINE name. https://codereview.chromium.org/2640023007/diff/40001/components/previews/cor... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2640023007/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:60: // Table names use a macro instead of a const, so the can be used inline in On 2017/01/25 23:17:53, Ryan Sturm wrote: > s/the/they/ Done. https://codereview.chromium.org/2640023007/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:186: scoped_refptr<base::SingleThreadTaskRunner> runner, On 2017/01/25 23:17:53, Ryan Sturm wrote: > You don't use |runner| for a bunch of these. thx https://codereview.chromium.org/2640023007/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:205: db->GetCachedStatement(SQL_FROM_HERE, kSqlInsert)); On 2017/01/25 23:17:53, Ryan Sturm wrote: > GetUniqueStatement instead since this will only be called once per profile > start-up. Done. https://codereview.chromium.org/2640023007/diff/40001/components/previews/cor... components/previews/core/previews_opt_out_store_sql.cc:240: int version = statement.ColumnInt(1); On 2017/01/25 23:17:53, Ryan Sturm wrote: > nit: You don't need the local variable for version or PreviewsType, but if you > think it helps with readability, I won't complain. Yeah, thought a bit more readable but don't feel strongly either way.
https://codereview.chromium.org/2640023007/diff/80001/components/previews/cor... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2640023007/diff/80001/components/previews/cor... components/previews/core/previews_experiments.cc:191: return version; I think the switch-case can be rewritten to get compile-time checks: switch (type) { case PreviewsType::OFFLINE: return blah; case PreviewsType::NONE: case PreviewsType::OFFLINE: NOTREACHED(); return 0; } See https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/switch...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by dougarnett@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by dougarnett@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 - added test case https://codereview.chromium.org/2640023007/diff/80001/components/previews/cor... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2640023007/diff/80001/components/previews/cor... components/previews/core/previews_experiments.cc:191: return version; On 2017/01/26 00:59:14, tbansal1 wrote: > I think the switch-case can be rewritten to get compile-time checks: > > switch (type) { > case PreviewsType::OFFLINE: > return blah; > case PreviewsType::NONE: > case PreviewsType::OFFLINE: > NOTREACHED(); > return 0; > } > > See > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/switch... Done.
The CQ bit was checked by dougarnett@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.
Sync up with bengr@, he is changing the field trials and that could impact what you are doing here. https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_experiments.cc:211: case PreviewsType::LAST: nit: change "case PreviewsType::LAST: break;" to "default: NOTREACHED(); return -1;" and get rid of things after the switch. There could be a unittest around this behavior so that calling GetPreviewsTypeVersion(PreviewsType::LAST - 1) fails if this switch isn't updated when a new enum value is added. https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:237: for (std::vector<std::pair<PreviewsType, int>>::const_iterator it = nit: for (auto it : enable_previews) might be a lot simpler unless there is a reason to explicitly say that it is a const_iterator. You can also declare enabled_previews as const and the auto *should* be std::vector<std::pair<PreviewsType, int>>::const. https://google.github.io/styleguide/cppguide.html#auto https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:238: enabled_previews->begin(); We might want to clear out a preview type that is not enabled. Does this model allow for that? https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... File components/previews/core/previews_opt_out_store_sql_unittest.cc (right): https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:280: EXPECT_TRUE(variations::AssociateVariationParams("ClientSidePreviews", This method is deprecated and moved to base/. Can you open a bug with the CleanUp Label under DataProxy components to transition all of our unit tests over to the new method? https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:310: EXPECT_EQ(black_list_map_->end(), iter); Call ResetFieldTrials at the end of this as well. There is weird field trial persistence when unittests run that I don't understand. E.g., if you add this test again with a different name AssociateVariationParams might return false on ios_simulator bot. I've hit that problem a couple times and it always bewilders me.
ryansturm@chromium.org changed reviewers: + bengr@chromium.org
+bengr@ since you are refactoring field trials.
Thanks, Ryan. Did a quick drive by as well... https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_experiments.cc:223: if (IsPreviewsTypeEnabled(PreviewsType::OFFLINE)) { I'd add the loop between NONE and LAST now, even though there's only one type. One less thing to fix when we add another type. https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_experiments.h:71: // Should only be called if |IsPreviewsTypeEnabled(type)| returns true. Why? Isn't a version independent of whether it is enabled? https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_experiments.h:75: std::unique_ptr<std::vector<std::pair<PreviewsType, int>>> GetEnabledPreviews(); Might be more readable if you define a PreviewsTypeList https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:221: void CheckAndReconcileEnabledPreviewsVersionWithDataBase(sql::Connection* db) { I'd factor out a version getter function.
Updated per Ryan's comments (haven't looked at Ben's yet). https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_experiments.cc:211: case PreviewsType::LAST: On 2017/02/08 23:51:00, Ryan Sturm wrote: > nit: change "case PreviewsType::LAST: break;" to "default: NOTREACHED(); return > -1;" and get rid of things after the switch. > > There could be a unittest around this behavior so that calling > GetPreviewsTypeVersion(PreviewsType::LAST - 1) fails if this switch isn't > updated when a new enum value is added. Tarun asked for explicit cases instead of default (which I had) so let's get you two to agree on approach. https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:237: for (std::vector<std::pair<PreviewsType, int>>::const_iterator it = On 2017/02/08 23:51:00, Ryan Sturm wrote: > nit: for (auto it : enable_previews) might be a lot simpler unless there is a > reason to explicitly say that it is a const_iterator. You can also declare > enabled_previews as const and the auto *should* be > std::vector<std::pair<PreviewsType, int>>::const. > > https://google.github.io/styleguide/cppguide.html#auto Done. https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:238: enabled_previews->begin(); On 2017/02/08 23:51:00, Ryan Sturm wrote: > We might want to clear out a preview type that is not enabled. Does this model > allow for that? Good topic to discuss. I was intentionally not trying to do that in case the preview is cycled on/off/on, we would have same behavior as before as long as the version didn't change. Seems like a separate feature to decide that cycling off and back on would reset the blacklist for that preview type - can add in follow on CL if we decide that. https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... File components/previews/core/previews_opt_out_store_sql_unittest.cc (right): https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:280: EXPECT_TRUE(variations::AssociateVariationParams("ClientSidePreviews", On 2017/02/08 23:51:00, Ryan Sturm wrote: > This method is deprecated and moved to base/. Can you open a bug with the > CleanUp Label under DataProxy components to transition all of our unit tests > over to the new method? Done. https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:310: EXPECT_EQ(black_list_map_->end(), iter); On 2017/02/08 23:51:00, Ryan Sturm wrote: > Call ResetFieldTrials at the end of this as well. There is weird field trial > persistence when unittests run that I don't understand. > > E.g., if you add this test again with a different name AssociateVariationParams > might return false on ios_simulator bot. > > I've hit that problem a couple times and it always bewilders me. Done.
https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_experiments.cc:211: case PreviewsType::LAST: On 2017/02/09 17:46:59, dougarnett wrote: > On 2017/02/08 23:51:00, Ryan Sturm wrote: > > nit: change "case PreviewsType::LAST: break;" to "default: NOTREACHED(); > return > > -1;" and get rid of things after the switch. > > > > There could be a unittest around this behavior so that calling > > GetPreviewsTypeVersion(PreviewsType::LAST - 1) fails if this switch isn't > > updated when a new enum value is added. > > Tarun asked for explicit cases instead of default (which I had) so let's get you > two to agree on approach. ryansturm@: What are the advantages of using tests vs. using compile time checks. Also, see a relevant discussion here: https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/switch...
https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_experiments.cc:211: case PreviewsType::LAST: On 2017/02/09 17:48:58, tbansal1 wrote: > On 2017/02/09 17:46:59, dougarnett wrote: > > On 2017/02/08 23:51:00, Ryan Sturm wrote: > > > nit: change "case PreviewsType::LAST: break;" to "default: NOTREACHED(); > > return > > > -1;" and get rid of things after the switch. > > > > > > There could be a unittest around this behavior so that calling > > > GetPreviewsTypeVersion(PreviewsType::LAST - 1) fails if this switch isn't > > > updated when a new enum value is added. > > > > Tarun asked for explicit cases instead of default (which I had) so let's get > you > > two to agree on approach. > > ryansturm@: What are the advantages of using tests vs. using compile time > checks. Also, see a relevant discussion here: > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/switch... If this will cause GCC to complain when a new value is added, I'm all for it. I thought that this would need to run with the new value to cause an error.
The CQ bit was checked by dougarnett@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 checked by dougarnett@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 - revised for Ben's plan that additional previews types will have their own field trial (with optional 'version' param that controls clear their blacklist entries) and also now will clear blacklist for types that were previously enabled but become no longer enabled. https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_experiments.cc:211: case PreviewsType::LAST: On 2017/02/09 17:54:38, Ryan Sturm wrote: > On 2017/02/09 17:48:58, tbansal1 wrote: > > On 2017/02/09 17:46:59, dougarnett wrote: > > > On 2017/02/08 23:51:00, Ryan Sturm wrote: > > > > nit: change "case PreviewsType::LAST: break;" to "default: NOTREACHED(); > > > return > > > > -1;" and get rid of things after the switch. > > > > > > > > There could be a unittest around this behavior so that calling > > > > GetPreviewsTypeVersion(PreviewsType::LAST - 1) fails if this switch isn't > > > > updated when a new enum value is added. > > > > > > Tarun asked for explicit cases instead of default (which I had) so let's get > > you > > > two to agree on approach. > > > > ryansturm@: What are the advantages of using tests vs. using compile time > > checks. Also, see a relevant discussion here: > > > https://groups.google.com/a/chromium.org/forum/#!searchin/chromium-dev/switch... > > If this will cause GCC to complain when a new value is added, I'm all for it. I > thought that this would need to run with the new value to cause an error. Here is example compile time error from warning: error: enumeration value 'SOME_NEW_ENUM_VALUE' not handled in switch [-Werror=switch] https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_experiments.cc:223: if (IsPreviewsTypeEnabled(PreviewsType::OFFLINE)) { On 2017/02/09 17:33:46, bengr wrote: > I'd add the loop between NONE and LAST now, even though there's only one type. > One less thing to fix when we add another type. Done. https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... File components/previews/core/previews_experiments.h (right): https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_experiments.h:71: // Should only be called if |IsPreviewsTypeEnabled(type)| returns true. On 2017/02/09 17:33:47, bengr wrote: > Why? Isn't a version independent of whether it is enabled? Done. https://codereview.chromium.org/2640023007/diff/140001/components/previews/co... components/previews/core/previews_experiments.h:75: std::unique_ptr<std::vector<std::pair<PreviewsType, int>>> GetEnabledPreviews(); On 2017/02/09 17:33:47, bengr wrote: > Might be more readable if you define a PreviewsTypeList Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
I like where this ended up. If I understand it correctly, when a user transitions from being enabled to disabled for a given type, entries of that type are removed. Seems reasonable to mm. lgtm % nits https://codereview.chromium.org/2640023007/diff/200001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2640023007/diff/200001/components/previews/co... components/previews/core/previews_experiments.cc:233: // Loop across all previews types (relies on sequential enum values). Can you add a similar comment about sequential enum values to the enum itself? https://codereview.chromium.org/2640023007/diff/200001/components/previews/co... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2640023007/diff/200001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:191: std::unique_ptr<std::map<PreviewsType, int>> GetStoredPreviews( Add a comment to all the helper methods here. Can you add a large comment somewhere (maybe near the schema) explaining what the tables are and how they are related. https://codereview.chromium.org/2640023007/diff/200001/components/previews/co... File components/previews/core/previews_opt_out_store_sql_unittest.cc (right): https://codereview.chromium.org/2640023007/diff/200001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:88: field_trials_.reset(); The first reset isn't necessary.
The CQ bit was checked by dougarnett@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...
https://codereview.chromium.org/2640023007/diff/200001/components/previews/co... File components/previews/core/previews_experiments.cc (right): https://codereview.chromium.org/2640023007/diff/200001/components/previews/co... components/previews/core/previews_experiments.cc:233: // Loop across all previews types (relies on sequential enum values). On 2017/02/15 00:30:29, Ryan Sturm wrote: > Can you add a similar comment about sequential enum values to the enum itself? Done. https://codereview.chromium.org/2640023007/diff/200001/components/previews/co... File components/previews/core/previews_opt_out_store_sql.cc (right): https://codereview.chromium.org/2640023007/diff/200001/components/previews/co... components/previews/core/previews_opt_out_store_sql.cc:191: std::unique_ptr<std::map<PreviewsType, int>> GetStoredPreviews( On 2017/02/15 00:30:29, Ryan Sturm wrote: > Add a comment to all the helper methods here. Can you add a large comment > somewhere (maybe near the schema) explaining what the tables are and how they > are related. Done. Please give it a look. I also renamed the macro table name for the original table to PREVIEWS_OPT_OUT_TABLE_NAME. https://codereview.chromium.org/2640023007/diff/200001/components/previews/co... File components/previews/core/previews_opt_out_store_sql_unittest.cc (right): https://codereview.chromium.org/2640023007/diff/200001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:88: field_trials_.reset(); On 2017/02/15 00:30:29, Ryan Sturm wrote: > The first reset isn't necessary. Added comment wrt this done to avoid DCHECK
lgtm. https://codereview.chromium.org/2640023007/diff/200001/components/previews/co... File components/previews/core/previews_opt_out_store_sql_unittest.cc (right): https://codereview.chromium.org/2640023007/diff/200001/components/previews/co... components/previews/core/previews_opt_out_store_sql_unittest.cc:88: field_trials_.reset(); On 2017/02/15 17:20:07, dougarnett wrote: > On 2017/02/15 00:30:29, Ryan Sturm wrote: > > The first reset isn't necessary. > > Added comment wrt this done to avoid DCHECK What a headache. Thanks for the comment.
The CQ bit was unchecked by dougarnett@chromium.org
The CQ bit was checked by dougarnett@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tbansal@chromium.org, bengr@chromium.org Link to the patchset: https://codereview.chromium.org/2640023007/#ps220001 (title: "Addressed Ryan recent 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": 220001, "attempt_start_ts": 1487180804108250,
"parent_rev": "e6d00f176148e0aa1067b237a2352e99ceff751c", "commit_rev":
"5e57dedf90886a800035abc8e70048266d1dd110"}
Message was sent while issue was closed.
Description was changed from ========== Adds PreviewsType version mechanism for clearing blacklist entries. Adds new table to the Previews opt-out store (PreviewsOptOutStoreSQL) for holding the current version of PreviewsType's used on that client with clean-up code to delete black list entries for a PreviewsType when the version of that PreviewsType changes. BUG=680620 ========== to ========== Adds PreviewsType version mechanism for clearing blacklist entries. Adds new table to the Previews opt-out store (PreviewsOptOutStoreSQL) for holding the current version of PreviewsType's used on that client with clean-up code to delete black list entries for a PreviewsType when the version of that PreviewsType changes. BUG=680620 Review-Url: https://codereview.chromium.org/2640023007 Cr-Commit-Position: refs/heads/master@{#450754} Committed: https://chromium.googlesource.com/chromium/src/+/5e57dedf90886a800035abc8e700... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/5e57dedf90886a800035abc8e700... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
