Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(156)

Issue 2727233003: Uses child views in Autofill Popup so we can trigger (Closed)

Created:
3 years, 9 months ago by csashi
Modified:
3 years, 9 months ago
CC:
chromium-reviews, tfarina, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, mathp+autofillwatch_chromium.org, mac-reviews_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, srahim+watch_chromium.org
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Uses child views in Autofill Popup so we can trigger GetAccessibleNodeData in each of the Autofill popup suggestions. This is a re-merge of https://codereview.chromium.org/2670003002 with the change to recreate child views when we redraw the popup. BUG=627860, 697466 Review-Url: https://codereview.chromium.org/2727233003 Cr-Commit-Position: refs/heads/master@{#458265} Committed: https://chromium.googlesource.com/chromium/src/+/d9c8ac4a549d3a365b60fc6abf7d23adfe4c389f

Patch Set 1 #

Patch Set 2 : Copy of previous commit (and then reverted). #

Patch Set 3 : Switches to CHECK to ensure crash is caught early. #

Total comments: 4

Patch Set 4 : C++ style static_cast #

Patch Set 5 : Recreates child views in UpdateBoundsAndRedrawPopup to match suggestion count. #

Patch Set 6 : Adds test coverage for changing number of suggestions. #

Patch Set 7 : Adds test coverage for changing number of suggestions. #

Patch Set 8 : Adds test coverage for changing number of suggestions. #

Total comments: 12

Patch Set 9 : Adds test coverage for changing number of suggestions. #

Total comments: 10

Patch Set 10 : Removes one-line virtual methods, uses mocks instead. #

Patch Set 11 : Removes wrapper testing virtual methods, uses mocks instead. #

Patch Set 12 : Removes NotifyAccessibilityEventForRow and InvalidateRow wrapper methods. #

Total comments: 9

Patch Set 13 : Combines 2 calls to InvalidateRow to 1 OnSelectedRowChanged call. #

Total comments: 3

Patch Set 14 : Combines 2 calls to InvalidateRow to 1 OnSelectedRowChanged call. #

Patch Set 15 : Removes NotifyAccessibilityEventForRow and InvalidateRow wrapper methods. #

Total comments: 18

Patch Set 16 : Uses int (instead of size_t) arguments for OnSelectedRowChanged because we need to pass -1. #

Total comments: 4

Patch Set 17 : Uses int (instead of size_t) arguments for OnSelectedRowChanged because we need to pass -1. #

Patch Set 18 : Uses int (instead of size_t) arguments for OnSelectedRowChanged because we need to pass -1. #

Patch Set 19 : Uses int (instead of size_t) arguments for OnSelectedRowChanged because we need to pass -1. #

Patch Set 20 : Uses base::Optional<size_t> instead of representing no selection as -1. #

Patch Set 21 : Uses base::Optional<size_t> instead of representing no selection as -1. #

Total comments: 31

Patch Set 22 : Calls OnSelectedRowChanged after updating selected_line_. #

Patch Set 23 : Calls OnSelectedRowChanged after updating selected_line_. #

Patch Set 24 : Calls OnSelectedRowChanged after updating selected_line_. #

Patch Set 25 : Calls OnSelectedRowChanged after updating selected_line_. #

Patch Set 26 : Calls OnSelectedRowChanged after updating selected_line_. #

Patch Set 27 : Removes static_cast to AutofillPopupChildView and class name check. #

Total comments: 1

Patch Set 28 : Resets selected_line_ in Show if it is pointing out of bounds. #

Patch Set 29 : Simplifies Views implementation by calling SchedulePaint instead of SchedulePaintInRect. #

Total comments: 14

Patch Set 30 : Switches from size_t to int when we are not referring to size of an object. #

Total comments: 2

Patch Set 31 : Start from -1 in SelectPreviousLine if there is no current selection. #

Patch Set 32 : Switches from size_t to int when we are not referring to size of an object. #

Patch Set 33 : Switches from size_t to int when we are not referring to size of an object. #

Total comments: 22

Patch Set 34 : Removes base/optional.h include and inline definitions. Uses NSInteger instead of int. #

Total comments: 4

Patch Set 35 : (int) to NSInteger #

