Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
330473003:
Offline blacklisting for SuggestionsService. (Closed)
|
Created:
6 years, 6 months ago by manzagop (departed) Modified:
6 years, 6 months ago CC:
chromium-reviews, skanuj+watch_chromium.org, melevin+watch_chromium.org, dhollowa+watch_chromium.org, dougw+watch_chromium.org, donnd+watch_chromium.org, dominich, jfweitz+watch_chromium.org, David Black, samarth+watch_chromium.org, kmadhusu+watch_chromium.org, Jered, Mike Lerman Base URL:
https://chromium.googlesource.com/chromium/src.git@master Visibility:
Public. |
DescriptionOffline blacklisting for SuggestionsService.
- Introduces a new pref to store a temporary local blacklist to.
- Adds the BlacklistStore class, which is only meant to be a temporary repository to blacklist to until the user comes back online. It's not meant to store large blacklists.
- Hooks up the BlacklistStore into SuggestionsService
BUG=386241
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=279721
Patch Set 1 : Base version #
Total comments: 40
Patch Set 2 : Address Math's comments. #Patch Set 3 : Add BlacklistFails unittest. #Patch Set 4 : Add UMA logging of local blacklist #
Total comments: 34
Patch Set 5 : Address second round of comments. #
Total comments: 6
Patch Set 6 : Switch BlacklistStoreLogTest to UMAHistogramHelper - test still fails then succeeds. #Patch Set 7 : Ensure StatisticsRecorder gets initialized in tests #Patch Set 8 : Final merge. #
Total comments: 14
Patch Set 9 : Address Alexei's comments. #Patch Set 10 : Move StatisticsRecorder Initialization from TestSuite to ContentTestSuiteBase #Patch Set 11 : Rebase. #Messages
Total messages: 38 (0 generated)
Hey Mathieu, Can you have an early look? I'm trying to get this in before the cut. I still need to add some tests for SuggestionsService. Thanks, Pierre
It's good that we have this. Overall comment: the logic is getting somewhat cumbersome i.e. it's getting harder to do a one-line fix without the fear of introducing another bug. Ideally we'd have a BlacklistManager that would take care of its list and uploading it and such, but I understand the logic is intertwined with existing requests so that's fine. Haven't looked at the tests yet. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... File chrome/browser/search/suggestions/blacklist_store.cc (right): https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.cc:63: bool BlacklistStore::GetSomeBlacklistedUrl(GURL* url) { Ah so this gets the first entry from the blacklist? Consider alternative name. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... File chrome/browser/search/suggestions/blacklist_store.h (right): https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.h:21: // in the profile's preference file. It also handles SuggestionsProfile profile's preference file -> Profile preferences https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.h:28: // Appends |url| to the blacklist. Returns false otherwise, unless |url| is second sentence is confusing. What about Returns true if successful or |url| was already in the blacklist. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.h:32: // Sets |url| to a blacklisted url. Returns false if the blacklist is empty. url -> URL? https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.h:33: virtual bool GetSomeBlacklistedUrl(GURL* url); It's not clear from the name or the doc what this function does. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.h:36: // valid. It is not considered an error if |url| is not in the blacklist. Returns true if |url| was not in the blacklist? https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.h:49: // Loads the blacklist data from the profile's preferences into profile preferences here and below https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.h:63: // The pref service used to persist the suggestions data. *suggestions blacklist https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:79: // Initial delay used when scheduling a blacklist request. nit: Initial -> Default? Also remove Initial from variable name https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:87: // this are rejected. This means the maximum backoff is at least 5400 / 2, ie *i.e. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:89: const unsigned int kBacklistMaxDelaySec = 5400; *Blacklist https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:89: const unsigned int kBacklistMaxDelaySec = 5400; How do you pick this value? I think it should be much shorter i.e. 5-10 minutes, because sessions on mobile are never 45mn https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:101: SuggestionsService::SuggestionsService( I think somewhere in the construction path (or something) we should schedule a blacklist upload. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:265: FilterAndServe(&suggestions); If it's an empty profile, why not keep DispatchRequestsAndClear (i.e. no need to filter?) https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:324: void SuggestionsService::IssueRequest(const GURL& url) { very nit: I would have this before OnURLFetchComplete, in a logical order https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:356: if (blacklist_store_->GetSomeBlacklistedUrl(&blacklist_url)) { Again "Some" seems random. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_service.h (right): https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.h:80: void UploadBlacklist(); can this be private? Also, name implies the whole blacklist gets uploaded https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.h:83: static bool IsBlacklistRequest(const net::URLFetcher& request); Let's make this private and encourage people to only use GetBlacklistedUrl and check for false. Less API means more! https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.h:85: // Determines which url a blacklist request was for, irrespective of the *URL
Addressed specific comments. Agreed about this class becoming too complex and that managing the blacklist uploads should move to a different class. Subsequent cl? https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... File chrome/browser/search/suggestions/blacklist_store.cc (right): https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.cc:63: bool BlacklistStore::GetSomeBlacklistedUrl(GURL* url) { On 2014/06/17 14:51:14, Mathieu Perreault wrote: > Ah so this gets the first entry from the blacklist? Consider alternative name. Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... File chrome/browser/search/suggestions/blacklist_store.h (right): https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.h:21: // in the profile's preference file. It also handles SuggestionsProfile On 2014/06/17 14:51:14, Mathieu Perreault wrote: > profile's preference file -> Profile preferences Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.h:28: // Appends |url| to the blacklist. Returns false otherwise, unless |url| is On 2014/06/17 14:51:14, Mathieu Perreault wrote: > second sentence is confusing. What about > > Returns true if successful or |url| was already in the blacklist. Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.h:32: // Sets |url| to a blacklisted url. Returns false if the blacklist is empty. On 2014/06/17 14:51:14, Mathieu Perreault wrote: > url -> URL? Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.h:33: virtual bool GetSomeBlacklistedUrl(GURL* url); On 2014/06/17 14:51:14, Mathieu Perreault wrote: > It's not clear from the name or the doc what this function does. Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.h:36: // valid. It is not considered an error if |url| is not in the blacklist. On 2014/06/17 14:51:14, Mathieu Perreault wrote: > Returns true if |url| was not in the blacklist? Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.h:49: // Loads the blacklist data from the profile's preferences into On 2014/06/17 14:51:14, Mathieu Perreault wrote: > profile preferences > > here and below Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/blacklist_store.h:63: // The pref service used to persist the suggestions data. On 2014/06/17 14:51:14, Mathieu Perreault wrote: > *suggestions blacklist Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:79: // Initial delay used when scheduling a blacklist request. On 2014/06/17 14:51:15, Mathieu Perreault wrote: > nit: Initial -> Default? > > Also remove Initial from variable name Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:87: // this are rejected. This means the maximum backoff is at least 5400 / 2, ie On 2014/06/17 14:51:15, Mathieu Perreault wrote: > *i.e. Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:89: const unsigned int kBacklistMaxDelaySec = 5400; On 2014/06/17 14:51:14, Mathieu Perreault wrote: > *Blacklist Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:89: const unsigned int kBacklistMaxDelaySec = 5400; On 2014/06/17 14:51:15, Mathieu Perreault wrote: > How do you pick this value? I think it should be much shorter i.e. 5-10 minutes, > because sessions on mobile are never 45mn Can this keep the phone awake and nuke the battery? https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:101: SuggestionsService::SuggestionsService( On 2014/06/17 14:51:14, Mathieu Perreault wrote: > I think somewhere in the construction path (or something) we should schedule a > blacklist upload. An upload will happen after the first attempt to get suggestions. Are you worried it will end up with too few suggestions because of client side blacklisting? https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:265: FilterAndServe(&suggestions); On 2014/06/17 14:51:14, Mathieu Perreault wrote: > If it's an empty profile, why not keep DispatchRequestsAndClear (i.e. no need to > filter?) Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:324: void SuggestionsService::IssueRequest(const GURL& url) { On 2014/06/17 14:51:14, Mathieu Perreault wrote: > very nit: I would have this before OnURLFetchComplete, in a logical order Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:356: if (blacklist_store_->GetSomeBlacklistedUrl(&blacklist_url)) { On 2014/06/17 14:51:14, Mathieu Perreault wrote: > Again "Some" seems random. Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_service.h (right): https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.h:80: void UploadBlacklist(); On 2014/06/17 14:51:15, Mathieu Perreault wrote: > can this be private? Also, name implies the whole blacklist gets uploaded Done. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.h:83: static bool IsBlacklistRequest(const net::URLFetcher& request); On 2014/06/17 14:51:15, Mathieu Perreault wrote: > Let's make this private and encourage people to only use GetBlacklistedUrl and > check for false. Less API means more! Yup, already removed. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.h:85: // Determines which url a blacklist request was for, irrespective of the On 2014/06/17 14:51:15, Mathieu Perreault wrote: > *URL Done.
Actually, see my questions for two of those comments.
Let me know when you want a full review (incl. tests?) https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... File chrome/browser/search/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:89: const unsigned int kBacklistMaxDelaySec = 5400; On 2014/06/17 15:42:23, manzagop wrote: > On 2014/06/17 14:51:15, Mathieu Perreault wrote: > > How do you pick this value? I think it should be much shorter i.e. 5-10 > minutes, > > because sessions on mobile are never 45mn > > Can this keep the phone awake and nuke the battery? > I don't think so, because this will not run if Chrome is not in the foreground (we could check though). However if it's too high, it will in effect never run. I said 5mn because network conditions could have changed in those 5mn. https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... chrome/browser/search/suggestions/suggestions_service.cc:101: SuggestionsService::SuggestionsService( On 2014/06/17 15:42:23, manzagop wrote: > On 2014/06/17 14:51:14, Mathieu Perreault wrote: > > I think somewhere in the construction path (or something) we should schedule a > > blacklist upload. > > An upload will happen after the first attempt to get suggestions. Are you > worried it will end up with too few suggestions because of client side > blacklisting? No, but we should make sure that the blacklist doesn't get too large. By the way, you should add histograms for the size of the blacklist, when SuggestionService is constructed. UMA_HISTOGRAM_SPARSE_SLOWLY I think is a good macro for that.
On 2014/06/17 19:54:36, Mathieu Perreault wrote: > Let me know when you want a full review (incl. tests?) Now would be good, up to the histogram, which I will add shortly. > https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... > File chrome/browser/search/suggestions/suggestions_service.cc (right): > > https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... > chrome/browser/search/suggestions/suggestions_service.cc:89: const unsigned int > kBacklistMaxDelaySec = 5400; > On 2014/06/17 15:42:23, manzagop wrote: > > On 2014/06/17 14:51:15, Mathieu Perreault wrote: > > > How do you pick this value? I think it should be much shorter i.e. 5-10 > > minutes, > > > because sessions on mobile are never 45mn > > > > Can this keep the phone awake and nuke the battery? > > > > I don't think so, because this will not run if Chrome is not in the foreground > (we could check though). However if it's too high, it will in effect never run. > I said 5mn because network conditions could have changed in those 5mn. Done. > https://codereview.chromium.org/330473003/diff/50001/chrome/browser/search/su... > chrome/browser/search/suggestions/suggestions_service.cc:101: > SuggestionsService::SuggestionsService( > On 2014/06/17 15:42:23, manzagop wrote: > > On 2014/06/17 14:51:14, Mathieu Perreault wrote: > > > I think somewhere in the construction path (or something) we should schedule > a > > > blacklist upload. > > > > An upload will happen after the first attempt to get suggestions. Are you > > worried it will end up with too few suggestions because of client side > > blacklisting? > > No, but we should make sure that the blacklist doesn't get too large. By the > way, you should add histograms for the size of the blacklist, when > SuggestionService is constructed. > > UMA_HISTOGRAM_SPARSE_SLOWLY I think is a good macro for that. I don't think adding a call would make much a a difference. We schedule an upload any time a request completes (incl. a blacklist request). Will add the histogram.
Added the UMA logging of the local blacklist's size. PTAL.
Adding asviktine@ for the histograms code (question in suggestions_service_unittest, too) Looking solid, more comments. Don't forget to rebase with recently submitted CL. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... File chrome/browser/search/suggestions/blacklist_store.cc (right): https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store.cc:48: LoadBlacklist(&blacklist_proto) ? blacklist_proto.urls_size() : 0); Doing actual stuff in the macro is probably bad form. Let's use a temp bool? https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... File chrome/browser/search/suggestions/blacklist_store.h (right): https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store.h:21: // in the Profile's preferences file. It also handles SuggestionsProfile nit: I prefer Profile preferences (can't assume it's a file) https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... File chrome/browser/search/suggestions/blacklist_store_unittest.cc (right): https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store_unittest.cc:126: scoped_ptr<BlacklistStore> blacklist_store(new BlacklistStore(&prefs)); I think you should log a dummy value to the histogram to make sure it's created; see https://codereview.chromium.org/111423005/diff/240001/chrome/browser/search/m.... Therefore the sequence would be (1) Initialize() (2) Log a value using the macro (3) Find the histogram, verify the dummy value you logged. (4) construct blacklist store (5) verify new value (6) etc. Alexei can confirm if this is still the way to do it. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store_unittest.cc:130: "Suggestions.LocalBlacklistSize"); indent 2 less https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:98: const char kSuggestionsFieldTrialBlacklistUrlParamParam[] = ParamParam -> Param? I know it's technically a param-param but one is probably enough :) https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:129: GURL((GetExperimentParam(kSuggestionsFieldTrialURLParam) + "?") + do you need the new set of ()? https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:194: GURL url(blacklist_url_prefix_ + Let's have an anonymous function that creates the GURL from that other GURL (candidate_url). We do it twice in this file. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:203: GetBlacklistUrlPrefix(), true); blacklist_url_prefix_? https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:207: if (!net::GetValueForKeyInQuery( Can you add a 1-line comment above this to mention you're extracting the url https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:386: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); needed if this is test seam? https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_service.h (right): https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.h:126: int GetBlacklistDelay() const; these could be blacklist_delay() and set_blacklist_delay(int delay) with the definition in the .h (and still be private). https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.h:167: FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, UpdateBlacklistDelay); you already have FRIEND_TEST above for another test https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_service_unittest.cc (right): https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_unittest.cc:407: // Expectations specific to the second request. Can this be a bit closer to the "second request" https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_unittest.cc:434: base::MessageLoop::current()->RunUntilIdle(); pretty sure two calls is redundant https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_unittest.cc:480: EXPECT_GE(suggestions_service->GetBlacklistDelay(), initial_delay); EXPECT_GT?
https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... File chrome/browser/search/suggestions/blacklist_store.cc (right): https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store.cc:46: HISTOGRAM_SPARSE_SLOWLY( UMA_HISTOGRAM_SPARSE_SLOWLY(), otherwise you're only recording a local one. However, I'm not sure that a sparse histogram is the right thing to use in this case. Unlike e.g. # of tiles on the NTP, which would be capped at 8 so you can monitor each possible unique value, it's not clear to me that # of URLs here is of the same magnitude. If the list can potentially be large, then using an exponentially distributed histogram is better. (i.e. one of the _COUNTS() variants).
Also, this is a large enough CL that it should have a corresponding crbug.
https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... File chrome/browser/search/suggestions/blacklist_store.cc (right): https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store.cc:46: HISTOGRAM_SPARSE_SLOWLY( On 2014/06/18 15:34:28, Alexei Svitkine wrote: > UMA_HISTOGRAM_SPARSE_SLOWLY(), otherwise you're only recording a local one. > > However, I'm not sure that a sparse histogram is the right thing to use in this > case. Unlike e.g. # of tiles on the NTP, which would be capped at 8 so you can > monitor each possible unique value, it's not clear to me that # of URLs here is > of the same magnitude. If the list can potentially be large, then using an > exponentially distributed histogram is better. (i.e. one of the _COUNTS() > variants). Oh, I had used the wrong macro in my own CLs, how dumb. Good thing we catched this before branch point, thanks!
Addressed comments. Still one issue with the Finch logging test though: BlacklistStoreTest.LogsBlacklistSize fails, then gets retried then succeeds. It also succeeds if run on its own. Alexei: any clue what might be wrong? https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... File chrome/browser/search/suggestions/blacklist_store.cc (right): https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store.cc:46: HISTOGRAM_SPARSE_SLOWLY( On 2014/06/18 15:48:06, Mathieu Perreault wrote: > On 2014/06/18 15:34:28, Alexei Svitkine wrote: > > UMA_HISTOGRAM_SPARSE_SLOWLY(), otherwise you're only recording a local one. > > > > However, I'm not sure that a sparse histogram is the right thing to use in > this > > case. Unlike e.g. # of tiles on the NTP, which would be capped at 8 so you can > > monitor each possible unique value, it's not clear to me that # of URLs here > is > > of the same magnitude. If the list can potentially be large, then using an > > exponentially distributed histogram is better. (i.e. one of the _COUNTS() > > variants). > > Oh, I had used the wrong macro in my own CLs, how dumb. Good thing we catched > this before branch point, thanks! Ack. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store.cc:46: HISTOGRAM_SPARSE_SLOWLY( On 2014/06/18 15:34:28, Alexei Svitkine wrote: > UMA_HISTOGRAM_SPARSE_SLOWLY(), otherwise you're only recording a local one. > > However, I'm not sure that a sparse histogram is the right thing to use in this > case. Unlike e.g. # of tiles on the NTP, which would be capped at 8 so you can > monitor each possible unique value, it's not clear to me that # of URLs here is > of the same magnitude. If the list can potentially be large, then using an > exponentially distributed histogram is better. (i.e. one of the _COUNTS() > variants). Done. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store.cc:48: LoadBlacklist(&blacklist_proto) ? blacklist_proto.urls_size() : 0); On 2014/06/18 13:56:22, Mathieu Perreault wrote: > Doing actual stuff in the macro is probably bad form. Let's use a temp bool? Done. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... File chrome/browser/search/suggestions/blacklist_store.h (right): https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store.h:21: // in the Profile's preferences file. It also handles SuggestionsProfile On 2014/06/18 13:56:22, Mathieu Perreault wrote: > nit: I prefer Profile preferences (can't assume it's a file) Done. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... File chrome/browser/search/suggestions/blacklist_store_unittest.cc (right): https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store_unittest.cc:126: scoped_ptr<BlacklistStore> blacklist_store(new BlacklistStore(&prefs)); On 2014/06/18 13:56:23, Mathieu Perreault wrote: > I think you should log a dummy value to the histogram to make sure it's created; > see > https://codereview.chromium.org/111423005/diff/240001/chrome/browser/search/m.... > > Therefore the sequence would be > (1) Initialize() > (2) Log a value using the macro > (3) Find the histogram, verify the dummy value you logged. > (4) construct blacklist store > (5) verify new value > (6) etc. > > Alexei can confirm if this is still the way to do it. Done. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store_unittest.cc:130: "Suggestions.LocalBlacklistSize"); On 2014/06/18 13:56:23, Mathieu Perreault wrote: > indent 2 less Done. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:98: const char kSuggestionsFieldTrialBlacklistUrlParamParam[] = On 2014/06/18 13:56:23, Mathieu Perreault wrote: > ParamParam -> Param? I know it's technically a param-param but one is probably > enough :) Done. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:129: GURL((GetExperimentParam(kSuggestionsFieldTrialURLParam) + "?") + On 2014/06/18 13:56:23, Mathieu Perreault wrote: > do you need the new set of ()? Done. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:194: GURL url(blacklist_url_prefix_ + On 2014/06/18 13:56:23, Mathieu Perreault wrote: > Let's have an anonymous function that creates the GURL from that other GURL > (candidate_url). We do it twice in this file. Done. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:203: GetBlacklistUrlPrefix(), true); On 2014/06/18 13:56:23, Mathieu Perreault wrote: > blacklist_url_prefix_? Nope! (static function) https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:207: if (!net::GetValueForKeyInQuery( On 2014/06/18 13:56:23, Mathieu Perreault wrote: > Can you add a 1-line comment above this to mention you're extracting the url Done. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:386: DCHECK(content::BrowserThread::CurrentlyOn(content::BrowserThread::UI)); On 2014/06/18 13:56:23, Mathieu Perreault wrote: > needed if this is test seam? Done. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_service.h (right): https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.h:126: int GetBlacklistDelay() const; On 2014/06/18 13:56:23, Mathieu Perreault wrote: > these could be blacklist_delay() and set_blacklist_delay(int delay) with the > definition in the .h (and still be private). Done. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.h:167: FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, UpdateBlacklistDelay); On 2014/06/18 13:56:23, Mathieu Perreault wrote: > you already have FRIEND_TEST above for another test Moved the one from above to here. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_service_unittest.cc (right): https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_unittest.cc:407: // Expectations specific to the second request. On 2014/06/18 13:56:23, Mathieu Perreault wrote: > Can this be a bit closer to the "second request" I believe (could be wrong) all expectations should be set before they start being exercised. Still, I need to keep the common expectations (eg GetFirstUrlFromBlacklist) bundled because of how gmock matches expectations (starts matching in reverse order). https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_unittest.cc:434: base::MessageLoop::current()->RunUntilIdle(); On 2014/06/18 13:56:23, Mathieu Perreault wrote: > pretty sure two calls is redundant Done. https://codereview.chromium.org/330473003/diff/110001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service_unittest.cc:480: EXPECT_GE(suggestions_service->GetBlacklistDelay(), initial_delay); On 2014/06/18 13:56:23, Mathieu Perreault wrote: > EXPECT_GT? Done. You're right it's probably better to break the test if someone uses a factor of 1, just to be extra sure.
lgtm https://codereview.chromium.org/330473003/diff/130001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/330473003/diff/130001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:129: extra line? https://codereview.chromium.org/330473003/diff/130001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:214: // Extract the blacklisted url from the blacklist request. *URL
What's the test failure output? On Wed, Jun 18, 2014 at 3:04 PM, <mathp@chromium.org> wrote: > lgtm > > > https://codereview.chromium.org/330473003/diff/130001/ > chrome/browser/search/suggestions/suggestions_service.cc > File chrome/browser/search/suggestions/suggestions_service.cc (right): > > https://codereview.chromium.org/330473003/diff/130001/ > chrome/browser/search/suggestions/suggestions_service.cc#newcode129 > chrome/browser/search/suggestions/suggestions_service.cc:129: > extra line? > > https://codereview.chromium.org/330473003/diff/130001/ > chrome/browser/search/suggestions/suggestions_service.cc#newcode214 > chrome/browser/search/suggestions/suggestions_service.cc:214: // Extract > the blacklisted url from the blacklist request. > *URL > > https://codereview.chromium.org/330473003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Can you try following the helper class I suggest below and see if that helps with the test? https://codereview.chromium.org/330473003/diff/130001/chrome/browser/search/s... File chrome/browser/search/suggestions/blacklist_store_unittest.cc (right): https://codereview.chromium.org/330473003/diff/130001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store_unittest.cc:130: scoped_ptr<base::HistogramSamples> samples(histogram->SnapshotSamples()); I think you're better off using this helper class for this sort of thing: https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/u...
On 2014/06/18 19:46:13, Alexei Svitkine wrote: > What's the test failure output? As it's running multiple tests it fails on the expectations for the "real" logging, retries then succeeds. I'll try using the helper class. <snip> [ RUN ] BlacklistStoreTest.LogsBlacklistSize ../../chrome/browser/search/suggestions/blacklist_store_unittest.cc:140: Failure Value of: samples->TotalCount() Actual: 2 Expected: 3 ../../chrome/browser/search/suggestions/blacklist_store_unittest.cc:141: Failure Value of: samples->GetCount(0) Actual: 0 Expected: 1 ../../chrome/browser/search/suggestions/blacklist_store_unittest.cc:150: Failure Value of: samples->TotalCount() Actual: 2 Expected: 4 ../../chrome/browser/search/suggestions/blacklist_store_unittest.cc:151: Failure Value of: samples->GetCount(2) Actual: 0 Expected: 1 [ FAILED ] BlacklistStoreTest.LogsBlacklistSize (2 ms) <snip> W 92.880s 0710 Will retry test, try #1.istSize >>ScopedMainEntryLogger Note: Google Test filter = BlacklistStoreTest.LogsBlacklistSize [==========] Running 1 test from 1 test case. [----------] Global test environment set-up. [----------] 1 test from BlacklistStoreTest [ RUN ] BlacklistStoreTest.LogsBlacklistSize [ OK ] BlacklistStoreTest.LogsBlacklistSize (1 ms) [----------] 1 test from BlacklistStoreTest (4 ms total) [----------] Global test environment tear-down [==========] 1 test from 1 test case ran. (6 ms total) [ PASSED ] 1 test. > On Wed, Jun 18, 2014 at 3:04 PM, <mailto:mathp@chromium.org> wrote: > > > lgtm > > > > > > https://codereview.chromium.org/330473003/diff/130001/ > > chrome/browser/search/suggestions/suggestions_service.cc > > File chrome/browser/search/suggestions/suggestions_service.cc (right): > > > > https://codereview.chromium.org/330473003/diff/130001/ > > chrome/browser/search/suggestions/suggestions_service.cc#newcode129 > > chrome/browser/search/suggestions/suggestions_service.cc:129: > > extra line? > > > > https://codereview.chromium.org/330473003/diff/130001/ > > chrome/browser/search/suggestions/suggestions_service.cc#newcode214 > > chrome/browser/search/suggestions/suggestions_service.cc:214: // Extract > > the blacklisted url from the blacklist request. > > *URL > > > > https://codereview.chromium.org/330473003/ > > > > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
Test still "fails then succeeds" when using UMAHistogramHelper. If I replace UMA_HISTOGRAM_COUNTS_10000 with UMA_HISTOGRAM_SPARSE_SLOWLY the test succeeds always. Still looking into it. https://codereview.chromium.org/330473003/diff/130001/chrome/browser/search/s... File chrome/browser/search/suggestions/blacklist_store_unittest.cc (right): https://codereview.chromium.org/330473003/diff/130001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store_unittest.cc:130: scoped_ptr<base::HistogramSamples> samples(histogram->SnapshotSamples()); On 2014/06/18 19:47:50, Alexei Svitkine wrote: > I think you're better off using this helper class for this sort of thing: > > https://code.google.com/p/chromium/codesearch#chromium/src/chrome/test/base/u... Made the switch. Still fails. https://codereview.chromium.org/330473003/diff/130001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/330473003/diff/130001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:129: On 2014/06/18 19:04:53, Mathieu Perreault wrote: > extra line? Done. Funny, clang-format was happy with it. https://codereview.chromium.org/330473003/diff/130001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:214: // Extract the blacklisted url from the blacklist request. On 2014/06/18 19:04:53, Mathieu Perreault wrote: > *URL Done. Arg! Sorry, I know I keep writing it lowercase.
Adding Pawel for base/test ownership. We figured out the issue. If the UMA logging macro gets called before StatisticsRecorder gets initialized, a function static pointer to a histogram is stored but the yet to be created recorder will have no notion of it. Agreed with Alexei initializing the recoder from the TestSuite seems like a good solution.
lgtm % comments https://codereview.chromium.org/330473003/diff/230001/base/test/test_suite.cc File base/test/test_suite.cc (right): https://codereview.chromium.org/330473003/diff/230001/base/test/test_suite.cc... base/test/test_suite.cc:285: // Record histograms, so we can get histograms data in tests. "Record histograms" isn't really accurate. Instead, maybe "Initialize the histograms subsystem, so that any histograms hit in tests are correctly registered with the statistics recorder and can be queried by tests." https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... File chrome/browser/search/suggestions/blacklist_store.cc (right): https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store.cc:47: LoadBlacklist(&blacklist_proto) ? blacklist_proto.urls_size() : 0; Nit: Wouldn't a failure of LoadBlacklist() not initialize the proto? In which case urls_size() should be 0, no? Maybe you don't need this extra logic and just log blacklist_proto.urls_size() below after a call of LoadBlackList(). https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store.cc:54: if (!url.is_valid()) return false; Nit: put on next line. https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... File chrome/browser/search/suggestions/blacklist_store_unittest.cc (right): https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store_unittest.cc:123: class BlacklistStoreLogTest : public testing::Test { If this is being used only for a single test, just put the two members at the top of the test and remove this class. https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store_unittest.cc:126: base::StatisticsRecorder::Initialize(); Is this still needed? https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:86: const unsigned int kBlacklistDefaultDelaySec = 1; Nit: Generally, we use size_t for unsigned ints in Chromium unless you're using this with an external library API that requires a specific type. Also, I think style guide suggests to prefer plain ints, unless (in the case of Chromium) these are compared/coming from stl collection sizes. In this specific case, it seems you're using it to initialize an int member anyway (blacklist_delay_sec_), so change to int. https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_service.h (right): https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.h:173: FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, BlacklistURLFails); Nit: I think these friend declarations should go right below the private: section start.
Adding Brett for base/test ownership, since it's late in PL-WAW for Pawel. Brett: I was hoping to get this cl in today. The base/test change is to ensure Finch tests work properly (details earlier on the review thread). Thanks,
Addressed Alexei's comments, except one nit. https://codereview.chromium.org/330473003/diff/230001/base/test/test_suite.cc File base/test/test_suite.cc (right): https://codereview.chromium.org/330473003/diff/230001/base/test/test_suite.cc... base/test/test_suite.cc:285: // Record histograms, so we can get histograms data in tests. On 2014/06/20 14:41:41, Alexei Svitkine wrote: > "Record histograms" isn't really accurate. > > Instead, maybe "Initialize the histograms subsystem, so that any histograms hit > in tests are correctly registered with the statistics recorder and can be > queried by tests." Done. https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... File chrome/browser/search/suggestions/blacklist_store.cc (right): https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store.cc:47: LoadBlacklist(&blacklist_proto) ? blacklist_proto.urls_size() : 0; On 2014/06/20 14:41:41, Alexei Svitkine wrote: > Nit: Wouldn't a failure of LoadBlacklist() not initialize the proto? In which > case urls_size() should be 0, no? Maybe you don't need this extra logic and just > log blacklist_proto.urls_size() below after a call of LoadBlackList(). Done. I didn't want to make that assumption, but agreed it's at a cost in code complexity. https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store.cc:54: if (!url.is_valid()) return false; On 2014/06/20 14:41:41, Alexei Svitkine wrote: > Nit: put on next line. Will leave as is, since the practice for files in this directory has been to use clang-format (and this formatting is indeed allowed by google c++ style). https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... File chrome/browser/search/suggestions/blacklist_store_unittest.cc (right): https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store_unittest.cc:123: class BlacklistStoreLogTest : public testing::Test { On 2014/06/20 14:41:41, Alexei Svitkine wrote: > If this is being used only for a single test, just put the two members at the > top of the test and remove this class. Done. https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... chrome/browser/search/suggestions/blacklist_store_unittest.cc:126: base::StatisticsRecorder::Initialize(); On 2014/06/20 14:41:41, Alexei Svitkine wrote: > Is this still needed? Done. https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_service.cc (right): https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.cc:86: const unsigned int kBlacklistDefaultDelaySec = 1; On 2014/06/20 14:41:41, Alexei Svitkine wrote: > Nit: Generally, we use size_t for unsigned ints in Chromium unless you're using > this with an external library API that requires a specific type. > > Also, I think style guide suggests to prefer plain ints, unless (in the case of > Chromium) these are compared/coming from stl collection sizes. > > In this specific case, it seems you're using it to initialize an int member > anyway (blacklist_delay_sec_), so change to int. Done. https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... File chrome/browser/search/suggestions/suggestions_service.h (right): https://codereview.chromium.org/330473003/diff/230001/chrome/browser/search/s... chrome/browser/search/suggestions/suggestions_service.h:173: FRIEND_TEST_ALL_PREFIXES(SuggestionsServiceTest, BlacklistURLFails); On 2014/06/20 14:41:41, Alexei Svitkine wrote: > Nit: I think these friend declarations should go right below the private: > section start. Done.
base/test LGTM
Thanks for the quick replies everyone! On Fri, Jun 20, 2014 at 12:22 PM, <phajdan.jr@chromium.org> wrote: > base/test LGTM > > https://codereview.chromium.org/330473003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by manzagop@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/manzagop@chromium.org/330473003/250001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_rel/bu...) mac_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/mac_chromium_rel/buil...) win_chromium_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...) win_chromium_x64_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_x64_rel/...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_dbg_simulator on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_dbg_simulator/bui...)
-brettw The change to TestSuite caused some tests in base/ to fail. Those tests create and delete a StatisticsRecorder and the constructor does not tolerate multiple instances (there's a check that a static is null). Discussed with Alexei and decided to move the change from TestSuite to ContentTestSuiteBase. This should avoid the issue of the base/ tests, yet improve the situation for UMA logging tests. Try servers seem to pass. Given this doesn't really change the CL, I'll go ahead and check the CQ. Feel free to chime in.
The CQ bit was checked by manzagop@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/manzagop@chromium.org/330473003/270001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: android_dbg_triggered_tests on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered...) chromium_presubmit on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...) ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...) linux_chromium_chromeos_clang_dbg on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...) linux_chromium_chromeos_rel on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/linux_chromium_chrome...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device_ninja on tryserver.chromium (http://build.chromium.org/p/tryserver.chromium/builders/ios_rel_device_ninja/...)
The CQ bit was checked by manzagop@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/manzagop@chromium.org/330473003/290001
Message was sent while issue was closed.
Change committed as 279721 |
