Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2669873002:
[ImportantSites] Implementing dialog level blacklisting and blacklist expiration (Closed)
Description[ImportantSites] Implementing dialog level blacklisting and blacklist expiration
R=dominickn,twellington
BUG=682015, 682016
Review-Url: https://codereview.chromium.org/2669873002
Cr-Commit-Position: refs/heads/master@{#448795}
Committed: https://chromium.googlesource.com/chromium/src/+/93fa43b36cf37af0b142fbe70d7e1cc3f05bca33
Patch Set 1 #
Total comments: 8
Patch Set 2 : Comments, still no tests #
Total comments: 19
Patch Set 3 : C++ tests #Patch Set 4 : Comments #
Total comments: 14
Patch Set 5 : comments #Patch Set 6 : rebase #
Total comments: 4
Patch Set 7 : comments, compile #
Total comments: 2
Patch Set 8 : compile and comments #Patch Set 9 : android compile #Messages
Total messages: 43 (24 generated)
dmurph@chromium.org changed reviewers: + dominickn@chromium.org
Hi Dominick, I'm improving important sites by adding dialog-level blacklisting and blacklist expiration. These are the C++ changes for this improvement, and I'll probably be putting the Java changes in here too once these seem good. Can you PTAL? Thanks, Daniel
https://codereview.chromium.org/2669873002/diff/1/chrome/browser/engagement/i... File chrome/browser/engagement/important_sites_util.cc (right): https://codereview.chromium.org/2669873002/diff/1/chrome/browser/engagement/i... chrome/browser/engagement/important_sites_util.cc:100: base::Int64ToString(base::Time::Now().ToInternalValue())); Instead of a string, can you use a double? It seems much more common to use a double for internal time values in DictionaryValues than strings, an that will save the string conversion. https://codereview.chromium.org/2669873002/diff/1/chrome/browser/engagement/i... chrome/browser/engagement/important_sites_util.cc:359: bool ImportantSitesUtil::IsDialogDisabled(Profile* profile) { This method doesn't seem to be called from anywhere? https://codereview.chromium.org/2669873002/diff/1/chrome/browser/engagement/i... chrome/browser/engagement/important_sites_util.cc:363: if (ClearBlacklistIfOld(update.Get())) { Nit: remove {} for consistency. https://codereview.chromium.org/2669873002/diff/1/chrome/browser/engagement/i... chrome/browser/engagement/important_sites_util.cc:453: std::unique_ptr<base::Value> value = map->GetWebsiteSetting( What is this line doing? It doesn't appear to be used.
Thanks for the comments! I added the java changes so that new static method is called. I haven't added the tests yet. https://codereview.chromium.org/2669873002/diff/1/chrome/browser/engagement/i... File chrome/browser/engagement/important_sites_util.cc (right): https://codereview.chromium.org/2669873002/diff/1/chrome/browser/engagement/i... chrome/browser/engagement/important_sites_util.cc:100: base::Int64ToString(base::Time::Now().ToInternalValue())); On 2017/02/01 22:22:04, dominickn wrote: > Instead of a string, can you use a double? It seems much more common to use a > double for internal time values in DictionaryValues than strings, an that will > save the string conversion. Done. https://codereview.chromium.org/2669873002/diff/1/chrome/browser/engagement/i... chrome/browser/engagement/important_sites_util.cc:359: bool ImportantSitesUtil::IsDialogDisabled(Profile* profile) { On 2017/02/01 22:22:04, dominickn wrote: > This method doesn't seem to be called from anywhere? Changed, we now call it from Java. https://codereview.chromium.org/2669873002/diff/1/chrome/browser/engagement/i... chrome/browser/engagement/important_sites_util.cc:363: if (ClearBlacklistIfOld(update.Get())) { On 2017/02/01 22:22:04, dominickn wrote: > Nit: remove {} for consistency. Done. https://codereview.chromium.org/2669873002/diff/1/chrome/browser/engagement/i... chrome/browser/engagement/important_sites_util.cc:453: std::unique_ptr<base::Value> value = map->GetWebsiteSetting( On 2017/02/01 22:22:04, dominickn wrote: > What is this line doing? It doesn't appear to be used. You're right. Removed.
Not sure if you wanted me to wait until the tests are there, but the additional context here helped clarify my thoughts. :) https://codereview.chromium.org/2669873002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/2669873002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:95: * @param dialogDisabled If the important dialog itself is blacklisted and shouldn't be You could clarify this comment ("blacklisting" a dialog seems weird). E.g. "True if the important dialog has been ignored too many times and should not be shown" https://codereview.chromium.org/2669873002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:101: boolean dialogBlacklisted); Nit: dialogDisabled, not dialogBlacklisted. The former is less strange. https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/important_sites_util.cc (right): https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:40: static const char kTimeLastIgnored[] = "TimeBlacklisted"; I'll mention that it took me the longest time to realise that the ignored / blacklist stuff was generic across *both* sites and the dialog itself. Perhaps make this constant "TimeLastIgnored" to match the constant name? https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:94: void IncrementTimesIgnoredAndSetDate(base::DictionaryValue* dict) { Nit: perhaps just RecordIgnore()? https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:102: bool ClearBlacklistIfOld(base::DictionaryValue* dict) { I'm not sure why the times_ignored < kTimesIgnoredForBlacklist logic isn't also in here. It makes sense to me to have this method: 1. was the last ignored time too long ago? reset, don't suppress 2. otherwise, check the total number of ignores, and suppress as necessary As a name, I think "ShouldSuppress()" reads more cleanly (especially when this is inlined in an if conditional), which involves flipping some of the logic in here. WDYT? https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:229: bool cleared_blacklist = ClearBlacklistIfOld(dict.get()); Inline this in the if statement? cleared_blacklist is a lot less descriptive than if (ShouldSuppress(dict.get())) https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:232: if (cleared_blacklist || Much nicer would be: if (ShouldSuppress(dict.get())) continue With the ignore logic folded into ShouldSuppress() https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:358: if (ClearBlacklistIfOld(update.Get())) Just having a return ShouldSuppress(update.Get()); would be much nicer here. https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:366: void ImportantSitesUtil::RegisterProfilePrefs( Is it necessary to register the pref with a default value like this? dict->GetInteger should leave the out variable untouched if it fails so you should just be able to get away without registering anything.
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Description was changed from ========== [ImportantSites] Implementing dialog level blacklisting and blacklist expiration BUG=682015,682016 ========== to ========== [ImportantSites] Implementing dialog level blacklisting and blacklist expiration R=dominickn,twellington BUG=682015,682016 ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dmurph@chromium.org changed reviewers: + gab@chromium.org, twellington@chromium.org
+twellington for Android changes +gab for browser_prefs.cc https://codereview.chromium.org/2669873002/diff/20001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java (right): https://codereview.chromium.org/2669873002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:95: * @param dialogDisabled If the important dialog itself is blacklisted and shouldn't be On 2017/02/03 19:30:53, dominickn wrote: > You could clarify this comment ("blacklisting" a dialog seems weird). E.g. > > "True if the important dialog has been ignored too many times and should not be > shown" Done. https://codereview.chromium.org/2669873002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:95: * @param dialogDisabled If the important dialog itself is blacklisted and shouldn't be On 2017/02/03 19:30:53, dominickn wrote: > You could clarify this comment ("blacklisting" a dialog seems weird). E.g. > > "True if the important dialog has been ignored too many times and should not be > shown" Done. https://codereview.chromium.org/2669873002/diff/20001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/PrefServiceBridge.java:101: boolean dialogBlacklisted); On 2017/02/03 19:30:53, dominickn wrote: > Nit: dialogDisabled, not dialogBlacklisted. The former is less strange. Done. https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... File chrome/browser/engagement/important_sites_util.cc (right): https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:40: static const char kTimeLastIgnored[] = "TimeBlacklisted"; On 2017/02/03 19:30:54, dominickn wrote: > I'll mention that it took me the longest time to realise that the ignored / > blacklist stuff was generic across *both* sites and the dialog itself. > > Perhaps make this constant "TimeLastIgnored" to match the constant name? Done, and added a comment note. https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:94: void IncrementTimesIgnoredAndSetDate(base::DictionaryValue* dict) { On 2017/02/03 19:30:54, dominickn wrote: > Nit: perhaps just RecordIgnore()? Done. https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:102: bool ClearBlacklistIfOld(base::DictionaryValue* dict) { On 2017/02/03 19:30:54, dominickn wrote: > I'm not sure why the times_ignored < kTimesIgnoredForBlacklist logic isn't also > in here. It makes sense to me to have this method: > > 1. was the last ignored time too long ago? reset, don't suppress > 2. otherwise, check the total number of ignores, and suppress as necessary > > As a name, I think "ShouldSuppress()" reads more cleanly (especially when this > is inlined in an if conditional), which involves flipping some of the logic in > here. WDYT? Yeah, that sounds much better. Thanks! https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:229: bool cleared_blacklist = ClearBlacklistIfOld(dict.get()); On 2017/02/03 19:30:54, dominickn wrote: > Inline this in the if statement? cleared_blacklist is a lot less descriptive > than if (ShouldSuppress(dict.get())) Done. https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:232: if (cleared_blacklist || On 2017/02/03 19:30:54, dominickn wrote: > Much nicer would be: > > if (ShouldSuppress(dict.get())) > continue > > With the ignore logic folded into ShouldSuppress() Done. https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:358: if (ClearBlacklistIfOld(update.Get())) On 2017/02/03 19:30:54, dominickn wrote: > Just having a return ShouldSuppress(update.Get()); would be much nicer here. Done. https://codereview.chromium.org/2669873002/diff/20001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:366: void ImportantSitesUtil::RegisterProfilePrefs( On 2017/02/03 19:30:54, dominickn wrote: > Is it necessary to register the pref with a default value like this? > dict->GetInteger should leave the out variable untouched if it fails so you > should just be able to get away without registering anything. I need to register the pref at least. Yeah, I can leave the default value out.
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-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
Mostly small things now. https://codereview.chromium.org/2669873002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2669873002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:598: int[] importantReasons, boolean dialogBlacklisted) { Nit: dialogDisabled https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... File chrome/browser/engagement/important_sites_util.cc (right): https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:107: int times_ignored = 0; Nit: move times_ignored down to line 117 (right before it's used). If we early exit from this method times_ignored is never touched. https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:112: dict->SetInteger(kNumTimesIgnoredName, 0); Perhaps dict->Remove(kTimeLastIgnored) as well? https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:117: if (dict->GetInteger(kNumTimesIgnoredName, ×_ignored) && Nit: just do return (dict->GetInteger(kNumTimesIgnoredName, ×_ignored) && times_ignored >= kTimesIgnoredForBlacklist); And remove the last two returns here. https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:467: dict->SetDouble(kTimeLastIgnored, base::Time::Now().ToDoubleT()); Nit: since you're clearing the kNumTimesIgnored value, should you just leave the TimeLastIgnored as unset? https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... File chrome/browser/engagement/important_sites_util_unittest.cc (right): https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util_unittest.cc:451: // Ignore all sites 3 times. Nit: we ignore twice first https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util_unittest.cc:464: ImportantSitesUtil::RecordBlacklistedAndIgnoredImportantSites( Nit: this is the third ignore
Android things lgtm % Dominick's nit about changing dialogBlacklisted to dialogDisabled
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
Thanks! Uploaded comments and rebased. https://codereview.chromium.org/2669873002/diff/60001/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java (right): https://codereview.chromium.org/2669873002/diff/60001/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/preferences/privacy/ClearBrowsingDataPreferences.java:598: int[] importantReasons, boolean dialogBlacklisted) { On 2017/02/03 21:14:51, dominickn wrote: > Nit: dialogDisabled Done. https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... File chrome/browser/engagement/important_sites_util.cc (right): https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:107: int times_ignored = 0; On 2017/02/03 21:14:51, dominickn wrote: > Nit: move times_ignored down to line 117 (right before it's used). If we early > exit from this method times_ignored is never touched. Done. https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:112: dict->SetInteger(kNumTimesIgnoredName, 0); On 2017/02/03 21:14:51, dominickn wrote: > Perhaps dict->Remove(kTimeLastIgnored) as well? Done. https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:117: if (dict->GetInteger(kNumTimesIgnoredName, ×_ignored) && On 2017/02/03 21:14:51, dominickn wrote: > Nit: just do > > return (dict->GetInteger(kNumTimesIgnoredName, ×_ignored) && times_ignored > >= kTimesIgnoredForBlacklist); > > And remove the last two returns here. Nice catch, thanks. https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util.cc:467: dict->SetDouble(kTimeLastIgnored, base::Time::Now().ToDoubleT()); On 2017/02/03 21:14:51, dominickn wrote: > Nit: since you're clearing the kNumTimesIgnored value, should you just leave the > TimeLastIgnored as unset? Ah true. Done. https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... File chrome/browser/engagement/important_sites_util_unittest.cc (right): https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util_unittest.cc:451: // Ignore all sites 3 times. On 2017/02/03 21:14:51, dominickn wrote: > Nit: we ignore twice first Done. https://codereview.chromium.org/2669873002/diff/60001/chrome/browser/engageme... chrome/browser/engagement/important_sites_util_unittest.cc:464: ImportantSitesUtil::RecordBlacklistedAndIgnoredImportantSites( On 2017/02/03 21:14:51, dominickn wrote: > Nit: this is the third ignore Done.
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_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
lgtm, thanks!
https://codereview.chromium.org/2669873002/diff/100001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2669873002/diff/100001/chrome/common/pref_nam... chrome/common/pref_names.cc:1345: // *************** LOCAL STATE *************** I know it's non-obvious... but profile prefs go above this line... And in fact, non-ifdefed prefs go above all the if defs blocks above this line (ideally grouped with related prefs if any but I'm aware it's a mess..) https://codereview.chromium.org/2669873002/diff/100001/chrome/common/pref_nam... chrome/common/pref_names.cc:2393: const char kImportantSitesDialogHistory[] = "important_sites.dialog_history"; "important_sites." is not an existing prefs namespace (i.e. no other important_sites.* prefs), can we find an existing one that makes sense? Otherwise we create a JSON dict just for it...
The CQ bit was checked by dmurph@chromium.org to run a CQ dry run
+tedchoc for chrome/browser/android/preferences/pref_service_bridge.cc https://codereview.chromium.org/2669873002/diff/100001/chrome/common/pref_nam... File chrome/common/pref_names.cc (right): https://codereview.chromium.org/2669873002/diff/100001/chrome/common/pref_nam... chrome/common/pref_names.cc:1345: // *************** LOCAL STATE *************** On 2017/02/06 20:34:01, gab wrote: > I know it's non-obvious... but profile prefs go above this line... And in fact, > non-ifdefed prefs go above all the if defs blocks above this line (ideally > grouped with related prefs if any but I'm aware it's a mess..) Done. https://codereview.chromium.org/2669873002/diff/100001/chrome/common/pref_nam... chrome/common/pref_names.cc:2393: const char kImportantSitesDialogHistory[] = "important_sites.dialog_history"; On 2017/02/06 20:34:01, gab wrote: > "important_sites." is not an existing prefs namespace (i.e. no other > important_sites.* prefs), can we find an existing one that makes sense? > Otherwise we create a JSON dict just for it... It doesn't look like there's anything else - putting it in a global.
dmurph@chromium.org changed reviewers: + tedchoc@chromium.org
+tedchoc (for real this time) for chrome/browser/android/preferences/pref_service_bridge.cc
The CQ bit was checked by dmurph@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: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
prefs lgtm w/ comment https://codereview.chromium.org/2669873002/diff/120001/chrome/common/pref_nam... File chrome/common/pref_names.h (right): https://codereview.chromium.org/2669873002/diff/120001/chrome/common/pref_nam... chrome/common/pref_names.h:38: extern const char kHomePage[]; This block was alpha-sorted, let's keep it that way by putting your pref here (same in .cc).
On 2017/02/07 15:38:11, gab wrote: > prefs lgtm w/ comment > > https://codereview.chromium.org/2669873002/diff/120001/chrome/common/pref_nam... > File chrome/common/pref_names.h (right): > > https://codereview.chromium.org/2669873002/diff/120001/chrome/common/pref_nam... > chrome/common/pref_names.h:38: extern const char kHomePage[]; > This block was alpha-sorted, let's keep it that way by putting your pref here > (same in .cc). pref_service_bridge - lgtm
Thanks! https://codereview.chromium.org/2669873002/diff/120001/chrome/common/pref_nam... File chrome/common/pref_names.h (right): https://codereview.chromium.org/2669873002/diff/120001/chrome/common/pref_nam... chrome/common/pref_names.h:38: extern const char kHomePage[]; On 2017/02/07 15:38:11, gab wrote: > This block was alpha-sorted, let's keep it that way by putting your pref here > (same in .cc). Done.
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from twellington@chromium.org, dominickn@chromium.org, gab@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2669873002/#ps140001 (title: "compile and comments")
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
The CQ bit was checked by dmurph@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dominickn@chromium.org, twellington@chromium.org, gab@chromium.org, tedchoc@chromium.org Link to the patchset: https://codereview.chromium.org/2669873002/#ps160001 (title: "android compile")
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": 160001, "attempt_start_ts": 1486507003350200,
"parent_rev": "aa57f762dfa15e283219c3ff3e9c8b9d611cae7c", "commit_rev":
"93fa43b36cf37af0b142fbe70d7e1cc3f05bca33"}
Message was sent while issue was closed.
Description was changed from ========== [ImportantSites] Implementing dialog level blacklisting and blacklist expiration R=dominickn,twellington BUG=682015,682016 ========== to ========== [ImportantSites] Implementing dialog level blacklisting and blacklist expiration R=dominickn,twellington BUG=682015,682016 Review-Url: https://codereview.chromium.org/2669873002 Cr-Commit-Position: refs/heads/master@{#448795} Committed: https://chromium.googlesource.com/chromium/src/+/93fa43b36cf37af0b142fbe70d7e... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/93fa43b36cf37af0b142fbe70d7e... |
