Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2517843002:
Http Bad: Put icon on the left of http warning message on Views (Closed)
|
Created:
4 years, 1 month ago by lshang Modified:
4 years ago CC:
chromium-reviews, rouslan+autofill_chromium.org, tfarina, sebsg+autofillwatch_chromium.org, jdonnelly+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, chrome-apps-syd-reviews_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionHttp Bad: Put icon on the left of http warning message on Views
The order of elements in http bad warning message is: icon, value, label. So this
CL puts icon on the left of autofill entry when drawing it. It also adjusts label
font size, name padding and icon padding accordingly.
specs: go/fns-ui-spec
screenshots: https://screenshot.googleplex.com/bD2gRETFRPo
https://screenshot.googleplex.com/t81q78qYwLk
BUG=662298
Committed: https://crrev.com/833bf5f37ae5a52da4669ecd9a8cc60b838d8d25
Cr-Commit-Position: refs/heads/master@{#435722}
Patch Set 1 #Patch Set 2 : add test #
Total comments: 8
Patch Set 3 : merge two Draw methods and add ascii art #
Total comments: 10
Patch Set 4 : add comments #
Total comments: 12
Patch Set 5 : update #
Total comments: 4
Patch Set 6 : update #Patch Set 7 : fix mac #Patch Set 8 : fix mac #
Messages
Total messages: 44 (27 generated)
Description was changed from ========== Http Bad: Put icon on the left of http warning message on Views BUG= ========== to ========== Http Bad: Put icon on the left of http warning message on Views The order of elements in http bad warning message is: icon, value, label. So this CL puts icon on the left of autofill entry when drawing it. It also adjusts label font size, name padding and icon padding accordingly. specs: go/fns-ui-spec screenshots: https://screenshot.googleplex.com/bD2gRETFRPo https://screenshot.googleplex.com/t81q78qYwLk BUG=662298 ==========
lshang@chromium.org changed reviewers: + mathp@chromium.org
Hi Mathieu, this CL changed style of http warning message on Views. Since the element order, padding and label font size are all different from normal autofill entries, I put the logic in a new method called DrawAutofillHttpWarningEntry, rather than mixed them together in DrawAutofillEntry. I think this looks more clear, but I'm also happy to hear your thoughts.
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: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
mathp@chromium.org changed reviewers: + estade@chromium.org
Hi Liu, here are some comments. I'm also adding Evan who is the owner of this code. estade@chromium.org: Please review changes in ui/autofill https://codereview.chromium.org/2517843002/diff/20001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2517843002/diff/20001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:189: if (id == POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE) curious, can't you do if (suggestions[index].frontend_id == POPUP_ITEM...) ? https://codereview.chromium.org/2517843002/diff/20001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc (right): https://codereview.chromium.org/2517843002/diff/20001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc:54: suggestions.push_back(Suggestion("x", "x", "http", -10)); nit: use the popup_item_id enum instead of -10? https://codereview.chromium.org/2517843002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2517843002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:128: void AutofillPopupViewViews::DrawAutofillHttpWarningEntry( While I do like a clean separation and appreciate the motivation, there is too much repetition here for my taste, sorry. It makes the code harder to maintain (what if there is a bug, and the person fixing it forgets to update this method?). From a quick comparison it looks like the code is 90% similar to DrawAutofillEntry so it shouldn't be too bad to have them part of the same method. Another point: Now that it's becoming non trivial, I think we could be better at documenting what this does, with ASCII art, similar to what PaymentRequest does, here: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... https://codereview.chromium.org/2517843002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:178: if (!controller_->GetSuggestionAt(index).label.empty()) { you can add this if condition in the main DrawAutofillEntry, no?
can you ping me after addressing mathp's comments to his satisfaction? 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 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...
Patchset #3 (id:40001) has been deleted
Thanks Mathieu, PTAL again? https://codereview.chromium.org/2517843002/diff/20001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2517843002/diff/20001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:189: if (id == POPUP_ITEM_ID_HTTP_NOT_SECURE_WARNING_MESSAGE) On 2016/11/22 14:36:45, Mathieu Perreault wrote: > curious, can't you do > > if (suggestions[index].frontend_id == POPUP_ITEM...) ? Done. https://codereview.chromium.org/2517843002/diff/20001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc (right): https://codereview.chromium.org/2517843002/diff/20001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc:54: suggestions.push_back(Suggestion("x", "x", "http", -10)); On 2016/11/22 14:36:45, Mathieu Perreault wrote: > nit: use the popup_item_id enum instead of -10? Done. https://codereview.chromium.org/2517843002/diff/20001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2517843002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:128: void AutofillPopupViewViews::DrawAutofillHttpWarningEntry( On 2016/11/22 14:36:45, Mathieu Perreault wrote: > While I do like a clean separation and appreciate the motivation, there is too > much repetition here for my taste, sorry. It makes the code harder to maintain > (what if there is a bug, and the person fixing it forgets to update this > method?). > > From a quick comparison it looks like the code is 90% similar to > DrawAutofillEntry so it shouldn't be too bad to have them part of the same > method. > > Another point: Now that it's becoming non trivial, I think we could be better at > documenting what this does, with ASCII art, similar to what PaymentRequest does, > here: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Done. Drawing the ASCII art is interesting. https://codereview.chromium.org/2517843002/diff/20001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:178: if (!controller_->GetSuggestionAt(index).label.empty()) { On 2016/11/22 14:36:45, Mathieu Perreault wrote: > you can add this if condition in the main DrawAutofillEntry, no? Done.
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...)
lgtm. There is no perfect solution but your drawings really help. I think we will eventually want to move to some layout model, where the view could just call GetIconAlignStart(), GetLabelAlignStart(), etc. https://codereview.chromium.org/2517843002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2517843002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:85: * ............................................................................ Thanks, I would add a note in this comment for the next person wanting to modify that code, indicating a test URL that would trigger the HTTP warning entry (you can use a goo.gl short link if it's too long). https://codereview.chromium.org/2517843002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:101: int icon_x_align_left = 0; would change to icon_x_align_start, so that it's clearer in the context of RTL (this naming style is used on Android). What do you think? https://codereview.chromium.org/2517843002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:119: if (is_http_warning) { I would add a comment to the reader such as // An icon was drawn; adjust the |icon_x_align_start| value for the next element https://codereview.chromium.org/2517843002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:137: if (is_http_warning) I would add {} here https://codereview.chromium.org/2517843002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:158: if (is_http_warning) I would add {} around the blocks
Thanks Mathieu. estade@ PTAL again? https://codereview.chromium.org/2517843002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2517843002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:85: * ............................................................................ On 2016/11/28 15:01:35, Mathieu Perreault wrote: > Thanks, I would add a note in this comment for the next person wanting to modify > that code, indicating a test URL that would trigger the HTTP warning entry (you > can use a goo.gl short link if it's too long). Done. https://codereview.chromium.org/2517843002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:101: int icon_x_align_left = 0; On 2016/11/28 15:01:35, Mathieu Perreault wrote: > would change to icon_x_align_start, so that it's clearer in the context of RTL > (this naming style is used on Android). What do you think? Not for this case I think. RTL or LTR decides where to draw the element(the position of the element in the whole gfx::Rect), in either mode |icon_x_align_start| is the left align point of the icon to place it, so this is a consistent definition. For other cases like paddings, I agree that start/end is better than left/right since they will be flipped around in different modes and definition of left/right will change. https://codereview.chromium.org/2517843002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:119: if (is_http_warning) { On 2016/11/28 15:01:35, Mathieu Perreault wrote: > I would add a comment to the reader such as > > // An icon was drawn; adjust the |icon_x_align_start| value for the next element Done. https://codereview.chromium.org/2517843002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:137: if (is_http_warning) On 2016/11/28 15:01:35, Mathieu Perreault wrote: > I would add {} here Done. https://codereview.chromium.org/2517843002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:158: if (is_http_warning) On 2016/11/28 15:01:35, Mathieu Perreault wrote: > I would add {} around the blocks Done.
just nits https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:120: bool isWarningMessage = (suggestions[row].frontend_id == is_warning_message https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:132: row_size += isWarningMessage ? icon_width + kHttpWarningIconPadding nit: row_size += icon_width + (is_warning ? kHttpWarningIconPadding : kIconPadding); https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model.h:49: static const int kHttpWarningIconPadding = 8; I'm confused by this for two reasons - first because the comment is identical to the above, and second because I didn't know there could be more than one icon ("between icons") https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:107: if ((is_http_warning && !is_rtl) || (!is_http_warning && is_rtl)) nit: is_http_warning != is_rtl https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:119: if (icon_x_align_left == value_rect.right()) nit: can you save the result of the directionality check above in a local variable instead of checking it this way? https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:127: icon_x_align_left += after this statement, this variable no longer really is what its name implies. Can you create a different variable for this purpose?
Thanks! Learnt a lot for coding style. https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_layout_model.cc (right): https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:120: bool isWarningMessage = (suggestions[row].frontend_id == On 2016/11/29 15:43:17, Evan Stade wrote: > is_warning_message Done. https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model.cc:132: row_size += isWarningMessage ? icon_width + kHttpWarningIconPadding On 2016/11/29 15:43:17, Evan Stade wrote: > nit: > > row_size += icon_width + (is_warning ? kHttpWarningIconPadding : kIconPadding); Done. https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/autof... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/autof... chrome/browser/ui/autofill/autofill_popup_layout_model.h:49: static const int kHttpWarningIconPadding = 8; On 2016/11/29 15:43:17, Evan Stade wrote: > I'm confused by this for two reasons - first because the comment is identical to > the above, and second because I didn't know there could be more than one icon > ("between icons") I advised the comments a bit to make it more clear. Are you OK with the new version? https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:107: if ((is_http_warning && !is_rtl) || (!is_http_warning && is_rtl)) On 2016/11/29 15:43:17, Evan Stade wrote: > nit: is_http_warning != is_rtl Done. https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:119: if (icon_x_align_left == value_rect.right()) On 2016/11/29 15:43:17, Evan Stade wrote: > nit: can you save the result of the directionality check above in a local > variable instead of checking it this way? Done. https://codereview.chromium.org/2517843002/diff/80001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:127: icon_x_align_left += On 2016/11/29 15:43:17, Evan Stade wrote: > after this statement, this variable no longer really is what its name implies. > Can you create a different variable for this purpose? Done.
thanks, lgtm https://codereview.chromium.org/2517843002/diff/100001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2517843002/diff/100001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.h:42: // The amount of padding around icons in pixels for HTTP warning message. nit: add the word horizontal? https://codereview.chromium.org/2517843002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2517843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:108: bool icon_on_the_right = false; I was kind of thinking bool icon_on_the_left = is_http_warning != is_rtl; int x_align_left = icon_on_the_left ? ... : ...;
Thanks Evan! https://codereview.chromium.org/2517843002/diff/100001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_layout_model.h (right): https://codereview.chromium.org/2517843002/diff/100001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_layout_model.h:42: // The amount of padding around icons in pixels for HTTP warning message. On 2016/11/30 16:22:30, Evan Stade wrote: > nit: add the word horizontal? Done. https://codereview.chromium.org/2517843002/diff/100001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2517843002/diff/100001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:108: bool icon_on_the_right = false; On 2016/11/30 16:22:30, Evan Stade wrote: > I was kind of thinking > > bool icon_on_the_left = is_http_warning != is_rtl; > int x_align_left = icon_on_the_left ? ... : ...; Done.
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, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2517843002/#ps120001 (title: "update")
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
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_...)
lshang@chromium.org changed reviewers: + avi@chromium.org
+avi@ for changes in autofill_popup_view_cocoa.mm, PTAL 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: This issue passed the CQ dry run.
lgtm
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, estade@chromium.org Link to the patchset: https://codereview.chromium.org/2517843002/#ps160001 (title: "fix mac")
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": 160001, "attempt_start_ts": 1480627843966780,
"parent_rev": "3576929d8977adf3cbf22317ab9074499b9c45ba", "commit_rev":
"c7893645991e3f2d8cc88bacf143a461eee7f14c"}
Message was sent while issue was closed.
Description was changed from ========== Http Bad: Put icon on the left of http warning message on Views The order of elements in http bad warning message is: icon, value, label. So this CL puts icon on the left of autofill entry when drawing it. It also adjusts label font size, name padding and icon padding accordingly. specs: go/fns-ui-spec screenshots: https://screenshot.googleplex.com/bD2gRETFRPo https://screenshot.googleplex.com/t81q78qYwLk BUG=662298 ========== to ========== Http Bad: Put icon on the left of http warning message on Views The order of elements in http bad warning message is: icon, value, label. So this CL puts icon on the left of autofill entry when drawing it. It also adjusts label font size, name padding and icon padding accordingly. specs: go/fns-ui-spec screenshots: https://screenshot.googleplex.com/bD2gRETFRPo https://screenshot.googleplex.com/t81q78qYwLk BUG=662298 ==========
Message was sent while issue was closed.
Committed patchset #8 (id:160001)
Message was sent while issue was closed.
Description was changed from ========== Http Bad: Put icon on the left of http warning message on Views The order of elements in http bad warning message is: icon, value, label. So this CL puts icon on the left of autofill entry when drawing it. It also adjusts label font size, name padding and icon padding accordingly. specs: go/fns-ui-spec screenshots: https://screenshot.googleplex.com/bD2gRETFRPo https://screenshot.googleplex.com/t81q78qYwLk BUG=662298 ========== to ========== Http Bad: Put icon on the left of http warning message on Views The order of elements in http bad warning message is: icon, value, label. So this CL puts icon on the left of autofill entry when drawing it. It also adjusts label font size, name padding and icon padding accordingly. specs: go/fns-ui-spec screenshots: https://screenshot.googleplex.com/bD2gRETFRPo https://screenshot.googleplex.com/t81q78qYwLk BUG=662298 Committed: https://crrev.com/833bf5f37ae5a52da4669ecd9a8cc60b838d8d25 Cr-Commit-Position: refs/heads/master@{#435722} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/833bf5f37ae5a52da4669ecd9a8cc60b838d8d25 Cr-Commit-Position: refs/heads/master@{#435722} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
