Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2387293009:
Removes a data-dependent DCHECK(). (Closed)
|
Created:
4 years, 2 months ago by tschumann Modified:
4 years, 2 months ago Reviewers:
Marc Treib CC:
chromium-reviews, ntp-dev+reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRemoves a data-dependent DCHECK().
This check actually triggered on the wrong condition (hence I also renamed a few things to reduce the confusion).
There was a valid point for the check, though: we should only fill the histogram when we don't get too many snippets.
Once we completely moved to the Zine cloud backend, this histogram should go away anyways.
Committed: https://crrev.com/f1130eeefa4757a7b851bbdd9f61bca8cd211658
Cr-Commit-Position: refs/heads/master@{#423749}
Patch Set 1 #Patch Set 2 : synced to head #
Total comments: 6
Patch Set 3 : review comments #Patch Set 4 : fix build #Patch Set 5 : merged to head #
Messages
Total messages: 26 (14 generated)
tschumann@chromium.org changed reviewers: + treib@chromium.org
The CQ bit was checked by tschumann@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...
LGTM with a few nits, thanks! https://codereview.chromium.org/2387293009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2387293009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:147: if (!value->GetAsDictionary(&dict)) { Ah, a fan of braces for single-line bodies... I'm still waiting for the epic fight against Bernhard :D https://codereview.chromium.org/2387293009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2387293009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:605: fetched_category.snippets.size() <= kMaxSnippetCount) { I'd prefer capping at kMaxSnippetCount, rather than not recording the histogram at all. https://codereview.chromium.org/2387293009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2387293009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:1190: SizeIs(service->GetMaxSnippetCountForTesting() + 1)); Is this actually the behavior we want?
On Thursday, October 6, 2016, <treib@chromium.org> wrote: > LGTM with a few nits, thanks! > > > https://codereview.chromium.org/2387293009/diff/20001/ > components/ntp_snippets/remote/ntp_snippets_fetcher.cc > File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): > > https://codereview.chromium.org/2387293009/diff/20001/ > components/ntp_snippets/remote/ntp_snippets_fetcher.cc#newcode147 > components/ntp_snippets/remote/ntp_snippets_fetcher.cc:147: if > (!value->GetAsDictionary(&dict)) { > Ah, a fan of braces for single-line bodies... I'm still waiting for the > epic fight against Bernhard :D > > https://codereview.chromium.org/2387293009/diff/20001/ > components/ntp_snippets/remote/ntp_snippets_service.cc > File components/ntp_snippets/remote/ntp_snippets_service.cc (right): > > https://codereview.chromium.org/2387293009/diff/20001/ > components/ntp_snippets/remote/ntp_snippets_service.cc#newcode605 > components/ntp_snippets/remote/ntp_snippets_service.cc:605: > fetched_category.snippets.size() <= kMaxSnippetCount) { > I'd prefer capping at kMaxSnippetCount, rather than not recording the > histogram at all. > Right. what about capping at max-size+1 to have a distinct bucket? > > > https://codereview.chromium.org/2387293009/diff/20001/ > components/ntp_snippets/remote/ntp_snippets_service_unittest.cc > File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc > (right): > > https://codereview.chromium.org/2387293009/diff/20001/ > components/ntp_snippets/remote/ntp_snippets_service_ > unittest.cc#newcode1190 > components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:1190: > SizeIs(service->GetMaxSnippetCountForTesting() + 1)); > Is this actually the behavior we want? > I guess for the time being it's probably safest -- it's an internal constant and nobody else should care. Plus it's been like this for a while. But trimming the results to the number of requested ones would not hurt. Just not sure if that's important enough for the bp. Let me know if you think differently. --Tim > > > https://codereview.chromium.org/2387293009/ > > -- > You received this message because you are subscribed to the Google Groups > "New Tab Page development" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to ntp-dev+unsubscribe@chromium.org > <javascript:_e(%7B%7D,'cvml','ntp-dev%2Bunsubscribe@chromium.org');>. > To post to this group, send email to ntp-dev@chromium.org > <javascript:_e(%7B%7D,'cvml','ntp-dev@chromium.org');>. > To view this discussion on the web visit https://groups.google.com/a/ > chromium.org/d/msgid/ntp-dev/001a1140e278c24ac3053e340de9%40google.com > <https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/001a1140e278c24ac305...> > . > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2016/10/06 15:48:32, chromium-reviews wrote: > On Thursday, October 6, 2016, <mailto:treib@chromium.org> wrote: > > > LGTM with a few nits, thanks! > > > > > > https://codereview.chromium.org/2387293009/diff/20001/ > > components/ntp_snippets/remote/ntp_snippets_fetcher.cc > > File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): > > > > https://codereview.chromium.org/2387293009/diff/20001/ > > components/ntp_snippets/remote/ntp_snippets_fetcher.cc#newcode147 > > components/ntp_snippets/remote/ntp_snippets_fetcher.cc:147: if > > (!value->GetAsDictionary(&dict)) { > > Ah, a fan of braces for single-line bodies... I'm still waiting for the > > epic fight against Bernhard :D > > > > https://codereview.chromium.org/2387293009/diff/20001/ > > components/ntp_snippets/remote/ntp_snippets_service.cc > > File components/ntp_snippets/remote/ntp_snippets_service.cc (right): > > > > https://codereview.chromium.org/2387293009/diff/20001/ > > components/ntp_snippets/remote/ntp_snippets_service.cc#newcode605 > > components/ntp_snippets/remote/ntp_snippets_service.cc:605: > > fetched_category.snippets.size() <= kMaxSnippetCount) { > > I'd prefer capping at kMaxSnippetCount, rather than not recording the > > histogram at all. > > > Right. what about capping at max-size+1 to have a distinct bucket? Yup, that sounds good! > > > > > > https://codereview.chromium.org/2387293009/diff/20001/ > > components/ntp_snippets/remote/ntp_snippets_service_unittest.cc > > File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc > > (right): > > > > https://codereview.chromium.org/2387293009/diff/20001/ > > components/ntp_snippets/remote/ntp_snippets_service_ > > unittest.cc#newcode1190 > > components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:1190: > > SizeIs(service->GetMaxSnippetCountForTesting() + 1)); > > Is this actually the behavior we want? > > > I guess for the time being it's probably safest -- it's an internal > constant and nobody else should care. Plus it's been like this for a while. > But trimming the results to the number of requested ones would not hurt. > Just not sure if that's important enough for the bp. Let me know if you > think differently. Nah, that's fine for now, this was more a comment for the future. > --Tim > > > > > > > https://codereview.chromium.org/2387293009/ > > > > > > -- > > You received this message because you are subscribed to the Google Groups > > "New Tab Page development" group. > > To unsubscribe from this group and stop receiving emails from it, send an > > email to mailto:ntp-dev+unsubscribe@chromium.org > > <javascript:_e(%7B%7D,'cvml','ntp-dev%2Bunsubscribe@chromium.org');>. > > To post to this group, send email to mailto:ntp-dev@chromium.org > > <javascript:_e(%7B%7D,'cvml','ntp-dev@chromium.org');>. > > To view this discussion on the web visit https://groups.google.com/a/ > > http://chromium.org/d/msgid/ntp-dev/001a1140e278c24ac3053e340de9%40google.com > > > <https://groups.google.com/a/chromium.org/d/msgid/ntp-dev/001a1140e278c24ac305...> > > . > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2387293009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_fetcher.cc (right): https://codereview.chromium.org/2387293009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_fetcher.cc:147: if (!value->GetAsDictionary(&dict)) { On 2016/10/06 15:36:13, Marc Treib wrote: > Ah, a fan of braces for single-line bodies... I'm still waiting for the epic > fight against Bernhard :D added those after adding debug statements and wondered why the test changed ;-) Yes, this is on my todo list :-) https://codereview.chromium.org/2387293009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service.cc (right): https://codereview.chromium.org/2387293009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service.cc:605: fetched_category.snippets.size() <= kMaxSnippetCount) { On 2016/10/06 15:36:13, Marc Treib wrote: > I'd prefer capping at kMaxSnippetCount, rather than not recording the histogram > at all. Done. https://codereview.chromium.org/2387293009/diff/20001/components/ntp_snippets... File components/ntp_snippets/remote/ntp_snippets_service_unittest.cc (right): https://codereview.chromium.org/2387293009/diff/20001/components/ntp_snippets... components/ntp_snippets/remote/ntp_snippets_service_unittest.cc:1190: SizeIs(service->GetMaxSnippetCountForTesting() + 1)); On 2016/10/06 15:36:13, Marc Treib wrote: > Is this actually the behavior we want? added a TODO().
The CQ bit was checked by tschumann@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2387293009/#ps40001 (title: "review 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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by tschumann@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2387293009/#ps60001 (title: "fix build")
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
Failed to apply patch for
components/ntp_snippets/remote/ntp_snippets_fetcher.cc:
While running git apply --index -3 -p1;
error: patch failed:
components/ntp_snippets/remote/ntp_snippets_fetcher.cc:270
Falling back to three-way merge...
Applied patch to 'components/ntp_snippets/remote/ntp_snippets_fetcher.cc' with
conflicts.
U components/ntp_snippets/remote/ntp_snippets_fetcher.cc
Patch: components/ntp_snippets/remote/ntp_snippets_fetcher.cc
Index: components/ntp_snippets/remote/ntp_snippets_fetcher.cc
diff --git a/components/ntp_snippets/remote/ntp_snippets_fetcher.cc
b/components/ntp_snippets/remote/ntp_snippets_fetcher.cc
index
b447572fe8b65433991268f54ec99c0a0593be8e..eae2b8999484425e37576015ebbbca4c1ca1748f
100644
--- a/components/ntp_snippets/remote/ntp_snippets_fetcher.cc
+++ b/components/ntp_snippets/remote/ntp_snippets_fetcher.cc
@@ -144,8 +144,9 @@ bool AddSnippetsFromListValue(bool content_suggestions_api,
NTPSnippet::PtrVector* snippets) {
for (const auto& value : list) {
const base::DictionaryValue* dict = nullptr;
- if (!value->GetAsDictionary(&dict))
+ if (!value->GetAsDictionary(&dict)) {
return false;
+ }
std::unique_ptr<NTPSnippet> snippet;
if (content_suggestions_api) {
@@ -153,8 +154,9 @@ bool AddSnippetsFromListValue(bool content_suggestions_api,
} else {
snippet = NTPSnippet::CreateFromChromeReaderDictionary(*dict);
}
- if (!snippet)
+ if (!snippet) {
return false;
+ }
snippets->push_back(std::move(snippet));
}
@@ -257,7 +259,7 @@ void NTPSnippetsFetcher::FetchSnippetsFromHosts(
int count,
bool interactive_request) {
if (!request_throttler_.DemandQuotaForRequest(interactive_request)) {
- FetchFinished(OptionalSnippets(),
+ FetchFinished(OptionalFetchedCategories(),
interactive_request
? FetchResult::INTERACTIVE_QUOTA_ERROR
: FetchResult::NON_INTERACTIVE_QUOTA_ERROR,
@@ -270,7 +272,7 @@ void NTPSnippetsFetcher::FetchSnippetsFromHosts(
excluded_ids_ = excluded_ids;
if (UsesHostRestrictions() && hosts_.empty()) {
- FetchFinished(OptionalSnippets(), FetchResult::EMPTY_HOSTS,
+ FetchFinished(OptionalFetchedCategories(), FetchResult::EMPTY_HOSTS,
/*extra_message=*/std::string());
return;
}
@@ -517,7 +519,7 @@ void NTPSnippetsFetcher::OnGetTokenFailure(
DLOG(ERROR) << "Unable to get token: " << error.ToString();
FetchFinished(
- OptionalSnippets(), FetchResult::OAUTH_TOKEN_ERROR,
+ OptionalFetchedCategories(), FetchResult::OAUTH_TOKEN_ERROR,
/*extra_message=*/base::StringPrintf(" (%s)", error.ToString().c_str()));
}
@@ -547,7 +549,8 @@ void NTPSnippetsFetcher::OnURLFetchComplete(const
URLFetcher* source) {
status.is_success() ? source->GetResponseCode() : status.error());
if (!status.is_success()) {
- FetchFinished(OptionalSnippets(), FetchResult::URL_REQUEST_STATUS_ERROR,
+ FetchFinished(OptionalFetchedCategories(),
+ FetchResult::URL_REQUEST_STATUS_ERROR,
/*extra_message=*/base::StringPrintf(" %d", status.error()));
} else if (source->GetResponseCode() != net::HTTP_OK) {
// TODO(jkrcal): https://crbug.com/609084
@@ -556,7 +559,7 @@ void NTPSnippetsFetcher::OnURLFetchComplete(const
URLFetcher* source) {
// fetch a new auth token). We should extract that into a common class
// instead of adding it to every single class that uses auth tokens.
FetchFinished(
- OptionalSnippets(), FetchResult::HTTP_ERROR,
+ OptionalFetchedCategories(), FetchResult::HTTP_ERROR,
/*extra_message=*/base::StringPrintf(" %d",
source->GetResponseCode()));
} else {
bool stores_result_to_string =
@@ -631,11 +634,12 @@ bool NTPSnippetsFetcher::JsonToSnippets(const base::Value&
parsed,
void NTPSnippetsFetcher::OnJsonParsed(std::unique_ptr<base::Value> parsed) {
FetchedCategoriesVector categories;
if (JsonToSnippets(*parsed, &categories)) {
- FetchFinished(OptionalSnippets(std::move(categories)),
FetchResult::SUCCESS,
+ FetchFinished(OptionalFetchedCategories(std::move(categories)),
+ FetchResult::SUCCESS,
/*extra_message=*/std::string());
} else {
LOG(WARNING) << "Received invalid snippets: " << last_fetch_json_;
- FetchFinished(OptionalSnippets(),
+ FetchFinished(OptionalFetchedCategories(),
FetchResult::INVALID_SNIPPET_CONTENT_ERROR,
/*extra_message=*/std::string());
}
@@ -645,14 +649,15 @@ void NTPSnippetsFetcher::OnJsonError(const std::string&
error) {
LOG(WARNING) << "Received invalid JSON (" << error
<< "): " << last_fetch_json_;
FetchFinished(
- OptionalSnippets(), FetchResult::JSON_PARSE_ERROR,
+ OptionalFetchedCategories(), FetchResult::JSON_PARSE_ERROR,
/*extra_message=*/base::StringPrintf(" (error %s)", error.c_str()));
}
-void NTPSnippetsFetcher::FetchFinished(OptionalSnippets snippets,
- FetchResult result,
- const std::string& extra_message) {
- DCHECK(result == FetchResult::SUCCESS || !snippets);
+void NTPSnippetsFetcher::FetchFinished(
+ OptionalFetchedCategories fetched_categories,
+ FetchResult result,
+ const std::string& extra_message) {
+ DCHECK(result == FetchResult::SUCCESS || !fetched_categories);
last_status_ = FetchResultToString(result) + extra_message;
// Don't record FetchTimes if the result indicates that a precondition
@@ -667,7 +672,7 @@ void NTPSnippetsFetcher::FetchFinished(OptionalSnippets
snippets,
DVLOG(1) << "Fetch finished: " << last_status_;
if (!snippets_available_callback_.is_null())
- snippets_available_callback_.Run(std::move(snippets));
+ snippets_available_callback_.Run(std::move(fetched_categories));
}
} // namespace ntp_snippets
The CQ bit was checked by tschumann@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from treib@chromium.org Link to the patchset: https://codereview.chromium.org/2387293009/#ps80001 (title: "merged to head")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Removes a data-dependent DCHECK(). This check actually triggered on the wrong condition (hence I also renamed a few things to reduce the confusion). There was a valid point for the check, though: we should only fill the histogram when we don't get too many snippets. Once we completely moved to the Zine cloud backend, this histogram should go away anyways. ========== to ========== Removes a data-dependent DCHECK(). This check actually triggered on the wrong condition (hence I also renamed a few things to reduce the confusion). There was a valid point for the check, though: we should only fill the histogram when we don't get too many snippets. Once we completely moved to the Zine cloud backend, this histogram should go away anyways. Committed: https://crrev.com/f1130eeefa4757a7b851bbdd9f61bca8cd211658 Cr-Commit-Position: refs/heads/master@{#423749} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/f1130eeefa4757a7b851bbdd9f61bca8cd211658 Cr-Commit-Position: refs/heads/master@{#423749} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
