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

Issue 2684343006: Make the account chooser and CVC dialog use the same icon with toolip for Views. (Closed)

Created:
3 years, 10 months ago by vasilii
Modified:
3 years, 10 months ago
Reviewers:
msw, Evan Stade
CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, tfarina, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Make the account chooser and CVC dialog use the same icon with toolip for Views. The CL changes the icon to (i). The account chooser reuses the implementation of the icon and the tooltip for the CVC dialog. BUG=690920 Review-Url: https://codereview.chromium.org/2684343006 Cr-Commit-Position: refs/heads/master@{#451273} Committed: https://chromium.googlesource.com/chromium/src/+/864f2986aa206bae957944685e1bbb0d7b4fdcc4

Patch Set 1 #

Total comments: 4

Patch Set 2 : move files #

Total comments: 56

Patch Set 3 : comments from msw@ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+158 lines, -636 lines) Patch
M chrome/browser/ui/BUILD.gn View 1 1 chunk +0 lines, -4 lines 0 comments Download
M chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc View 1 2 3 chunks +7 lines, -3 lines 0 comments Download
D chrome/browser/ui/views/autofill/info_bubble.h View 1 1 chunk +0 lines, -77 lines 0 comments Download
D chrome/browser/ui/views/autofill/info_bubble.cc View 1 1 chunk +0 lines, -154 lines 0 comments Download
D chrome/browser/ui/views/autofill/tooltip_icon.h View 1 1 chunk +0 lines, -87 lines 0 comments Download
M chrome/browser/ui/views/autofill/tooltip_icon.cc View 1 1 chunk +0 lines, -148 lines 0 comments Download
M chrome/browser/ui/views/passwords/credentials_item_view.h View 1 2 3 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/passwords/credentials_item_view.cc View 1 2 6 chunks +5 lines, -20 lines 0 comments Download
M ui/views/BUILD.gn View 1 1 chunk +4 lines, -0 lines 0 comments Download
A ui/views/bubble/info_bubble.h View 1 2 1 chunk +60 lines, -0 lines 0 comments Download
A + ui/views/bubble/info_bubble.cc View 1 2 5 chunks +37 lines, -62 lines 0 comments Download
A + ui/views/bubble/tooltip_icon.h View 1 2 2 chunks +20 lines, -23 lines 0 comments Download
A + ui/views/bubble/tooltip_icon.cc View 1 2 7 chunks +22 lines, -55 lines 0 comments Download

Messages

Total messages: 33 (17 generated)
vasilii
Hi Evan, I have few questions - What's the magic constant '233' in info_bubble.cc? Due ...
3 years, 10 months ago (2017-02-10 14:15:16 UTC) #4
Evan Stade
On 2017/02/10 14:15:16, vasilii wrote: > Hi Evan, > > I have few questions > ...
3 years, 10 months ago (2017-02-10 15:43:38 UTC) #7
vasilii
> > - The Enamel team wants the icon to consume the click. It's not ...
3 years, 10 months ago (2017-02-10 16:54:22 UTC) #8
vasilii
> Not user visible? Wouldn't it mean that clicking no longer focuses the cvc > ...
3 years, 10 months ago (2017-02-10 18:33:20 UTC) #9
Evan Stade
I may be misremembering but I thought the icon was inside the text field. I'm ...
3 years, 10 months ago (2017-02-10 19:06:15 UTC) #10
vasilii
On 2017/02/10 19:06:15, Evan Stade wrote: > I may be misremembering but I thought the ...
3 years, 10 months ago (2017-02-13 15:26:51 UTC) #11
vasilii
https://codereview.chromium.org/2684343006/diff/1/chrome/browser/ui/views/passwords/credentials_item_view.cc File chrome/browser/ui/views/passwords/credentials_item_view.cc (right): https://codereview.chromium.org/2684343006/diff/1/chrome/browser/ui/views/passwords/credentials_item_view.cc#newcode66 chrome/browser/ui/views/passwords/credentials_item_view.cc:66: }; On 2017/02/10 15:43:38, Evan Stade wrote: > disallow ...
3 years, 10 months ago (2017-02-15 16:29:21 UTC) #14
vasilii
Hi Mike, please review the files added to ui/views/bubble. I think it's the right place.
3 years, 10 months ago (2017-02-15 16:34:04 UTC) #16
msw
Have we considered just using standard views tooltips? https://codereview.chromium.org/2684343006/diff/20001/chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc File chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc (right): https://codereview.chromium.org/2684343006/diff/20001/chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc#newcode239 chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:239: icon->set_bubble_width(233); ...
3 years, 10 months ago (2017-02-15 21:06:39 UTC) #19
vasilii
The standard tooltip is used now in the account chooser. There was a request from ...
3 years, 10 months ago (2017-02-16 14:28:07 UTC) #20
msw
I would *highly* encourage use of the standard Views tooltip, as one-off solutions like this ...
3 years, 10 months ago (2017-02-16 19:04:29 UTC) #25
Evan Stade
On 2017/02/16 19:04:29, msw wrote: > I would *highly* encourage use of the standard Views ...
3 years, 10 months ago (2017-02-16 19:16:23 UTC) #26
hwi1
On 2017/02/16 19:04:29, msw wrote: > I would *highly* encourage use of the standard Views ...
3 years, 10 months ago (2017-02-17 04:58:49 UTC) #27
hwi1
On 2017/02/16 19:16:23, Evan Stade wrote: > On 2017/02/16 19:04:29, msw wrote: > > I ...
3 years, 10 months ago (2017-02-17 05:09:07 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2684343006/40001
3 years, 10 months ago (2017-02-17 09:41:02 UTC) #30
commit-bot: I haz the power
3 years, 10 months ago (2017-02-17 09:45:51 UTC) #33
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/864f2986aa206bae957944685e1b...

Powered by Google App Engine
This is Rietveld 408576698