Unified diffs Side-by-side diffs Delta from patch set Stats (+483 lines, -197 lines) Patch
M chrome/app/generated_resources.grd View 1 chunk +3 lines, -0 lines 0 comments Download
M chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 3 chunks +6 lines, -5 lines 0 comments Download
M chrome/browser/ui/android/autofill/autofill_popup_view_android.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 1 chunk +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/android/autofill/autofill_popup_view_android.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 3 chunks +6 lines, -4 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 3 chunks +10 lines, -8 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_controller_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 5 chunks +19 lines, -19 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_controller_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 16 chunks +52 lines, -68 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 18 chunks +86 lines, -38 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_layout_model_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_view.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 2 chunks +7 lines, -4 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_view_delegate.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/autofill/password_generation_popup_controller_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/autofill/password_generation_popup_controller_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +2 lines, -4 lines 0 comments Download
M chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 4 chunks +6 lines, -4 lines 0 comments Download
M chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 2 chunks +12 lines, -4 lines 0 comments Download
M chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 1 chunk +1 line, -1 line 0 comments Download
M chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 10 chunks +14 lines, -13 lines 0 comments Download
M chrome/browser/ui/cocoa/autofill/password_generation_popup_view_cocoa_unittest.mm View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/autofill/autofill_popup_base_view.h View 1 2 3 4 5 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/autofill/autofill_popup_base_view_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +2 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/autofill/autofill_popup_view_views.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 2 chunks +13 lines, -3 lines 0 comments Download
M chrome/browser/ui/views/autofill/autofill_popup_view_views.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 5 chunks +70 lines, -6 lines 0 comments Download
A chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 1 chunk +157 lines, -0 lines 0 comments Download
M chrome/test/BUILD.gn View 1 2 3 4 5 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 158 (96 generated)
csashi
Hi, This is a re-merge of https://codereview.chromium.org/2670003002 (which we reverted) to M-59. Mathieu and I ...
3 years, 9 months ago (2017-03-02 22:25:45 UTC) #2
sky
Can you please upload the first patch as the revert and then upload this patch ...
3 years, 9 months ago (2017-03-02 23:56:14 UTC) #3
csashi
On 2017/03/02 23:56:14, sky wrote: > Can you please upload the first patch as the ...
3 years, 9 months ago (2017-03-03 00:28:21 UTC) #4
sky
+estade. Evan is an owner here and may not what is going wrong. https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File ...
3 years, 9 months ago (2017-03-03 03:35:46 UTC) #6
sky
That should be 'may *know* what is going wrong.' GAH! On Thu, Mar 2, 2017 ...
3 years, 9 months ago (2017-03-03 04:07:05 UTC) #7
csashi
Hi Scott, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode128 chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:128: CHECK_LT(row, ...
3 years, 9 months ago (2017-03-03 04:39:42 UTC) #8
Evan Stade
this code has changed a lot since I last looked at it. Still a fun ...
3 years, 9 months ago (2017-03-03 04:41:29 UTC) #9
csashi
https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode73 chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:73: AutofillPopupChildView* child = new AutofillPopupChildView(controller, i); On 2017/03/03 04:41:29, ...
3 years, 9 months ago (2017-03-03 04:48:19 UTC) #10
Evan Stade
On 2017/03/03 04:48:19, csashi wrote: > https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): > > https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode73 > ...
3 years, 9 months ago (2017-03-03 05:04:46 UTC) #11
csashi
On 2017/03/03 05:04:46, Evan Stade wrote: > On 2017/03/03 04:48:19, csashi wrote: > > > ...
3 years, 9 months ago (2017-03-03 05:23:55 UTC) #12
Evan Stade
On 2017/03/03 05:23:55, csashi wrote: > On 2017/03/03 05:04:46, Evan Stade wrote: > > On ...
3 years, 9 months ago (2017-03-03 05:26:56 UTC) #13
Mathieu
On 2017/03/03 05:26:56, Evan Stade wrote: > On 2017/03/03 05:23:55, csashi wrote: > > On ...
3 years, 9 months ago (2017-03-03 17:45:00 UTC) #14
csashi
On 2017/03/03 17:45:00, Mathieu Perreault wrote: > On 2017/03/03 05:26:56, Evan Stade wrote: > > ...
3 years, 9 months ago (2017-03-03 18:03:10 UTC) #15
Mathieu
lgtm
3 years, 9 months ago (2017-03-03 18:06:25 UTC) #16
sky
Please add test coverage of this case.
3 years, 9 months ago (2017-03-03 19:14:45 UTC) #18
csashi
On 2017/03/03 19:14:45, sky wrote: > Please add test coverage of this case. Hi Scott, ...
3 years, 9 months ago (2017-03-04 02:59:50 UTC) #19
sky
Evan, can you review this too? https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc (right): https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc#newcode1 chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:1: // Copyright (c) ...
3 years, 9 months ago (2017-03-06 18:02:57 UTC) #20
sky
On 2017/03/06 18:02:57, sky wrote: > Evan, can you review this too? > > https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc ...
3 years, 9 months ago (2017-03-06 18:03:33 UTC) #21
csashi
Hi Scott & Evan, Please take a look. -sashi. https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc (right): https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc#newcode1 chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:1: ...
3 years, 9 months ago (2017-03-06 19:27:07 UTC) #22
Evan Stade
https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right): https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc#newcode265 chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:265: EXPECT_CALL(*autofill_popup_controller_, NotifyAccessibilityEventForRow(_, _)) perhaps it's beyond the scope of ...
3 years, 9 months ago (2017-03-06 21:45:34 UTC) #23
csashi
Hi Evan & Scott, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right): https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc#newcode265 ...
3 years, 9 months ago (2017-03-07 01:53:53 UTC) #24
Evan Stade
https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode98 chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:98: // suggestion content may change. On 2017/03/07 01:53:53, csashi ...
3 years, 9 months ago (2017-03-08 18:03:21 UTC) #33
csashi
Hi Evan & Scott, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode98 ...
3 years, 9 months ago (2017-03-08 20:40:41 UTC) #34
Evan Stade
unfortunately I'll be ooo until next Tuesday or Wednesday so I won't be able to ...
3 years, 9 months ago (2017-03-09 03:21:16 UTC) #39
csashi
Hi Evan & Scott, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode51 ...
3 years, 9 months ago (2017-03-09 05:16:34 UTC) #40
sky
csashi, I would prefer to have Evan review this as he is a local owner ...
3 years, 9 months ago (2017-03-09 20:42:33 UTC) #54
Evan Stade
thanks for your patience. https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode65 chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:65: constexpr int AutofillPopupView::kNoSelection; I'm confused ...
3 years, 9 months ago (2017-03-15 15:18:59 UTC) #55
csashi
Hi Evan and Scott, Please take a look. Thanks! -sashi. https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode65 ...
3 years, 9 months ago (2017-03-15 21:31:09 UTC) #56
Evan Stade
https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode65 chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:65: constexpr int AutofillPopupView::kNoSelection; On 2017/03/15 21:31:09, csashi wrote: > ...
3 years, 9 months ago (2017-03-16 01:23:44 UTC) #65
csashi
https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode65 chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:65: constexpr int AutofillPopupView::kNoSelection; On 2017/03/16 01:23:44, Evan Stade wrote: ...
3 years, 9 months ago (2017-03-16 01:50:55 UTC) #66
csashi
https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode65 chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:65: constexpr int AutofillPopupView::kNoSelection; On 2017/03/16 01:50:55, csashi wrote: > ...
3 years, 9 months ago (2017-03-16 01:53:41 UTC) #67
csashi
https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/views/autofill/autofill_popup_view_views.cc#newcode65 chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:65: constexpr int AutofillPopupView::kNoSelection; On 2017/03/16 01:53:41, csashi wrote: > ...
3 years, 9 months ago (2017-03-16 02:18:23 UTC) #70
Evan Stade
On 2017/03/16 01:50:55, csashi wrote: > > I did not use optional based on following ...
3 years, 9 months ago (2017-03-16 14:24:18 UTC) #71
csashi
Hi Evan, Please take a look. Thanks, -sashi.
3 years, 9 months ago (2017-03-16 16:06:40 UTC) #72
Evan Stade
https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc#newcode266 chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:266: if (!CanAccept(suggestions_[selected_line_.value()].frontend_id)) *selected_line_ https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc#newcode398 chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:398: view_->OnSelectedRowChanged(selected_line_, selected_line); calling this ...
3 years, 9 months ago (2017-03-16 17:39:00 UTC) #81
csashi
Hi Evan, Thanks for the time you are investing in this review. Please take a ...
3 years, 9 months ago (2017-03-16 22:42:06 UTC) #82
Evan Stade
> Thanks for the time you are investing in this review. Please take a look. ...
3 years, 9 months ago (2017-03-17 14:11:55 UTC) #99
csashi
Hi Evan, Can you take a look? Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc#newcode398 chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:398: ...
3 years, 9 months ago (2017-03-17 16:30:36 UTC) #100
Evan Stade
https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc#newcode398 chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:398: view_->OnSelectedRowChanged(selected_line_, selected_line); On 2017/03/17 16:30:35, csashi wrote: > On ...
3 years, 9 months ago (2017-03-17 19:08:54 UTC) #105
csashi
Hi Evan, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc#newcode398 chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:398: view_->OnSelectedRowChanged(selected_line_, ...
3 years, 9 months ago (2017-03-17 21:55:21 UTC) #106
csashi
Hi Evan, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc#newcode398 chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:398: view_->OnSelectedRowChanged(selected_line_, ...
3 years, 9 months ago (2017-03-17 22:52:26 UTC) #109
Evan Stade
https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc#newcode114 chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:114: if (selected_line_ && *selected_line_ >= suggestions_.size()) { nit: no ...
3 years, 9 months ago (2017-03-18 00:47:04 UTC) #112
csashi
Hi Evan, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc#newcode114 chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:114: if ...
3 years, 9 months ago (2017-03-18 02:25:09 UTC) #113
Evan Stade
lgtm(!) with last two issues addressed https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc#newcode376 chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:376: return selected_line_ && ...
3 years, 9 months ago (2017-03-18 02:37:48 UTC) #116
csashi
Hi Evan, Please take a look. Thanks! -sashi. https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc#newcode376 chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:376: return ...
3 years, 9 months ago (2017-03-18 03:51:37 UTC) #119
csashi
Hi Scott, Can you approve for chrome/browser/ui ? Thanks, -sashi.
3 years, 9 months ago (2017-03-18 03:53:55 UTC) #120
sky
https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h#newcode15 chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h:15: #include "base/optional.h" The use of optional comes from chrome/browser/ui/autofill/autofill_popup_view.h, ...
3 years, 9 months ago (2017-03-20 15:50:32 UTC) #133
csashi
Hi Scott, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h#newcode15 chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h:15: #include ...
3 years, 9 months ago (2017-03-20 18:11:43 UTC) #134
sky
Did you upload a new patch? https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h#newcode15 chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h:15: #include "base/optional.h" On ...
3 years, 9 months ago (2017-03-20 20:10:50 UTC) #135
sky
+groby for the cocoa files
3 years, 9 months ago (2017-03-20 20:11:38 UTC) #137
groby-ooo-7-16
Sorry for repeated comments :( I'm still uncomfortable with the API change, because it pushes ...
3 years, 9 months ago (2017-03-20 20:42:55 UTC) #138
Evan Stade
> I'm still uncomfortable with the API change, because it pushes logic from the > ...
3 years, 9 months ago (2017-03-20 21:36:58 UTC) #139
csashi
On 2017/03/20 20:10:50, sky wrote: > Did you upload a new patch? Oops. Please take ...
3 years, 9 months ago (2017-03-20 21:40:10 UTC) #140
Evan Stade
https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm#newcode48 chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:48: void AutofillPopupViewBridge::OnSelectedRowChanged( On 2017/03/20 21:36:57, Evan Stade wrote: > ...
3 years, 9 months ago (2017-03-20 21:41:06 UTC) #141
csashi
Hi, My response to a couple of the comments crossed Evan's. Please take a look. ...
3 years, 9 months ago (2017-03-20 21:41:52 UTC) #142
sky
The android and other random bits LGTM - wait for groby@ on the cocoa side ...
3 years, 9 months ago (2017-03-20 21:52:18 UTC) #145
groby-ooo-7-16
Thank you for the quick reply! cocoa LGTM w/ two tiny nits (ax_enum.h, NSInteger) https://codereview.chromium.org/2727233003/diff/240001/chrome/browser/ui/autofill/autofill_popup_controller_impl.cc ...
3 years, 9 months ago (2017-03-20 22:53:10 UTC) #146
Evan Stade
https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm#newcode48 chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:48: void AutofillPopupViewBridge::OnSelectedRowChanged( On 2017/03/20 22:53:10, groby wrote: > On ...
3 years, 9 months ago (2017-03-20 22:57:30 UTC) #147
groby-ooo-7-16
https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm#newcode48 chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:48: void AutofillPopupViewBridge::OnSelectedRowChanged( On 2017/03/20 22:57:30, Evan Stade wrote: > ...
3 years, 9 months ago (2017-03-20 22:58:42 UTC) #148
csashi
https://codereview.chromium.org/2727233003/diff/660001/chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h (right): https://codereview.chromium.org/2727233003/diff/660001/chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h#newcode18 chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:18: #include "ui/accessibility/ax_enums.h" On 2017/03/20 22:53:10, groby wrote: > Probably ...
3 years, 9 months ago (2017-03-20 23:13:30 UTC) #149
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2727233003/680001
3 years, 9 months ago (2017-03-21 00:58:01 UTC) #155
commit-bot: I haz the power
3 years, 9 months ago (2017-03-21 01:05:27 UTC) #158
Message was sent while issue was closed.
Committed patchset #35 (id:680001) as
https://chromium.googlesource.com/chromium/src/+/d9c8ac4a549d3a365b60fc6abf7d...

Powered by Google App Engine
This is Rietveld 408576698