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

Issue 13845022: Mac: Display a native bubble (instead of the JS one) after the web signin flow. (branched from http… (Closed)

Created:
7 years, 8 months ago by noms (inactive)
Modified:
7 years, 7 months ago
Base URL:
https://chromium.googlesource.com/chromium/src.git@ntpBubble
Visibility:
Public.

Description

Mac: Display a native bubble (instead of the JS one) after the web signin flow. XIB changes: Deleted "Undo" button from the Bubble dialog, and copied the InformativePlaceholderText from chrome/app/nibs/OneClickSigninDialog.xib, which allows the "Learn More" link to be displayed. Part of a 3 CL Voltron, along with: [Win] https://codereview.chromium.org/13979003/ [GTK] https://codereview.chromium.org/14258007/ (branched off https://codereview.chromium.org/13979003/) BUG=119728 TEST=Open Chrome. Delete all your profiles, so that you have a fresh one. Open a new tab, and press the "sign in" link in the top right corner. Sign in. Upon completion, this should show a native popup bubble, anchored off the wrench menu. If you try to log in with the same email in another profile, the bubble should display a sad error message. Check out the issue description for screenshots. Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=197769

Patch Set 1 #

Patch Set 2 : Added learn more link #

Patch Set 3 : Fix bot issues #

Total comments: 23

Patch Set 4 : Review comments #

Total comments: 7

Patch Set 5 : Review comments #

Total comments: 6

Patch Set 6 : #

Total comments: 9

Patch Set 7 : Nit picking #

Patch Set 8 : Fixed compile error #

Patch Set 9 : Rebase with master #

Patch Set 10 : Rebase with master #

Patch Set 11 : Rebase with master - fix mis-merged line #

Patch Set 12 : Ditto #

Unified diffs Side-by-side diffs Delta from patch set Stats (+276 lines, -167 lines) Patch
M chrome/app/nibs/OneClickSigninBubble.xib View 1 17 chunks +79 lines, -102 lines 0 comments Download
M chrome/browser/ui/cocoa/browser_window_cocoa.mm View 1 2 3 4 5 1 chunk +5 lines, -2 lines 0 comments Download
M chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h View 1 2 3 4 1 chunk +10 lines, -4 lines 0 comments Download
M chrome/browser/ui/cocoa/one_click_signin_bubble_controller.mm View 1 2 3 4 5 6 7 3 chunks +9 lines, -3 lines 0 comments Download
A chrome/browser/ui/cocoa/one_click_signin_bubble_controller_browsertest.mm View 1 2 3 4 5 6 1 chunk +93 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/one_click_signin_bubble_controller_unittest.mm View 1 2 3 4 5 6 7 8 2 chunks +8 lines, -16 lines 0 comments Download
M chrome/browser/ui/cocoa/one_click_signin_dialog_controller.mm View 1 2 3 4 5 6 7 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/cocoa/one_click_signin_dialog_controller_browsertest.mm View 1 2 3 4 5 6 1 chunk +1 line, -2 lines 0 comments Download
M chrome/browser/ui/cocoa/one_click_signin_view_controller.h View 1 2 3 4 5 6 7 8 9 2 chunks +10 lines, -6 lines 0 comments Download
M chrome/browser/ui/cocoa/one_click_signin_view_controller.mm View 1 2 3 4 5 6 7 8 9 10 11 4 chunks +58 lines, -31 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 19 (0 generated)
Alexei Svitkine (slow)
Haven't looked at the code yet, but first: Please add a TEST= line specifying how ...
7 years, 8 months ago (2013-04-19 19:21:26 UTC) #1
noms
Done! Sorry! On 2013/04/19 19:21:26, Alexei Svitkine wrote: > Haven't looked at the code yet, ...
7 years, 8 months ago (2013-04-19 19:26:33 UTC) #2
Alexei Svitkine (slow)
Here's some initial comments. https://codereview.chromium.org/13845022/diff/6002/chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h (right): https://codereview.chromium.org/13845022/diff/6002/chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h#newcode36 chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:36: errorMessage:(const string16&)errorMessage Please document the ...
7 years, 8 months ago (2013-04-19 19:54:15 UTC) #3
noms (inactive)
https://codereview.chromium.org/13845022/diff/6002/chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h File chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h (right): https://codereview.chromium.org/13845022/diff/6002/chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h#newcode36 chrome/browser/ui/cocoa/one_click_signin_bubble_controller.h:36: errorMessage:(const string16&)errorMessage On 2013/04/19 19:54:15, Alexei Svitkine wrote: > ...
7 years, 8 months ago (2013-04-22 15:28:07 UTC) #4
Alexei Svitkine (slow)
https://codereview.chromium.org/13845022/diff/6002/chrome/browser/ui/cocoa/one_click_signin_view_controller.mm File chrome/browser/ui/cocoa/one_click_signin_view_controller.mm (right): https://codereview.chromium.org/13845022/diff/6002/chrome/browser/ui/cocoa/one_click_signin_view_controller.mm#newcode61 chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:61: if (isSyncDialog_ && !startSyncCallback_.is_null()) { On 2013/04/22 15:28:07, Monica ...
7 years, 8 months ago (2013-04-22 18:16:51 UTC) #5
noms (inactive)
https://codereview.chromium.org/13845022/diff/6002/chrome/browser/ui/cocoa/one_click_signin_view_controller.mm File chrome/browser/ui/cocoa/one_click_signin_view_controller.mm (right): https://codereview.chromium.org/13845022/diff/6002/chrome/browser/ui/cocoa/one_click_signin_view_controller.mm#newcode61 chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:61: if (isSyncDialog_ && !startSyncCallback_.is_null()) { Ok, did some analysis: ...
7 years, 8 months ago (2013-04-23 18:52:48 UTC) #6
Alexei Svitkine (slow)
https://codereview.chromium.org/13845022/diff/6002/chrome/browser/ui/cocoa/one_click_signin_view_controller.mm File chrome/browser/ui/cocoa/one_click_signin_view_controller.mm (right): https://codereview.chromium.org/13845022/diff/6002/chrome/browser/ui/cocoa/one_click_signin_view_controller.mm#newcode61 chrome/browser/ui/cocoa/one_click_signin_view_controller.mm:61: if (isSyncDialog_ && !startSyncCallback_.is_null()) { On 2013/04/23 18:52:48, Monica ...
7 years, 8 months ago (2013-04-23 19:43:23 UTC) #7
noms (inactive)
https://codereview.chromium.org/13845022/diff/25001/chrome/browser/ui/cocoa/browser_window_cocoa.mm File chrome/browser/ui/cocoa/browser_window_cocoa.mm (right): https://codereview.chromium.org/13845022/diff/25001/chrome/browser/ui/cocoa/browser_window_cocoa.mm#newcode497 chrome/browser/ui/cocoa/browser_window_cocoa.mm:497: error_message) On 2013/04/23 19:43:24, Alexei Svitkine wrote: > This ...
7 years, 8 months ago (2013-04-24 14:49:29 UTC) #8
Alexei Svitkine (slow)
Awesome! Just a few more nits in your new file. https://codereview.chromium.org/13845022/diff/33001/chrome/browser/ui/cocoa/one_click_signin_bubble_controller_browsertest.mm File chrome/browser/ui/cocoa/one_click_signin_bubble_controller_browsertest.mm (right): https://codereview.chromium.org/13845022/diff/33001/chrome/browser/ui/cocoa/one_click_signin_bubble_controller_browsertest.mm#newcode18 ...
7 years, 8 months ago (2013-04-24 15:26:27 UTC) #9
noms (inactive)
https://codereview.chromium.org/13845022/diff/33001/chrome/browser/ui/cocoa/one_click_signin_bubble_controller_browsertest.mm File chrome/browser/ui/cocoa/one_click_signin_bubble_controller_browsertest.mm (right): https://codereview.chromium.org/13845022/diff/33001/chrome/browser/ui/cocoa/one_click_signin_bubble_controller_browsertest.mm#newcode18 chrome/browser/ui/cocoa/one_click_signin_bubble_controller_browsertest.mm:18: : InProcessBrowserTest(), On 2013/04/24 15:26:27, Alexei Svitkine wrote: > ...
7 years, 7 months ago (2013-04-25 18:29:58 UTC) #10
Alexei Svitkine (slow)
lgtm
7 years, 7 months ago (2013-04-25 18:32:51 UTC) #11
noms (inactive)
+nico for nibs owners: chrome/app/nibs/OneClickSigninBubble.xib
7 years, 7 months ago (2013-04-25 18:50:29 UTC) #12
Nico
lgtm stamp asvitkine: Do you want to add yourself to chrome/app/nibs/OWNERS?
7 years, 7 months ago (2013-04-25 19:11:57 UTC) #13
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/13845022/74002
7 years, 7 months ago (2013-05-01 20:38:10 UTC) #14
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 7 months ago (2013-05-01 21:02:26 UTC) #15
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/13845022/74002
7 years, 7 months ago (2013-05-01 21:05:20 UTC) #16
commit-bot: I haz the power
Sorry for I got bad news for ya. Compile failed with a clobber build on ...
7 years, 7 months ago (2013-05-01 21:14:48 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/13845022/74002
7 years, 7 months ago (2013-05-01 22:02:35 UTC) #18
commit-bot: I haz the power
7 years, 7 months ago (2013-05-02 01:34:24 UTC) #19
Message was sent while issue was closed.
Change committed as 197769

Powered by Google App Engine
This is Rietveld 408576698