New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 857127 link

Starred by 1 user
Status: Fixed
Owner: ----
Closed: Jul 10
Cc:
skare@chromium.org
isherman@chromium.org
ios-bugs@chromium.org
asvitk...@chromium.org
█ rkaplow@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac
Pri: 3
Type: Task



Sign in to add a comment

AppendVariationHeaders Should Have Shorter API

Project Member Reported by mpear...@chromium.org, Jun 27 2018 

Tons of people call AppendVariationHeaders() with variations::SignedIn::kNo with the following comment:
  // Note: It's OK to pass SignedIn::kNo if it's unknown, as it does not affect
  // transmission of experiments coming from the variations server.

We should simply make a version without the sign-in parameter and remove those many comments in the code.  The comment about ::kNo is already by the function declaration.

Comment 1 by asvitk...@chromium.org, Jun 27 2018 

FYI: The API was recently replaced with CreateSimpleURLLoaderWithVariationsHeaders() due to network service migration.

https://chromium.googlesource.com/chromium/src/+/6260d6caff3f410586afb03191f721b308357941

Though I think the point still applies to the new API.

Comment 2 by mpear...@chromium.org, Jun 27 2018

Labels: Hotlist-GoodFirstBug

Comment 3 by wanghui0...@gmail.com, Jul 1 

Hi 
   I had upload a cl for this please review it. thanks.

   https://chromium-review.googlesource.com/c/chromium/src/+/1121669

Hui
Project Member

Comment 4 by bugdroid1@chromium.org, Jul 9 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/00fff442a632182b024ed982c48dc09071c82ab1

commit 00fff442a632182b024ed982c48dc09071c82ab1
Author: Wang Hui <wanghui07050707@gmail.com>
Date: Mon Jul 09 23:50:23 2018

Make shorter api of AppendVariationHeaders and
CreateSimpleURLLoaderWithVariationHeaders.

Tons of people call AppendVariationHeaders() with variations::SignedIn::kNo with
the following comment:
  // Note: It's OK to pass SignedIn::kNo if it's unknown, as it does not affect
  // transmission of experiments coming from the variations server.
This CL simply make a version without the sign-in parameter of
AppendVariationHeaders and CreateSimpleULRLoaderWithVariationHeader and remove
those many comments in the code.

BUG= 857127 

Cq-Include-Trybots: luci.chromium.try:ios-simulator-full-configs;master.tryserver.chromium.mac:ios-simulator-cronet
Change-Id: Id0af5394ce31b117e109033f44a43720d4407197
Reviewed-on: https://chromium-review.googlesource.com/1121669
Commit-Queue: Theresa <twellington@chromium.org>
Reviewed-by: Theresa <twellington@chromium.org>
Reviewed-by: Zachary Kuznia <zork@chromium.org>
Reviewed-by: Marc Treib <treib@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Nicolas Zea <zea@chromium.org>
Reviewed-by: Vaclav Brozek <vabr@chromium.org>
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Stepan Khapugin <stkhapugin@chromium.org>
Reviewed-by: Samuel Huang <huangs@chromium.org>
Reviewed-by: Mathieu Perreault <mathp@chromium.org>
Reviewed-by: Alexei Svitkine <asvitkine@chromium.org>
Reviewed-by: Mark Pearson <mpearson@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573534}
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/AUTHORS
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/chrome/browser/android/contextualsearch/contextual_search_delegate.cc
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/chrome/browser/net/variations_http_headers_browsertest.cc
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/chrome/browser/search/one_google_bar/one_google_bar_loader_impl.cc
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/components/autofill/core/browser/autofill_download_manager.cc
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/components/feedback/feedback_uploader.cc
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/components/ntp_snippets/breaking_news/subscription_json_request.cc
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/components/ntp_snippets/remote/json_request.cc
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/components/omnibox/browser/base_search_provider.cc
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/components/omnibox/browser/contextual_suggestions_service.cc
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/components/omnibox/browser/search_provider.cc
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/components/suggestions/suggestions_service_impl.cc
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/components/variations/net/variations_http_headers.cc
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/components/variations/net/variations_http_headers.h
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/ios/chrome/browser/ui/contextual_search/contextual_search_delegate.cc
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/ios/chrome/browser/ui/location_bar/location_bar_coordinator.mm
[modify] https://crrev.com/00fff442a632182b024ed982c48dc09071c82ab1/ios/chrome/browser/ui/location_bar/location_bar_legacy_coordinator.mm

Comment 5 by mpear...@chromium.org, Jul 10

Status: Fixed (was: Available)
Thanks Hui!
►

Sign in to add a comment