Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2955963002:
Update Chrome Upstream flow to reflect new UI mocks (Closed)
|
Created:
3 years, 5 months ago by Jared Saul Modified:
3 years, 4 months ago CC:
chromium-reviews, groby+bubble_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, hcarmona+bubble_chromium.org, tfarina, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, rouslan+bubble_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionUpdate Chrome Upstream flow (Chrome -> Payments upload credit card save for Autofill) to reflect new UI mocks
Mocks: https://docs.google.com/presentation/d/135dRphfYbqnbdWqsHf2Kjth1HzMjidKwLq94i-0RCOQ/edit#slide=id.g1e6c4c1b7a_2_49
BUG=736944
Review-Url: https://codereview.chromium.org/2955963002
Cr-Commit-Position: refs/heads/master@{#493080}
Committed: https://chromium.googlesource.com/chromium/src/+/19ef81cf629066734a0fd1d1699d43ee307af659
Patch Set 1 : Update comments #
Total comments: 16
Patch Set 2 : Code review changes #
Total comments: 8
Patch Set 3 : Code review changes; change label; fix trybot #
Total comments: 28
Patch Set 4 : Actioning first round of tapted/msw code review comments #
Total comments: 19
Patch Set 5 : Actioning code review comments from Patch Set 4 #
Total comments: 8
Patch Set 6 : Actioning code review comments from Patch Set 5 #Patch Set 7 : Put changes behind AutofillUpstreamShowNewUi flag #
Total comments: 12
Patch Set 8 : Actioning code review comments from Patch Set 7 #Patch Set 9 : Add missing imports #Patch Set 10 : Add non-Android macro for flag enabled check #Patch Set 11 : Update enums.xml to fix AboutFlagsHistogramTest #
Total comments: 2
Patch Set 12 : Prevent AddNewUiFlagStateToRequestIfExperimentOn test from running on Android #Messages
Total messages: 60 (29 generated)
Patchset #1 (id:1) has been deleted
jsaul@google.com changed reviewers: + jiahuiguo@google.com, mathp@chromium.org, szhangcs@google.com
Hey guys, PTAL. This code represents the first 3 out of 4 slides in this section: https://docs.google.com/presentation/d/135dRphfYbqnbdWqsHf2Kjth1HzMjidKwLq94i... Letting you all take the first crack at it before I add the other OWNERS. :) Thanks!
https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right): https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:145: IDS_AUTOFILL_SAVE_CARD_PROMPT_TITLE_TO_CLOUD); Could do return l10n_util::GetStringFUTF16( show_upload_confirm_title_ ? msg1 ; msg2);? https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc (left): https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:407: TEST_F(SaveCardBubbleControllerImplTest, Metrics_Upload_Reshows_LearnMore) { Do we have tests for the Upload part? https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right): https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.h:64: bool ShouldShowCloseButton() const override; Should we add a "Should" here? Looks ShowCloseButton() is enough?
https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right): https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:145: IDS_AUTOFILL_SAVE_CARD_PROMPT_TITLE_TO_CLOUD); On 2017/06/27 01:21:10, jiahuiguo wrote: > Could do return l10n_util::GetStringFUTF16( > show_upload_confirm_title_ ? msg1 ; msg2);? Can't; they're two different functions. The first one has an extra letter in its name and takes a parameter. https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc (left): https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl_unittest.cc:407: TEST_F(SaveCardBubbleControllerImplTest, Metrics_Upload_Reshows_LearnMore) { On 2017/06/27 01:21:10, jiahuiguo wrote: > Do we have tests for the Upload part? We had them; I'm getting rid of them because the "Learn more" link is going away for Upload. It's sticking around for local save AFAIK though, and we didn't have any tests for that for some reason, so I just moved them. https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right): https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.h:64: bool ShouldShowCloseButton() const override; On 2017/06/27 01:21:10, jiahuiguo wrote: > Should we add a "Should" here? Looks ShowCloseButton() is enough? It's overriding a method in views::WidgetDelegate: https://cs.chromium.org/chromium/src/ui/views/widget/widget_delegate.h?l=87&r...
Could you put the new UI mocks in the description. https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right): https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:69: void setShowUploadConfirmTitle(bool show_upload_confirm_title) override; s/set/Set. Looks like the first character of every other function name is capitalized. https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:94: if (GetCurrentFlowStep() == LOCAL_SAVE_ONLY_STEP) { if (GetCurrentFlowStep() != LOCAL_SAVE_ONLY_STEP) return nullptr; https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:146: if (controller_) DCHECK(controller_) ? Can controller be null at this step? https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:189: return l10n_util::GetStringUTF16(IDS_AUTOFILL_SAVE_CARD_PROMPT_ACCEPT); Nit: use switch instead. https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right): https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.h:88: UPLOAD_SAVE_CVC_FIX_FLOW_STEP_1, It would be better if replace step_1 with a more specific name. ditto for step 2.
Description was changed from ========== Update Chrome Upstream flow to reflect new UI mocks BUG=736944 ========== to ========== Update Chrome Upstream flow to reflect new UI mocks Mocks: https://docs.google.com/presentation/d/135dRphfYbqnbdWqsHf2Kjth1HzMjidKwLq94i... BUG=736944 ==========
Thanks guys! https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right): https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:69: void setShowUploadConfirmTitle(bool show_upload_confirm_title) override; On 2017/06/27 04:00:31, Shanfeng wrote: > s/set/Set. Looks like the first character of every other function name is > capitalized. Done. https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:94: if (GetCurrentFlowStep() == LOCAL_SAVE_ONLY_STEP) { On 2017/06/27 04:00:31, Shanfeng wrote: > if (GetCurrentFlowStep() != LOCAL_SAVE_ONLY_STEP) > return nullptr; Done. https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:146: if (controller_) On 2017/06/27 04:00:31, Shanfeng wrote: > DCHECK(controller_) ? Can controller be null at this step? Done. https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:189: return l10n_util::GetStringUTF16(IDS_AUTOFILL_SAVE_CARD_PROMPT_ACCEPT); On 2017/06/27 04:00:31, Shanfeng wrote: > Nit: use switch instead. Done. https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right): https://codereview.chromium.org/2955963002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.h:88: UPLOAD_SAVE_CVC_FIX_FLOW_STEP_1, On 2017/06/27 04:00:31, Shanfeng wrote: > It would be better if replace step_1 with a more specific name. ditto for step > 2. I kept the STEP_1 and STEP_2 parts so it's clear what order these things happen in, but I added _OFFER_UPLOAD and _REQUEST_CVC to make it more descriptive about what each step entails.
The CQ bit was checked by jsaul@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
Looks mostly good, just some comments! https://codereview.chromium.org/2955963002/diff/40001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right): https://codereview.chromium.org/2955963002/diff/40001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:138: if (is_uploading_) { if (is_uploading_) { return show_upload_confirm_title_ ? ... : ...; } return l10n_util::GetStringUTF16(IDS_AUTOFILL_SAVE_CARD_PROMPT_TITLE_LOCAL); https://codereview.chromium.org/2955963002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2955963002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:315: ChromeLayoutProvider::Get()->GetDistanceMetric( use |provider| here? https://codereview.chromium.org/2955963002/diff/40001/components/autofill_str... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2955963002/diff/40001/components/autofill_str... components/autofill_strings.grdp:224: Save this card for faster checkout? did you verify this copy? https://codereview.chromium.org/2955963002/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2955963002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:545: if (footnote_container_ && footnote_container_->child_count() >= 1) {}
lgtm
lgtm
https://codereview.chromium.org/2955963002/diff/40001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right): https://codereview.chromium.org/2955963002/diff/40001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:138: if (is_uploading_) { On 2017/06/27 21:12:06, Mathieu wrote: > if (is_uploading_) { > return show_upload_confirm_title_ ? ... : ...; > } > return l10n_util::GetStringUTF16(IDS_AUTOFILL_SAVE_CARD_PROMPT_TITLE_LOCAL); Done. https://codereview.chromium.org/2955963002/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2955963002/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:315: ChromeLayoutProvider::Get()->GetDistanceMetric( On 2017/06/27 21:12:06, Mathieu wrote: > use |provider| here? Good catch; done. https://codereview.chromium.org/2955963002/diff/40001/components/autofill_str... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2955963002/diff/40001/components/autofill_str... components/autofill_strings.grdp:224: Save this card for faster checkout? On 2017/06/27 21:12:06, Mathieu wrote: > did you verify this copy? Yep. Though the mocks are now out of date, it was discussed here: https://docs.google.com/document/d/1XmJMiZ66pEO7Dgqm6m3IupLEyeEdwbvNtkoj54OiV... Newer mocks here: https://docs.google.com/presentation/d/135dRphfYbqnbdWqsHf2Kjth1HzMjidKwLq94i... srahim@ was a big part of the discussions. Alex and Chris are going to get it rolling for final Chrome UI review this week. https://codereview.chromium.org/2955963002/diff/40001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2955963002/diff/40001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:545: if (footnote_container_ && footnote_container_->child_count() >= 1) On 2017/06/27 21:12:06, Mathieu wrote: > {} Factored out the footnote_container_ check and added curly braces here. Is there a reason this one gets braces but not the one below?
jsaul@google.com changed reviewers: + msw@chromium.org, tapted@chromium.org
Hi msw@ and tapted@, PTAL. msw@: Could you please review save_card_bubble_views.h/.cc since estade@ is OOO until July 10th? tapted@: Could you please review bubble_frame_view.cc (since we chatted about this recently)? I guess msw@ owns that area too though, so feel free to defer. Thank you!
https://codereview.chromium.org/2955963002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2955963002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:548: footnote_container_->child_at(0)->visible()); Note for msw@/tapted@: This changing of the footnote_container_'s visibility happens inside a const function. Do you have a better suggestion, or is this acceptable? Thanks.
The CQ bit was checked by jsaul@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
perhaps mention something about autofill in the CL description? ("Upstream" has
other loaded meanings for source code)
https://codereview.chromium.org/2955963002/diff/60001/ui/views/bubble/bubble_...
File ui/views/bubble/bubble_frame_view.cc (right):
https://codereview.chromium.org/2955963002/diff/60001/ui/views/bubble/bubble_...
ui/views/bubble/bubble_frame_view.cc:548:
footnote_container_->child_at(0)->visible());
On 2017/06/27 21:52:26, Jared Saul wrote:
> Note for msw@/tapted@: This changing of the footnote_container_'s visibility
> happens inside a const function. Do you have a better suggestion, or is this
> acceptable? Thanks.
We need something added to bubble_frame_view_unittest.cc for this use case
And.. footnote_container_ already has a BoxLayout would should correctly account
for its child view visibility. Is the killer that GetHeightForWidth(..) still
includes |content_margins_| ? Maybe comment about that.
In which case a new View subclass for footnote_container_ like
FootnoteContainerView (e.g. in an anonymous namespace), which has an override of
ChildVisibilityChanged() might be the nicest way to propagate the visibility
"up" to the footnote_container_.
(The pattern is somewhat similar to DialogClientView::ButtonRowContainer in
dialog_client_view.cc )
https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right): https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:139: return show_upload_confirm_title_ Can this instead calls something like save_card_bubble_view_->GetWindowTitle() or maybe do something like switch (save_card_bubble_view_->GetCurrentFlowStep()) case X: return Y, etc.? https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right): https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:122: bool show_upload_confirm_title_{false}; nit: this curly-bracket init style is very uncommon for primitive members in the codebase (afaict); consider using | = false;| here and elsewhere in this file, unless I missed some very new style guidance on this topic. https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:66: SaveCardBubbleViews::CurrentFlowStep SaveCardBubbleViews::GetCurrentFlowStep() nit: reorder this (and dtor) to match decl (just before CreateMainContentView) https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:104: int SaveCardBubbleViews::GetDialogButtons() const { nit: reorder to match decl (above CreateExtraView) https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:122: for (const LegalMessageLine& line : controller_->GetLegalMessageLines()) nit: add curly braces for two-line body https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:206: // Local save should have a [No thanks] button, but Upload save should surface nit: Use a consistent "No thanks" / [No thanks] comment style. https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right): https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.h:48: // views::DialogDelegate optional nit: trailing ':' here and elsewhere, ie. // views::DialogDelegate: https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.h:49: int GetDialogButtons() const override; optionally put this in the views::BubbleDialogDelegateView: overrides section https://codereview.chromium.org/2955963002/diff/60001/components/autofill_str... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2955963002/diff/60001/components/autofill_str... components/autofill_strings.grdp:228: <if expr="is_linux or is_chromeos"> Should this apply to any desktop platform (ie. Mac and Windows too), or really just Linux and Chrome OS? https://codereview.chromium.org/2955963002/diff/60001/components/autofill_str... components/autofill_strings.grdp:230: <message name="IDS_AUTOFILL_SAVE_CARD_PROMPT_UPLOAD_EXPLANATION" desc="Explanation of the effect of the Autofill save card prompt when the card is to be saved by uploading it to Google Payments. The prompt can be either a bubble or an infobar."> nit: it's probably not an infobar (at least on desktop?) update comment(s)? https://codereview.chromium.org/2955963002/diff/60001/components/autofill_str... components/autofill_strings.grdp:240: <message name="IDS_AUTOFILL_SAVE_CARD_PROMPT_ENTER_CVC_TITLE" desc="Title text to show for the Autofill upload save credit card prompt when more information (e.g., CVC) is needed in order to save the card."> nit: maybe mention that this is showing the card network and last four digits? https://codereview.chromium.org/2955963002/diff/60001/components/autofill_str... components/autofill_strings.grdp:244: The CVC is used to validate your card and won't be saved in your account optional nit: s/won't/will not/?
Description was changed from ========== Update Chrome Upstream flow to reflect new UI mocks Mocks: https://docs.google.com/presentation/d/135dRphfYbqnbdWqsHf2Kjth1HzMjidKwLq94i... BUG=736944 ========== to ========== Update Chrome Upstream flow (Chrome -> Payments upload credit card save for Autofill) to reflect new UI mocks Mocks: https://docs.google.com/presentation/d/135dRphfYbqnbdWqsHf2Kjth1HzMjidKwLq94i... BUG=736944 ==========
Thanks for the reviews! PTAL. msw@: Left a question for you in save_card_bubble_controller_impl.cc tapted@: Resolved your big comment in bubble_frame_view.cc, please let me know if it's not enough. https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right): https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:139: return show_upload_confirm_title_ On 2017/06/29 20:10:24, msw wrote: > Can this instead calls something like save_card_bubble_view_->GetWindowTitle() > or maybe do something like switch (save_card_bubble_view_->GetCurrentFlowStep()) > case X: return Y, etc.? The problem with that is that SaveCardBubbleView [1] is a cross-platform interface for SaveCardBubbleViews, which is the one with all the logic (incl. GetCurrentFlowStep()). I'd have to add a GetWindowTitle() function to SaveCardBubbleView and every platform would need to implement it separately. Thoughts? That being said, I believe there are only two platforms (Mac/non-Mac) and if I need to duplicate a lot of these changes for Mac, it may be worth it anyway... [1] https://cs.chromium.org/chromium/src/chrome/browser/ui/autofill/save_card_bub... https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.h (right): https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl.h:122: bool show_upload_confirm_title_{false}; On 2017/06/29 20:10:24, msw wrote: > nit: this curly-bracket init style is very uncommon for primitive members in the > codebase (afaict); consider using | = false;| here and elsewhere in this file, > unless I missed some very new style guidance on this topic. I can't recall/find who originally told me to initialize them in this way, just the request to do it here instead of in the ctor. Changed to | = false;|. https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:66: SaveCardBubbleViews::CurrentFlowStep SaveCardBubbleViews::GetCurrentFlowStep() On 2017/06/29 20:10:25, msw wrote: > nit: reorder this (and dtor) to match decl (just before CreateMainContentView) Thanks, nice catch. Also reordered IsDialogButtonEnabled and ContentsChanged to match as well. https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:104: int SaveCardBubbleViews::GetDialogButtons() const { On 2017/06/29 20:10:25, msw wrote: > nit: reorder to match decl (above CreateExtraView) Done. https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:122: for (const LegalMessageLine& line : controller_->GetLegalMessageLines()) On 2017/06/29 20:10:24, msw wrote: > nit: add curly braces for two-line body Done. https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:206: // Local save should have a [No thanks] button, but Upload save should surface On 2017/06/29 20:10:25, msw wrote: > nit: Use a consistent "No thanks" / [No thanks] comment style. Done; thanks. https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/save_card_bubble_views.h (right): https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.h:48: // views::DialogDelegate On 2017/06/29 20:10:25, msw wrote: > optional nit: trailing ':' here and elsewhere, ie. // views::DialogDelegate: Done. https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/save_card_bubble_views.h:49: int GetDialogButtons() const override; On 2017/06/29 20:10:25, msw wrote: > optionally put this in the views::BubbleDialogDelegateView: overrides section Done. https://codereview.chromium.org/2955963002/diff/60001/components/autofill_str... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2955963002/diff/60001/components/autofill_str... components/autofill_strings.grdp:228: <if expr="is_linux or is_chromeos"> On 2017/06/29 20:10:25, msw wrote: > Should this apply to any desktop platform (ie. Mac and Windows too), or really > just Linux and Chrome OS? I checked again, because it turns out an ex-colleague's design doc had some incorrect info in it. This should actually be *just* Linux because every other platform will also save the card locally (crbug.com/162735). https://codereview.chromium.org/2955963002/diff/60001/components/autofill_str... components/autofill_strings.grdp:230: <message name="IDS_AUTOFILL_SAVE_CARD_PROMPT_UPLOAD_EXPLANATION" desc="Explanation of the effect of the Autofill save card prompt when the card is to be saved by uploading it to Google Payments. The prompt can be either a bubble or an infobar."> On 2017/06/29 20:10:25, msw wrote: > nit: it's probably not an infobar (at least on desktop?) update comment(s)? True; if this is the Linux-only section, it won't ever be an Android infobar. Done. https://codereview.chromium.org/2955963002/diff/60001/components/autofill_str... components/autofill_strings.grdp:240: <message name="IDS_AUTOFILL_SAVE_CARD_PROMPT_ENTER_CVC_TITLE" desc="Title text to show for the Autofill upload save credit card prompt when more information (e.g., CVC) is needed in order to save the card."> On 2017/06/29 20:10:25, msw wrote: > nit: maybe mention that this is showing the card network and last four digits? Done. https://codereview.chromium.org/2955963002/diff/60001/components/autofill_str... components/autofill_strings.grdp:244: The CVC is used to validate your card and won't be saved in your account On 2017/06/29 20:10:25, msw wrote: > optional nit: s/won't/will not/? Can't change this one; it's how it was requested by UX. https://codereview.chromium.org/2955963002/diff/60001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2955963002/diff/60001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:548: footnote_container_->child_at(0)->visible()); On 2017/06/27 23:36:15, tapted wrote: > On 2017/06/27 21:52:26, Jared Saul wrote: > > Note for msw@/tapted@: This changing of the footnote_container_'s visibility > > happens inside a const function. Do you have a better suggestion, or is this > > acceptable? Thanks. > > We need something added to bubble_frame_view_unittest.cc for this use case > > And.. footnote_container_ already has a BoxLayout would should correctly account > for its child view visibility. Is the killer that GetHeightForWidth(..) still > includes |content_margins_| ? Maybe comment about that. > > In which case a new View subclass for footnote_container_ like > FootnoteContainerView (e.g. in an anonymous namespace), which has an override of > ChildVisibilityChanged() might be the nicest way to propagate the visibility > "up" to the footnote_container_. > > (The pattern is somewhat similar to DialogClientView::ButtonRowContainer in > dialog_client_view.cc ) 1-Unit test) Added a test "FootnoteContainerViewShouldMatchVisibilityOfFirstChild". Any suggestions for others? Testing GetBoundsForClientView wasn't useful because footnote_container_view->height() was 0 even when setting it to StaticSizedView(gfx::Size(200, 200)), and GetSizeForClientSize was private. 2-Reasoning) Good call! Yes, that appears to be it. The extra 27 pixels are coming from non-child elements at this point: https://cs.chromium.org/chromium/src/ui/views/layout/box_layout.cc?l=593,598&... I'll leave comments to that effect. 3-Propagation) Done. The only snag is that ChildVisibilityChanged doesn't happen on AddChildView, so I had to make an UpdateVisibility call when the container is set up, but it's not in the const function anymore so that's good. (Thanks so much for including the ButtonRowContainer example; that made it SO much easier to understand.)
mostly lg with minor comments. https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc (right): https://codereview.chromium.org/2955963002/diff/60001/chrome/browser/ui/autof... chrome/browser/ui/autofill/save_card_bubble_controller_impl.cc:139: return show_upload_confirm_title_ On 2017/07/05 19:36:50, Jared Saul wrote: > On 2017/06/29 20:10:24, msw wrote: > > Can this instead calls something like save_card_bubble_view_->GetWindowTitle() > > or maybe do something like switch > (save_card_bubble_view_->GetCurrentFlowStep()) > > case X: return Y, etc.? > > The problem with that is that SaveCardBubbleView [1] is a cross-platform > interface for SaveCardBubbleViews, which is the one with all the logic (incl. > GetCurrentFlowStep()). I'd have to add a GetWindowTitle() function to > SaveCardBubbleView and every platform would need to implement it separately. > Thoughts? > > That being said, I believe there are only two platforms (Mac/non-Mac) and if I > need to duplicate a lot of these changes for Mac, it may be worth it anyway... > > [1] > https://cs.chromium.org/chromium/src/chrome/browser/ui/autofill/save_card_bub... Setting a |show_upload_confirm_title_| flag is just a bit weird, but it's okay. https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:75: // Simple container to bubble child view changes up the view hierarchy. nit: s/bubble/propagate/, or consider "A container that changes visibility with its contents." https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:145: EXPECT_EQ(footnote_dummy_view->parent(), (View*)frame.footnote_container_); nit: this cast shouldn't be needed or should use static_cast. https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:177: View* footnote_container_view = (View*)frame.footnote_container_; nit: this cast shouldn't be needed or should use static_cast. https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:178: EXPECT_EQ(footnote_dummy_view->parent(), footnote_container_view); Please just retrieve the footnote's container via footnote_dummy_view->parent() and revert your bubble_frame_view.h changes: make |footnote_container_| a views::View, remove the FootnoteContainerView forward decl in the header, remove the friend test, and make the FootnoteContainerView class local to bubble_frame_view.cc file (in an anon namespace).
looks good - thanks for adding the test! A few comments. (and I'm in a weird timezone no need to wait for an lg from me if you get one from msw) https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:81: if (child_count() >= 1) { I think this is always true? ChildVisibilityChanged() can't be invoked if there are no children. Perhaps DCHECK(..) or just do this in ChildVisibilityChanged (see later comment) https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:433: footnote_container_ = new FootnoteContainerView(); I see msw commented about this in the test :) - the step needed here might still be subtle. I.e. you'll either need to do something like FootnoteContainerView* footnote_container = new FootnoteContainerView(); ... footnote_container->UpdateVisibility(); AddChildView(footnote_container); footnote_container_ = footnote_container; } or, ... https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:441: footnote_container_->UpdateVisibility(); or, perhaps simpler, just call footnote_container_->SetVisible(view->visible()); then remove UpdateVisibility(), instead void ChildVisibilityChanged(View* child) override { SetVisible(child->visible()); } https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:184: } > > 1-Unit test) Added a test > "FootnoteContainerViewShouldMatchVisibilityOfFirstChild". Any suggestions for > others? Testing GetBoundsForClientView wasn't useful because > footnote_container_view->height() was 0 even when setting it to > StaticSizedView(gfx::Size(200, 200)), and GetSizeForClientSize was private. the visibility test is good.. I think I see the tricky part. For something more compelling, we'd probably want access to BubbleDialogDelegateView::SizeToContents(), but there's no BubbleDialogDelegateView in BubbleFrameViewTest. But we still have access to GetPreferredSize -- there's even a test for that (see below) https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:491: } I'd add some lines here like, // Adding a footnote should increase the preferred size, but only when the // footnote is visible. constexpr int kFootnoteHeight = 20; const gfx::Size no_footnote_size = expected_size; View* footnote = new StaticSizedView(gfx::Size(10, kFootnoteHeight)); footnote->SetVisible(false); frame.AddFootnoteView(footnote); EXPECT_EQ(no_footnote_size, frame.GetPreferredSize()); // No change. footnote->Setvisible(true); expected_size.Enlarge(0, kFootnoteHeight + frame.content_margins().height()); EXPECT_EQ(expected_size, frame.GetPreferredSize()); footnote->Setvisible(false); EXPECT_EQ(no_footnote_size, frame.GetPreferredSize()); (haven't tested those, but that's probably the right gist)
Thanks again for the detailed comments/instructions! tapted@: PTAL, the new test code is off by a single pixel for some reason. Would you happen to have any ideas why that might be before I try to delve into the layout complexities? https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:75: // Simple container to bubble child view changes up the view hierarchy. On 2017/07/05 20:45:30, msw - vacation july 7 wrote: > nit: s/bubble/propagate/, or consider "A container that changes visibility with > its contents." Done; took your full suggestion. https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:81: if (child_count() >= 1) { On 2017/07/06 01:11:58, tapted wrote: > I think this is always true? ChildVisibilityChanged() can't be invoked if there > are no children. Perhaps DCHECK(..) or just do this in ChildVisibilityChanged > (see later comment) Done. https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:433: footnote_container_ = new FootnoteContainerView(); On 2017/07/06 01:11:59, tapted wrote: > I see msw commented about this in the test :) - the step needed here might still > be subtle. I.e. you'll either need to do something like > > FootnoteContainerView* footnote_container = new FootnoteContainerView(); > ... > footnote_container->UpdateVisibility(); > AddChildView(footnote_container); > footnote_container_ = footnote_container; > } > > or, ... Thanks; this was really useful. https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view.cc:441: footnote_container_->UpdateVisibility(); On 2017/07/06 01:11:58, tapted wrote: > or, perhaps simpler, just call > > footnote_container_->SetVisible(view->visible()); > > then remove UpdateVisibility(), instead > > void ChildVisibilityChanged(View* child) override { > SetVisible(child->visible()); > } Done. https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:145: EXPECT_EQ(footnote_dummy_view->parent(), (View*)frame.footnote_container_); On 2017/07/05 20:45:30, msw - vacation july 7 wrote: > nit: this cast shouldn't be needed or should use static_cast. Yep; it's good now that I reverted it to a View*, thanks. https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:177: View* footnote_container_view = (View*)frame.footnote_container_; On 2017/07/05 20:45:30, msw - vacation july 7 wrote: > nit: this cast shouldn't be needed or should use static_cast. Done. https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:178: EXPECT_EQ(footnote_dummy_view->parent(), footnote_container_view); On 2017/07/05 20:45:30, msw - vacation july 7 wrote: > Please just retrieve the footnote's container via footnote_dummy_view->parent() > and revert your bubble_frame_view.h changes: make |footnote_container_| a > views::View, remove the FootnoteContainerView forward decl in the header, remove > the friend test, and make the FootnoteContainerView class local to > bubble_frame_view.cc file (in an anon namespace). Done; thank you for the detailed instructions! https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:491: } On 2017/07/06 01:11:59, tapted wrote: > I'd add some lines here like, > > // Adding a footnote should increase the preferred size, but only when the > // footnote is visible. > constexpr int kFootnoteHeight = 20; > const gfx::Size no_footnote_size = expected_size; > View* footnote = new StaticSizedView(gfx::Size(10, kFootnoteHeight)); > footnote->SetVisible(false); > frame.AddFootnoteView(footnote); > EXPECT_EQ(no_footnote_size, frame.GetPreferredSize()); // No change. > > footnote->Setvisible(true); > expected_size.Enlarge(0, kFootnoteHeight + frame.content_margins().height()); > EXPECT_EQ(expected_size, frame.GetPreferredSize()); > > footnote->Setvisible(false); > EXPECT_EQ(no_footnote_size, frame.GetPreferredSize()); > > > > (haven't tested those, but that's probably the right gist) Thanks for the code! There was about a 10x17-18 px area it wasn't account for that I figure had to do with the borders, so in order to keep it testing GetPreferredSize I split it into two parts. However, there's still a 1px different in height when the footnote is added (GetPreferredSize() is 312 instead of 311). Do you know off the top of your head where this might be coming from?
lgtm % nits, but please wait for msw@ https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:491: } On 2017/07/07 20:12:28, Jared Saul wrote: > On 2017/07/06 01:11:59, tapted wrote: > > I'd add some lines here like, > > > > // Adding a footnote should increase the preferred size, but only when the > > // footnote is visible. > > constexpr int kFootnoteHeight = 20; > > const gfx::Size no_footnote_size = expected_size; > > View* footnote = new StaticSizedView(gfx::Size(10, kFootnoteHeight)); > > footnote->SetVisible(false); > > frame.AddFootnoteView(footnote); > > EXPECT_EQ(no_footnote_size, frame.GetPreferredSize()); // No change. > > > > footnote->Setvisible(true); > > expected_size.Enlarge(0, kFootnoteHeight + frame.content_margins().height()); > > EXPECT_EQ(expected_size, frame.GetPreferredSize()); > > > > footnote->Setvisible(false); > > EXPECT_EQ(no_footnote_size, frame.GetPreferredSize()); > > > > > > > > (haven't tested those, but that's probably the right gist) > > Thanks for the code! There was about a 10x17-18 px area it wasn't account for > that I figure had to do with the borders, so in order to keep it testing > GetPreferredSize I split it into two parts. > > However, there's still a 1px different in height when the footnote is added > (GetPreferredSize() is 312 instead of 311). Do you know off the top of your > head where this might be coming from? Probably the line in SetFootnoteView: footnote_container_->SetBorder( CreateSolidSidedBorder(1, 0, 0, 0, kFootnoteBorderColor)); https://codereview.chromium.org/2955963002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2955963002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:82: DCHECK(child_count() >= 1); nit: Is there a reason to use >= rather than == ? The former suggests there might be more complexity than there actually is, which can confuse readers. E.g. If there _were_ more child views, it would be important to communicate that only the first view is responsible for controlling the visibility of the footnote. Whereas, DCHECK_EQ(child_count(), 1); SetVisible(child->visible()); makes it clear that there is always exactly one child view, so it must be the |child| view that's being passed in -- i.e. the one that is actually changing its visibility https://codereview.chromium.org/2955963002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:438: footnote_container->SetVisible(view->visible()); Using SetVisible (rather than UpdateVisibility), means you no longer need the concrete type (you just need View*), so I'd go back to just assigning to |footnote_container_| directly. (sorry - I gave you too many options!) https://codereview.chromium.org/2955963002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2955963002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view_unittest.cc:499: const gfx::Size no_footnote_size = frame.GetPreferredSize(); To cater for the border, probably something like constexpr int kFootnoteTopBorderThickness = 1;
lgtm with a nit, and Trent's comments addressed. https://codereview.chromium.org/2955963002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2955963002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view_unittest.cc:493: { nit: Make a separate new test fixture GetPreferredSizeWithFootnote, instead of making separate scoped blocks within the existing fixture.
Thanks for the reviews! FYI: Turns out I won't be able to submit this just yet; we've just been asked to tweak the branding and because of that I don't want this to make it into M61. It's highly likely I'll end up adding the Mac-based bubble changes into this CL as well. I'll let you know if this leads to new code I'd like eyes on again--sorry for the trouble. https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2955963002/diff/80001/ui/views/bubble/bubble_... ui/views/bubble/bubble_frame_view_unittest.cc:491: } On 2017/07/10 03:52:04, tapted wrote: > On 2017/07/07 20:12:28, Jared Saul wrote: > > On 2017/07/06 01:11:59, tapted wrote: > > > I'd add some lines here like, > > > > > > // Adding a footnote should increase the preferred size, but only when the > > > // footnote is visible. > > > constexpr int kFootnoteHeight = 20; > > > const gfx::Size no_footnote_size = expected_size; > > > View* footnote = new StaticSizedView(gfx::Size(10, kFootnoteHeight)); > > > footnote->SetVisible(false); > > > frame.AddFootnoteView(footnote); > > > EXPECT_EQ(no_footnote_size, frame.GetPreferredSize()); // No change. > > > > > > footnote->Setvisible(true); > > > expected_size.Enlarge(0, kFootnoteHeight + > frame.content_margins().height()); > > > EXPECT_EQ(expected_size, frame.GetPreferredSize()); > > > > > > footnote->Setvisible(false); > > > EXPECT_EQ(no_footnote_size, frame.GetPreferredSize()); > > > > > > > > > > > > (haven't tested those, but that's probably the right gist) > > > > Thanks for the code! There was about a 10x17-18 px area it wasn't account for > > that I figure had to do with the borders, so in order to keep it testing > > GetPreferredSize I split it into two parts. > > > > However, there's still a 1px different in height when the footnote is added > > (GetPreferredSize() is 312 instead of 311). Do you know off the top of your > > head where this might be coming from? > > Probably the line in SetFootnoteView: > > footnote_container_->SetBorder( > CreateSolidSidedBorder(1, 0, 0, 0, kFootnoteBorderColor)); > Hah, I skimmed right over it. Thanks! https://codereview.chromium.org/2955963002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view.cc (right): https://codereview.chromium.org/2955963002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:82: DCHECK(child_count() >= 1); On 2017/07/10 03:52:05, tapted wrote: > nit: Is there a reason to use >= rather than == ? The former suggests there > might be more complexity than there actually is, which can confuse readers. E.g. > If there _were_ more child views, it would be important to communicate that only > the first view is responsible for controlling the visibility of the footnote. > > Whereas, > > DCHECK_EQ(child_count(), 1); > SetVisible(child->visible()); > > makes it clear that there is always exactly one child view, so it must be the > |child| view that's being passed in -- i.e. the one that is actually changing > its visibility No big reason; just an abundance of caution in case I misunderstood how footnote views work. And using child_at(0) instead of |child| was just an oversight. Changed; thanks for catching! https://codereview.chromium.org/2955963002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view.cc:438: footnote_container->SetVisible(view->visible()); On 2017/07/10 03:52:05, tapted wrote: > Using SetVisible (rather than UpdateVisibility), means you no longer need the > concrete type (you just need View*), so I'd go back to just assigning to > |footnote_container_| directly. (sorry - I gave you too many options!) np, done. https://codereview.chromium.org/2955963002/diff/100001/ui/views/bubble/bubble... File ui/views/bubble/bubble_frame_view_unittest.cc (right): https://codereview.chromium.org/2955963002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view_unittest.cc:493: { On 2017/07/10 19:02:24, msw wrote: > nit: Make a separate new test fixture GetPreferredSizeWithFootnote, instead of > making separate scoped blocks within the existing fixture. Done. https://codereview.chromium.org/2955963002/diff/100001/ui/views/bubble/bubble... ui/views/bubble/bubble_frame_view_unittest.cc:499: const gfx::Size no_footnote_size = frame.GetPreferredSize(); On 2017/07/10 03:52:05, tapted wrote: > To cater for the border, probably something like > > constexpr int kFootnoteTopBorderThickness = 1; Done.
lgtm
The CQ bit was checked by jsaul@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
reviewed the feature controls, still lgtm with nits https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/about_f... chrome/browser/about_flags.cc:2881: flag_descriptions::kEnableAutofillCreditCardUploadNewUiDescription, kOsAll, kOsDesktop? https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:157: // For upload save when the new UI experiment is disabled, don't show the it seems like you will be here if the experiment is enabled? I would update the comment https://codereview.chromium.org/2955963002/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_experiments.h (right): https://codereview.chromium.org/2955963002/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_experiments.h:131: bool IsAutofillUpstreamShowNewUiExperimentEnabled(); Upstream -> CardUpload?
Thanks Mathieu! https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/about_f... File chrome/browser/about_flags.cc (right): https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/about_f... chrome/browser/about_flags.cc:2881: flag_descriptions::kEnableAutofillCreditCardUploadNewUiDescription, kOsAll, On 2017/08/09 00:17:23, Mathieu wrote: > kOsDesktop? I guess so. I intend this to be usable for Android too, but since it's not implemented I should keep this as kOsDesktop for now and change later once Android UI changes are implemented. Done. https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/save_card_bubble_views.cc (right): https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/save_card_bubble_views.cc:157: // For upload save when the new UI experiment is disabled, don't show the On 2017/08/09 00:17:23, Mathieu wrote: > it seems like you will be here if the experiment is enabled? I would update the > comment Wow, good catch! Done, thanks. https://codereview.chromium.org/2955963002/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_experiments.h (right): https://codereview.chromium.org/2955963002/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_experiments.h:131: bool IsAutofillUpstreamShowNewUiExperimentEnabled(); On 2017/08/09 00:17:23, Mathieu wrote: > Upstream -> CardUpload? I'm not opposed, but there are three other experiments starting with kAutofillUpstream, so I'm going to keep this for consistency.
https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/flag_de... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/flag_de... chrome/browser/flag_descriptions.cc:259: "Enable updated UI for Autofill credit card upload"; Add desktop macro? https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/flag_de... File chrome/browser/flag_descriptions.h (right): https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/flag_de... chrome/browser/flag_descriptions.h:184: extern const char kEnableAutofillCreditCardUploadNewUiName[]; Move down to the desktop only section? https://codereview.chromium.org/2955963002/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_experiments.cc (right): https://codereview.chromium.org/2955963002/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_experiments.cc:268: bool IsAutofillUpstreamShowNewUiExperimentEnabled() { Also add macro?
https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/flag_de... File chrome/browser/flag_descriptions.cc (right): https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/flag_de... chrome/browser/flag_descriptions.cc:259: "Enable updated UI for Autofill credit card upload"; On 2017/08/09 00:33:37, jiahuiguo wrote: > Add desktop macro? Discussed offline; going to keep this here for now; we do want to do Android in the very near future. https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/flag_de... File chrome/browser/flag_descriptions.h (right): https://codereview.chromium.org/2955963002/diff/140001/chrome/browser/flag_de... chrome/browser/flag_descriptions.h:184: extern const char kEnableAutofillCreditCardUploadNewUiName[]; On 2017/08/09 00:33:37, jiahuiguo wrote: > Move down to the desktop only section? ditto https://codereview.chromium.org/2955963002/diff/140001/components/autofill/co... File components/autofill/core/browser/autofill_experiments.cc (right): https://codereview.chromium.org/2955963002/diff/140001/components/autofill/co... components/autofill/core/browser/autofill_experiments.cc:268: bool IsAutofillUpstreamShowNewUiExperimentEnabled() { On 2017/08/09 00:33:37, jiahuiguo wrote: > Also add macro? Done; discussed offline.
The CQ bit was checked by jsaul@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jsaul@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from szhangcs@google.com, jiahuiguo@google.com, msw@chromium.org, tapted@chromium.org, mathp@chromium.org Link to the patchset: https://codereview.chromium.org/2955963002/#ps220001 (title: "Update enums.xml to fix AboutFlagsHistogramTest")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm https://codereview.chromium.org/2955963002/diff/220001/components/autofill_st... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2955963002/diff/220001/components/autofill_st... components/autofill_strings.grdp:228: <if expr="is_linux and not is_chromeos"> if is_linux is true, how will is_chromeos be true in the meantime?
lgtm
https://codereview.chromium.org/2955963002/diff/220001/components/autofill_st... File components/autofill_strings.grdp (right): https://codereview.chromium.org/2955963002/diff/220001/components/autofill_st... components/autofill_strings.grdp:228: <if expr="is_linux and not is_chromeos"> On 2017/08/09 17:06:34, Shanfeng wrote: > if is_linux is true, how will is_chromeos be true in the meantime? I wondered about that myself, but this is a followup of what's already here (line 210). I tried to dig into it a little and got the impression that Chrome OS also triggers is_linux, but I'd be happy to change them if that's incorrect.
The CQ bit was checked by jsaul@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, szhangcs@google.com, msw@chromium.org, jiahuiguo@google.com, tapted@chromium.org Link to the patchset: https://codereview.chromium.org/2955963002/#ps240001 (title: "Prevent AddNewUiFlagStateToRequestIfExperimentOn test from running on Android")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1502300433511220,
"parent_rev": "e78297f19a802b8e4f26b170f2e4c026a226fba7", "commit_rev":
"6bf0c139bcc0fe57def4f4709dff5cea59d2ac2e"}
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1502300433511220,
"parent_rev": "8a32759c578e9235a916f8830cbc08e440388f93", "commit_rev":
"5cfa11b6ab8669a0dbe8615c284e37c49f0b1226"}
CQ is committing da patch.
Bot data: {"patchset_id": 240001, "attempt_start_ts": 1502300433511220,
"parent_rev": "fb04104d0e09de58995e9776d77bab6e6da14f09", "commit_rev":
"19ef81cf629066734a0fd1d1699d43ee307af659"}
Message was sent while issue was closed.
Description was changed from ========== Update Chrome Upstream flow (Chrome -> Payments upload credit card save for Autofill) to reflect new UI mocks Mocks: https://docs.google.com/presentation/d/135dRphfYbqnbdWqsHf2Kjth1HzMjidKwLq94i... BUG=736944 ========== to ========== Update Chrome Upstream flow (Chrome -> Payments upload credit card save for Autofill) to reflect new UI mocks Mocks: https://docs.google.com/presentation/d/135dRphfYbqnbdWqsHf2Kjth1HzMjidKwLq94i... BUG=736944 Review-Url: https://codereview.chromium.org/2955963002 Cr-Commit-Position: refs/heads/master@{#493080} Committed: https://chromium.googlesource.com/chromium/src/+/19ef81cf629066734a0fd1d1699d... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:240001) as https://chromium.googlesource.com/chromium/src/+/19ef81cf629066734a0fd1d1699d... |
