Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
957523002:
Autofill card unmasking prompt - Make "verifying..." message an overlay (Closed)
|
Created:
5 years, 10 months ago by Evan Stade Modified:
5 years, 9 months ago Reviewers:
sky CC:
chromium-reviews, estade+watch_chromium.org, rouslan+autofillwatch_chromium.org, tfarina Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAutofill card unmasking prompt - Make "verifying..." message an overlay
on desktop, as it is on mobile.
BUG=none
Committed: https://crrev.com/e334e1785e2e6a03ba24d9cf76f12735d99d4074
Cr-Commit-Position: refs/heads/master@{#318547}
Patch Set 1 #Patch Set 2 : . #
Total comments: 5
Patch Set 3 : leave filllayout alone #
Total comments: 6
Patch Set 4 : . #Messages
Total messages: 13 (3 generated)
estade@chromium.org changed reviewers: + sky@chromium.org
https://codereview.chromium.org/957523002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc (right): https://codereview.chromium.org/957523002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:279: progress_label_ = new views::Label(base::ASCIIToUTF16("Verifying...")); Do we only care about english now? :) https://codereview.chromium.org/957523002/diff/20001/ui/views/layout/fill_lay... File ui/views/layout/fill_layout.cc (right): https://codereview.chromium.org/957523002/diff/20001/ui/views/layout/fill_lay... ui/views/layout/fill_layout.cc:20: host->child_at(i)->SetBoundsRect(host->GetContentsBounds()); If you want to support multiple children shouldn't the overall preferred size be the max of all the children? Also, I'm surprised that this code doesn't inset based on the insets of the host as GetPreferredSize accounts for them.
https://codereview.chromium.org/957523002/diff/20001/chrome/browser/ui/views/... File chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc (right): https://codereview.chromium.org/957523002/diff/20001/chrome/browser/ui/views/... chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:279: progress_label_ = new views::Label(base::ASCIIToUTF16("Verifying...")); On 2015/02/24 21:35:55, sky wrote: > Do we only care about english now? :) as long as the UI is in some state of flux, I have been punting that since it's easy to fix later. https://codereview.chromium.org/957523002/diff/20001/ui/views/layout/fill_lay... File ui/views/layout/fill_layout.cc (right): https://codereview.chromium.org/957523002/diff/20001/ui/views/layout/fill_lay... ui/views/layout/fill_layout.cc:20: host->child_at(i)->SetBoundsRect(host->GetContentsBounds()); On 2015/02/24 21:35:55, sky wrote: > If you want to support multiple children shouldn't the overall preferred size be > the max of all the children? That would be one possibility, but for this use case it's not what we want. It's hard to judge what's going to be more generally useful. No other FillLayout clients have multiple children yet, but if a lot of views start wanting FillLayout to behave that way we could change it. > Also, I'm surprised that this code doesn't inset based on the insets of the host > as GetPreferredSize accounts for them. isn't that what GetContentsBounds() does?
https://codereview.chromium.org/957523002/diff/20001/ui/views/layout/fill_lay... File ui/views/layout/fill_layout.cc (right): https://codereview.chromium.org/957523002/diff/20001/ui/views/layout/fill_lay... ui/views/layout/fill_layout.cc:20: host->child_at(i)->SetBoundsRect(host->GetContentsBounds()); On 2015/02/24 22:07:02, Evan Stade wrote: > On 2015/02/24 21:35:55, sky wrote: > > If you want to support multiple children shouldn't the overall preferred size > be > > the max of all the children? > > That would be one possibility, but for this use case it's not what we want. It's > hard to judge what's going to be more generally useful. No other FillLayout > clients have multiple children yet, but if a lot of views start wanting > FillLayout to behave that way we could change it. I'm pretty sure we don't want this behavior for FillLayout. I would rather you create your own LayoutManager for this behavior if you want it. > > > Also, I'm surprised that this code doesn't inset based on the insets of the > host > > as GetPreferredSize accounts for them. > > isn't that what GetContentsBounds() does? My mistake. You are right.
removed changes to FillLayout
LGTM https://codereview.chromium.org/957523002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc (right): https://codereview.chromium.org/957523002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:94: void Layout() override { This one shouldn't be const. https://codereview.chromium.org/957523002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:104: rect.Inset(-host->GetInsets()); No host. https://codereview.chromium.org/957523002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:108: int GetPreferredHeightForWidth(int width) const override { GetHeightForWidth.
I sent that last one out for review too early, sorry. https://codereview.chromium.org/957523002/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc (right): https://codereview.chromium.org/957523002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:94: void Layout() override { On 2015/02/26 17:18:43, sky wrote: > This one shouldn't be const. it's not (?) https://codereview.chromium.org/957523002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:104: rect.Inset(-host->GetInsets()); On 2015/02/26 17:18:43, sky wrote: > No host. Done. https://codereview.chromium.org/957523002/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:108: int GetPreferredHeightForWidth(int width) const override { On 2015/02/26 17:18:43, sky wrote: > GetHeightForWidth. Done.
The CQ bit was checked by estade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org Link to the patchset: https://codereview.chromium.org/957523002/#ps60001 (title: ".")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/957523002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/e334e1785e2e6a03ba24d9cf76f12735d99d4074 Cr-Commit-Position: refs/heads/master@{#318547} |
