Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(44)

Issue 2066703004: Change fill-on-account-select to use Features API, not a custom flag (Closed)

Created:
4 years, 6 months ago by jww
Modified:
4 years, 5 months ago
CC:
asvitkine+watch_chromium.org, browser-components-watch_chromium.org, chromium-reviews, estade+watch_chromium.org, gcasto+watchlist_chromium.org, jdonnelly+autofillwatch_chromium.org, mkwst+watchlist-passwords_chromium.org, rouslan+autofill_chromium.org, vabr+watchlistpasswordmanager_chromium.org, vabr+watchlistautofill_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Change fill-on-account-select to use Features API, not a custom flag Previously, the fill-on-account-select experiment in the password manager had used a custom flag and Finch to manage the experiment. This updates the experiment to use the Features API which handles all of that automagically. BUG=568713 R=vabr@chromium.org

Patch Set 1 #

Total comments: 5

Patch Set 2 : Nits from vabr #

Patch Set 3 : Fix tests on iOS #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+52 lines, -64 lines) Patch
M chrome/browser/about_flags.cc View 1 3 chunks +4 lines, -11 lines 1 comment Download
M chrome/browser/chrome_content_browser_client.cc View 1 chunk +0 lines, -2 lines 0 comments Download
M chrome/renderer/DEPS View 1 1 chunk +1 line, -0 lines 0 comments Download
M chrome/renderer/autofill/password_autofill_agent_browsertest.cc View 1 10 chunks +17 lines, -16 lines 0 comments Download
M components/autofill/core/common/autofill_switches.h View 1 chunk +0 lines, -3 lines 0 comments Download
M components/autofill/core/common/autofill_switches.cc View 2 chunks +0 lines, -8 lines 0 comments Download
M components/password_manager/core/browser/password_form_manager.cc View 4 chunks +4 lines, -20 lines 0 comments Download
M components/password_manager/core/browser/password_form_manager_unittest.cc View 4 chunks +7 lines, -4 lines 0 comments Download
M components/password_manager/core/browser/password_manager_unittest.cc View 1 2 2 chunks +13 lines, -0 lines 1 comment Download
M components/password_manager/core/common/password_manager_features.h View 1 chunk +1 line, -0 lines 0 comments Download
M components/password_manager/core/common/password_manager_features.cc View 1 chunk +5 lines, -0 lines 0 comments Download

Messages

