Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2872573002:
Use |min| param to query a variable number of suggestions (Closed)
|
Created:
3 years, 7 months ago by fhorschig Modified:
3 years, 7 months ago Reviewers:
Marc Treib CC:
chromium-reviews Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUse |min| param to query a variable number of suggestions
In order to use the newly implemented server-side functionality of
receiving less than 8 suggestions, the client has to append a new
query parameter to the URL.
Therefore, this CL is blocked on cr/155185623
BUG=709959
Review-Url: https://codereview.chromium.org/2872573002
Cr-Commit-Position: refs/heads/master@{#470874}
Committed: https://chromium.googlesource.com/chromium/src/+/ec72b3b0616de6e11b11db904b12a5a7e2a1a9da
Patch Set 1 #Patch Set 2 : Remove unnecessary includes #
Total comments: 4
Patch Set 3 : Use format strings to concat parameters #
Total comments: 6
Patch Set 4 : Control number of suggestion with variation param #
Total comments: 2
Patch Set 5 : Use GetFieldTrialParamByFeatureAsInt instead deprecated GetVariationParamByFeatureAsInt #Patch Set 6 : Rebase (blackboxing unittest) #
Messages
Total messages: 24 (15 generated)
Description was changed from ========== Use |all| param to query more then 8 suggestions In order to use the newly implemented server-side functionality of receiving less than 8 suggestions, the client has to append a new query parameter to the URL. Therefore, this CL is blocked on cr/155185623 BUG=709959 ========== to ========== Use |all| param to query more then 8 suggestions In order to use the newly implemented server-side functionality of receiving less than 8 suggestions, the client has to append a new query parameter to the URL. Therefore, this CL is blocked on cr/155185623 BUG=709959 ==========
Description was changed from ========== Use |all| param to query more then 8 suggestions In order to use the newly implemented server-side functionality of receiving less than 8 suggestions, the client has to append a new query parameter to the URL. Therefore, this CL is blocked on cr/155185623 BUG=709959 ========== to ========== Use |all| param to query less than 8 suggestions In order to use the newly implemented server-side functionality of receiving less than 8 suggestions, the client has to append a new query parameter to the URL. Therefore, this CL is blocked on cr/155185623 BUG=709959 ==========
fhorschig@chromium.org changed reviewers: + treib@chromium.org
Hi Marc, could you please take a look at the client side of the cl/155185623? (I will not land this change until the server-side is submitted as the name of the parameter is subject to change.)
https://codereview.chromium.org/2872573002/diff/20001/components/suggestions/... File components/suggestions/suggestions_service_impl.cc (right): https://codereview.chromium.org/2872573002/diff/20001/components/suggestions/... components/suggestions/suggestions_service_impl.cc:124: const char kSuggestionsUseAllParam[] = "all&"; Hm, I'm not a fan of "hiding" the "&" in here. IMO it makes kSuggestionsURLFormat a bit hard to read. Somewhat related: How about making this a "min=n" param for finer control? https://codereview.chromium.org/2872573002/diff/20001/components/suggestions/... File components/suggestions/suggestions_service_impl.h (right): https://codereview.chromium.org/2872573002/diff/20001/components/suggestions/... components/suggestions/suggestions_service_impl.h:91: BuildUrlWithAllParameterForFewFeature); Heads-up: You'll probably get merge conflicts with some changes I've done recently around here. (Should be easy to resolve though)
Patchset #3 (id:40001) has been deleted
https://codereview.chromium.org/2872573002/diff/20001/components/suggestions/... File components/suggestions/suggestions_service_impl.cc (right): https://codereview.chromium.org/2872573002/diff/20001/components/suggestions/... components/suggestions/suggestions_service_impl.cc:124: const char kSuggestionsUseAllParam[] = "all&"; On 2017/05/08 14:26:04, Marc Treib wrote: > Hm, I'm not a fan of "hiding" the "&" in here. IMO it makes > kSuggestionsURLFormat a bit hard to read. Done. > Somewhat related: How about making this a "min=n" param for finer control? Done. (also on the server side) https://codereview.chromium.org/2872573002/diff/20001/components/suggestions/... File components/suggestions/suggestions_service_impl.h (right): https://codereview.chromium.org/2872573002/diff/20001/components/suggestions/... components/suggestions/suggestions_service_impl.h:91: BuildUrlWithAllParameterForFewFeature); On 2017/05/08 14:26:04, Marc Treib wrote: > Heads-up: You'll probably get merge conflicts with some changes I've done > recently around here. (Should be easy to resolve though) Acknowledged.
https://codereview.chromium.org/2872573002/diff/60001/components/suggestions/... File components/suggestions/suggestions_service_impl.cc (right): https://codereview.chromium.org/2872573002/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl.cc:94: const char kSuggestionsUseAllParam[] = "min=0"; The "0" should probably be configurable via a variation param? https://codereview.chromium.org/2872573002/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl.cc:256: query = base::StringPrintf("%s&%s", query.c_str(), kSuggestionsUseAllParam); nit: I find this a bit confusing. Maybe don't reuse the "query" variable?
https://codereview.chromium.org/2872573002/diff/60001/components/suggestions/... File components/suggestions/suggestions_service_impl.cc (right): https://codereview.chromium.org/2872573002/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl.cc:94: const char kSuggestionsUseAllParam[] = "min=0"; On 2017/05/10 10:08:02, Marc Treib wrote: > The "0" should probably be configurable via a variation param? Hmm, okay. I currently see no application, though. https://codereview.chromium.org/2872573002/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl.cc:256: query = base::StringPrintf("%s&%s", query.c_str(), kSuggestionsUseAllParam); On 2017/05/10 10:08:02, Marc Treib wrote: > nit: I find this a bit confusing. Maybe don't reuse the "query" variable? Done.
lgtm https://codereview.chromium.org/2872573002/diff/60001/components/suggestions/... File components/suggestions/suggestions_service_impl.cc (right): https://codereview.chromium.org/2872573002/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl.cc:94: const char kSuggestionsUseAllParam[] = "min=0"; On 2017/05/10 12:25:30, fhorschig wrote: > On 2017/05/10 10:08:02, Marc Treib wrote: > > The "0" should probably be configurable via a variation param? > > Hmm, okay. I currently see no application, though. You never do, until you need it :D Seriously though: If we can't control it server-side, there's little reason to have the int in the first place. https://codereview.chromium.org/2872573002/diff/80001/components/suggestions/... File components/suggestions/suggestions_service_impl.cc (right): https://codereview.chromium.org/2872573002/diff/80001/components/suggestions/... components/suggestions/suggestions_service_impl.cc:116: return variations::GetVariationParamByFeatureAsInt( This is deprecated, you should use base::GetFieldTrialParamByFeatureAsInt() instead (as per comment on variations::GetVariationParamByFeatureAsInt).
Thanks! (Waiting for https://codereview.chromium.org/2869013004/ to land first.) https://codereview.chromium.org/2872573002/diff/60001/components/suggestions/... File components/suggestions/suggestions_service_impl.cc (right): https://codereview.chromium.org/2872573002/diff/60001/components/suggestions/... components/suggestions/suggestions_service_impl.cc:94: const char kSuggestionsUseAllParam[] = "min=0"; On 2017/05/10 12:32:24, Marc Treib wrote: > On 2017/05/10 12:25:30, fhorschig wrote: > > On 2017/05/10 10:08:02, Marc Treib wrote: > > > The "0" should probably be configurable via a variation param? > > > > Hmm, okay. I currently see no application, though. > > You never do, until you need it :D > > Seriously though: If we can't control it server-side, there's little reason to > have the int in the first place. It has direct a direct effect on the server-side. Still limited though, but it has. This is why I waited so long to address the first comment here. https://codereview.chromium.org/2872573002/diff/80001/components/suggestions/... File components/suggestions/suggestions_service_impl.cc (right): https://codereview.chromium.org/2872573002/diff/80001/components/suggestions/... components/suggestions/suggestions_service_impl.cc:116: return variations::GetVariationParamByFeatureAsInt( Used GetFieldTrialParamByFeatureAsInt.
Description was changed from ========== Use |all| param to query less than 8 suggestions In order to use the newly implemented server-side functionality of receiving less than 8 suggestions, the client has to append a new query parameter to the URL. Therefore, this CL is blocked on cr/155185623 BUG=709959 ========== to ========== Use |min| param to query a variable number of suggestions In order to use the newly implemented server-side functionality of receiving less than 8 suggestions, the client has to append a new query parameter to the URL. Therefore, this CL is blocked on cr/155185623 BUG=709959 ==========
Patchset #6 (id:120001) has been deleted
Patchset #6 (id:140001) has been deleted
The CQ bit was checked by fhorschig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by fhorschig@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/2872573002/#ps160001 (title: "Rebase (blackboxing unittest)")
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": 1494490551477940,
"parent_rev": "e8e5df6b838075a0dc6e584853f1f795ac5af0b3", "commit_rev":
"ec72b3b0616de6e11b11db904b12a5a7e2a1a9da"}
Message was sent while issue was closed.
Description was changed from ========== Use |min| param to query a variable number of suggestions In order to use the newly implemented server-side functionality of receiving less than 8 suggestions, the client has to append a new query parameter to the URL. Therefore, this CL is blocked on cr/155185623 BUG=709959 ========== to ========== Use |min| param to query a variable number of suggestions In order to use the newly implemented server-side functionality of receiving less than 8 suggestions, the client has to append a new query parameter to the URL. Therefore, this CL is blocked on cr/155185623 BUG=709959 Review-Url: https://codereview.chromium.org/2872573002 Cr-Commit-Position: refs/heads/master@{#470874} Committed: https://chromium.googlesource.com/chromium/src/+/ec72b3b0616de6e11b11db904b12... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:160001) as https://chromium.googlesource.com/chromium/src/+/ec72b3b0616de6e11b11db904b12... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
