components/suggestions/features.cc - Issue 2858883002: Add flag for disabling minimum of ntp tile suggestions

Side by Side Diff: components/suggestions/features.cc

Issue 2858883002: Add flag for disabling minimum of ntp tile suggestions (Closed)

Patch Set: Replace switch with feature Created 3 years, 7 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

OLD	NEW
(Empty)
	1 // Copyright 2017 The Chromium Authors. All rights reserved.

	2 // Use of this source code is governed by a BSD-style license that can be

	3 // found in the LICENSE file.

	4

	5 #include "components/suggestions/features.h"

	6

	7 namespace suggestions {

	8

	9 // If this feature is enabled, we request and use suggestions even if there are

	10 // only very few of them.
	Marc Treib 2017/05/03 11:43:56 s/only very few/less than 8/ ? (That's really what s/only very few/less than 8/ ? (That's really what it means, right?) Also, should this comment be in the header? fhorschig 2017/05/03 14:19:42 Hmm, actually the server-side config differentiate Show quoted text On 2017/05/03 11:43:56, Marc Treib wrote: > s/only very few/less than 8/ ? (That's really what it means, right?) Hmm, actually the server-side config differentiates between mobile with 8 suggestions and desktop with 4. Currently, we would only see the effect for mobile but technically there is a difference. Show quoted text > Also, should this comment be in the header? Moved. Although I am not sure. (We do this for ntp_{tiles,snippets} but large lists don't: content/public/common/content_features.cc https://cs.chromium.org/search/?q=f:%5C.cc+%5Csconst%5Csbase::Feature Is there a standard? Is it important enough to think about deeply?) Marc Treib 2017/05/03 14:28:41 ...wait, really? If anything, shouldn't it be the Show quoted text On 2017/05/03 14:19:42, fhorschig wrote: > On 2017/05/03 11:43:56, Marc Treib wrote: > > s/only very few/less than 8/ ? (That's really what it means, right?) > Hmm, actually the server-side config differentiates between mobile > with 8 suggestions and desktop with 4. > Currently, we would only see the effect for mobile but technically > there is a difference. ...wait, really? If anything, shouldn't it be the other way around? Show quoted text > > Also, should this comment be in the header? > > Moved. Although I am not sure. > > (We do this for ntp_{tiles,snippets} but large lists don't: > content/public/common/content_features.cc > https://cs.chromium.org/search/?q=f:%5C.cc+%5Csconst%5Csbase::Feature > Is there a standard? Is it important enough to think about deeply?) Personally, I'm more likely to look in the header for a description, but I don't know if there's a standard. Nope, not important enough to worry over :) fhorschig 2017/05/04 07:39:58 As discussed, this is the default config that migh Show quoted text On 2017/05/03 14:28:41, Marc Treib wrote: > On 2017/05/03 14:19:42, fhorschig wrote: > > On 2017/05/03 11:43:56, Marc Treib wrote: > > > s/only very few/less than 8/ ? (That's really what it means, right?) > > Hmm, actually the server-side config differentiates between mobile > > with 8 suggestions and desktop with 4. > > Currently, we would only see the effect for mobile but technically > > there is a difference. > > ...wait, really? If anything, shouldn't it be the other way around? As discussed, this is the default config that might be overridden. Show quoted text > > > > Also, should this comment be in the header? > > > > Moved. Although I am not sure. > > > > (We do this for ntp_{tiles,snippets} but large lists don't: > > content/public/common/content_features.cc > > https://cs.chromium.org/search/?q=f:%5C.cc+%5Csconst%5Csbase::Feature > > Is there a standard? Is it important enough to think about deeply?) > > Personally, I'm more likely to look in the header for a description, but I don't > know if there's a standard. Nope, not important enough to worry over :) Thanks!
	11 const base::Feature kDisableSuggestionsMinimumFeature{

	12 "DisableSuggestionsMinimum", base::FEATURE_DISABLED_BY_DEFAULT};
	Marc Treib 2017/05/03 11:43:56 Hm, not a fan of a feature that starts with "Disab Hm, not a fan of a feature that starts with "Disable". I find it confusing to figure out what the actual state is when the feature is disabled... How about something like "SuggestionsNoMinimum"? fhorschig 2017/05/03 14:19:42 Renamed. Show quoted text On 2017/05/03 11:43:56, Marc Treib wrote: > Hm, not a fan of a feature that starts with "Disable". I find it confusing to > figure out what the actual state is when the feature is disabled... > > How about something like "SuggestionsNoMinimum"? Renamed.
	13

	14 } // namespace suggestions

OLD	NEW

« chrome/browser/flag_descriptions.cc ('K') | « components/suggestions/features.h ('k') | tools/metrics/histograms/histograms.xml » ('j') | no next file with comments »