Total messages: 31 (12 generated)
jww
vabr@, can you take a look? Thanks!
4 years, 6 months ago (2016-06-14 21:59:16 UTC) #3
vabr (Chromium)
Awesome, LGTM, thanks a lot! Vaclav https://codereview.chromium.org/2066703004/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2066703004/diff/1/chrome/browser/about_flags.cc#newcode1907 chrome/browser/about_flags.cc:1907: {"enable-fill-on-account-select", IDS_FILL_ON_ACCOUNT_SELECT_NAME, optional: ...
4 years, 6 months ago (2016-06-15 09:15:34 UTC) #4
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066703004/1
4 years, 6 months ago (2016-06-15 09:15:59 UTC) #6
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/builds/21465)
4 years, 6 months ago (2016-06-15 09:33:19 UTC) #8
jww
jochen@chromium.org, can you verify that my addition to chrome/renderer/DEPS is cool? https://codereview.chromium.org/2066703004/diff/1/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (right): ...
4 years, 6 months ago (2016-06-15 17:17:56 UTC) #10
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066703004/20001
4 years, 6 months ago (2016-06-15 18:21:04 UTC) #12
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/builds/21694)
4 years, 6 months ago (2016-06-15 18:39:30 UTC) #14
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066703004/40001
4 years, 6 months ago (2016-06-15 22:56:27 UTC) #16
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 6 months ago (2016-06-16 00:21:23 UTC) #18
vabr (Chromium)
Thanks, Joel, your changes in https://codereview.chromium.org/2066703004/#ps40001 LGTM. Cheers, Vaclav https://codereview.chromium.org/2066703004/diff/1/chrome/renderer/autofill/password_autofill_agent_browsertest.cc File chrome/renderer/autofill/password_autofill_agent_browsertest.cc (right): https://codereview.chromium.org/2066703004/diff/1/chrome/renderer/autofill/password_autofill_agent_browsertest.cc#newcode47 chrome/renderer/autofill/password_autofill_agent_browsertest.cc:47: ...
4 years, 6 months ago (2016-06-16 07:03:39 UTC) #19
jww
On 2016/06/16 07:03:39, vabr (Chromium) wrote: > Thanks, Joel, your changes in > https://codereview.chromium.org/2066703004/#ps40001 LGTM. ...
4 years, 6 months ago (2016-06-20 02:55:33 UTC) #20
jochen (gone - plz use gerrit)
lgtm with comment https://codereview.chromium.org/2066703004/diff/40001/chrome/browser/about_flags.cc File chrome/browser/about_flags.cc (left): https://codereview.chromium.org/2066703004/diff/40001/chrome/browser/about_flags.cc#oldcode364 chrome/browser/about_flags.cc:364: { IDS_FLAGS_FILL_ON_ACCOUNT_SELECT_ENABLE_HIGHLIGHTING, this string is unused ...
4 years, 6 months ago (2016-06-20 11:34:13 UTC) #21
vabr (Chromium)
On 2016/06/20 11:34:13, jochen wrote: > lgtm with comment > > https://codereview.chromium.org/2066703004/diff/40001/chrome/browser/about_flags.cc > File chrome/browser/about_flags.cc ...
4 years, 6 months ago (2016-06-20 12:20:44 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2066703004/40001
4 years, 6 months ago (2016-06-20 12:21:29 UTC) #24
commit-bot: I haz the power
Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/23258) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, ...
4 years, 6 months ago (2016-06-20 12:23:19 UTC) #26
vabr (Chromium)
On 2016/06/20 12:23:19, commit-bot: I haz the power wrote: > Try jobs failed on following ...
4 years, 6 months ago (2016-06-20 12:25:05 UTC) #27
vabr (Chromium)
On 2016/06/20 12:25:05, vabr (Chromium) wrote: > On 2016/06/20 12:23:19, commit-bot: I haz the power ...
4 years, 6 months ago (2016-06-20 12:35:00 UTC) #28
jww
On 2016/06/20 12:35:00, vabr (Chromium) wrote: > On 2016/06/20 12:25:05, vabr (Chromium) wrote: > > ...
4 years, 5 months ago (2016-07-20 22:36:58 UTC) #30
jww
4 years, 5 months ago (2016-07-20 22:37:52 UTC) #31
Message was sent while issue was closed.
On 2016/07/20 22:36:58, jww wrote:
> On 2016/06/20 12:35:00, vabr (Chromium) wrote:
> > On 2016/06/20 12:25:05, vabr (Chromium) wrote:
> > > On 2016/06/20 12:23:19, commit-bot: I haz the power wrote:
> > > > Try jobs failed on following builders:
> > > >   ios-device on tryserver.chromium.mac (JOB_FAILED,
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
> > > >   ios-device-gn on tryserver.chromium.mac (JOB_FAILED,
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...)
> > > >   ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED,
> > > >
> > >
> >
>
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
> > > 
> > > OK, the patch is out of date.
> > > 
> > > So instead I'll upload a rebased version of it, with Jochen's comment
> > addressed,
> > > and TBR it to Jochen and jww@ (because I don't expect Rietveld will let me
> > > update this patch).
> > > 
> > > Cheers,
> > > Vaclav
> > 
> > Done in https://codereview.chromium.org/2080263002/.
> 
> Thanks, vabr@! Since this was committed in another CL, I'm closing this one.

Haha, it's already closed, nevermind!

Powered by Google App Engine
This is Rietveld 408576698