Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2614773008:
Add a config class for the settings reset prompt. (Closed)
DescriptionAdd a config class for the settings reset prompt.
The config class encapsulates the configuration data for the settings
reset prompt experiment.
BUG=
Review-Url: https://codereview.chromium.org/2614773008
Cr-Commit-Position: refs/heads/master@{#443337}
Committed: https://chromium.googlesource.com/chromium/src/+/d0fa9cf8f06c7cba55a18246a71038618e0463f5
Patch Set 1 #
Total comments: 16
Patch Set 2 : Addressed csharp's comments. #
Total comments: 10
Patch Set 3 : Addressed Robert's comments. #Patch Set 4 : Fixed typos. #
Total comments: 2
Patch Set 5 : Added a test case. #Patch Set 6 : Rebase and fixed constructor call in unittest. #
Total comments: 4
Patch Set 7 : Address rkaplow@'s comments #
Messages
Total messages: 29 (13 generated)
alito@chromium.org changed reviewers: + csharp@chromium.org, robertshield@chromium.org
First self-contained CL for your reviewing pleasure. PTAL.
https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc (right): https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:67: // Try to match each sensible suffix of the URL host with the hashes Maybe worth mentioning why we want to try each suffix https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:107: CONFIG_ERROR_OK = 1, What about setting this to 0? (0 seem a bit more like an ERROR_OK value than 1) https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:148: std::unordered_map<SHA256Hash, int, SHA256HashHasher> domain_hashes; What about just clearing domains_hashes_ here and using it directly instead of using domain_hashes_ first? https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:151: std::string hash_string = iter.key(); const std::string&? https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.h (right): https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.h:39: // useful when reporting metrics. Pointer to where the ID mapping for positive values can be found? Or a better description of what they can mean. https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config_unittest.cc (right): https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config_unittest.cc:46: scoped_feature_list_.InitAndEnableFeature(kSettingsResetPrompt); What about just calling AddDefaultFeatureParams here instead, then the code to enable the feature is the same in all the tests https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config_unittest.cc:50: TEST_F(SettingsResetPromptConfigTest, Create) { Could you add some tests to ensure that all of the invalid cases are properly caught https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config_unittest.cc:57: TEST_F(SettingsResetPromptConfigTest, UrlToResetDomainId) { Can you add some test cases where the input url is invalid
Addressed comments. https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc (right): https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:67: // Try to match each sensible suffix of the URL host with the hashes On 2017/01/09 21:54:59, csharp wrote: > Maybe worth mentioning why we want to try each suffix Done. https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:107: CONFIG_ERROR_OK = 1, On 2017/01/09 21:54:59, csharp wrote: > What about setting this to 0? (0 seem a bit more like an ERROR_OK value than 1) In general, I find reporting zero values to be a hassle during analysis since our tools rely so heavily on returning zero values by default when no data is present (so disambiguation of "no data" vs "present but zero value" becomes a hassle). Since the actual integer values do not matter, I'd like to avoid using zero. https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:148: std::unordered_map<SHA256Hash, int, SHA256HashHasher> domain_hashes; On 2017/01/09 21:54:59, csharp wrote: > What about just clearing domains_hashes_ here and using it directly instead of > using domain_hashes_ first? Done. https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:151: std::string hash_string = iter.key(); On 2017/01/09 21:54:59, csharp wrote: > const std::string&? Done. https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.h (right): https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.h:39: // useful when reporting metrics. On 2017/01/09 21:54:59, csharp wrote: > Pointer to where the ID mapping for positive values can be found? Or a better > description of what they can mean. Added a bit more explanation. However, the mapping of IDs to domains is internal to the prompt configuration. https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config_unittest.cc (right): https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config_unittest.cc:46: scoped_feature_list_.InitAndEnableFeature(kSettingsResetPrompt); On 2017/01/09 21:54:59, csharp wrote: > What about just calling AddDefaultFeatureParams here instead, then the code to > enable the feature is the same in all the tests Done. https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config_unittest.cc:50: TEST_F(SettingsResetPromptConfigTest, Create) { On 2017/01/09 21:54:59, csharp wrote: > Could you add some tests to ensure that all of the invalid cases are properly > caught Done. Also caught yet another edge case. https://codereview.chromium.org/2614773008/diff/1/chrome/browser/safe_browsin... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config_unittest.cc:57: TEST_F(SettingsResetPromptConfigTest, UrlToResetDomainId) { On 2017/01/09 21:54:59, csharp wrote: > Can you add some test cases where the input url is invalid Done.
looks great, few nits and one main question https://codereview.chromium.org/2614773008/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc (right): https://codereview.chromium.org/2614773008/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:71: // "www.sub.domain.com", try hashes for for: nit: for for https://codereview.chromium.org/2614773008/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:75: // "com" This seems more like TLD+0 than TLD+1, don't we want to stop at domain.com? If we pushed a matching hash for "com" accidentally would we reset all .com domains? https://codereview.chromium.org/2614773008/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:163: int domain_id; Maybe initialize to -1? https://codereview.chromium.org/2614773008/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.h (right): https://codereview.chromium.org/2614773008/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.h:31: // SettingsResetPromptConfig. Returns nullptr if |IsPromptEnabled()| is nit: wrapping looks weird here. https://codereview.chromium.org/2614773008/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.h:63: } // namespace safe_browsing. nit: no .
Added a check against trying to match TLDs. PTAL. https://codereview.chromium.org/2614773008/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc (right): https://codereview.chromium.org/2614773008/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:71: // "www.sub.domain.com", try hashes for for: On 2017/01/10 02:55:22, robertshield_slow_reviews wrote: > nit: for for Done. https://codereview.chromium.org/2614773008/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:75: // "com" On 2017/01/10 02:55:22, robertshield_slow_reviews wrote: > This seems more like TLD+0 than TLD+1, don't we want to stop at domain.com? If > we pushed a matching hash for "com" accidentally would we reset all .com > domains? Added a check against trying to match the top level domains. https://codereview.chromium.org/2614773008/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:163: int domain_id; On 2017/01/10 02:55:22, robertshield_slow_reviews wrote: > Maybe initialize to -1? Done. https://codereview.chromium.org/2614773008/diff/20001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.h (right): https://codereview.chromium.org/2614773008/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.h:31: // SettingsResetPromptConfig. Returns nullptr if |IsPromptEnabled()| is On 2017/01/10 02:55:22, robertshield_slow_reviews wrote: > nit: wrapping looks weird here. Done. https://codereview.chromium.org/2614773008/diff/20001/chrome/browser/safe_bro... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.h:63: } // namespace safe_browsing. On 2017/01/10 02:55:22, robertshield_slow_reviews wrote: > nit: no . Done.
lgtm https://codereview.chromium.org/2614773008/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config_unittest.cc (right): https://codereview.chromium.org/2614773008/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config_unittest.cc:162: "bffd48c8162466106a84f42945bfbbcfe501c9f0931219e02ce46e275f05ba51"; Could you also add a test for ".uk" here? I reckon that should work too., just want to check.
Added requested test case. Thx. https://codereview.chromium.org/2614773008/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config_unittest.cc (right): https://codereview.chromium.org/2614773008/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config_unittest.cc:162: "bffd48c8162466106a84f42945bfbbcfe501c9f0931219e02ce46e275f05ba51"; On 2017/01/11 20:40:12, robertshield_slow_reviews wrote: > Could you also add a test for ".uk" here? I reckon that should work too., just > want to check. Done.
lgtm
The CQ bit was checked by alito@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from robertshield@chromium.org Link to the patchset: https://codereview.chromium.org/2614773008/#ps80001 (title: "Added a test case.")
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
Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
alito@chromium.org changed reviewers: + rkaplow@chromium.org
Adding rkaplow@ for OWNER's approval for histograms.xml.
The CQ bit was checked by alito@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 histogram lg https://codereview.chromium.org/2614773008/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc (right): https://codereview.chromium.org/2614773008/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:137: UMA_HISTOGRAM_ENUMERATION(kConfigErrorMetric, error, CONFIG_ERROR_MAX); generally people inline the metric name https://codereview.chromium.org/2614773008/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2614773008/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60885: + Indicates if an error was detected in the settings reset prompt config data. nit, would be good to mention when this gets triggered
Addressed all comments. https://codereview.chromium.org/2614773008/diff/100001/chrome/browser/safe_br... File chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc (right): https://codereview.chromium.org/2614773008/diff/100001/chrome/browser/safe_br... chrome/browser/safe_browsing/settings_reset_prompt/settings_reset_prompt_config.cc:137: UMA_HISTOGRAM_ENUMERATION(kConfigErrorMetric, error, CONFIG_ERROR_MAX); On 2017/01/12 18:42:33, rkaplow wrote: > generally people inline the metric name Done. https://codereview.chromium.org/2614773008/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2614773008/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:60885: + Indicates if an error was detected in the settings reset prompt config data. On 2017/01/12 18:42:33, rkaplow wrote: > nit, would be good to mention when this gets triggered Done.
lgtm
L
G
T
M
The CQ bit was checked by alito@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharp@chromium.org, robertshield@chromium.org Link to the patchset: https://codereview.chromium.org/2614773008/#ps120001 (title: "Address rkaplow@'s 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": 120001, "attempt_start_ts": 1484247625498490,
"parent_rev": "2546ba44189a370193e64d005aa554bd44490e1d", "commit_rev":
"d0fa9cf8f06c7cba55a18246a71038618e0463f5"}
Message was sent while issue was closed.
Description was changed from ========== Add a config class for the settings reset prompt. The config class encapsulates the configuration data for the settings reset prompt experiment. BUG= ========== to ========== Add a config class for the settings reset prompt. The config class encapsulates the configuration data for the settings reset prompt experiment. BUG= Review-Url: https://codereview.chromium.org/2614773008 Cr-Commit-Position: refs/heads/master@{#443337} Committed: https://chromium.googlesource.com/chromium/src/+/d0fa9cf8f06c7cba55a18246a710... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:120001) as https://chromium.googlesource.com/chromium/src/+/d0fa9cf8f06c7cba55a18246a710... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
