|
|
Created:
3 years, 5 months ago by melandory Modified:
3 years, 4 months ago CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionSkeleton for showing "Show all saved passwords row"
Shows "Show all saved passwords row" for the case when the password was autofilled
and user clicks on the password field for Linux/CrOs/Windows platforms.
The appearance of the row is guarded only buy flag.
No metrics are added in this CL.
TBR=mahmadi@chromium.org
BUG=739343
Review-Url: https://codereview.chromium.org/2971783002
Cr-Commit-Position: refs/heads/master@{#490365}
Committed: https://chromium.googlesource.com/chromium/src/+/04fc429cf5d4c16316402d43c5435c36d09c480f
Patch Set 1 : more tests #
Total comments: 15
Patch Set 2 : move vector icons include #Patch Set 3 : fix ios compilation and adress some vasilii@ comments #Patch Set 4 : more comments adaressed #
Total comments: 16
Patch Set 5 : . #
Total comments: 1
Patch Set 6 : Fix ios compilation #
Total comments: 13
Patch Set 7 : comments #
Total comments: 31
Patch Set 8 : adressed some comments of estade@ #Patch Set 9 : . #Patch Set 10 : . #
Total comments: 1
Patch Set 11 : . #
Total comments: 27
Patch Set 12 : remove android guard from autofill_popup_layout_model_unittest #Patch Set 13 : . #
Total comments: 12
Patch Set 14 : . #
Total comments: 4
Patch Set 15 : . #Messages
Total messages: 192 (155 generated)
The CQ bit was checked by melandory@chromium.org 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 checked by melandory@chromium.org 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_...)
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by melandory@chromium.org 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...
Patchset #1 (id:1) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
Description was changed from ========== Skeleton for showing "Show all saved passwords row" Shows "Show all saved passwords row" for the case when the password was autofilled and user clicks on the password field for Linux/CrOs/Windows platforms. The appearance of the row is guarded only buy flag. BUG=739343 ========== to ========== Skeleton for showing "Show all saved passwords row" Shows "Show all saved passwords row" for the case when the password was autofilled and user clicks on the password field for Linux/CrOs/Windows platforms. The appearance of the row is guarded only buy flag. BUG=739343 ==========
The CQ bit was checked by melandory@chromium.org 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...
Patchset #1 (id:40001) has been deleted
Description was changed from ========== Skeleton for showing "Show all saved passwords row" Shows "Show all saved passwords row" for the case when the password was autofilled and user clicks on the password field for Linux/CrOs/Windows platforms. The appearance of the row is guarded only buy flag. BUG=739343 ========== to ========== Skeleton for showing "Show all saved passwords row" Shows "Show all saved passwords row" for the case when the password was autofilled and user clicks on the password field for Linux/CrOs/Windows platforms. The appearance of the row is guarded only buy flag. No metrics are added in this CL. BUG=739343 ==========
melandory@chromium.org changed reviewers: + vasilii@chromium.org
PTAL. Screenshot is attached to the bug. https://codereview.chromium.org/2971783002/diff/60001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/2971783002/diff/60001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:906: // message comes last among suggestions. This formating looks strange to me, but it's what cl format does and running cl format is presubmit check for this folder.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by melandory@chromium.org 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: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
I spent most of my time on autofill_popup_view_views.cc. I hope you'll make it easier to understand :-) I didn't get why you have two screenshots attached while you definitely implemented icon on the left. https://codereview.chromium.org/2971783002/diff/60001/chrome/app/vector_icons... File chrome/app/vector_icons/show_all_saved_passwords.1x.icon (right): https://codereview.chromium.org/2971783002/diff/60001/chrome/app/vector_icons... chrome/app/vector_icons/show_all_saved_passwords.1x.icon:1: // Copyright 2017 The Chromium Authors. All rights reserved. What's the point of having two identical files? https://codereview.chromium.org/2971783002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:181: if (is_icon_in_front_of_text_row) { Here something strange is going on. - Do you want to reuse kHttpWarningIconPadding deliberately? Then it should be renamed. - The code is incomprehensible due to usage of |is_rtl|. It should be gone. There are only two cases: icon on the left, icon on the right. https://codereview.chromium.org/2971783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:201: if (is_icon_in_front_of_text_row) { Is |is_icon_in_front_of_text_row| saying "there is an icon"? The "if" is again complicated. Condition inside condition for no reason https://codereview.chromium.org/2971783002/diff/60001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/2971783002/diff/60001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:254: options & autofill::IS_PASSWORD_FIELD) { Can we have () please? https://codereview.chromium.org/2971783002/diff/60001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/2971783002/diff/60001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:869: TEST_F(PasswordAutofillManagerTest, ShowAllPasswordsOptionOnPasswordField) { A comment? https://codereview.chromium.org/2971783002/diff/60001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:901: I'd split the test here. It would be easier to launch the feature then. https://codereview.chromium.org/2971783002/diff/60001/components/password_man... File components/password_manager/core/common/password_manager_features.cc (right): https://codereview.chromium.org/2971783002/diff/60001/components/password_man... components/password_manager/core/common/password_manager_features.cc:24: "enable-manual-fallbacks-filling", base::FEATURE_DISABLED_BY_DEFAULT}; It's recommended to use CamelCase in feature_list.h
The CQ bit was checked by melandory@chromium.org 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 checked by melandory@chromium.org 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2971783002/diff/60001/chrome/app/vector_icons... File chrome/app/vector_icons/show_all_saved_passwords.1x.icon (right): https://codereview.chromium.org/2971783002/diff/60001/chrome/app/vector_icons... chrome/app/vector_icons/show_all_saved_passwords.1x.icon:1: // Copyright 2017 The Chromium Authors. All rights reserved. On 2017/07/05 17:12:15, vasilii wrote: > What's the point of having two identical files? Done. https://codereview.chromium.org/2971783002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:181: if (is_icon_in_front_of_text_row) { On 2017/07/05 17:12:15, vasilii wrote: > Here something strange is going on. > - Do you want to reuse kHttpWarningIconPadding deliberately? Then it should be > renamed. Yep, deliberately. Renamed. > - The code is incomprehensible due to usage of |is_rtl|. It should be gone. > There are only two cases: icon on the left, icon on the right. https://codereview.chromium.org/2971783002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:201: if (is_icon_in_front_of_text_row) { On 2017/07/05 17:12:15, vasilii wrote: > Is |is_icon_in_front_of_text_row| saying "there is an icon"? > > The "if" is again complicated. Condition inside condition for no reason Done. https://codereview.chromium.org/2971783002/diff/60001/components/password_man... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/2971783002/diff/60001/components/password_man... components/password_manager/core/browser/password_autofill_manager.cc:254: options & autofill::IS_PASSWORD_FIELD) { On 2017/07/05 17:12:15, vasilii wrote: > Can we have () please? Done. https://codereview.chromium.org/2971783002/diff/60001/components/password_man... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/2971783002/diff/60001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:869: TEST_F(PasswordAutofillManagerTest, ShowAllPasswordsOptionOnPasswordField) { On 2017/07/05 17:12:15, vasilii wrote: > A comment? Done. https://codereview.chromium.org/2971783002/diff/60001/components/password_man... components/password_manager/core/browser/password_autofill_manager_unittest.cc:901: On 2017/07/05 17:12:15, vasilii wrote: > I'd split the test here. It would be easier to launch the feature then. Done. https://codereview.chromium.org/2971783002/diff/60001/components/password_man... File components/password_manager/core/common/password_manager_features.cc (right): https://codereview.chromium.org/2971783002/diff/60001/components/password_man... components/password_manager/core/common/password_manager_features.cc:24: "enable-manual-fallbacks-filling", base::FEATURE_DISABLED_BY_DEFAULT}; On 2017/07/05 17:12:15, vasilii wrote: > It's recommended to use CamelCase in feature_list.h Done.
https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:131: row_size += is_warning_message ? kHttpWarningNamePadding : kNamePadding; It's strange that kHttpWarningNamePadding isn't used in the Views code. https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:137: (is_warning_message ? kRightHandSideIconPapping : kIconPadding); Isn't it applicable to the new state too? It seems that you draw NOT_SECURE_WARNING and your icon equally. https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:154: const bool icon_in_front_of_text_row = icon_in_front_of_text? https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:184: ? AutofillPopupLayoutModel::kRightHandSideIconPapping Two questions: - what is papping? - Why "right hand side"? it seems to be the padding between the icon and text for two concrete cases. https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:200: is_rtl ? value_rect.right() - value_width : value_rect.x(); The biggest question I have about all of this is - if (icon_in_front_of_text_row == icon exists) always If yes then line 197 handles all cases. If no then this line is incorrect as the text is drawn on top of the icon. https://codereview.chromium.org/2971783002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/2971783002/diff/120001/components/password_ma... components/password_manager/core/browser/password_autofill_manager_unittest.cc:888: base::string16 show_all_saved_row_text = Unused https://codereview.chromium.org/2971783002/diff/120001/components/password_ma... File components/password_manager/core/common/password_manager_features.cc (right): https://codereview.chromium.org/2971783002/diff/120001/components/password_ma... components/password_manager/core/common/password_manager_features.cc:24: "enableManualFallbacksFilling", base::FEATURE_DISABLED_BY_DEFAULT}; EnableManualFallbacksFilling
The CQ bit was checked by melandory@chromium.org 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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by melandory@chromium.org 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...
Patchset #5 (id:140001) has been deleted
https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:137: (is_warning_message ? kRightHandSideIconPapping : kIconPadding); On 2017/07/06 15:15:21, vasilii wrote: > Isn't it applicable to the new state too? > It seems that you draw NOT_SECURE_WARNING and your icon equally. Done. https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:154: const bool icon_in_front_of_text_row = On 2017/07/06 15:15:21, vasilii wrote: > icon_in_front_of_text? Done. https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:184: ? AutofillPopupLayoutModel::kRightHandSideIconPapping On 2017/07/06 15:15:21, vasilii wrote: > Two questions: > - what is papping? mistyped padding =) > - Why "right hand side"? it seems to be the padding between the icon and text > for two concrete cases. I wanted to emphasize that it's padding for right hand side icon https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:200: is_rtl ? value_rect.right() - value_width : value_rect.x(); On 2017/07/06 15:15:21, vasilii wrote: > The biggest question I have about all of this is > - if (icon_in_front_of_text_row == icon exists) always It seems so to me for now, I'll get a break and look to it on Monday with fresh eye and will ask the owner to double check. > If yes then line 197 handles all cases. If no then this line is incorrect as the > text is drawn on top of the icon. https://codereview.chromium.org/2971783002/diff/120001/components/password_ma... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/2971783002/diff/120001/components/password_ma... components/password_manager/core/browser/password_autofill_manager_unittest.cc:888: base::string16 show_all_saved_row_text = On 2017/07/06 15:15:21, vasilii wrote: > Unused Done. https://codereview.chromium.org/2971783002/diff/120001/components/password_ma... File components/password_manager/core/common/password_manager_features.cc (right): https://codereview.chromium.org/2971783002/diff/120001/components/password_ma... components/password_manager/core/common/password_manager_features.cc:24: "enableManualFallbacksFilling", base::FEATURE_DISABLED_BY_DEFAULT}; On 2017/07/06 15:15:21, vasilii wrote: > EnableManualFallbacksFilling Done.
https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:184: ? AutofillPopupLayoutModel::kRightHandSideIconPapping On 2017/07/07 12:26:31, melandory wrote: > On 2017/07/06 15:15:21, vasilii wrote: > > Two questions: > > - what is papping? > mistyped padding =) > > - Why "right hand side"? it seems to be the padding between the icon and text > > for two concrete cases. > I wanted to emphasize that it's padding for right hand side icon > But the icon is actually on the left side https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:200: is_rtl ? value_rect.right() - value_width : value_rect.x(); On 2017/07/07 12:26:31, melandory wrote: > On 2017/07/06 15:15:21, vasilii wrote: > > The biggest question I have about all of this is > > - if (icon_in_front_of_text_row == icon exists) always > It seems so to me for now, I'll get a break and look to it on Monday with fresh > eye and will ask the owner to double check. > > If yes then line 197 handles all cases. If no then this line is incorrect as > the > > text is drawn on top of the icon. > Then I think this block can be simplified. https://codereview.chromium.org/2971783002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:224: label_x_align_left += is_rtl ? 0 : -label_width; This block is also complicated
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by melandory@chromium.org 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 checked by melandory@chromium.org 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...
Patchset #6 (id:180001) has been deleted
https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:200: is_rtl ? value_rect.right() - value_width : value_rect.x(); On 2017/07/07 12:51:27, vasilii wrote: > On 2017/07/07 12:26:31, melandory wrote: > > On 2017/07/06 15:15:21, vasilii wrote: > > > The biggest question I have about all of this is > > > - if (icon_in_front_of_text_row == icon exists) always > > It seems so to me for now, I'll get a break and look to it on Monday with > fresh > > eye and will ask the owner to double check. > > > If yes then line 197 handles all cases. If no then this line is incorrect as > > the > > > text is drawn on top of the icon. > > > > Then I think this block can be simplified. There is a view where icon is behind the text: https://goo.gl/photos/jfpUnxTEHXGt8sJTA
melandory@chromium.org changed reviewers: + estade@chromium.org
estade@chromium.org: Please review changes in *autofill*
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:125: bool is_row_with_right_handside_icon = it is left side https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:133: if (with_label) {} https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:134: row_size += is_row_with_right_handside_icon ? kHttpWarningNamePadding I don't understand how presence of an icon affects padding between the text and label. https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.h:45: // The amount of horizontal padding around icons in pixels for HTTP warning Update the comment https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.h:47: static const int kLeftSideIconTextPadding = 8; I think that the name is confusing. I feel like it describes some left padding. https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:166: int x_align_left = icon_on_the_right ? value_rect.right() : value_rect.x(); I think the code below 190 is very difficult to understand due to those variables. How about moving them into the block in #170. There we can inset |value_rect| so the icon is basically unknown to the rest of the function.
The CQ bit was checked by melandory@chromium.org 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...
https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:125: bool is_row_with_right_handside_icon = On 2017/07/10 17:17:57, vasilii wrote: > it is left side Done. https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:133: if (with_label) On 2017/07/10 17:17:57, vasilii wrote: > {} Done. https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:134: row_size += is_row_with_right_handside_icon ? kHttpWarningNamePadding On 2017/07/10 17:17:57, vasilii wrote: > I don't understand how presence of an icon affects padding between the text and > label. It's not only presence of the icon, but as I understood it's different for this icon and e.g. credit card icon. https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.h:45: // The amount of horizontal padding around icons in pixels for HTTP warning On 2017/07/10 17:17:57, vasilii wrote: > Update the comment Done. https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.h:47: static const int kLeftSideIconTextPadding = 8; On 2017/07/10 17:17:57, vasilii wrote: > I think that the name is confusing. I feel like it describes some left padding. Did another try to rename it. https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:166: int x_align_left = icon_on_the_right ? value_rect.right() : value_rect.x(); On 2017/07/10 17:17:58, vasilii wrote: > I think the code below 190 is very difficult to understand due to those > variables. How about moving them into the block in #170. There we can inset > |value_rect| so the icon is basically unknown to the rest of the function. I'm not sure that I followed you suggestion correctly, but did my best.
melandory@chromium.org changed reviewers: + boliu@chromium.org
boliu@chromium.org: Please review changes in *android_webview*
On 2017/07/11 16:00:18, melandory wrote: > mailto:boliu@chromium.org: Please review changes in > > > *android_webview* rubberstamp lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
estade@, friendly ping :)
https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/200001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:166: int x_align_left = icon_on_the_right ? value_rect.right() : value_rect.x(); On 2017/07/11 16:00:02, melandory wrote: > On 2017/07/10 17:17:58, vasilii wrote: > > I think the code below 190 is very difficult to understand due to those > > variables. How about moving them into the block in #170. There we can inset > > |value_rect| so the icon is basically unknown to the rest of the function. > > I'm not sure that I followed you suggestion correctly, but did my best. I meant that you move |x_align_left| into the if block if it's even needed there. The rest of the code from line 189 uses only |value_rect|. You can inset that rect if an icon was drawn. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:135: is_row_with_left_handside_icon ? kHttpWarningNamePadding : kNamePadding; Can we also rename |kHttpWarningNamePadding|? https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.h:46: // message and for Show all passwords fallback. Remove a space
sorry I missed this due to some combination of skimming through the post-vacation email backlog, having switched my workflow over to Gerrit, and the assumption that passwords in my inbox = not me. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:125: bool is_row_with_left_handside_icon = nit: const bool also, "lefthand side" would translate to "lefthand_side" in c_hacker_syntax, but to be more technically accurate in the context of a possible RTL layout I think you'd call this something like "icon_positioned_at_start". https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:131: int row_size = kEndPadding; imo this would be easier to follow if you just started with int row_size = 2 * (kEndPadding + kPopupBorderThickness); instead of adding things little by little. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:133: if (with_label) { it's surprising to me there can be rows with no label. After some digging I realize it's because "label" in this case is the subtext after the "value" aka "name". Can we improve the variable names to make this more apparent? "RowWidthWithoutText" is especially confusing to pair with "with_label". Maybe RowWidthWithoutText(int row, bool has_subtext) https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:135: is_row_with_left_handside_icon ? kHttpWarningNamePadding : kNamePadding; On 2017/07/12 13:20:13, vasilii wrote: > Can we also rename |kHttpWarningNamePadding|? technically this is still only used for http warnings, right? Because the show-all-passwords row doesn't have subtext. I would change the condition back to just checking if it's a warning message instead of is_row_with_left_handside_icon. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:225: gfx::ImageSkia AutofillPopupLayoutModel::GetIconImage(size_t index) const { side nit: I believe this interface should return a gfx::Image https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:232: POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE) { nit: why this check? aren't the string comparisons (admittedly quite icky) sufficient? https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:244: kHttpWarningIconWidth, gfx::kChromeIconGrey); nit: kHttpWarningIconWidth is one I'd rename. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:252: #endif nit: can you add what condition this is the end of https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.h:46: // message and for Show all passwords fallback. On 2017/07/12 13:20:13, vasilii wrote: > Remove a space I would rewrite this entire comment to be more generic (as you've done with the variable name) rather than mention the two specific types of row that use it. Also, all these measurements which claim to be in pixels are actually in dip. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc:90: #if !defined(OS_ANDROID) hmm, why do we even compile this file on android? https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:145: void AutofillPopupViewViews::DrawAutofillEntry(gfx::Canvas* canvas, This entire function with all its directionality conditionals is almost totally unreadable. It's not your fault, but every additional change makes it even more complicated and is nefariously difficult to verify correctness for. Is it not possible to do all the calculations in an LTR world and then permute the coordinates for canvas calls at the last second? I'm thinking of promoting the views::View::GetMirrored* functions to some sort of utility that you can call for arbitrary rects. (The bigger task of actually using views instead of this crazy bespoke layout would be nice but is much harder.) https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:153: int current_row_frontend_id = controller_->GetSuggestionAt(index).frontend_id; nit: const also, to shorten the variable name I think it's ok to get rid of "current_row_" https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:169: bool icon_on_the_right = icon_in_front_of_text == is_rtl; nit: const https://codereview.chromium.org/2971783002/diff/220001/components/autofill/co... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/2971783002/diff/220001/components/autofill/co... components/autofill/core/browser/autofill_client.h:205: virtual void ShowPasswordsSettingsPage() = 0; it seems like it would be a bit simpler (you wouldn't have to keep changing this interface and all the multitudinous implementations) if you just had one generic "ExecuteCommand(PopupItemId id)" function. This wouldn't cover every case (credit card scanning requires a callback; some rows aren't actionable; etc), but it would simplify the case of these fire-and-forget actions that map 1:1 with item IDs.
The CQ bit was checked by melandory@chromium.org 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...
https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:125: bool is_row_with_left_handside_icon = On 2017/07/12 17:53:52, Evan Stade wrote: > nit: const bool > > also, "lefthand side" would translate to "lefthand_side" in c_hacker_syntax, but > to be more technically accurate in the context of a possible RTL layout I think > you'd call this something like "icon_positioned_at_start". Done. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:131: int row_size = kEndPadding; On 2017/07/12 17:53:51, Evan Stade wrote: > imo this would be easier to follow if you just started with > > int row_size = 2 * (kEndPadding + kPopupBorderThickness); > > instead of adding things little by little. Done. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:133: if (with_label) { On 2017/07/12 17:53:52, Evan Stade wrote: > it's surprising to me there can be rows with no label. After some digging I > realize it's because "label" in this case is the subtext after the "value" aka > "name". > > Can we improve the variable names to make this more apparent? > "RowWidthWithoutText" is especially confusing to pair with "with_label". > > Maybe > > RowWidthWithoutText(int row, bool has_subtext) Done. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:135: is_row_with_left_handside_icon ? kHttpWarningNamePadding : kNamePadding; On 2017/07/12 17:53:51, Evan Stade wrote: > On 2017/07/12 13:20:13, vasilii wrote: > > Can we also rename |kHttpWarningNamePadding|? > > technically this is still only used for http warnings, right? Because the > show-all-passwords row doesn't have subtext. I would change the condition back > to just checking if it's a warning message instead of > is_row_with_left_handside_icon. Done. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:225: gfx::ImageSkia AutofillPopupLayoutModel::GetIconImage(size_t index) const { On 2017/07/12 17:53:51, Evan Stade wrote: > side nit: I believe this interface should return a gfx::Image will address tomorrow, code search doesn't work for me now and I prefer to use it in order to find what to use instead of CreateVectorIcon in order to get Image instead of ImageSkia. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:232: POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE) { On 2017/07/12 17:53:52, Evan Stade wrote: > nit: why this check? aren't the string comparisons (admittedly quite icky) > sufficient? Done. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:244: kHttpWarningIconWidth, gfx::kChromeIconGrey); On 2017/07/12 17:53:52, Evan Stade wrote: > nit: kHttpWarningIconWidth is one I'd rename. Done. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:252: #endif On 2017/07/12 17:53:51, Evan Stade wrote: > nit: can you add what condition this is the end of Done. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.h:46: // message and for Show all passwords fallback. On 2017/07/12 17:53:52, Evan Stade wrote: > On 2017/07/12 13:20:13, vasilii wrote: > > Remove a space > > I would rewrite this entire comment to be more generic (as you've done with the > variable name) rather than mention the two specific types of row that use it. > > Also, all these measurements which claim to be in pixels are actually in dip. Done. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc:90: #if !defined(OS_ANDROID) On 2017/07/12 17:53:52, Evan Stade wrote: > hmm, why do we even compile this file on android? Done. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:145: void AutofillPopupViewViews::DrawAutofillEntry(gfx::Canvas* canvas, On 2017/07/12 17:53:52, Evan Stade wrote: > This entire function with all its directionality conditionals is almost totally > unreadable. It's not your fault, but every additional change makes it even more > complicated and is nefariously difficult to verify correctness for. Is it not > possible to do all the calculations in an LTR world and then permute the > coordinates for canvas calls at the last second? I'm thinking of promoting the > views::View::GetMirrored* functions to some sort of utility that you can call > for arbitrary rects. > > (The bigger task of actually using views instead of this crazy bespoke layout > would be nice but is much harder.) The initial change to this function was more straight forward: https://codereview.chromium.org/2971783002/diff/60001/chrome/browser/ui/views... And code in this patchset is result of some "simplification" we come up with. Since we have tight deadline for the project which includes "Show all paswords", what I suggest is to revert this file to the initial modification + adding TODO for myself. This way other steps of the project will not be blocked on this CL. We'll have to do something about this code either way, because our mocks include significant changes in how password suggestion is displayed, and this one definitely can't be implemented without simplifying this function.
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_...)
The CQ bit was checked by melandory@chromium.org 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_...)
The CQ bit was checked by melandory@chromium.org 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...
Patchset #9 (id:260001) has been deleted
The CQ bit was checked by melandory@chromium.org 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...
Patchset #9 (id:280001) has been deleted
The CQ bit was checked by melandory@chromium.org 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...
https://codereview.chromium.org/2971783002/diff/220001/components/autofill/co... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/2971783002/diff/220001/components/autofill/co... components/autofill/core/browser/autofill_client.h:205: virtual void ShowPasswordsSettingsPage() = 0; On 2017/07/12 17:53:52, Evan Stade wrote: > it seems like it would be a bit simpler (you wouldn't have to keep changing this > interface and all the multitudinous implementations) if you just had one generic > "ExecuteCommand(PopupItemId id)" function. This wouldn't cover every case > (credit card scanning requires a callback; some rows aren't actionable; etc), > but it would simplify the case of these fire-and-forget actions that map 1:1 > with item IDs. Done.
https://codereview.chromium.org/2971783002/diff/220001/components/autofill/co... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/2971783002/diff/220001/components/autofill/co... components/autofill/core/browser/autofill_client.h:205: virtual void ShowPasswordsSettingsPage() = 0; On 2017/07/12 17:53:52, Evan Stade wrote: > it seems like it would be a bit simpler (you wouldn't have to keep changing this > interface and all the multitudinous implementations) if you just had one generic > "ExecuteCommand(PopupItemId id)" function. This wouldn't cover every case > (credit card scanning requires a callback; some rows aren't actionable; etc), > but it would simplify the case of these fire-and-forget actions that map 1:1 > with item IDs. Done.
Patchset #10 (id:320001) has been deleted
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_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 melandory@chromium.org 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...
Patchset #9 (id:300001) has been deleted
melandory@chromium.org changed reviewers: + mahmadi@chromium.org
PTAL at changes in: ios/chrome/browser/ui/autofill/autofill_client_ios.*
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...)
The CQ bit was checked by melandory@chromium.org 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...
Patchset #9 (id:340001) has been deleted
To estade@ and vasilii@, so it does very minimal modification to the chrome/browser/ui/views/autofill/autofill_popup_view_views.cc. I'd be happy to do refactoring for chrome/browser/ui/views/autofill/autofill_popup_view_views.cc in another CL. WDYT?
On 2017/07/13 13:06:17, melandory wrote: > To estade@ and vasilii@, so it does very minimal modification to the s/it/new patch/ > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc. > > I'd be happy to do refactoring for > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc in another CL. > > > WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
The CQ bit was checked by melandory@chromium.org 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_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
mathp@chromium.org changed reviewers: + mathp@chromium.org
https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:145: void AutofillPopupViewViews::DrawAutofillEntry(gfx::Canvas* canvas, On 2017/07/12 23:13:47, melandory wrote: > On 2017/07/12 17:53:52, Evan Stade wrote: > > This entire function with all its directionality conditionals is almost > totally > > unreadable. It's not your fault, but every additional change makes it even > more > > complicated and is nefariously difficult to verify correctness for. Is it not > > possible to do all the calculations in an LTR world and then permute the > > coordinates for canvas calls at the last second? I'm thinking of promoting the > > views::View::GetMirrored* functions to some sort of utility that you can call > > for arbitrary rects. > > > > (The bigger task of actually using views instead of this crazy bespoke layout > > would be nice but is much harder.) > > The initial change to this function was more straight forward: > https://codereview.chromium.org/2971783002/diff/60001/chrome/browser/ui/views... > > And code in this patchset is result of some "simplification" we come up with. > > Since we have tight deadline for the project which includes "Show all > paswords", what I suggest is to revert this file to the initial modification + > adding TODO for myself. This way other steps of the project will not be blocked > on this CL. > > We'll have to do something about this code either way, because our mocks include > significant changes in how password suggestion is displayed, and this one > definitely can't be implemented without simplifying this function. > > I agree with Evan, we need to rework this function with views. I think we should work together on a refactor of this dropdown that will make this much nicer in the M62 timeframe (and make your future password work much easier). The password generation popup has done it this way. For now there are not many changes to this file, so I wouldn't block this for M61, but it's still weird to land this a few days before branch. https://codereview.chromium.org/2971783002/diff/380001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/380001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:132: if (has_substext) { has_subtext
On 2017/07/13 14:16:04, Mathieu wrote: > https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/view... > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): > > https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/view... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:145: void > AutofillPopupViewViews::DrawAutofillEntry(gfx::Canvas* canvas, > On 2017/07/12 23:13:47, melandory wrote: > > On 2017/07/12 17:53:52, Evan Stade wrote: > > > This entire function with all its directionality conditionals is almost > > totally > > > unreadable. It's not your fault, but every additional change makes it even > > more > > > complicated and is nefariously difficult to verify correctness for. Is it > not > > > possible to do all the calculations in an LTR world and then permute the > > > coordinates for canvas calls at the last second? I'm thinking of promoting > the > > > views::View::GetMirrored* functions to some sort of utility that you can > call > > > for arbitrary rects. > > > > > > (The bigger task of actually using views instead of this crazy bespoke > layout > > > would be nice but is much harder.) > > > > The initial change to this function was more straight forward: > > > https://codereview.chromium.org/2971783002/diff/60001/chrome/browser/ui/views... > > > > And code in this patchset is result of some "simplification" we come up with. > > > > Since we have tight deadline for the project which includes "Show all > > paswords", what I suggest is to revert this file to the initial modification + > > adding TODO for myself. This way other steps of the project will not be > blocked > > on this CL. > > > > We'll have to do something about this code either way, because our mocks > include > > significant changes in how password suggestion is displayed, and this one > > definitely can't be implemented without simplifying this function. > > > > > > I agree with Evan, we need to rework this function with views. > > I think we should work together on a refactor of this dropdown that will make > this much nicer in the M62 timeframe (and make your future password work much > easier). The password generation popup has done it this way. > > For now there are not many changes to this file, so I wouldn't block this for > M61, but it's still weird to land this a few days before branch. This code behind the flag and I have no intention to launch it in M61. The only point I want to make is that I preffer to do refactoring in the separate CL. Basically, the CL which will solely concentrate on refactoring. > > https://codereview.chromium.org/2971783002/diff/380001/chrome/browser/ui/auto... > File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): > > https://codereview.chromium.org/2971783002/diff/380001/chrome/browser/ui/auto... > chrome/browser/ui/autofill/autofill_popup_layout_model.cc:132: if (has_substext) > { > has_subtext
On 2017/07/13 14:18:46, melandory wrote: > On 2017/07/13 14:16:04, Mathieu wrote: > > > https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/view... > > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): > > > > > https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/view... > > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:145: void > > AutofillPopupViewViews::DrawAutofillEntry(gfx::Canvas* canvas, > > On 2017/07/12 23:13:47, melandory wrote: > > > On 2017/07/12 17:53:52, Evan Stade wrote: > > > > This entire function with all its directionality conditionals is almost > > > totally > > > > unreadable. It's not your fault, but every additional change makes it even > > > more > > > > complicated and is nefariously difficult to verify correctness for. Is it > > not > > > > possible to do all the calculations in an LTR world and then permute the > > > > coordinates for canvas calls at the last second? I'm thinking of promoting > > the > > > > views::View::GetMirrored* functions to some sort of utility that you can > > call > > > > for arbitrary rects. > > > > > > > > (The bigger task of actually using views instead of this crazy bespoke > > layout > > > > would be nice but is much harder.) > > > > > > The initial change to this function was more straight forward: > > > > > > https://codereview.chromium.org/2971783002/diff/60001/chrome/browser/ui/views... > > > > > > And code in this patchset is result of some "simplification" we come up > with. > > > > > > Since we have tight deadline for the project which includes "Show all > > > paswords", what I suggest is to revert this file to the initial modification > + > > > adding TODO for myself. This way other steps of the project will not be > > blocked > > > on this CL. > > > > > > We'll have to do something about this code either way, because our mocks > > include > > > significant changes in how password suggestion is displayed, and this one > > > definitely can't be implemented without simplifying this function. > > > > > > > > > > I agree with Evan, we need to rework this function with views. > > > > I think we should work together on a refactor of this dropdown that will make > > this much nicer in the M62 timeframe (and make your future password work much > > easier). The password generation popup has done it this way. > > > > For now there are not many changes to this file, so I wouldn't block this for > > M61, but it's still weird to land this a few days before branch. > This code behind the flag and I have no intention to launch it in M61. The only > point I want to make is that I preffer to do refactoring in the separate CL. > Basically, the CL which will solely concentrate on refactoring. > > > > > https://codereview.chromium.org/2971783002/diff/380001/chrome/browser/ui/auto... > > File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): > > > > > https://codereview.chromium.org/2971783002/diff/380001/chrome/browser/ui/auto... > > chrome/browser/ui/autofill/autofill_popup_layout_model.cc:132: if > (has_substext) > > { > > has_subtext Apologies I missed this distinction! Feel free to reach out when you do get to the refacoteing.
The CQ bit was checked by melandory@chromium.org 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:47: const int kRowWithLeftHandsideIcon = 16; Confusing. The name shouldn't include 'Row' but should include "width" https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.h:35: // in dpi. I think Evan meant Device-independent pixel, not DPI https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:413: ShowHttpNotSecureExplanation(); Should you drop ShowHttpNotSecureExplanation() from the interface and make it non-virtual? https://codereview.chromium.org/2971783002/diff/400001/components/password_ma... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/2971783002/diff/400001/components/password_ma... components/password_manager/core/browser/password_autofill_manager_unittest.cc:643: EXPECT_CALL(*autofill_client, ExecuteCommand(_)) Please specify the concrete id here and below
fine with me to refactor DrawAutofillEntry later. https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:225: gfx::ImageSkia AutofillPopupLayoutModel::GetIconImage(size_t index) const { On 2017/07/12 23:13:47, melandory wrote: > On 2017/07/12 17:53:51, Evan Stade wrote: > > side nit: I believe this interface should return a gfx::Image > > will address tomorrow, code search doesn't work for me now and I prefer to use > it in order to find what to use instead of CreateVectorIcon in order to get > Image instead of ImageSkia. By "side nit" I intended to convey it wasn't important to address as part of this patch but maybe instead in a later cleanup. In the case of CreateVectorIcon you would have to wrap in gfx::Image() which is no big win or loss. But for the resource bundle images, you'd use GetNativeImageNamed() which I believe avoids a conversion from skia image to NSImage for Cocoa. https://codereview.chromium.org/2971783002/diff/400001/chrome/app/vector_icon... File chrome/app/vector_icons/show_all_saved_passwords.icon (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/app/vector_icon... chrome/app/vector_icons/show_all_saved_passwords.icon:5: MOVE_TO, 38, 38, it appears this is the "launch"/"open in new" icon from go/icons. If so please make the file name match (i.e. open_in_new.icon) https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:47: const int kRowWithLeftHandsideIcon = 16; On 2017/07/13 16:11:51, vasilii wrote: > Confusing. The name shouldn't include 'Row' but should include "width" I would define this in the one function where it's relevant and just call it kIconSize https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:134: } nit: no curlies https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.h:35: // in dpi. On 2017/07/13 16:11:51, vasilii wrote: > I think Evan meant Device-independent pixel, not DPI that's right. https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc:90: #if !defined(OS_ANDROID) these android ifdefs no longer necessary https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:380: void ChromeAutofillClient::StartSigninFlow() { I believe you can make this one use ExecuteCommand as well, with id == POPUP_ITEM_ID_CREDIT_CARD_SIGNIN_PROMO https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:410: bool ChromeAutofillClient::ExecuteCommand(int id) { nit: please keep function implementations in the same order as the declarations https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:413: ShowHttpNotSecureExplanation(); On 2017/07/13 16:11:51, vasilii wrote: > Should you drop ShowHttpNotSecureExplanation() from the interface and make it > non-virtual? yes please https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:422: void ChromeAutofillClient::ShowPasswordsSettingsPage() { doesn't seem worth adding this function since it's a one-liner https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:153: int current_row_frontend_id = controller_->GetSuggestionAt(index).frontend_id; same nits as before: const int and no "current_row_" in the name https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:186: ? -AutofillPopupLayoutModel::kPaddingBetweenLeftSideIconAndText nit: can we rename kPaddingBetweenLeftSideIconAndText to PaddingAfterLeadingIcon or something
The CQ bit was checked by melandory@chromium.org 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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by melandory@chromium.org 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...
Patchset #12 (id:420001) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by melandory@chromium.org 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...
Patchset #12 (id:440001) has been deleted
The CQ bit was checked by melandory@chromium.org 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 checked by melandory@chromium.org 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...
Patchset #12 (id:450027) has been deleted
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by melandory@chromium.org 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...
https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/220001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:225: gfx::ImageSkia AutofillPopupLayoutModel::GetIconImage(size_t index) const { On 2017/07/12 23:13:47, melandory wrote: > On 2017/07/12 17:53:51, Evan Stade wrote: > > side nit: I believe this interface should return a gfx::Image > > will address tomorrow, code search doesn't work for me now and I prefer to use > it in order to find what to use instead of CreateVectorIcon in order to get > Image instead of ImageSkia. Could you suggest what method should I use in order to create gfx::Image https://codereview.chromium.org/2971783002/diff/400001/chrome/app/vector_icon... File chrome/app/vector_icons/show_all_saved_passwords.icon (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/app/vector_icon... chrome/app/vector_icons/show_all_saved_passwords.icon:5: MOVE_TO, 38, 38, On 2017/07/13 16:48:43, Evan Stade wrote: > it appears this is the "launch"/"open in new" icon from go/icons. If so please > make the file name match (i.e. open_in_new.icon) Done. https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:47: const int kRowWithLeftHandsideIcon = 16; On 2017/07/13 16:48:43, Evan Stade wrote: > On 2017/07/13 16:11:51, vasilii wrote: > > Confusing. The name shouldn't include 'Row' but should include "width" > > I would define this in the one function where it's relevant and just call it > kIconSize Done. https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:134: } On 2017/07/13 16:48:43, Evan Stade wrote: > nit: no curlies Done. https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.h:35: // in dpi. On 2017/07/13 16:48:43, Evan Stade wrote: > On 2017/07/13 16:11:51, vasilii wrote: > > I think Evan meant Device-independent pixel, not DPI > > that's right. Done. https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc:90: #if !defined(OS_ANDROID) On 2017/07/13 16:48:43, Evan Stade wrote: > these android ifdefs no longer necessary It leads to compilation failured. Compare patchset 12 and 13. Also: https://chromium-review.googlesource.com/#/c/584758/ I can investigate it, preferably in different CL ;) https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:380: void ChromeAutofillClient::StartSigninFlow() { On 2017/07/13 16:48:43, Evan Stade wrote: > I believe you can make this one use ExecuteCommand as well, with id == > POPUP_ITEM_ID_CREDIT_CARD_SIGNIN_PROMO Done. https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:410: bool ChromeAutofillClient::ExecuteCommand(int id) { On 2017/07/13 16:48:43, Evan Stade wrote: > nit: please keep function implementations in the same order as the declarations Done. https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:413: ShowHttpNotSecureExplanation(); On 2017/07/13 16:48:43, Evan Stade wrote: > On 2017/07/13 16:11:51, vasilii wrote: > > Should you drop ShowHttpNotSecureExplanation() from the interface and make it > > non-virtual? > > yes please Done. https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:422: void ChromeAutofillClient::ShowPasswordsSettingsPage() { On 2017/07/13 16:48:43, Evan Stade wrote: > doesn't seem worth adding this function since it's a one-liner Done. https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:153: int current_row_frontend_id = controller_->GetSuggestionAt(index).frontend_id; On 2017/07/13 16:48:43, Evan Stade wrote: > same nits as before: const int and no "current_row_" in the name Done. https://codereview.chromium.org/2971783002/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:186: ? -AutofillPopupLayoutModel::kPaddingBetweenLeftSideIconAndText On 2017/07/13 16:48:43, Evan Stade wrote: > nit: can we rename kPaddingBetweenLeftSideIconAndText to PaddingAfterLeadingIcon > or something Done. https://codereview.chromium.org/2971783002/diff/400001/components/password_ma... File components/password_manager/core/browser/password_autofill_manager_unittest.cc (right): https://codereview.chromium.org/2971783002/diff/400001/components/password_ma... components/password_manager/core/browser/password_autofill_manager_unittest.cc:643: EXPECT_CALL(*autofill_client, ExecuteCommand(_)) On 2017/07/13 16:11:51, vasilii wrote: > Please specify the concrete id here and below Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_chromium_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 melandory@chromium.org 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...
Patchset #14 (id:510001) has been deleted
https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:216: const int kIconSize = 16; constexpr https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:379: password_manager::metrics_util::LogShowedHttpNotSecureExplanation(); You double record this in autofill_external_delegate.cc https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:382: } else if (id == autofill::POPUP_ITEM_ID_ALL_SAVED_PASSWORDS_ENTRY) { no need for "else" https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:392: auto* window = web_contents()->GetNativeView()->GetWindowAndroid(); align https://codereview.chromium.org/2971783002/diff/530001/components/autofill/co... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/2971783002/diff/530001/components/autofill/co... components/autofill/core/browser/autofill_client.h:193: // Handles simple actions for the password fields popups. Is it really limited for passwords? https://codereview.chromium.org/2971783002/diff/530001/components/password_ma... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/2971783002/diff/530001/components/password_ma... components/password_manager/core/browser/password_autofill_manager.cc:339: if (!autofill_client_->ExecuteCommand(identifier)) { I wonder if it's possible that some autofill_client doesn't handle an id but we don't want to autofill is as a suggestion below.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by melandory@chromium.org 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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by melandory@chromium.org 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 checked by melandory@chromium.org 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...
https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:216: const int kIconSize = 16; On 2017/07/26 12:17:36, vasilii wrote: > constexpr Done. https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/chrome_autofill_client.cc (right): https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:379: password_manager::metrics_util::LogShowedHttpNotSecureExplanation(); On 2017/07/26 12:17:36, vasilii wrote: > You double record this in autofill_external_delegate.cc Done. https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:382: } else if (id == autofill::POPUP_ITEM_ID_ALL_SAVED_PASSWORDS_ENTRY) { On 2017/07/26 12:17:36, vasilii wrote: > no need for "else" Done. https://codereview.chromium.org/2971783002/diff/530001/chrome/browser/ui/auto... chrome/browser/ui/autofill/chrome_autofill_client.cc:392: auto* window = web_contents()->GetNativeView()->GetWindowAndroid(); On 2017/07/26 12:17:36, vasilii wrote: > align Done. https://codereview.chromium.org/2971783002/diff/530001/components/autofill/co... File components/autofill/core/browser/autofill_client.h (right): https://codereview.chromium.org/2971783002/diff/530001/components/autofill/co... components/autofill/core/browser/autofill_client.h:193: // Handles simple actions for the password fields popups. On 2017/07/26 12:17:36, vasilii wrote: > Is it really limited for passwords? Done. https://codereview.chromium.org/2971783002/diff/530001/components/password_ma... File components/password_manager/core/browser/password_autofill_manager.cc (right): https://codereview.chromium.org/2971783002/diff/530001/components/password_ma... components/password_manager/core/browser/password_autofill_manager.cc:339: if (!autofill_client_->ExecuteCommand(identifier)) { On 2017/07/26 12:17:36, vasilii wrote: > I wonder if it's possible that some autofill_client doesn't handle an id but we > don't want to autofill is as a suggestion below. So you're proposing to split check for Filling the suggestion and execution the commend. It makes sense I think.
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by melandory@chromium.org 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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by melandory@chromium.org 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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
Patchset #21 (id:670001) has been deleted
The CQ bit was checked by melandory@chromium.org 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...
Patchset #20 (id:650001) has been deleted
Patchset #19 (id:630001) has been deleted
Patchset #18 (id:610001) has been deleted
Patchset #15 (id:550001) has been deleted
Patchset #15 (id:570001) has been deleted
Patchset #15 (id:590001) has been deleted
Patchset #12 (id:470001) has been deleted
mahmadi@ PTAL at ios files.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
estade@ please take another look
lgtm https://codereview.chromium.org/2971783002/diff/690001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/690001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:122: bool has_substext) const { nit: s/substext/subtext/g https://codereview.chromium.org/2971783002/diff/690001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:126: const bool is_row_with_icon_positioned_at_start = nit: variable name is a mouthful. Can we go without "is_row_with_"?
Description was changed from ========== Skeleton for showing "Show all saved passwords row" Shows "Show all saved passwords row" for the case when the password was autofilled and user clicks on the password field for Linux/CrOs/Windows platforms. The appearance of the row is guarded only buy flag. No metrics are added in this CL. BUG=739343 ========== to ========== Skeleton for showing "Show all saved passwords row" Shows "Show all saved passwords row" for the case when the password was autofilled and user clicks on the password field for Linux/CrOs/Windows platforms. The appearance of the row is guarded only buy flag. No metrics are added in this CL. TBR=mahmadi@chromium.org BUG=739343 ==========
https://codereview.chromium.org/2971783002/diff/690001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2971783002/diff/690001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:122: bool has_substext) const { On 2017/07/27 18:17:57, Evan Stade wrote: > nit: s/substext/subtext/g Done. https://codereview.chromium.org/2971783002/diff/690001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:126: const bool is_row_with_icon_positioned_at_start = On 2017/07/27 18:17:57, Evan Stade wrote: > nit: variable name is a mouthful. Can we go without "is_row_with_"? Done.
The CQ bit was checked by melandory@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from boliu@chromium.org, vasilii@chromium.org, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2971783002/#ps710001 (title: ".")
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": 710001, "attempt_start_ts": 1501237570071420, "parent_rev": "a3969c63cf6cb952a941187bcffc432d82a06bfa", "commit_rev": "04fc429cf5d4c16316402d43c5435c36d09c480f"}
Message was sent while issue was closed.
Description was changed from ========== Skeleton for showing "Show all saved passwords row" Shows "Show all saved passwords row" for the case when the password was autofilled and user clicks on the password field for Linux/CrOs/Windows platforms. The appearance of the row is guarded only buy flag. No metrics are added in this CL. TBR=mahmadi@chromium.org BUG=739343 ========== to ========== Skeleton for showing "Show all saved passwords row" Shows "Show all saved passwords row" for the case when the password was autofilled and user clicks on the password field for Linux/CrOs/Windows platforms. The appearance of the row is guarded only buy flag. No metrics are added in this CL. TBR=mahmadi@chromium.org BUG=739343 Review-Url: https://codereview.chromium.org/2971783002 Cr-Commit-Position: refs/heads/master@{#490365} Committed: https://chromium.googlesource.com/chromium/src/+/04fc429cf5d4c16316402d43c543... ==========
Message was sent while issue was closed.
Committed patchset #15 (id:710001) as https://chromium.googlesource.com/chromium/src/+/04fc429cf5d4c16316402d43c543... |