Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
16749002:
Talk to production wallet server when Autocheckout experiment is enabled (Closed)
|
Created:
7 years, 6 months ago by Raman Kakilate Modified:
7 years, 6 months ago CC:
chromium-reviews, benquan, jam, ahutter, browser-components-watch_chromium.org, dbeam+watch-autofill_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, Ilya Sherman, rouslan+autofillwatch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionTalk to production wallet server when Autocheckout experiment is enabled
BUG=232075
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=207904
Patch Set 1 #
Messages
Total messages: 10 (0 generated)
ahutter/isherman - PTAL
Why do we need to couple these? On Mon, Jun 10, 2013 at 12:28 PM, <ramankk@chromium.org> wrote: > Reviewers: ahutter, Ilya Sherman, > > Message: > ahutter/isherman - PTAL > > Description: > Talk to production wallet server when Autocheckout experiment is enabled > > BUG=232075 > > Please review this at https://codereview.chromium.**org/16749002/<https://codereview.chromium.org/1... > > SVN Base: svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > Affected files: > M components/autofill/content/**browser/wallet/wallet_service_**url.cc > > > Index: components/autofill/content/**browser/wallet/wallet_service_** > url.cc > diff --git a/components/autofill/content/**browser/wallet/wallet_service_* > *url.cc b/components/autofill/content/**browser/wallet/wallet_service_** > url.cc > index aa5cf991b0da5c573a8deac9f76895**2a3e391682..** > 04f805c93ea427a774b3097c680fb4**edf25b5892 100644 > --- a/components/autofill/content/**browser/wallet/wallet_service_**url.cc > +++ b/components/autofill/content/**browser/wallet/wallet_service_**url.cc > @@ -29,7 +29,8 @@ const char kSandboxWalletSecureServiceUrl**[] = > bool IsWalletProductionEnabled() { > const CommandLine& command_line = *CommandLine::**ForCurrentProcess(); > return command_line.HasSwitch(**switches::**kWalletServiceUseProd) || > - base::FieldTrialList::**FindFullName("**WalletProductionService") > == "Yes"; > + base::FieldTrialList::**FindFullName("**WalletProductionService") > == "Yes" || > + base::FieldTrialList::**FindFullName("Autocheckout") == "Yes"; > } > > GURL GetWalletHostUrl() { > > > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
On 2013/06/10 19:42:31, Albert Bodenhamer wrote: > Why do we need to couple these? > "Autocheckout" field trial is already used to fetch Autocheckout's field-mappings; just reusing the same field trial here. rAc could use WalletProductionService field trial to enable dogfood? > > On Mon, Jun 10, 2013 at 12:28 PM, <mailto:ramankk@chromium.org> wrote: > > > Reviewers: ahutter, Ilya Sherman, > > > > Message: > > ahutter/isherman - PTAL > > > > Description: > > Talk to production wallet server when Autocheckout experiment is enabled > > > > BUG=232075 > > > > Please review this at > https://codereview.chromium.**org/16749002/%3Chttps://codereview.chromium.org...> > > > > SVN Base: > svn://svn.chromium.org/chrome/**trunk/src<http://svn.chromium.org/chrome/trunk/src> > > > > Affected files: > > M components/autofill/content/**browser/wallet/wallet_service_**url.cc > > > > > > Index: components/autofill/content/**browser/wallet/wallet_service_** > > url.cc > > diff --git a/components/autofill/content/**browser/wallet/wallet_service_* > > *url.cc b/components/autofill/content/**browser/wallet/wallet_service_** > > url.cc > > index aa5cf991b0da5c573a8deac9f76895**2a3e391682..** > > 04f805c93ea427a774b3097c680fb4**edf25b5892 100644 > > --- a/components/autofill/content/**browser/wallet/wallet_service_**url.cc > > +++ b/components/autofill/content/**browser/wallet/wallet_service_**url.cc > > @@ -29,7 +29,8 @@ const char kSandboxWalletSecureServiceUrl**[] = > > bool IsWalletProductionEnabled() { > > const CommandLine& command_line = *CommandLine::**ForCurrentProcess(); > > return command_line.HasSwitch(**switches::**kWalletServiceUseProd) || > > - base::FieldTrialList::**FindFullName("**WalletProductionService") > > == "Yes"; > > + base::FieldTrialList::**FindFullName("**WalletProductionService") > > == "Yes" || > > + base::FieldTrialList::**FindFullName("Autocheckout") == "Yes"; > > } > > > > GURL GetWalletHostUrl() { > > > > > > > > > -- > Albert Bodenhamer | Software Engineer | mailto:abodenha@chromium.<abodenha@google.com> > org
On Mon, Jun 10, 2013 at 1:06 PM, <ramankk@chromium.org> wrote: > On 2013/06/10 19:42:31, Albert Bodenhamer wrote: > >> Why do we need to couple these? >> > > > "Autocheckout" field trial is already used to fetch Autocheckout's > field-mappings; just reusing the same field trial here. > > rAc could use WalletProductionService field trial to enable dogfood? WalletProductionService controls whether we're talking to sandbox or prod. The intent was to use it for either rAc or autocheckout dogfood and we kept it separate for flexibility. Note that I don't have an objection to coupling them. I just want to understand the reasons. > > > > On Mon, Jun 10, 2013 at 12:28 PM, <mailto:ramankk@chromium.org> wrote: >> > > > Reviewers: ahutter, Ilya Sherman, >> > >> > Message: >> > ahutter/isherman - PTAL >> > >> > Description: >> > Talk to production wallet server when Autocheckout experiment is enabled >> > >> > BUG=232075 >> > >> > Please review this at >> > > https://codereview.chromium.****org/16749002/%3Chttps://codere** > view.chromium.org/16749002/ <http://codereview.chromium.org/16749002/>> > >> > >> > SVN Base: >> > > svn://svn.chromium.org/chrome/****trunk/src<http://svn.chromium.org/chrome/**trunk/src> > <http://svn.**chromium.org/chrome/trunk/src<http://svn.chromium.org/chrome/tru... > > > >> > >> > Affected files: >> > M components/autofill/content/****browser/wallet/wallet_service_** >> **url.cc >> > >> > >> > Index: components/autofill/content/****browser/wallet/wallet_service_** >> ** >> > url.cc >> > diff --git a/components/autofill/content/****browser/wallet/wallet_** >> service_* >> > *url.cc b/components/autofill/content/****browser/wallet/wallet_** >> service_** >> > url.cc >> > index aa5cf991b0da5c573a8deac9f76895****2a3e391682..** >> > 04f805c93ea427a774b3097c680fb4****edf25b5892 100644 >> > --- a/components/autofill/content/****browser/wallet/wallet_** >> service_**url.cc >> > +++ b/components/autofill/content/****browser/wallet/wallet_** >> service_**url.cc >> > @@ -29,7 +29,8 @@ const char kSandboxWalletSecureServiceUrl****[] = >> > bool IsWalletProductionEnabled() { >> > const CommandLine& command_line = *CommandLine::**** >> ForCurrentProcess(); >> > return command_line.HasSwitch(****switches::****kWalletServiceUseProd) >> || >> > - base::FieldTrialList::****FindFullName("**** >> WalletProductionService") >> > == "Yes"; >> > + base::FieldTrialList::****FindFullName("**** >> WalletProductionService") >> > == "Yes" || >> > + base::FieldTrialList::****FindFullName("Autocheckout") == "Yes"; >> > } >> > >> > GURL GetWalletHostUrl() { >> > >> > >> > >> > > > -- >> Albert Bodenhamer | Software Engineer | >> > mailto:abodenha@chromium.<abod**enha@google.com <abodenha@google.com>> > >> org >> > > > > https://codereview.chromium.**org/16749002/<https://codereview.chromium.org/1... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
On 2013/06/10 20:15:47, Albert Bodenhamer wrote: > On Mon, Jun 10, 2013 at 1:06 PM, <mailto:ramankk@chromium.org> wrote: > > > On 2013/06/10 19:42:31, Albert Bodenhamer wrote: > > > >> Why do we need to couple these? > >> > > > > > > "Autocheckout" field trial is already used to fetch Autocheckout's > > field-mappings; just reusing the same field trial here. > > > > rAc could use WalletProductionService field trial to enable dogfood? > > > WalletProductionService controls whether we're talking to sandbox or prod. > The intent was to use it for either rAc or autocheckout dogfood and we > kept it separate for flexibility. > > Note that I don't have an objection to coupling them. I just want to > understand the reasons. > Advantage I see is having different dogfood cycles for rAc and autocheckout. Though, If rAc is always more broadly available than autocheckout at all times, just guarding it with WalletProductionService trial would be sufficient. > > > > > > > > > On Mon, Jun 10, 2013 at 12:28 PM, <mailto:ramankk@chromium.org> wrote: > >> > > > > > Reviewers: ahutter, Ilya Sherman, > >> > > >> > Message: > >> > ahutter/isherman - PTAL > >> > > >> > Description: > >> > Talk to production wallet server when Autocheckout experiment is enabled > >> > > >> > BUG=232075 > >> > > >> > Please review this at > >> > > > > https://codereview.chromium.****org/16749002/%253Chttps://codere** > > view.chromium.org/16749002/ <http://codereview.chromium.org/16749002/>> > > > >> > > >> > SVN Base: > >> > > > > > svn://svn.chromium.org/chrome/****trunk/src<http://svn.chromium.org/chrome/**trunk/src> > > > <http://svn.**chromium.org/chrome/trunk/src%3Chttp://svn.chromium.org/chrome/t...> > > > > > > >> > > >> > Affected files: > >> > M components/autofill/content/****browser/wallet/wallet_service_** > >> **url.cc > >> > > >> > > >> > Index: components/autofill/content/****browser/wallet/wallet_service_** > >> ** > >> > url.cc > >> > diff --git a/components/autofill/content/****browser/wallet/wallet_** > >> service_* > >> > *url.cc b/components/autofill/content/****browser/wallet/wallet_** > >> service_** > >> > url.cc > >> > index aa5cf991b0da5c573a8deac9f76895****2a3e391682..** > >> > 04f805c93ea427a774b3097c680fb4****edf25b5892 100644 > >> > --- a/components/autofill/content/****browser/wallet/wallet_** > >> service_**url.cc > >> > +++ b/components/autofill/content/****browser/wallet/wallet_** > >> service_**url.cc > >> > @@ -29,7 +29,8 @@ const char kSandboxWalletSecureServiceUrl****[] = > >> > bool IsWalletProductionEnabled() { > >> > const CommandLine& command_line = *CommandLine::**** > >> ForCurrentProcess(); > >> > return command_line.HasSwitch(****switches::****kWalletServiceUseProd) > >> || > >> > - base::FieldTrialList::****FindFullName("**** > >> WalletProductionService") > >> > == "Yes"; > >> > + base::FieldTrialList::****FindFullName("**** > >> WalletProductionService") > >> > == "Yes" || > >> > + base::FieldTrialList::****FindFullName("Autocheckout") == "Yes"; > >> > } > >> > > >> > GURL GetWalletHostUrl() { > >> > > >> > > >> > > >> > > > > > > -- > >> Albert Bodenhamer | Software Engineer | > >> > > mailto:abodenha@chromium.<abod**enha@google.com <mailto:abodenha@google.com>> > > > >> org > >> > > > > > > > > > https://codereview.chromium.**org/16749002/%3Chttps://codereview.chromium.org...> > > > > > > -- > Albert Bodenhamer | Software Engineer | mailto:abodenha@chromium.<abodenha@google.com> > org
On Mon, Jun 10, 2013 at 1:23 PM, <ramankk@chromium.org> wrote: > On 2013/06/10 20:15:47, Albert Bodenhamer wrote: > > On Mon, Jun 10, 2013 at 1:06 PM, <mailto:ramankk@chromium.org> wrote: >> > > > On 2013/06/10 19:42:31, Albert Bodenhamer wrote: >> > >> >> Why do we need to couple these? >> >> >> > >> > >> > "Autocheckout" field trial is already used to fetch Autocheckout's >> > field-mappings; just reusing the same field trial here. >> > >> > rAc could use WalletProductionService field trial to enable dogfood? >> > > > WalletProductionService controls whether we're talking to sandbox or prod. >> The intent was to use it for either rAc or autocheckout dogfood and we >> kept it separate for flexibility. >> > > Note that I don't have an objection to coupling them. I just want to >> understand the reasons. >> > > > Advantage I see is having different dogfood cycles for rAc and > autocheckout. > Though, If rAc is always more broadly available than autocheckout at all > times, > just guarding it with WalletProductionService trial would be sufficient. > The effect is the same. WalletProductionService just controls prod. We'll want to flip WalletProductionService for rAc, but rAc is no longer behind a flag. The only way to keep them separate would be to create separate production flags for rAc and autocheckout, but I don't think that's worth doing. Alex convinced me off line that tieing the flags together is simpler for autocheckout. So go for it. > > > > >> > >> > >> > On Mon, Jun 10, 2013 at 12:28 PM, <mailto:ramankk@chromium.org> wrote: >> >> >> > >> > > Reviewers: ahutter, Ilya Sherman, >> >> > >> >> > Message: >> >> > ahutter/isherman - PTAL >> >> > >> >> > Description: >> >> > Talk to production wallet server when Autocheckout experiment is >> enabled >> >> > >> >> > BUG=232075 >> >> > >> >> > Please review this at >> >> >> > >> > https://codereview.chromium.******org/16749002/%253Chttps://**codere** >> > view.chromium.org/16749002/ <http://codereview.chromium.**org/16749002/<http://codereview.chromium.org/167... >> >> >> > >> >> > >> >> > SVN Base: >> >> >> > >> > >> > > svn://svn.chromium.org/chrome/******trunk/src<http://svn.chromium.org/chrome/****trunk/src> > <http://svn.**chromium.org/chrome/**trunk/**src<http://svn.chromium.org/chrome... > > > >> > >> > > <http://svn.**chromium.org/**chrome/trunk/src%3Chttp://svn.** > chromium.org/chrome/trunk/src<http://chromium.org/chrome/trunk/src%3Chttp://svn.chromium.org/chrome/trunk/src> > > > >> > > >> > >> >> > >> >> > Affected files: >> >> > M components/autofill/content/******browser/wallet/wallet_** >> service_** >> >> **url.cc >> >> > >> >> > >> >> > Index: components/autofill/content/******browser/wallet/wallet_** >> service_** >> >> ** >> >> > url.cc >> >> > diff --git a/components/autofill/content/** >> ****browser/wallet/wallet_** >> >> service_* >> >> > *url.cc b/components/autofill/content/******browser/wallet/wallet_** >> >> service_** >> >> > url.cc >> >> > index aa5cf991b0da5c573a8deac9f76895******2a3e391682..** >> >> > 04f805c93ea427a774b3097c680fb4******edf25b5892 100644 >> >> > --- a/components/autofill/content/******browser/wallet/wallet_** >> >> service_**url.cc >> >> > +++ b/components/autofill/content/******browser/wallet/wallet_** >> >> service_**url.cc >> >> > @@ -29,7 +29,8 @@ const char kSandboxWalletSecureServiceUrl******[] >> = >> >> > bool IsWalletProductionEnabled() { >> >> > const CommandLine& command_line = *CommandLine::**** >> >> ForCurrentProcess(); >> >> > return command_line.HasSwitch(******switches::****** >> kWalletServiceUseProd) >> >> || >> >> > - base::FieldTrialList::******FindFullName("**** >> >> WalletProductionService") >> >> > == "Yes"; >> >> > + base::FieldTrialList::******FindFullName("**** >> >> WalletProductionService") >> >> > == "Yes" || >> >> > + base::FieldTrialList::******FindFullName("Autocheckout") == >> "Yes"; >> >> >> > } >> >> > >> >> > GURL GetWalletHostUrl() { >> >> > >> >> > >> >> > >> >> >> > >> > >> > -- >> >> Albert Bodenhamer | Software Engineer | >> >> >> > mailto:abodenha@chromium.<**abod**enha@google.com >> > <mailto:abodenha@google.com>> > >> > >> >> org >> >> >> > >> > >> > >> > >> > > https://codereview.chromium.****org/16749002/%3Chttps://codere** > view.chromium.org/16749002/ <http://codereview.chromium.org/16749002/>> > > > >> > > > > -- >> Albert Bodenhamer | Software Engineer | >> > mailto:abodenha@chromium.<abod**enha@google.com <abodenha@google.com>> > >> org >> > > > > https://codereview.chromium.**org/16749002/<https://codereview.chromium.org/1... > -- Albert Bodenhamer | Software Engineer | abodenha@chromium.<abodenha@google.com> org
lgtm
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ramankk@chromium.org/16749002/1
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ramankk@chromium.org/16749002/1
Message was sent while issue was closed.
Change committed as 207904 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
