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

Issue 10911169: Setup field trial for one-click signin inforbar. (Closed)

Created:
8 years, 3 months ago by Roger Tawa OOO till Jul 10th
Modified:
8 years, 3 months ago
Reviewers:
Peter Kasting, SteveT, Steve, sky
CC:
chromium-reviews, ncarter (slow), akalin, Raghu Simha, tfarina, haitaol1, tim (not reviewing)
Visibility:
Public.

Description

Setup field trial for one-click signin inforbar. BUG=147227 TEST=There are two variants: the normal infobar and the blue-on-white infobar. Start chrome with one of the following command line options to force one or the other: --force-fieldtrials="OneClickSignIn/Standard/" --force-fieldtrials="OneClickSignIn/BlueOnWhite/" Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=156715

Patch Set 1 : Simplify non-windows platforms #

Patch Set 2 : rebased #

Total comments: 10

Patch Set 3 : rebased #

Patch Set 4 : Address Steve's review comments #

Total comments: 25

Patch Set 5 : rebased #

Patch Set 6 : Address Peter's review comments #

Patch Set 7 : Add comment to DarkenColor function #

Patch Set 8 : rebased #

Unified diffs Side-by-side diffs Delta from patch set Stats (+370 lines, -63 lines) Patch
A chrome/browser/api/infobars/one_click_signin_infobar_delegate.h View 1 2 3 4 5 1 chunk +46 lines, -0 lines 0 comments Download
A chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc View 1 2 3 4 5 1 chunk +18 lines, -0 lines 0 comments Download
M chrome/browser/chrome_browser_field_trials.cc View 1 2 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/ui/sync/one_click_signin_helper.h View 1 2 3 4 5 3 chunks +6 lines, -3 lines 0 comments Download
M chrome/browser/ui/sync/one_click_signin_helper.cc View 1 2 3 4 5 11 chunks +90 lines, -50 lines 0 comments Download
M chrome/browser/ui/views/infobars/confirm_infobar.h View 1 2 3 4 5 2 chunks +7 lines, -1 line 0 comments Download
M chrome/browser/ui/views/infobars/infobar_background.h View 1 2 3 4 5 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/ui/views/infobars/infobar_background.cc View 1 2 3 4 5 1 chunk +2 lines, -5 lines 0 comments Download
M chrome/browser/ui/views/infobars/infobar_view.cc View 1 2 3 4 5 1 chunk +3 lines, -1 line 0 comments Download
A chrome/browser/ui/views/infobars/one_click_signin_infobar.h View 1 2 3 4 5 1 chunk +31 lines, -0 lines 0 comments Download
A chrome/browser/ui/views/infobars/one_click_signin_infobar.cc View 1 2 3 4 5 6 1 chunk +147 lines, -0 lines 0 comments Download
M chrome/browser/ui/views/infobars/translate_infobar_base.cc View 1 2 3 4 5 2 chunks +3 lines, -1 line 0 comments Download
M chrome/chrome_browser.gypi View 1 2 3 4 5 6 7 2 chunks +8 lines, -0 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 2 chunks +4 lines, -0 lines 0 comments Download

Messages

Total messages: 11 (0 generated)
Roger Tawa OOO till Jul 10th
Hi Peter, Scott, Steve, This a redo combining CL 10914146 and CL 10913121. Steve: nothing ...
8 years, 3 months ago (2012-09-09 13:14:42 UTC) #1
Peter Kasting
Sorry I didn't get to this today; I'll try tomorrow.
8 years, 3 months ago (2012-09-11 01:39:08 UTC) #2
SteveT
FieldTrial code lgtm. Some nits for you to look at... http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/sync/one_click_signin_helper.cc File chrome/browser/ui/sync/one_click_signin_helper.cc (right): http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/sync/one_click_signin_helper.cc#newcode70 ...
8 years, 3 months ago (2012-09-11 02:16:25 UTC) #3
Roger Tawa OOO till Jul 10th
Thanks Steve. Comments addressed, changes uploaded. http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/sync/one_click_signin_helper.cc File chrome/browser/ui/sync/one_click_signin_helper.cc (right): http://codereview.chromium.org/10911169/diff/11016/chrome/browser/ui/sync/one_click_signin_helper.cc#newcode70 chrome/browser/ui/sync/one_click_signin_helper.cc:70: On 2012/09/11 02:16:25, ...
8 years, 3 months ago (2012-09-11 19:23:21 UTC) #4
Peter Kasting
LGTM http://codereview.chromium.org/10911169/diff/10008/chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc File chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc (right): http://codereview.chromium.org/10911169/diff/10008/chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc#newcode8 chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc:8: SkColorSetARGB(0, 0, 0, 0); Nit: Unless you need ...
8 years, 3 months ago (2012-09-11 23:02:07 UTC) #5
Roger Tawa OOO till Jul 10th
Thanks Peter. Comments addressed, changes uploaded. http://codereview.chromium.org/10911169/diff/10008/chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc File chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc (right): http://codereview.chromium.org/10911169/diff/10008/chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc#newcode8 chrome/browser/api/infobars/one_click_signin_infobar_delegate.cc:8: SkColorSetARGB(0, 0, 0, ...
8 years, 3 months ago (2012-09-12 15:07:13 UTC) #6
Peter Kasting
http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/infobars/confirm_infobar.h File chrome/browser/ui/views/infobars/confirm_infobar.h (right): http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/infobars/confirm_infobar.h#newcode33 chrome/browser/ui/views/infobars/confirm_infobar.h:33: // InfoBarView: On 2012/09/12 15:07:14, Roger Tawa wrote: > ...
8 years, 3 months ago (2012-09-12 17:21:23 UTC) #7
Roger Tawa OOO till Jul 10th
http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/infobars/one_click_signin_infobar.cc File chrome/browser/ui/views/infobars/one_click_signin_infobar.cc (right): http://codereview.chromium.org/10911169/diff/10008/chrome/browser/ui/views/infobars/one_click_signin_infobar.cc#newcode59 chrome/browser/ui/views/infobars/one_click_signin_infobar.cc:59: SkColor InfoBarColoredButtonBorder::DarkenColor(SkColor color) { On 2012/09/12 17:21:24, Peter Kasting ...
8 years, 3 months ago (2012-09-12 18:36:55 UTC) #8
sky
LGTM
8 years, 3 months ago (2012-09-13 23:32:13 UTC) #9
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/rogerta@chromium.org/10911169/16001
8 years, 3 months ago (2012-09-13 23:33:18 UTC) #10
commit-bot: I haz the power
8 years, 3 months ago (2012-09-14 02:11:45 UTC) #11
Change committed as 156715

Powered by Google App Engine
This is Rietveld 408576698