Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2496683003:
Http Bad: Add a PopupItemId to identify http warning message (Closed)
|
Created:
4 years, 1 month ago by lshang Modified:
4 years, 1 month ago CC:
chromium-reviews, jam, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, tfarina, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, darin-cc_chromium.org, agrieve+watch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org, android-webview-reviews_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHttp Bad: Add a PopupItemId to identify http warning message
Since Http bad warning message has somewhat different UI from other autofill
popup items (see specs of http bad warning message in: go/fns-ui-spec), this CL
adds a new PopupItemId POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE to identify
http warning message. And renamed POPUP_ITEM_ID_WARNING_MESSAGE to
POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE accordingly to be more
specific.
This CL also changes text color of the message to show an example of use this
new PopupItemId in UI code.
BUG=662298, 662297
Committed: https://crrev.com/4d6fb24de5f4de7bf92ca572b7be8c3a05376043
Cr-Commit-Position: refs/heads/master@{#433092}
Patch Set 1 #Patch Set 2 : advise test #Patch Set 3 : minor change #
Total comments: 29
Patch Set 4 : address review comments #
Total comments: 18
Patch Set 5 : add DropdownItemImpl to handle default settings #Patch Set 6 : back iswarning #
Total comments: 9
Patch Set 7 : minor change #
Total comments: 12
Patch Set 8 : update #
Total comments: 20
Patch Set 9 : update #Messages
Total messages: 76 (48 generated)
The CQ bit was checked by lshang@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...
Description was changed from ========== Http Bad: Add a PopupItemId to identify http warning message add item and change color rebase Merge branch 'master' into add_dropdown_to_autofill format change BUG= ========== to ========== Http Bad: Add a PopupItemId to identify http warning message Since Http bad warning message has a quite different UI from other autofill popup items (see specs of http bad warning message in: go/fns-ui-spec), this CL adds a new PopupItemId to identify http warning message. Also changes text color of the message to indicate an example of use this new PopupItemId in UI code. BUG=662298, 662297 ==========
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...)
Description was changed from ========== Http Bad: Add a PopupItemId to identify http warning message Since Http bad warning message has a quite different UI from other autofill popup items (see specs of http bad warning message in: go/fns-ui-spec), this CL adds a new PopupItemId to identify http warning message. Also changes text color of the message to indicate an example of use this new PopupItemId in UI code. BUG=662298, 662297 ========== to ========== Http Bad: Add a PopupItemId to identify http warning message Since Http bad warning message has a quite different UI from other autofill popup items (see specs of http bad warning message in: go/fns-ui-spec), this CL adds a new PopupItemId to identify http warning message. Also changes text color of the message to indicate an example of use this new PopupItemId in UI code. BUG=662298, 662297 ==========
Description was changed from ========== Http Bad: Add a PopupItemId to identify http warning message Since Http bad warning message has a quite different UI from other autofill popup items (see specs of http bad warning message in: go/fns-ui-spec), this CL adds a new PopupItemId to identify http warning message. Also changes text color of the message to indicate an example of use this new PopupItemId in UI code. BUG=662298, 662297 ========== to ========== Http Bad: Add a PopupItemId to identify http warning message Since Http bad warning message has somewhat different UI from other autofill popup items (see specs of http bad warning message in: go/fns-ui-spec), this CL adds a new PopupItemId to identify http warning message. Also changes text color of the message to indicate an example of use this new PopupItemId in UI code. BUG=662298, 662297 ==========
Description was changed from ========== Http Bad: Add a PopupItemId to identify http warning message Since Http bad warning message has somewhat different UI from other autofill popup items (see specs of http bad warning message in: go/fns-ui-spec), this CL adds a new PopupItemId to identify http warning message. Also changes text color of the message to indicate an example of use this new PopupItemId in UI code. BUG=662298, 662297 ========== to ========== Http Bad: Add a PopupItemId to identify http warning message Since Http bad warning message has somewhat different UI from other autofill popup items (see specs of http bad warning message in: go/fns-ui-spec), this CL adds a new PopupItemId to identify http warning message. Also changes text color of the message to show an example of use this new PopupItemId in UI code. BUG=662298, 662297 ==========
lshang@chromium.org changed reviewers: + mathp@chromium.org
Hey Mathieu, could you take a look at this CL? I added a new PopupItemId for the http warning message and tried to change the text color on both Android and Views. Thanks!
The CQ bit was checked by lshang@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_chromeos_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 lshang@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: This issue passed the CQ dry run.
mathp@chromium.org changed reviewers: + csashi@google.com
Thanks, some comments! Adding +csashi for the design question on Android. https://codereview.chromium.org/2496683003/diff/20002/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_controller.h (right): https://codereview.chromium.org/2496683003/diff/20002/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_controller.h:63: virtual SkColor GetTextColorForRow(int index) const = 0; would name GetValueFontColorForRow and ideally would go in the AutofillPopupLayoutModel (also bring along GetBackgroundColorForRow if possible) https://codereview.chromium.org/2496683003/diff/20002/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2496683003/diff/20002/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:291: return suggestions_[index].frontend_id == POPUP_ITEM_ID_WARNING_MESSAGE || this warning item id now becomes oddly non-specific. Please rename to POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE or something. https://codereview.chromium.org/2496683003/diff/20002/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:396: return IsWarning(index) ? kLabelTextColor : kValueTextColor; Remove the IsWarning function completely (it was only used in one place and GetValueFontColorForRow can replace its functionality. switch (suggestions_[index].frontend_id) { case POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE: return kHttpsWarningTextColor; case POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE: return kLabelTextColor; default: return kValueTextColor; } https://codereview.chromium.org/2496683003/diff/20002/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:475: id != POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE; shouldn't the row be clickable? if so, but a TODO to change this https://codereview.chromium.org/2496683003/diff/20002/components/autofill/and... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java (right): https://codereview.chromium.org/2496683003/diff/20002/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:72: public boolean isHttpWarningMessage() { Too specific. Let me try to make a suggestion... Instead of this, could it be better to have @Override methods from DropdownItem that are more generic? (FYI Label means the "value" in this file and Sublabel means "label") @Override public int getLabelFontColor() { // look at mSuggestionId to make a decision } @Override public isBiggerSublabelFontSize() { // same } That way you can use those methods in DropdownAdapter without referencing Autofill functionality. +csashi to get his opinion because he is looking at this code as well https://codereview.chromium.org/2496683003/diff/20002/components/autofill/cor... File components/autofill/core/browser/autofill_external_delegate.cc (right): https://codereview.chromium.org/2496683003/diff/20002/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:34: // Warning message should display on top of suggestion list. nit: Warning messages* https://codereview.chromium.org/2496683003/diff/20002/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:297: if (unique_id == POPUP_ITEM_ID_WARNING_MESSAGE || reuse IsAutofillWarningEntry()? https://codereview.chromium.org/2496683003/diff/20002/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2496683003/diff/20002/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:581: autofill::POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE; not need for autofill:: https://codereview.chromium.org/2496683003/diff/20002/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/2496683003/diff/20002/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:1632: "", "", -10), let's use proper popup_item_ids values. https://codereview.chromium.org/2496683003/diff/20002/ui/android/java/res/val... File ui/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2496683003/diff/20002/ui/android/java/res/val... ui/android/java/res/values/colors.xml:12: <color name="http_bad_warning_message_text">#c93929</color> to be consistent with other colors here, let's have the color in capital letters i.e. #C93929 https://codereview.chromium.org/2496683003/diff/20002/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/DropdownItem.java (right): https://codereview.chromium.org/2496683003/diff/20002/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/DropdownItem.java:41: boolean isHttpWarningMessage(); This is too specific for DropdownItem, SelectPopupItem and DropdownAdapter which are not necessarily Autofill-related.
https://codereview.chromium.org/2496683003/diff/20002/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java (right): https://codereview.chromium.org/2496683003/diff/20002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:128: * @param isHttpWarningMeddage Whether the item is a HTTP bad warning message. s/Meddage/Message https://codereview.chromium.org/2496683003/diff/20002/components/autofill/and... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java (right): https://codereview.chromium.org/2496683003/diff/20002/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:72: public boolean isHttpWarningMessage() { On 2016/11/12 12:52:05, Mathieu Perreault wrote: > Too specific. Let me try to make a suggestion... Instead of this, could it be > better to have @Override methods from DropdownItem that are more generic? > > (FYI Label means the "value" in this file and Sublabel means "label") > > @Override > public int getLabelFontColor() { > // look at mSuggestionId to make a decision > } > > @Override > public isBiggerSublabelFontSize() { > // same > } > > That way you can use those methods in DropdownAdapter without referencing > Autofill functionality. > > +csashi to get his opinion because he is looking at this code as well For context, I am working on experimenting with credit card auto fill popups (crbug.com/664367). For this experiment, I was planning to add interfaces such that Dropdown Adapter is not cognizant of whether we are in a credit card form (along the same lines as Mathieu's suggestion). I was planning on making AutofillPopupLayoutModel aware of whether the popup layout is for a credit card. https://codereview.chromium.org/2496683003/diff/20002/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right): https://codereview.chromium.org/2496683003/diff/20002/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/DropdownAdapter.java:105: labelView.setTextSize(14); Should the text size be in resources too?
Patchset #4 (id:50001) has been deleted
https://codereview.chromium.org/2496683003/diff/20002/chrome/android/java/src... File chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java (right): https://codereview.chromium.org/2496683003/diff/20002/chrome/android/java/src... chrome/android/java/src/org/chromium/chrome/browser/autofill/AutofillPopupBridge.java:128: * @param isHttpWarningMeddage Whether the item is a HTTP bad warning message. On 2016/11/12 22:45:55, csashi wrote: > s/Meddage/Message Done. https://codereview.chromium.org/2496683003/diff/20002/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_controller.h (right): https://codereview.chromium.org/2496683003/diff/20002/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_controller.h:63: virtual SkColor GetTextColorForRow(int index) const = 0; On 2016/11/12 12:52:05, Mathieu Perreault wrote: > would name GetValueFontColorForRow and ideally would go in the > AutofillPopupLayoutModel (also bring along GetBackgroundColorForRow if possible) Renamed GetValueFontColorForRow and moved it into layout model, but GetBackgroundColorForRow can't be moved because it depends on selected_line_ in controller. https://codereview.chromium.org/2496683003/diff/20002/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2496683003/diff/20002/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:291: return suggestions_[index].frontend_id == POPUP_ITEM_ID_WARNING_MESSAGE || On 2016/11/12 12:52:05, Mathieu Perreault wrote: > this warning item id now becomes oddly non-specific. Please rename to > POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE or something. Done. https://codereview.chromium.org/2496683003/diff/20002/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:396: return IsWarning(index) ? kLabelTextColor : kValueTextColor; On 2016/11/12 12:52:05, Mathieu Perreault wrote: > Remove the IsWarning function completely (it was only used in one place and > GetValueFontColorForRow can replace its functionality. > > switch (suggestions_[index].frontend_id) { > case POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE: > return kHttpsWarningTextColor; > case POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE: > return kLabelTextColor; > default: > return kValueTextColor; > } Done. https://codereview.chromium.org/2496683003/diff/20002/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:475: id != POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE; On 2016/11/12 12:52:05, Mathieu Perreault wrote: > shouldn't the row be clickable? if so, but a TODO to change this Yeah we need it to direct to a help center page. A TODO is added here. https://codereview.chromium.org/2496683003/diff/20002/components/autofill/and... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java (right): https://codereview.chromium.org/2496683003/diff/20002/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:72: public boolean isHttpWarningMessage() { On 2016/11/12 12:52:05, Mathieu Perreault wrote: > Too specific. Let me try to make a suggestion... Instead of this, could it be > better to have @Override methods from DropdownItem that are more generic? > > (FYI Label means the "value" in this file and Sublabel means "label") > > @Override > public int getLabelFontColor() { > // look at mSuggestionId to make a decision > } > > @Override > public isBiggerSublabelFontSize() { > // same > } > > That way you can use those methods in DropdownAdapter without referencing > Autofill functionality. > > +csashi to get his opinion because he is looking at this code as well I try to add interfaces to get font color and font size from here, and use id references defined in colors.xml and dimens.xml, but compiler complains about symbol not found because those resources are in ui/android/java/res/. I'm not sure how to get resources form another root directory. Any idea of fixing this? I would also suggest use a simple e.g. getDropdownType() in DropdownItem and adjust UI in DropdownAdapter according to different types. I think that might makes code simpler because according to the autofill http warning message mocks (https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Form%20Not%...) we might have several places to adjust, like paddings, position of icons, which need to add several more interfaces here. But I don't have enough backgrounds of dropdowns so I'm happy to do the way as you and csashi@ strongly prefer :-) https://codereview.chromium.org/2496683003/diff/20002/components/autofill/cor... File components/autofill/core/browser/autofill_external_delegate.cc (right): https://codereview.chromium.org/2496683003/diff/20002/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:34: // Warning message should display on top of suggestion list. On 2016/11/12 12:52:05, Mathieu Perreault wrote: > nit: Warning messages* Done. https://codereview.chromium.org/2496683003/diff/20002/components/autofill/cor... components/autofill/core/browser/autofill_external_delegate.cc:297: if (unique_id == POPUP_ITEM_ID_WARNING_MESSAGE || On 2016/11/12 12:52:05, Mathieu Perreault wrote: > reuse IsAutofillWarningEntry()? Done. https://codereview.chromium.org/2496683003/diff/20002/components/autofill/cor... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2496683003/diff/20002/components/autofill/cor... components/autofill/core/browser/autofill_manager.cc:581: autofill::POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE; On 2016/11/12 12:52:05, Mathieu Perreault wrote: > not need for autofill:: Done. https://codereview.chromium.org/2496683003/diff/20002/components/autofill/cor... File components/autofill/core/browser/autofill_manager_unittest.cc (right): https://codereview.chromium.org/2496683003/diff/20002/components/autofill/cor... components/autofill/core/browser/autofill_manager_unittest.cc:1632: "", "", -10), On 2016/11/12 12:52:05, Mathieu Perreault wrote: > let's use proper popup_item_ids values. Done. https://codereview.chromium.org/2496683003/diff/20002/ui/android/java/res/val... File ui/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2496683003/diff/20002/ui/android/java/res/val... ui/android/java/res/values/colors.xml:12: <color name="http_bad_warning_message_text">#c93929</color> On 2016/11/12 12:52:05, Mathieu Perreault wrote: > to be consistent with other colors here, let's have the color in capital letters > i.e. #C93929 Done. https://codereview.chromium.org/2496683003/diff/20002/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/DropdownAdapter.java (right): https://codereview.chromium.org/2496683003/diff/20002/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/DropdownAdapter.java:105: labelView.setTextSize(14); On 2016/11/12 22:45:55, csashi wrote: > Should the text size be in resources too? Done. https://codereview.chromium.org/2496683003/diff/20002/ui/android/java/src/org... File ui/android/java/src/org/chromium/ui/DropdownItem.java (right): https://codereview.chromium.org/2496683003/diff/20002/ui/android/java/src/org... ui/android/java/src/org/chromium/ui/DropdownItem.java:41: boolean isHttpWarningMessage(); On 2016/11/12 12:52:05, Mathieu Perreault wrote: > This is too specific for DropdownItem, SelectPopupItem and DropdownAdapter which > are not necessarily Autofill-related. > As above, how about use a getType()?
Hi Liu, I prefer what you have here because it's less specific to Autofill in the Dropdown classes. I'll defer to csashi for the vision long-term. https://codereview.chromium.org/2496683003/diff/20002/components/autofill/and... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java (right): https://codereview.chromium.org/2496683003/diff/20002/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:72: public boolean isHttpWarningMessage() { On 2016/11/14 10:51:01, lshang wrote: > On 2016/11/12 12:52:05, Mathieu Perreault wrote: > > Too specific. Let me try to make a suggestion... Instead of this, could it be > > better to have @Override methods from DropdownItem that are more generic? > > > > (FYI Label means the "value" in this file and Sublabel means "label") > > > > @Override > > public int getLabelFontColor() { > > // look at mSuggestionId to make a decision > > } > > > > @Override > > public isBiggerSublabelFontSize() { > > // same > > } > > > > That way you can use those methods in DropdownAdapter without referencing > > Autofill functionality. > > > > +csashi to get his opinion because he is looking at this code as well > > I try to add interfaces to get font color and font size from here, and use id > references defined in colors.xml and dimens.xml, but compiler complains about > symbol not found because those resources are in ui/android/java/res/. I'm not > sure how to get resources form another root directory. Any idea of fixing this? > > I would also suggest use a simple e.g. getDropdownType() in DropdownItem and > adjust UI in DropdownAdapter according to different types. I think that might > makes code simpler because according to the autofill http warning message mocks > (https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Form%20Not%...) > we might have several places to adjust, like paddings, position of icons, which > need to add several more interfaces here. But I don't have enough backgrounds of > dropdowns so I'm happy to do the way as you and csashi@ strongly prefer :-) In order to access dimens and color from components/autofill, perhaps you can copy what was done using this file, in keyboard accessory view? https://cs.chromium.org/chromium/src/components/autofill/android/java/res/val... Regarding using resources from ui/android/...xml in components/autofill, I'm not sure. https://codereview.chromium.org/2496683003/diff/70001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2496683003/diff/70001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:194: nit: remove extra line https://codereview.chromium.org/2496683003/diff/70001/components/autofill/and... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java (right): https://codereview.chromium.org/2496683003/diff/70001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:78: } else { no else needed when you have returned https://codereview.chromium.org/2496683003/diff/70001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:79: return R.drawable.dropdown_label_color; perhaps you can call the super class's getLabelFontColor? https://codereview.chromium.org/2496683003/diff/70001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:86: return R.dimen.dropdown_item_smaller_label_size; This you would have to copy in your own dimens.xml file in components. https://codereview.chromium.org/2496683003/diff/70001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:87: } else { no else needed https://codereview.chromium.org/2496683003/diff/70001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:88: return R.dimen.dropdown_item_label_size; could we call the super class here? https://codereview.chromium.org/2496683003/diff/70001/ui/android/java/res/lay... File ui/android/java/res/layout/dropdown_item.xml (right): https://codereview.chromium.org/2496683003/diff/70001/ui/android/java/res/lay... ui/android/java/res/layout/dropdown_item.xml:48: android:textSize="14sp" /> use the variable you just defined? dropdown_item_smaller_label_font_size https://codereview.chromium.org/2496683003/diff/70001/ui/android/java/res/val... File ui/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2496683003/diff/70001/ui/android/java/res/val... ui/android/java/res/values/colors.xml:12: <color name="http_bad_warning_message_text">#C93929</color> ideally this wouldn't be in this file because it's too specific for ui/android/... https://codereview.chromium.org/2496683003/diff/70001/ui/android/java/res/val... File ui/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2496683003/diff/70001/ui/android/java/res/val... ui/android/java/res/values/dimens.xml:10: <dimen name="dropdown_item_label_size">18sp</dimen> dropdown_item_label_font_size? same below
https://codereview.chromium.org/2496683003/diff/20002/components/autofill/and... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java (right): https://codereview.chromium.org/2496683003/diff/20002/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:72: public boolean isHttpWarningMessage() { On 2016/11/14 14:40:15, Mathieu Perreault wrote: > On 2016/11/14 10:51:01, lshang wrote: > > On 2016/11/12 12:52:05, Mathieu Perreault wrote: > > > Too specific. Let me try to make a suggestion... Instead of this, could it > be > > > better to have @Override methods from DropdownItem that are more generic? > > > > > > (FYI Label means the "value" in this file and Sublabel means "label") > > > > > > @Override > > > public int getLabelFontColor() { > > > // look at mSuggestionId to make a decision > > > } > > > > > > @Override > > > public isBiggerSublabelFontSize() { > > > // same > > > } > > > > > > That way you can use those methods in DropdownAdapter without referencing > > > Autofill functionality. > > > > > > +csashi to get his opinion because he is looking at this code as well > > > > I try to add interfaces to get font color and font size from here, and use id > > references defined in colors.xml and dimens.xml, but compiler complains about > > symbol not found because those resources are in ui/android/java/res/. I'm not > > sure how to get resources form another root directory. Any idea of fixing > this? > > > > I would also suggest use a simple e.g. getDropdownType() in DropdownItem and > > adjust UI in DropdownAdapter according to different types. I think that might > > makes code simpler because according to the autofill http warning message > mocks > > > (https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Form%20Not%...) > > we might have several places to adjust, like paddings, position of icons, > which > > need to add several more interfaces here. But I don't have enough backgrounds > of > > dropdowns so I'm happy to do the way as you and csashi@ strongly prefer :-) > > In order to access dimens and color from components/autofill, perhaps you can > copy what was done using this file, in keyboard accessory view? > https://cs.chromium.org/chromium/src/components/autofill/android/java/res/val... > > Regarding using resources from ui/android/...xml in components/autofill, I'm not > sure. The other suggestions I made regarding calling the super class, if they work and are acceptable, would avoid you needing to access resources from ui/android in this file.
On 2016/11/14 14:41:59, Mathieu Perreault wrote: > https://codereview.chromium.org/2496683003/diff/20002/components/autofill/and... > File > components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java > (right): > > https://codereview.chromium.org/2496683003/diff/20002/components/autofill/and... > components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:72: > public boolean isHttpWarningMessage() { > On 2016/11/14 14:40:15, Mathieu Perreault wrote: > > On 2016/11/14 10:51:01, lshang wrote: > > > On 2016/11/12 12:52:05, Mathieu Perreault wrote: > > > > Too specific. Let me try to make a suggestion... Instead of this, could it > > be > > > > better to have @Override methods from DropdownItem that are more generic? > > > > > > > > (FYI Label means the "value" in this file and Sublabel means "label") > > > > > > > > @Override > > > > public int getLabelFontColor() { > > > > // look at mSuggestionId to make a decision > > > > } > > > > > > > > @Override > > > > public isBiggerSublabelFontSize() { > > > > // same > > > > } > > > > > > > > That way you can use those methods in DropdownAdapter without referencing > > > > Autofill functionality. > > > > > > > > +csashi to get his opinion because he is looking at this code as well > > > > > > I try to add interfaces to get font color and font size from here, and use > id > > > references defined in colors.xml and dimens.xml, but compiler complains > about > > > symbol not found because those resources are in ui/android/java/res/. I'm > not > > > sure how to get resources form another root directory. Any idea of fixing > > this? > > > > > > I would also suggest use a simple e.g. getDropdownType() in DropdownItem and > > > adjust UI in DropdownAdapter according to different types. I think that > might > > > makes code simpler because according to the autofill http warning message > > mocks > > > > > > (https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Form%20Not%...) > > > we might have several places to adjust, like paddings, position of icons, > > which > > > need to add several more interfaces here. But I don't have enough > backgrounds > > of > > > dropdowns so I'm happy to do the way as you and csashi@ strongly prefer :-) > > > > In order to access dimens and color from components/autofill, perhaps you can > > copy what was done using this file, in keyboard accessory view? > > > https://cs.chromium.org/chromium/src/components/autofill/android/java/res/val... > > > > Regarding using resources from ui/android/...xml in components/autofill, I'm > not > > sure. > > The other suggestions I made regarding calling the super class, if they work and > are acceptable, would avoid you needing to access resources from ui/android in > this file. I don't think we can call the super class, because AutofillSuggestion's supe class DropdownItem is an interface. So one way I can think of is to add a class (e.g DropdownItemImpl) which extends DropdownItem and make AutofillSuggestion and SelectPopupItem to extends DropdownItemImpl, and in DropdownItemImpl we handle all the default settings. DropdownItemImpl can also be in ui/android. Another way is to copy all resources in components/autofill/android/java/res and content/public/android/java/res(for SelectPopupItem). WDYT?
Patchset #5 (id:90001) has been deleted
The CQ bit was checked by lshang@chromium.org to run a CQ dry run
https://codereview.chromium.org/2496683003/diff/70001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2496683003/diff/70001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:194: On 2016/11/14 14:40:15, Mathieu Perreault wrote: > nit: remove extra line Done. https://codereview.chromium.org/2496683003/diff/70001/components/autofill/and... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java (right): https://codereview.chromium.org/2496683003/diff/70001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:78: } else { On 2016/11/14 14:40:15, Mathieu Perreault wrote: > no else needed when you have returned Done. https://codereview.chromium.org/2496683003/diff/70001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:79: return R.drawable.dropdown_label_color; On 2016/11/14 14:40:15, Mathieu Perreault wrote: > perhaps you can call the super class's getLabelFontColor? Done. I added a class which implements DropdownItem interface and handle all default settings. So here we can call super methods. https://codereview.chromium.org/2496683003/diff/70001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:86: return R.dimen.dropdown_item_smaller_label_size; On 2016/11/14 14:40:15, Mathieu Perreault wrote: > This you would have to copy in your own dimens.xml file in components. Done. https://codereview.chromium.org/2496683003/diff/70001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:87: } else { On 2016/11/14 14:40:15, Mathieu Perreault wrote: > no else needed Done. https://codereview.chromium.org/2496683003/diff/70001/components/autofill/and... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:88: return R.dimen.dropdown_item_label_size; On 2016/11/14 14:40:15, Mathieu Perreault wrote: > could we call the super class here? Done. https://codereview.chromium.org/2496683003/diff/70001/ui/android/java/res/lay... File ui/android/java/res/layout/dropdown_item.xml (right): https://codereview.chromium.org/2496683003/diff/70001/ui/android/java/res/lay... ui/android/java/res/layout/dropdown_item.xml:48: android:textSize="14sp" /> On 2016/11/14 14:40:15, Mathieu Perreault wrote: > use the variable you just defined? dropdown_item_smaller_label_font_size The smaller font size is used only for autofill popups now, so I added it in components/autofill/'s dimens.xml. https://codereview.chromium.org/2496683003/diff/70001/ui/android/java/res/val... File ui/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2496683003/diff/70001/ui/android/java/res/val... ui/android/java/res/values/colors.xml:12: <color name="http_bad_warning_message_text">#C93929</color> On 2016/11/14 14:40:15, Mathieu Perreault wrote: > ideally this wouldn't be in this file because it's too specific for > ui/android/... Done. Moved it to components/autofill/ https://codereview.chromium.org/2496683003/diff/70001/ui/android/java/res/val... File ui/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2496683003/diff/70001/ui/android/java/res/val... ui/android/java/res/values/dimens.xml:10: <dimen name="dropdown_item_label_size">18sp</dimen> On 2016/11/14 14:40:15, Mathieu Perreault wrote: > dropdown_item_label_font_size? > > same below Done.
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...)
The CQ bit was checked by lshang@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: This issue passed the CQ dry run.
Description was changed from ========== Http Bad: Add a PopupItemId to identify http warning message Since Http bad warning message has somewhat different UI from other autofill popup items (see specs of http bad warning message in: go/fns-ui-spec), this CL adds a new PopupItemId to identify http warning message. Also changes text color of the message to show an example of use this new PopupItemId in UI code. BUG=662298, 662297 ========== to ========== Http Bad: Add a PopupItemId to identify http warning message Since Http bad warning message has somewhat different UI from other autofill popup items (see specs of http bad warning message in: go/fns-ui-spec), this CL adds a new PopupItemId POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE to identify http warning message. And renamed POPUP_ITEM_ID_WARNING_MESSAGE to POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE accordingly to be more specific. This CL also changes text color of the message to show an example of use this new PopupItemId in UI code. BUG=662298, 662297 ==========
https://codereview.chromium.org/2496683003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (left): https://codereview.chromium.org/2496683003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:84: controller_->IsWarning(index) ? kLabelTextColor : kValueTextColor, Didn't remove IsWarning() because it is also used on Mac[1]. [1]:https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm?q=IsWarning&sq=package:chromium&l=199&dr=C
On 2016/11/15 00:14:04, lshang wrote: > On 2016/11/14 14:41:59, Mathieu Perreault wrote: > > > https://codereview.chromium.org/2496683003/diff/20002/components/autofill/and... > > File > > > components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java > > (right): > > > > > https://codereview.chromium.org/2496683003/diff/20002/components/autofill/and... > > > components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:72: > > public boolean isHttpWarningMessage() { > > On 2016/11/14 14:40:15, Mathieu Perreault wrote: > > > On 2016/11/14 10:51:01, lshang wrote: > > > > On 2016/11/12 12:52:05, Mathieu Perreault wrote: > > > > > Too specific. Let me try to make a suggestion... Instead of this, could > it > > > be > > > > > better to have @Override methods from DropdownItem that are more > generic? > > > > > > > > > > (FYI Label means the "value" in this file and Sublabel means "label") > > > > > > > > > > @Override > > > > > public int getLabelFontColor() { > > > > > // look at mSuggestionId to make a decision > > > > > } > > > > > > > > > > @Override > > > > > public isBiggerSublabelFontSize() { > > > > > // same > > > > > } > > > > > > > > > > That way you can use those methods in DropdownAdapter without > referencing > > > > > Autofill functionality. > > > > > > > > > > +csashi to get his opinion because he is looking at this code as well > > > > > > > > I try to add interfaces to get font color and font size from here, and use > > id > > > > references defined in colors.xml and dimens.xml, but compiler complains > > about > > > > symbol not found because those resources are in ui/android/java/res/. I'm > > not > > > > sure how to get resources form another root directory. Any idea of fixing > > > this? > > > > > > > > I would also suggest use a simple e.g. getDropdownType() in DropdownItem > and > > > > adjust UI in DropdownAdapter according to different types. I think that > > might > > > > makes code simpler because according to the autofill http warning message > > > mocks > > > > > > > > > > (https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Form%20Not%...) > > > > we might have several places to adjust, like paddings, position of icons, > > > which > > > > need to add several more interfaces here. But I don't have enough > > backgrounds > > > of > > > > dropdowns so I'm happy to do the way as you and csashi@ strongly prefer > :-) > > > > > > In order to access dimens and color from components/autofill, perhaps you > can > > > copy what was done using this file, in keyboard accessory view? > > > > > > https://cs.chromium.org/chromium/src/components/autofill/android/java/res/val... > > > > > > Regarding using resources from ui/android/...xml in components/autofill, I'm > > not > > > sure. > > > > The other suggestions I made regarding calling the super class, if they work > and > > are acceptable, would avoid you needing to access resources from ui/android in > > this file. > > I don't think we can call the super class, because AutofillSuggestion's supe > class DropdownItem is an interface. > > So one way I can think of is to add a class (e.g DropdownItemImpl) which extends > DropdownItem and make AutofillSuggestion and SelectPopupItem to extends > DropdownItemImpl, and in DropdownItemImpl we handle all the default settings. > DropdownItemImpl can also be in ui/android. > > Another way is to copy all resources in components/autofill/android/java/res and > content/public/android/java/res(for SelectPopupItem). > > WDYT? It's not a bad solution. In this case I would prefer calling it DropdownItemBase, which makes it more clear that it has a base implementation for some things. I
Thanks for your patience, almost there. Please engage with the Android reviewer as well to validate the class structure. https://codereview.chromium.org/2496683003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (left): https://codereview.chromium.org/2496683003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:84: controller_->IsWarning(index) ? kLabelTextColor : kValueTextColor, On 2016/11/15 07:25:19, lshang wrote: > Didn't remove IsWarning() because it is also used on Mac[1]. > > [1]:https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm?q=IsWarning&sq=package:chromium&l=199&dr=C Can you make a change (now or later) to use GetValueFontColorForRow on Mac? You can transfer the SkColor to NSColor https://cs.chromium.org/chromium/src/skia/ext/skia_utils_mac.h?rcl=0&l=64 https://codereview.chromium.org/2496683003/diff/130001/components/autofill/an... File components/autofill/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2496683003/diff/130001/components/autofill/an... components/autofill/android/java/res/values/colors.xml:8: <color name="http_bad_warning_message_text">#C93929</color> Let's have a <!-- comment --> https://codereview.chromium.org/2496683003/diff/130001/components/autofill/an... File components/autofill/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2496683003/diff/130001/components/autofill/an... components/autofill/android/java/res/values/dimens.xml:19: <dimen name="dropdown_item_smaller_label_font_size">14sp</dimen> Let's put a <!-- comment --> to give an example for when this is used https://codereview.chromium.org/2496683003/diff/130001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownItemImpl.java (right): https://codereview.chromium.org/2496683003/diff/130001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownItemImpl.java:8: * DropdownItem implementation used to get default settings to show the item. Add a note that subclassing is the proper way to use this.
The CQ bit was checked by lshang@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...
lshang@chromium.org changed reviewers: + tedchoc@chromium.org
Thanks Mathieu! +tedchoc@ for Android changes. PTAL thanks! https://codereview.chromium.org/2496683003/diff/130001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (left): https://codereview.chromium.org/2496683003/diff/130001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:84: controller_->IsWarning(index) ? kLabelTextColor : kValueTextColor, On 2016/11/16 05:09:46, Mathieu Perreault wrote: > On 2016/11/15 07:25:19, lshang wrote: > > Didn't remove IsWarning() because it is also used on Mac[1]. > > > > > [1]:https://cs.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm?q=IsWarning&sq=package:chromium&l=199&dr=C > > Can you make a change (now or later) to use GetValueFontColorForRow on Mac? You > can transfer the SkColor to NSColor > https://cs.chromium.org/chromium/src/skia/ext/skia_utils_mac.h?rcl=0&l=64 A TODO is added. I'll address this in a separate CL. https://codereview.chromium.org/2496683003/diff/130001/components/autofill/an... File components/autofill/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2496683003/diff/130001/components/autofill/an... components/autofill/android/java/res/values/colors.xml:8: <color name="http_bad_warning_message_text">#C93929</color> On 2016/11/16 05:09:46, Mathieu Perreault wrote: > Let's have a <!-- comment --> Done. https://codereview.chromium.org/2496683003/diff/130001/components/autofill/an... File components/autofill/android/java/res/values/dimens.xml (right): https://codereview.chromium.org/2496683003/diff/130001/components/autofill/an... components/autofill/android/java/res/values/dimens.xml:19: <dimen name="dropdown_item_smaller_label_font_size">14sp</dimen> On 2016/11/16 05:09:46, Mathieu Perreault wrote: > Let's put a <!-- comment --> to give an example for when this is used Done. https://codereview.chromium.org/2496683003/diff/130001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownItemImpl.java (right): https://codereview.chromium.org/2496683003/diff/130001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownItemImpl.java:8: * DropdownItem implementation used to get default settings to show the item. On 2016/11/16 05:09:46, Mathieu Perreault wrote: > Add a note that subclassing is the proper way to use this. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, lgtm https://codereview.chromium.org/2496683003/diff/150001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://codereview.chromium.org/2496683003/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:199: // TODO(lshang): Use AutofillPopupLayoutModel::GetValueFontColorForRow() nit: the format for TODO is TODO(crbug.com/xxxxxx) https://codereview.chromium.org/2496683003/diff/150001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SelectPopupItem.java (right): https://codereview.chromium.org/2496683003/diff/150001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SelectPopupItem.java:48: public boolean isMultilineLabel() { this is already the base implementation. Should we remove (and perhaps others too)?
lgtm https://codereview.chromium.org/2496683003/diff/150001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2496683003/diff/150001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:185: switch (suggestions[index].frontend_id) { Because you only need the frontend_id would it be better to add a method to the delegate like so: GetFrontendId(int index); Or, have a variation of GetSuggestions that returns a const ref. (we use GetSuggestions in other places also). May be compiler optimizes this call too but I am not sure. https://codereview.chromium.org/2496683003/diff/150001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2496683003/diff/150001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:573: POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE; Unrelated to this change, but may be have a suggestion constructor that takes string and frontend_id? There are many places where we create suggestion and immediately fill in frontend_id. https://codereview.chromium.org/2496683003/diff/150001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SelectPopupItem.java (right): https://codereview.chromium.org/2496683003/diff/150001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SelectPopupItem.java:60: } Do you need these 2? https://codereview.chromium.org/2496683003/diff/150001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownItemBase.java (right): https://codereview.chromium.org/2496683003/diff/150001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownItemBase.java:10: * customized dropdown items, see AutofillSeggestion as an example. s/Seggestion/Suggestion
The CQ bit was checked by lshang@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...
Thanks! https://codereview.chromium.org/2496683003/diff/150001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2496683003/diff/150001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:185: switch (suggestions[index].frontend_id) { On 2016/11/17 01:41:31, csashi wrote: > Because you only need the frontend_id would it be better to add a method to the > delegate like so: > GetFrontendId(int index); > Or, have a variation of GetSuggestions that returns a const ref. (we use > GetSuggestions in other places also). May be compiler optimizes this call too > but I am not sure. Yes GetSuggestions() is used all over this file to get the frontend_id, so I think it should be fine to do this. My guess is the suggestion list won't be too long so it's ok to return the vector here. https://codereview.chromium.org/2496683003/diff/150001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://codereview.chromium.org/2496683003/diff/150001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:199: // TODO(lshang): Use AutofillPopupLayoutModel::GetValueFontColorForRow() On 2016/11/16 22:29:36, Mathieu Perreault wrote: > nit: the format for TODO is TODO(crbug.com/xxxxxx) Done. https://codereview.chromium.org/2496683003/diff/150001/components/autofill/co... File components/autofill/core/browser/autofill_manager.cc (right): https://codereview.chromium.org/2496683003/diff/150001/components/autofill/co... components/autofill/core/browser/autofill_manager.cc:573: POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE; On 2016/11/17 01:41:31, csashi wrote: > Unrelated to this change, but may be have a suggestion constructor that takes > string and frontend_id? There are many places where we create suggestion and > immediately fill in frontend_id. I moved this block of code into an method in the following CL, cause by then we need to handle icon and label as well, so it's better to move the block into a method in the anonymous namespace. Suggestion has a constructor which takes in all value, label, icon and frontend_id, but don't have constructor which takes only value and frontend_id. I think create suggestion and immediately fill in frontend_id is fine here, it doesn't make too much difference in performance and it's more clear to indicate the frontend_id:-) https://codereview.chromium.org/2496683003/diff/150001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SelectPopupItem.java (right): https://codereview.chromium.org/2496683003/diff/150001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SelectPopupItem.java:48: public boolean isMultilineLabel() { On 2016/11/16 22:29:36, Mathieu Perreault wrote: > this is already the base implementation. Should we remove (and perhaps others > too)? Done. https://codereview.chromium.org/2496683003/diff/150001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SelectPopupItem.java:60: } On 2016/11/17 01:41:31, csashi wrote: > Do you need these 2? Done. Removed. https://codereview.chromium.org/2496683003/diff/150001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownItemBase.java (right): https://codereview.chromium.org/2496683003/diff/150001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownItemBase.java:10: * customized dropdown items, see AutofillSeggestion as an example. On 2016/11/17 01:41:31, csashi wrote: > s/Seggestion/Suggestion Done.
lshang@chromium.org changed reviewers: + dvadym@chromium.org, msw@chromium.org
msw@chromium.org: Please OWNER review changes in chrome/browser/ui/ dvadym@chromium.org: Please OWNER review changes in passwoed_autofill_manager.cc tedchoc@chromium.org: Please OWNER review changes in ui/android/ content/public/android/ Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
password_autofill_manager.cc LGTM
https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... File components/autofill/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... components/autofill/android/java/res/values/colors.xml:9: <color name="http_bad_warning_message_text">#C93929</color> I think you want: #c53929 Same as google_red_700 in chrome/android/java/res/values/colors.xml as well as the native implementation. https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java (right): https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:17: private static final int ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE = -10; We support generating java constants from a C++ enum, so we should do that instead of manually requiring these to be in sync. I believe this is relatively up to date: https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/3xr44Rk_cHQ https://codereview.chromium.org/2496683003/diff/170001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownItem.java (right): https://codereview.chromium.org/2496683003/diff/170001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownItem.java:41: int getLabelFontColor(); For this and getLabelFontSize, I would add the suffix ResId (resource ID for short). Both colors and font sizes can be presented as integers, so this could cause user confusion if they were to return Color.RED and it wouldn't work as there is no resource ID associated with it. I would also update the comments to make it even more abundantly clear that this is a android resource ID. https://codereview.chromium.org/2496683003/diff/170001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownItemBase.java (right): https://codereview.chromium.org/2496683003/diff/170001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownItemBase.java:9: * show the item. Subclassing is the proper way to use this class to create I think the Subclassing sentence is unnecessary (and in particular the reference to AutofillSuggestion is likely to get out of date due to renames). The name of DropdownItemBase is clear enough for me.
https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:162: case POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE: nit: reorder after POPUP_ITEM_ID_SEPARATOR to better match enum ordering. https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/popup_constants.h (right): https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/auto... chrome/browser/ui/autofill/popup_constants.h:15: // Various colors used in the Autofill popup. These ought to be replaced by getting colors from ui::NativeTheme; please follow up. (it's important for consistency and accessibility). https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/auto... chrome/browser/ui/autofill/popup_constants.h:19: const SkColor kHttpWarningTextColor = SkColorSetRGB(0xC5, 0x39, 0x29); If not NativeTheme, use kGoogleRed700 from ui/gfx/color_palette.h https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:199: // TODO(crbug.com/666189): Use Put this comment above the entire statement. https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:81: canvas->DrawStringRectWithFlags( This view should probably use a Label, but that's out of scope for this CL.
lshang@chromium.org changed reviewers: + hwi@chromium.org
hwi@, could you confirm the text color on mobile? Thanks! https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... File components/autofill/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... components/autofill/android/java/res/values/colors.xml:9: <color name="http_bad_warning_message_text">#C93929</color> On 2016/11/17 17:52:26, Ted C wrote: > I think you want: > #c53929 > > Same as google_red_700 in chrome/android/java/res/values/colors.xml as well as > the native implementation. Yeah I noticed google_red_700, but as to the specs I got (https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Form%20Not%...), text color on mobile is #c53929. +hwi for clarification.
On 2016/11/17 22:42:18, lshang wrote: > hwi@, could you confirm the text color on mobile? Thanks! > > https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... > File components/autofill/android/java/res/values/colors.xml (right): > > https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... > components/autofill/android/java/res/values/colors.xml:9: <color > name="http_bad_warning_message_text">#C93929</color> > On 2016/11/17 17:52:26, Ted C wrote: > > I think you want: > > #c53929 > > > > Same as google_red_700 in chrome/android/java/res/values/colors.xml as well as > > the native implementation. > > Yeah I noticed google_red_700, but as to the specs I got > (https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Form%20Not%...), > text color on mobile is #c53929. +hwi for clarification. You both are correct. It was a typo on the spec. Sorry about that. Just updated the spec. ⊙﹏⊙
On 2016/11/17 22:48:07, hwi1 wrote: > On 2016/11/17 22:42:18, lshang wrote: > > hwi@, could you confirm the text color on mobile? Thanks! > > > > > https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... > > File components/autofill/android/java/res/values/colors.xml (right): > > > > > https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... > > components/autofill/android/java/res/values/colors.xml:9: <color > > name="http_bad_warning_message_text">#C93929</color> > > On 2016/11/17 17:52:26, Ted C wrote: > > > I think you want: > > > #c53929 > > > > > > Same as google_red_700 in chrome/android/java/res/values/colors.xml as well > as > > > the native implementation. > > > > Yeah I noticed google_red_700, but as to the specs I got > > > (https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20Form%20Not%...), > > text color on mobile is #c53929. +hwi for clarification. > > You both are correct. It was a typo on the spec. Sorry about that. Just updated > the spec. ⊙﹏⊙ Thanks for confirmation!
Thanks all your comments! Those are really good advice! PTAL again? https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:162: case POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE: On 2016/11/17 18:49:39, msw wrote: > nit: reorder after POPUP_ITEM_ID_SEPARATOR to better match enum ordering. Done. https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/popup_constants.h (right): https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/auto... chrome/browser/ui/autofill/popup_constants.h:15: // Various colors used in the Autofill popup. On 2016/11/17 18:49:39, msw wrote: > These ought to be replaced by getting colors from ui::NativeTheme; please follow > up. (it's important for consistency and accessibility). Done. Added a TODO here. https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/auto... chrome/browser/ui/autofill/popup_constants.h:19: const SkColor kHttpWarningTextColor = SkColorSetRGB(0xC5, 0x39, 0x29); On 2016/11/17 18:49:39, msw wrote: > If not NativeTheme, use kGoogleRed700 from ui/gfx/color_palette.h Done. https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:199: // TODO(crbug.com/666189): Use On 2016/11/17 18:49:39, msw wrote: > Put this comment above the entire statement. Done. https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2496683003/diff/170001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:81: canvas->DrawStringRectWithFlags( On 2016/11/17 18:49:39, msw wrote: > This view should probably use a Label, but that's out of scope for this CL. Thanks, maybe I'll do it in the next CL. https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... File components/autofill/android/java/res/values/colors.xml (right): https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... components/autofill/android/java/res/values/colors.xml:9: <color name="http_bad_warning_message_text">#C93929</color> On 2016/11/17 17:52:26, Ted C wrote: > I think you want: > #c53929 > > Same as google_red_700 in chrome/android/java/res/values/colors.xml as well as > the native implementation. Done. Corrected the color. https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java (right): https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:17: private static final int ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE = -10; On 2016/11/17 17:52:26, Ted C wrote: > We support generating java constants from a C++ enum, so we should do that > instead of manually requiring these to be in sync. > > I believe this is relatively up to date: > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/3xr44Rk_cHQ Yeah you're right, that way we can also get rid of the manually defined ITEM_ID_SEPARATOR_ENTRY in AutofillPopup.java. I've filed a bug and added a TODO here. I think this CL is large enough, I'll address this in a separate CL. https://codereview.chromium.org/2496683003/diff/170001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownItem.java (right): https://codereview.chromium.org/2496683003/diff/170001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownItem.java:41: int getLabelFontColor(); On 2016/11/17 17:52:26, Ted C wrote: > For this and getLabelFontSize, I would add the suffix ResId (resource ID for > short). Both colors and font sizes can be presented as integers, so this could > cause user confusion if they were to return Color.RED and it wouldn't work as > there is no resource ID associated with it. > > I would also update the comments to make it even more abundantly clear that this > is a android resource ID. Good suggestion! Done. https://codereview.chromium.org/2496683003/diff/170001/ui/android/java/src/or... File ui/android/java/src/org/chromium/ui/DropdownItemBase.java (right): https://codereview.chromium.org/2496683003/diff/170001/ui/android/java/src/or... ui/android/java/src/org/chromium/ui/DropdownItemBase.java:9: * show the item. Subclassing is the proper way to use this class to create On 2016/11/17 17:52:26, Ted C wrote: > I think the Subclassing sentence is unnecessary (and in particular the reference > to AutofillSuggestion is likely to get out of date due to renames). The name of > DropdownItemBase is clear enough for me. Done.
c/b/ui lgtm, thanks
lgtm https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... File components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java (right): https://codereview.chromium.org/2496683003/diff/170001/components/autofill/an... components/autofill/android/java/src/org/chromium/components/autofill/AutofillSuggestion.java:17: private static final int ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE = -10; On 2016/11/18 00:03:19, lshang wrote: > On 2016/11/17 17:52:26, Ted C wrote: > > We support generating java constants from a C++ enum, so we should do that > > instead of manually requiring these to be in sync. > > > > I believe this is relatively up to date: > > > https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/3xr44Rk_cHQ > > Yeah you're right, that way we can also get rid of the manually defined > ITEM_ID_SEPARATOR_ENTRY in AutofillPopup.java. > > I've filed a bug and added a TODO here. I think this CL is large enough, I'll > address this in a separate CL. Generating java from c++ is usually like 5 or 6 lines, so I don't know if I agree with the assessment. But.....I don't care too much to block this change on that.
The CQ bit was checked by lshang@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: This issue passed the CQ dry run.
The CQ bit was checked by lshang@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, csashi@google.com, dvadym@chromium.org Link to the patchset: https://codereview.chromium.org/2496683003/#ps190001 (title: "update")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Http Bad: Add a PopupItemId to identify http warning message Since Http bad warning message has somewhat different UI from other autofill popup items (see specs of http bad warning message in: go/fns-ui-spec), this CL adds a new PopupItemId POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE to identify http warning message. And renamed POPUP_ITEM_ID_WARNING_MESSAGE to POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE accordingly to be more specific. This CL also changes text color of the message to show an example of use this new PopupItemId in UI code. BUG=662298, 662297 ========== to ========== Http Bad: Add a PopupItemId to identify http warning message Since Http bad warning message has somewhat different UI from other autofill popup items (see specs of http bad warning message in: go/fns-ui-spec), this CL adds a new PopupItemId POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE to identify http warning message. And renamed POPUP_ITEM_ID_WARNING_MESSAGE to POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE accordingly to be more specific. This CL also changes text color of the message to show an example of use this new PopupItemId in UI code. BUG=662298, 662297 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:190001)
Message was sent while issue was closed.
Description was changed from ========== Http Bad: Add a PopupItemId to identify http warning message Since Http bad warning message has somewhat different UI from other autofill popup items (see specs of http bad warning message in: go/fns-ui-spec), this CL adds a new PopupItemId POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE to identify http warning message. And renamed POPUP_ITEM_ID_WARNING_MESSAGE to POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE accordingly to be more specific. This CL also changes text color of the message to show an example of use this new PopupItemId in UI code. BUG=662298, 662297 ========== to ========== Http Bad: Add a PopupItemId to identify http warning message Since Http bad warning message has somewhat different UI from other autofill popup items (see specs of http bad warning message in: go/fns-ui-spec), this CL adds a new PopupItemId POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE to identify http warning message. And renamed POPUP_ITEM_ID_WARNING_MESSAGE to POPUP_ITEM_ID_INSECURE_CONTEXT_PAYMENT_DISABLED_MESSAGE accordingly to be more specific. This CL also changes text color of the message to show an example of use this new PopupItemId in UI code. BUG=662298, 662297 Committed: https://crrev.com/4d6fb24de5f4de7bf92ca572b7be8c3a05376043 Cr-Commit-Position: refs/heads/master@{#433092} ==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/4d6fb24de5f4de7bf92ca572b7be8c3a05376043 Cr-Commit-Position: refs/heads/master@{#433092} |
