|
|
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. |
DescriptionUses 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 #Messages
Total messages: 158 (96 generated)
csashi@google.com changed reviewers: + mathp@chromium.org, sky@chromium.org
Hi, This is a re-merge of https://codereview.chromium.org/2670003002 (which we reverted) to M-59. Mathieu and I looked at the root cause of the crash in bug 697466 and we could not identify the cause or reproduce it in Windows, Chrome OS or Linux. We have a few theories and solutions but I would prefer to implement the solution once I know what is causing the crash. Because we have more time on M-59, I added some CHECK asserts to help us identify root cause. I will submit this change only after the head is M-59. Thanks, -sashi.
Can you please upload the first patch as the revert and then upload this patch next? That way it's easy to see what changed.
On 2017/03/02 23:56:14, sky wrote: > Can you please upload the first patch as the revert and then upload this patch > next? That way it's easy to see what changed. Hi Scott, I could not figure out a way to delete Patch Set 1, so I created Patch Set 2 as the previous commit (which we reverted) and Patch Set 3 (same as Patch Set 1). Hopefully you can look at diff between Patch Set 3 and Patch Set 2. I added CHECKs in chrome/browser/ui/views/autofill/autofill_popup_view_views.cc and in chrome/browser/ui/autofill/autofill_popup_controller_impl.cc. -sashi.
sky@chromium.org changed reviewers: + estade@chromium.org
+estade. Evan is an owner here and may not what is going wrong. https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:128: CHECK_LT(row, (size_t)child_count()); Use C++ style casts, .e.g static_cast<size_t>(child_count());
That should be 'may *know* what is going wrong.' GAH! On Thu, Mar 2, 2017 at 7:35 PM, <sky@chromium.org> wrote: > +estade. Evan is an owner here and may not what is going wrong. > > > https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc > (right): > > https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:128: > CHECK_LT(row, (size_t)child_count()); > Use C++ style casts, .e.g static_cast<size_t>(child_count()); > > https://codereview.chromium.org/2727233003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Hi Scott, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:128: CHECK_LT(row, (size_t)child_count()); On 2017/03/03 03:35:46, sky wrote: > Use C++ style casts, .e.g static_cast<size_t>(child_count()); Done.
this code has changed a lot since I last looked at it. Still a fun challenge to debug though. https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:73: AutofillPopupChildView* child = new AutofillPopupChildView(controller, i); You can repro this crash by filling in a form, then clicking in the form again (popup shows again) then pressing space. AutofillPopupControllerImpl::Show() is called with new suggestions. It reuses the previous AutofillPopupView. Here in the ctor you are assuming that the suggestions won't change over the lifetime of the AutofillPopupView. Unfortunately this is an incorrect assumption. The newly modified suggestions don't match the view hierarchy you've created here.
https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:73: AutofillPopupChildView* child = new AutofillPopupChildView(controller, i); On 2017/03/03 04:41:29, Evan Stade wrote: > You can repro this crash by filling in a form, then clicking in the form again > (popup shows again) then pressing space. AutofillPopupControllerImpl::Show() is > called with new suggestions. It reuses the previous AutofillPopupView. > > Here in the ctor you are assuming that the suggestions won't change over the > lifetime of the AutofillPopupView. Unfortunately this is an incorrect > assumption. The newly modified suggestions don't match the view hierarchy you've > created here. Thanks! This was Mathieu's hypothesis as well but I did not know how to force this scenario. Will try this and fix accordingly.
On 2017/03/03 04:48:19, csashi wrote: > https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): > > https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:73: > AutofillPopupChildView* child = new AutofillPopupChildView(controller, i); > On 2017/03/03 04:41:29, Evan Stade wrote: > > You can repro this crash by filling in a form, then clicking in the form again > > (popup shows again) then pressing space. AutofillPopupControllerImpl::Show() > is > > called with new suggestions. It reuses the previous AutofillPopupView. > > > > Here in the ctor you are assuming that the suggestions won't change over the > > lifetime of the AutofillPopupView. Unfortunately this is an incorrect > > assumption. The newly modified suggestions don't match the view hierarchy > you've > > created here. > > Thanks! This was Mathieu's hypothesis as well but I did not know how to force > this scenario. Will try this and fix accordingly. I have to say that it seems fairly perilous to create and maintain this invisible hierarchy of Views. Good luck.
On 2017/03/03 05:04:46, Evan Stade wrote: > On 2017/03/03 04:48:19, csashi wrote: > > > https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... > > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): > > > > > https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... > > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:73: > > AutofillPopupChildView* child = new AutofillPopupChildView(controller, i); > > On 2017/03/03 04:41:29, Evan Stade wrote: > > > You can repro this crash by filling in a form, then clicking in the form > again > > > (popup shows again) then pressing space. AutofillPopupControllerImpl::Show() > > is > > > called with new suggestions. It reuses the previous AutofillPopupView. > > > > > > Here in the ctor you are assuming that the suggestions won't change over the > > > lifetime of the AutofillPopupView. Unfortunately this is an incorrect > > > assumption. The newly modified suggestions don't match the view hierarchy > > you've > > > created here. > > > > Thanks! This was Mathieu's hypothesis as well but I did not know how to force > > this scenario. Will try this and fix accordingly. > > I have to say that it seems fairly perilous to create and maintain this > invisible hierarchy of Views. Good luck. Hi Evan, The plan was to make the child views visible - i.e. move the rendering also to the child view. Not sure if that changes your opinion. -i.e. are you concerned about the "invisible" part (which we plan to make visible) or the "hierarchy" part. We don't need the child views for this change - I can process all the accessibility events on the parent view - but it was recommended that the event be forwarded to the child view. -sashi.
On 2017/03/03 05:23:55, csashi wrote: > On 2017/03/03 05:04:46, Evan Stade wrote: > > On 2017/03/03 04:48:19, csashi wrote: > > > > > > https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... > > > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): > > > > > > > > > https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... > > > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:73: > > > AutofillPopupChildView* child = new AutofillPopupChildView(controller, i); > > > On 2017/03/03 04:41:29, Evan Stade wrote: > > > > You can repro this crash by filling in a form, then clicking in the form > > again > > > > (popup shows again) then pressing space. > AutofillPopupControllerImpl::Show() > > > is > > > > called with new suggestions. It reuses the previous AutofillPopupView. > > > > > > > > Here in the ctor you are assuming that the suggestions won't change over > the > > > > lifetime of the AutofillPopupView. Unfortunately this is an incorrect > > > > assumption. The newly modified suggestions don't match the view hierarchy > > > you've > > > > created here. > > > > > > Thanks! This was Mathieu's hypothesis as well but I did not know how to > force > > > this scenario. Will try this and fix accordingly. > > > > I have to say that it seems fairly perilous to create and maintain this > > invisible hierarchy of Views. Good luck. > > Hi Evan, > The plan was to make the child views visible - i.e. move the rendering also to > the > child view. Not sure if that changes your opinion. -i.e. are you concerned about > the "invisible" part (which we plan to make visible) or the "hierarchy" part. > > We don't need the child views for this change - I can process all the > accessibility > events on the parent view - but it was recommended that the event be forwarded > to the child view. > -sashi. using an actual view hierarchy for everything (rendering, events, etc.) would be less worrisome. You still need to worry about keeping it in sync with the controller but perhaps it's enough to rebuild it all in UpdateBoundsAndRedrawPopup.
On 2017/03/03 05:26:56, Evan Stade wrote: > On 2017/03/03 05:23:55, csashi wrote: > > On 2017/03/03 05:04:46, Evan Stade wrote: > > > On 2017/03/03 04:48:19, csashi wrote: > > > > > > > > > > https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... > > > > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... > > > > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:73: > > > > AutofillPopupChildView* child = new AutofillPopupChildView(controller, i); > > > > On 2017/03/03 04:41:29, Evan Stade wrote: > > > > > You can repro this crash by filling in a form, then clicking in the form > > > again > > > > > (popup shows again) then pressing space. > > AutofillPopupControllerImpl::Show() > > > > is > > > > > called with new suggestions. It reuses the previous AutofillPopupView. > > > > > > > > > > Here in the ctor you are assuming that the suggestions won't change over > > the > > > > > lifetime of the AutofillPopupView. Unfortunately this is an incorrect > > > > > assumption. The newly modified suggestions don't match the view > hierarchy > > > > you've > > > > > created here. > > > > > > > > Thanks! This was Mathieu's hypothesis as well but I did not know how to > > force > > > > this scenario. Will try this and fix accordingly. > > > > > > I have to say that it seems fairly perilous to create and maintain this > > > invisible hierarchy of Views. Good luck. > > > > Hi Evan, > > The plan was to make the child views visible - i.e. move the rendering also to > > the > > child view. Not sure if that changes your opinion. -i.e. are you concerned > about > > the "invisible" part (which we plan to make visible) or the "hierarchy" part. > > > > We don't need the child views for this change - I can process all the > > accessibility > > events on the parent view - but it was recommended that the event be forwarded > > to the child view. > > -sashi. > > using an actual view hierarchy for everything (rendering, events, etc.) would be > less worrisome. You still need to worry about keeping it in sync with the > controller but perhaps it's enough to rebuild it all in > UpdateBoundsAndRedrawPopup. Thanks Evan, I could see the problematic codepath but couldn't find a repro!
On 2017/03/03 17:45:00, Mathieu Perreault wrote: > On 2017/03/03 05:26:56, Evan Stade wrote: > > On 2017/03/03 05:23:55, csashi wrote: > > > On 2017/03/03 05:04:46, Evan Stade wrote: > > > > On 2017/03/03 04:48:19, csashi wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... > > > > > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2727233003/diff/40001/chrome/browser/ui/views... > > > > > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:73: > > > > > AutofillPopupChildView* child = new AutofillPopupChildView(controller, > i); > > > > > On 2017/03/03 04:41:29, Evan Stade wrote: > > > > > > You can repro this crash by filling in a form, then clicking in the > form > > > > again > > > > > > (popup shows again) then pressing space. > > > AutofillPopupControllerImpl::Show() > > > > > is > > > > > > called with new suggestions. It reuses the previous AutofillPopupView. > > > > > > > > > > > > Here in the ctor you are assuming that the suggestions won't change > over > > > the > > > > > > lifetime of the AutofillPopupView. Unfortunately this is an incorrect > > > > > > assumption. The newly modified suggestions don't match the view > > hierarchy > > > > > you've > > > > > > created here. > > > > > > > > > > Thanks! This was Mathieu's hypothesis as well but I did not know how to > > > force > > > > > this scenario. Will try this and fix accordingly. > > > > > > > > I have to say that it seems fairly perilous to create and maintain this > > > > invisible hierarchy of Views. Good luck. > > > > > > Hi Evan, > > > The plan was to make the child views visible - i.e. move the rendering also > to > > > the > > > child view. Not sure if that changes your opinion. -i.e. are you concerned > > about > > > the "invisible" part (which we plan to make visible) or the "hierarchy" > part. > > > > > > We don't need the child views for this change - I can process all the > > > accessibility > > > events on the parent view - but it was recommended that the event be > forwarded > > > to the child view. > > > -sashi. > > > > using an actual view hierarchy for everything (rendering, events, etc.) would > be > > less worrisome. You still need to worry about keeping it in sync with the > > controller but perhaps it's enough to rebuild it all in > > UpdateBoundsAndRedrawPopup. > > Thanks Evan, I could see the problematic codepath but couldn't find a repro! Hi, Can you please take a look? I was able to reproduce the problem and verified that recreating the child views fixes the problem. I updated bug 697466 with instructions to reproduce. I still have the CHECK here in case there are other code paths that cause a problem. Thanks for pointing out the root cause. -sashi.
lgtm
Description was changed from ========== 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 only change that we use CHECKS to ensure the crash in bug 697466. BUG=627860,697466 ========== to ========== 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. We also include CHECKS to ensure the crash in bug 697466 is caught at the appropriate point in the code. BUG=627860,697466 ==========
Please add test coverage of this case.
On 2017/03/03 19:14:45, sky wrote: > Please add test coverage of this case. Hi Scott, Please take a look. Thanks. -sashi.
Evan, can you review this too? https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc (right): https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. No (c) and 2017. https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:31: const int kNumInitialSuggestions = 3; constexpr https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:65: }; private: DISALLOW... https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:75: }; private: DISALLOW... https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:99: // We intentionally do not destroy this view in the test because of This it leak then? If so, that seems potentially error prone. https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:102: }; private: DISALLOW...
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/view... > File chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc > (right): > > https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... > chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:1: // > Copyright (c) 2012 The Chromium Authors. All rights reserved. > No (c) and 2017. > > https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... > chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:31: > const int kNumInitialSuggestions = 3; > constexpr > > https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... > chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:65: }; > private: DISALLOW... > > https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... > chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:75: }; > private: DISALLOW... > > https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... > chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:99: // > We intentionally do not destroy this view in the test because of > This it leak then? If so, that seems potentially error prone. > > https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... > chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:102: > }; > private: DISALLOW... For clarity, estade should review all changes to c/b/ui/views/autofill as he is an OWNER there.
Hi Scott & Evan, Please take a look. -sashi. https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc (right): https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:1: // Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2017/03/06 18:02:57, sky wrote: > No (c) and 2017. Done. https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:31: const int kNumInitialSuggestions = 3; On 2017/03/06 18:02:56, sky wrote: > constexpr Done. https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:65: }; On 2017/03/06 18:02:57, sky wrote: > private: DISALLOW... Done. https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:75: }; On 2017/03/06 18:02:57, sky wrote: > private: DISALLOW... Done. https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:99: // We intentionally do not destroy this view in the test because of On 2017/03/06 18:02:57, sky wrote: > This it leak then? If so, that seems potentially error prone. The browser_tests runs fine. If this is important, I can make RemoveObserver virtual and mock it. FWIW, AutofillPopupBaseViewTest also has the same issue. https://codereview.chromium.org/2727233003/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:102: }; On 2017/03/06 18:02:57, sky wrote: > private: DISALLOW... Done.
https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right): https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:265: EXPECT_CALL(*autofill_popup_controller_, NotifyAccessibilityEventForRow(_, _)) perhaps it's beyond the scope of this change, but it seems like a better test API would be to mock out the view instead of the controller. Then you don't have to write one-line functions which only exist for testing and you also verify that the controller is communicating with the view correctly (here, you don't have any actual coverage of that one line that's in the function you're overriding). https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:98: // suggestion content may change. this comment makes me even more skeptical you want a hierarchy of views here. The views amount to a container for an index. You could just make GetAccessibleNodeData for |this| smarter (i.e. it knows it's handling a row selection event) and call NotifyAccessibilityEvent on |this|. Or, you could create just one child view to represent the selected row and update its index as needed. I would also think that since these child views don't have any visible bounds, they aren't playing nicely with the screen reader which shows a very big and obvious orange highlight on the thing that's selected (row in a menu, link with keyboard focus, etc.). See the |location| property of AxNodeData. https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:34: void UpdateBoundsAndRedrawPopup() override; probably better to change this to a more generic name now. Something like OnSuggestionsChanged. https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:46: // Creates one child view for every suggestion. nit: newline before this line. Also a more informative comment might be: "Recreates child views based on the suggestions given by |controller_|. These child views are used for accessibility events only."
Hi Evan & Scott, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc (right): https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc:265: EXPECT_CALL(*autofill_popup_controller_, NotifyAccessibilityEventForRow(_, _)) On 2017/03/06 21:45:33, Evan Stade wrote: > perhaps it's beyond the scope of this change, but it seems like a better test > API would be to mock out the view instead of the controller. Then you don't have > to write one-line functions which only exist for testing and you also verify > that the controller is communicating with the view correctly (here, you don't > have any actual coverage of that one line that's in the function you're > overriding). Done. https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:98: // suggestion content may change. On 2017/03/06 21:45:33, Evan Stade wrote: > this comment makes me even more skeptical you want a hierarchy of views here. > The views amount to a container for an index. You could just make > GetAccessibleNodeData for |this| smarter (i.e. it knows it's handling a row > selection event) and call NotifyAccessibilityEvent on |this|. Or, you could > create just one child view to represent the selected row and update its index as > needed. > > I would also think that since these child views don't have any visible bounds, > they aren't playing nicely with the screen reader which shows a very big and > obvious orange highlight on the thing that's selected (row in a menu, link with > keyboard focus, etc.). See the |location| property of AxNodeData. The child views are limited to accessibility events for now in order to limit the scope of this change, but we want to move more of the rendering to the child views. If we still want to eliminate child views, we have to make an assumption that GetAccessibleNodeData corresponds 1-1 with a row selection event. For e.g., we will return the incorrect AXNodeData for the following sequence of calls. Select row 1 Select row 2 GetAccessibleNodeData for first selection. Perhaps this sequence can never happen now, but I was not sure about it. FWIW, Dominic suggested firing the event on the specific selection rather than the containing view (see https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views...). Reg. the visible bounds - yes, the orange box is not highlighting correctly but I am not sure if a limitation of Chrome VOX (which we hope to fix) may also be contributing to that - the orange box leaves the popup and goes to the HTML elements (see https://codereview.chromium.org/2670003002/#msg20). In any event, speaking the selections seemed like an improvement, even if orange box may not be correct. https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.h (right): https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:34: void UpdateBoundsAndRedrawPopup() override; On 2017/03/06 21:45:33, Evan Stade wrote: > probably better to change this to a more generic name now. Something like > OnSuggestionsChanged. Done. https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.h:46: // Creates one child view for every suggestion. On 2017/03/06 21:45:33, Evan Stade wrote: > nit: newline before this line. Also a more informative comment might be: > "Recreates child views based on the suggestions given by |controller_|. These > child views are used for accessibility events only." Done.
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:98: // suggestion content may change. On 2017/03/07 01:53:53, csashi wrote: > On 2017/03/06 21:45:33, Evan Stade wrote: > > this comment makes me even more skeptical you want a hierarchy of views here. > > The views amount to a container for an index. You could just make > > GetAccessibleNodeData for |this| smarter (i.e. it knows it's handling a row > > selection event) and call NotifyAccessibilityEvent on |this|. Or, you could > > create just one child view to represent the selected row and update its index > as > > needed. > > > > I would also think that since these child views don't have any visible bounds, > > they aren't playing nicely with the screen reader which shows a very big and > > obvious orange highlight on the thing that's selected (row in a menu, link > with > > keyboard focus, etc.). See the |location| property of AxNodeData. > > The child views are limited to accessibility events for now in order to limit > the scope of this change, but we want to move more of the rendering to the child > views. > > If we still want to eliminate child views, we have to make an assumption that > GetAccessibleNodeData corresponds 1-1 with a row selection event. For e.g., we > will return the incorrect AXNodeData for the following sequence of calls. > > Select row 1 > Select row 2 > GetAccessibleNodeData for first selection. > > Perhaps this sequence can never happen now, but I was not sure about it. FWIW, > Dominic suggested firing the event on the specific selection rather than the > containing view (see > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views...). > > Reg. the visible bounds - yes, the orange box is not highlighting correctly but > I am not sure if a limitation of Chrome VOX (which we hope to fix) may also be > contributing to that - the orange box leaves the popup and goes to the HTML > elements (see https://codereview.chromium.org/2670003002/#msg20). In any event, > speaking the selections seemed like an improvement, even if orange box may not > be correct. this is all good info, I'm just trying to judge this change on its own merits. It's hard to imagine what the code will look like in the future world where the views are being used for rendering as well --- what needs to go into the child views? Will NotifyAccessibilityEventForRow be needed at all? It seems like a lot of the changes you're making now might be undone soon. Since we've never had good a11y for this menu it doesn't seem urgent to get it in now before we've solved the problematic controller/view split. tl;dr I appreciate the desire to limit the scope of a change but this seems to be tackling the problems we have in the wrong order.
Hi Evan & Scott, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:98: // suggestion content may change. On 2017/03/08 18:03:21, Evan Stade wrote: > On 2017/03/07 01:53:53, csashi wrote: > > On 2017/03/06 21:45:33, Evan Stade wrote: > > > this comment makes me even more skeptical you want a hierarchy of views > here. > > > The views amount to a container for an index. You could just make > > > GetAccessibleNodeData for |this| smarter (i.e. it knows it's handling a row > > > selection event) and call NotifyAccessibilityEvent on |this|. Or, you could > > > create just one child view to represent the selected row and update its > index > > as > > > needed. > > > > > > I would also think that since these child views don't have any visible > bounds, > > > they aren't playing nicely with the screen reader which shows a very big and > > > obvious orange highlight on the thing that's selected (row in a menu, link > > with > > > keyboard focus, etc.). See the |location| property of AxNodeData. > > > > The child views are limited to accessibility events for now in order to limit > > the scope of this change, but we want to move more of the rendering to the > child > > views. > > > > If we still want to eliminate child views, we have to make an assumption that > > GetAccessibleNodeData corresponds 1-1 with a row selection event. For e.g., we > > will return the incorrect AXNodeData for the following sequence of calls. > > > > Select row 1 > > Select row 2 > > GetAccessibleNodeData for first selection. > > > > Perhaps this sequence can never happen now, but I was not sure about it. FWIW, > > Dominic suggested firing the event on the specific selection rather than the > > containing view (see > > > https://codereview.chromium.org/2670003002/diff/40001/chrome/browser/ui/views...). > > > > Reg. the visible bounds - yes, the orange box is not highlighting correctly > but > > I am not sure if a limitation of Chrome VOX (which we hope to fix) may also be > > contributing to that - the orange box leaves the popup and goes to the HTML > > elements (see https://codereview.chromium.org/2670003002/#msg20). In any > event, > > speaking the selections seemed like an improvement, even if orange box may not > > be correct. > > this is all good info, I'm just trying to judge this change on its own merits. > It's hard to imagine what the code will look like in the future world where the > views are being used for rendering as well --- what needs to go into the child > views? Will NotifyAccessibilityEventForRow be needed at all? It seems like a lot > of the changes you're making now might be undone soon. Since we've never had > good a11y for this menu it doesn't seem urgent to get it in now before we've > solved the problematic controller/view split. > > tl;dr I appreciate the desire to limit the scope of a change but this seems to > be tackling the problems we have in the wrong order. The intent is to move the code from OnPaint in the parent view to the child view. I have not looked at this in detail because the immediate need was fixing accessibility but I don't think the code for accessibility will change when we move the rendering. Having said this, I am happy to remove the child views if we can be sure that there is a 1-1 correspondence between |NotifyAccessibilityEvent| and |GetAXNodeData|. I did not want to assume that. I have removed NotifyAccessibilityEventForRow from this class and other wrapper functions. It wasn't needed. But I had to one more bool parameter to |InvalidateRow|. I think that is a reasonable parameter to add even without the accessibility change - for e.g. we way want to render the selected row differently.
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
unfortunately I'll be ooo until next Tuesday or Wednesday so I won't be able to continue with this review for a while. https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:51: // suggestions. See | AutofillPopupControllerImpl::CanAccept|. nit: I'm not sure this comment is necessary, simply because it would be unwieldy to add a comment to the code for every thing we /don't/ do. https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:53: node_data->SetName(controller_->GetElidedValueAt(index_)); it seems like we shouldn't be using the elided version but the full value https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:114: CHECK_EQ(controller_->GetLineCount(), static_cast<size_t>(child_count())); nit: s/check/dcheck https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:130: void AutofillPopupViewViews::InvalidateRow(size_t row, bool is_selected) { I think this is reasonable, but you should change the name of the function to reflect what it's actually doing, and you can probably just call it once instead of twice. SelectedRowChanged(size_t previous_row_selection, size_t current_row_selection) { ... }
Hi Evan & Scott, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:51: // suggestions. See | AutofillPopupControllerImpl::CanAccept|. On 2017/03/09 03:21:15, Evan Stade wrote: > nit: I'm not sure this comment is necessary, simply because it would be unwieldy > to add a comment to the code for every thing we /don't/ do. Done. https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:53: node_data->SetName(controller_->GetElidedValueAt(index_)); On 2017/03/09 03:21:16, Evan Stade wrote: > it seems like we shouldn't be using the elided version but the full value Done. https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:114: CHECK_EQ(controller_->GetLineCount(), static_cast<size_t>(child_count())); On 2017/03/09 03:21:16, Evan Stade wrote: > nit: s/check/dcheck Done. https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:114: CHECK_EQ(controller_->GetLineCount(), static_cast<size_t>(child_count())); On 2017/03/09 03:21:16, Evan Stade wrote: > nit: s/check/dcheck Done. https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:130: void AutofillPopupViewViews::InvalidateRow(size_t row, bool is_selected) { On 2017/03/09 03:21:16, Evan Stade wrote: > I think this is reasonable, but you should change the name of the function to > reflect what it's actually doing, and you can probably just call it once instead > of twice. > > SelectedRowChanged(size_t previous_row_selection, size_t current_row_selection) > { > ... > } Done.
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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. We also include CHECKS to ensure the crash in bug 697466 is caught at the appropriate point in the code. BUG=627860,697466 ========== to ========== 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 ==========
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...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
csashi, I would prefer to have Evan review this as he is a local owner and more knowledgeable of the code than I. Can you wait until he is back? -Scott On Wed, Mar 8, 2017 at 9:16 PM, csashi via Chromium-reviews <chromium-reviews@chromium.org> wrote: > Hi Evan & Scott, > Please take a look. Thanks, > -sashi. > > > https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... > File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc > (right): > > https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:51: // > suggestions. See | AutofillPopupControllerImpl::CanAccept|. > On 2017/03/09 03:21:15, Evan Stade wrote: >> nit: I'm not sure this comment is necessary, simply because it would > be unwieldy >> to add a comment to the code for every thing we /don't/ do. > > Done. > > https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:53: > node_data->SetName(controller_->GetElidedValueAt(index_)); > On 2017/03/09 03:21:16, Evan Stade wrote: >> it seems like we shouldn't be using the elided version but the full > value > > Done. > > https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:114: > CHECK_EQ(controller_->GetLineCount(), > static_cast<size_t>(child_count())); > On 2017/03/09 03:21:16, Evan Stade wrote: >> nit: s/check/dcheck > > Done. > > https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:114: > CHECK_EQ(controller_->GetLineCount(), > static_cast<size_t>(child_count())); > On 2017/03/09 03:21:16, Evan Stade wrote: >> nit: s/check/dcheck > > Done. > > https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... > chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:130: void > AutofillPopupViewViews::InvalidateRow(size_t row, bool is_selected) { > On 2017/03/09 03:21:16, Evan Stade wrote: >> I think this is reasonable, but you should change the name of the > function to >> reflect what it's actually doing, and you can probably just call it > once instead >> of twice. >> >> SelectedRowChanged(size_t previous_row_selection, size_t > current_row_selection) >> { >> ... >> } > > Done. > > https://codereview.chromium.org/2727233003/ -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
thanks for your patience. https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:65: constexpr int AutofillPopupView::kNoSelection; I'm confused why you need this line or what it's doing https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:98: // suggestion content may change. I think we should remove this conditional and the comment about it. I don't think the optimization is buying us much and may not be valid in the near future. https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:133: if (previous_row_selection != static_cast<size_t>(kNoSelection) && it seems off that you're casting -1 to a size_t. Either use int for the row indexing or use base::Optional<size_t> (I prefer the latter) https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:136: controller_->layout_model().GetRowBounds(previous_row_selection)); nit: please use curlies and add a newline after https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:138: DCHECK_GE(current_row_selection, static_cast<size_t>(0)); is it even possible for this check to fail? size_t is always >= 0 https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:139: DCHECK_LT(current_row_selection, controller_->GetLineCount()); I think these DCHECKs are a little over the top and are making this function hard to read. It's not the view's job to verify the controller is acting sensibly. https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:144: DCHECK(child); nit: this is redundant with the check on L142 I think all DCHECKs in this function can boil down to making sure child_count() == GetLineCount(). But it should be enough that you added that in OnPaint.
Hi Evan and Scott, Please take a look. Thanks! -sashi. https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:65: constexpr int AutofillPopupView::kNoSelection; On 2017/03/15 15:18:58, Evan Stade wrote: > I'm confused why you need this line or what it's doing It is a constant to indicate no selection. I tried removing it and converting everything to Optional, but because we don't want to pass Optional as argument, I still need a special value to indicate there was no selection. If I remove this, I would need to convert the parameters to size_t * and pass nullptr to mean no selection, but that seemed less desirable than having a special value for no selection. https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:98: // suggestion content may change. On 2017/03/15 15:18:58, Evan Stade wrote: > I think we should remove this conditional and the comment about it. I don't > think the optimization is buying us much and may not be valid in the near > future. Done. https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:133: if (previous_row_selection != static_cast<size_t>(kNoSelection) && On 2017/03/15 15:18:58, Evan Stade wrote: > it seems off that you're casting -1 to a size_t. Either use int for the row > indexing or use base::Optional<size_t> (I prefer the latter) Done. I did not use optional because of following documentation: https://codesearch.chromium.org/chromium/src/docs/optional.md?q=base::Optiona... https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:136: controller_->layout_model().GetRowBounds(previous_row_selection)); On 2017/03/15 15:18:58, Evan Stade wrote: > nit: please use curlies and add a newline after Done. https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:138: DCHECK_GE(current_row_selection, static_cast<size_t>(0)); On 2017/03/15 15:18:58, Evan Stade wrote: > is it even possible for this check to fail? size_t is always >= 0 Done. https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:139: DCHECK_LT(current_row_selection, controller_->GetLineCount()); On 2017/03/15 15:18:58, Evan Stade wrote: > I think these DCHECKs are a little over the top and are making this function > hard to read. It's not the view's job to verify the controller is acting > sensibly. Done. Agreed. I did not know suggestions could changes without recreating this class and could not understand the root cause of the bug and went overboard. https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:144: DCHECK(child); On 2017/03/15 15:18:58, Evan Stade wrote: > nit: this is redundant with the check on L142 > > I think all DCHECKs in this function can boil down to making sure child_count() > == GetLineCount(). But it should be enough that you added that in OnPaint. Done.
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:65: constexpr int AutofillPopupView::kNoSelection; On 2017/03/15 21:31:09, csashi wrote: > On 2017/03/15 15:18:58, Evan Stade wrote: > > I'm confused why you need this line or what it's doing > > It is a constant to indicate no selection. I understand the purpose of kNoSelection, but it's declared and defined in a header and then re-declared here with no definition. This line serves what purpose? > I tried removing it and converting > everything to Optional, but because we don't want to pass Optional as argument, why not?
https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... 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: > On 2017/03/15 21:31:09, csashi wrote: > > On 2017/03/15 15:18:58, Evan Stade wrote: > > > I'm confused why you need this line or what it's doing > > > > It is a constant to indicate no selection. > > I understand the purpose of kNoSelection, but it's declared and defined in a > header and then re-declared here with no definition. This line serves what > purpose? Don't we need an initializer for static variable? > > > I tried removing it and converting > > everything to Optional, but because we don't want to pass Optional as > argument, > > why not? I did not use optional based on following documentation: https://codesearch.chromium.org/chromium/src/docs/optional.md?q=base::Optiona...
https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:65: constexpr int AutofillPopupView::kNoSelection; On 2017/03/16 01:50:55, csashi wrote: > On 2017/03/16 01:23:44, Evan Stade wrote: > > On 2017/03/15 21:31:09, csashi wrote: > > > On 2017/03/15 15:18:58, Evan Stade wrote: > > > > I'm confused why you need this line or what it's doing > > > > > > It is a constant to indicate no selection. > > > > I understand the purpose of kNoSelection, but it's declared and defined in a > > header and then re-declared here with no definition. This line serves what > > purpose? > > Don't we need an initializer for static variable? Removed. I must have misunderstood. > > > > > > I tried removing it and converting > > > everything to Optional, but because we don't want to pass Optional as > > argument, > > > > why not? > > > I did not use optional based on following documentation: > https://codesearch.chromium.org/chromium/src/docs/optional.md?q=base::Optiona...
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/280001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:65: constexpr int AutofillPopupView::kNoSelection; On 2017/03/16 01:53:41, csashi wrote: > On 2017/03/16 01:50:55, csashi wrote: > > On 2017/03/16 01:23:44, Evan Stade wrote: > > > On 2017/03/15 21:31:09, csashi wrote: > > > > On 2017/03/15 15:18:58, Evan Stade wrote: > > > > > I'm confused why you need this line or what it's doing > > > > > > > > It is a constant to indicate no selection. > > > > > > I understand the purpose of kNoSelection, but it's declared and defined in a > > > header and then re-declared here with no definition. This line serves what > > > purpose? > > > > Don't we need an initializer for static variable? > > Removed. I must have misunderstood. > > > > > > > > > > I tried removing it and converting > > > > everything to Optional, but because we don't want to pass Optional as > > > argument, > > > > > > why not? > > > > > > I did not use optional based on following documentation: > > > https://codesearch.chromium.org/chromium/src/docs/optional.md?q=base::Optiona... > Sorry for the flip-flop. I put it back based on the advice @ https://engdoc.corp.google.com/eng/doc/devguide/cpp/primer.shtml?cl=head#stat.... I noticed that there are some cases in Chrome where we have skipped the definition in the .cc (e.g. kMaxPolymorphicMapCount) and some cases where we have both the declaration and definition (e.g. RenderText::kDragToEndIfOutsideVerticalBounds).
On 2017/03/16 01:50:55, csashi wrote: > > I did not use optional based on following documentation: > https://codesearch.chromium.org/chromium/src/docs/optional.md?q=base::Optiona... that documentation goes on to say: "it is recommended to keep using `T*` for arguments that can be omitted, with `nullptr` representing no value." However, I think that is a recommendation and not a "rule" per se. The only reason kNoSelection currently exists is that base::Optional wasn't around when it was first written.
Hi Evan, Please take a look. Thanks, -sashi.
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... 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/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:398: view_->OnSelectedRowChanged(selected_line_, selected_line); calling this before you change the value of selected_line_ is a little weird because it means |current_selected_line| will not match controller_->selected_line(). You might be able to get by without even adding any params to this function, assuming you call it after updating selected_line_. I'm not sure if scheduling a paint in just the selected and unselected lines actually paints less than if you repainted the whole popup. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.h:102: base::Optional<size_t> selected_line() const override; is this called from somewhere? https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:37: // |controller| should not be NULL. in this case you can probably make controller_ (and the ctor param) a const ref. On the other hand, for now (and probably in the future) you can get by with just passing a reference to an autofill::Suggestion. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:93: RemoveAllChildViews(true /* delete_children */); nit: seems like this should be inside CreateChildViews https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:126: previous_row_selection < controller_->GetLineCount()) { why is this second check required? https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:133: current_row_selection.value())); s/current_row_selection.value()/*current_row_selection/ (here and elsewhere) https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:137: DCHECK_EQ(child->GetClassName(), AutofillPopupChildView::kViewClassName); not sure this is necessary https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:272: AddChildView(child); nit: combine these two lines.
Hi Evan, Thanks for the time you are investing in this review. Please take a look. -sashi, https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:266: if (!CanAccept(suggestions_[selected_line_.value()].frontend_id)) On 2017/03/16 17:39:00, Evan Stade wrote: > *selected_line_ Done. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:398: view_->OnSelectedRowChanged(selected_line_, selected_line); On 2017/03/16 17:39:00, Evan Stade wrote: > calling this before you change the value of selected_line_ is a little weird > because it means |current_selected_line| will not match > controller_->selected_line(). Done. > > You might be able to get by without even adding any params to this function, > assuming you call it after updating selected_line_. I'm not sure if scheduling a > paint in just the selected and unselected lines actually paints less than if you > repainted the whole popup. Perhaps - but seems better if the view can decide this - no? https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.h:102: base::Optional<size_t> selected_line() const override; On 2017/03/16 17:39:00, Evan Stade wrote: > is this called from somewhere? Currently only from the tests. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:37: // |controller| should not be NULL. On 2017/03/16 17:39:00, Evan Stade wrote: > in this case you can probably make controller_ (and the ctor param) a const ref. > > On the other hand, for now (and probably in the future) you can get by with just > passing a reference to an autofill::Suggestion. Done. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:93: RemoveAllChildViews(true /* delete_children */); On 2017/03/16 17:39:00, Evan Stade wrote: > nit: seems like this should be inside CreateChildViews Done. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:126: previous_row_selection < controller_->GetLineCount()) { On 2017/03/16 17:39:00, Evan Stade wrote: > why is this second check required? Because the previous_row_selection could be for a row that has been deleted? I ported this check from the check that was present in the controller (https://codesearch.chromium.org/chromium/src/chrome/browser/ui/autofill/autof...) https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:133: current_row_selection.value())); On 2017/03/16 17:39:00, Evan Stade wrote: > s/current_row_selection.value()/*current_row_selection/ > > (here and elsewhere) Done. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:137: DCHECK_EQ(child->GetClassName(), AutofillPopupChildView::kViewClassName); On 2017/03/16 17:39:00, Evan Stade wrote: > not sure this is necessary Acknowledged. A previous reviewer asked that I add it in case other types of child views were added. Seemed reasonable to check this. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:272: AddChildView(child); On 2017/03/16 17:39:00, Evan Stade wrote: > nit: combine these two lines. Done.
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
> Thanks for the time you are investing in this review. Please take a look. And thanks again for your patience. I should have cleaned this code up more when I was active on it. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:398: view_->OnSelectedRowChanged(selected_line_, selected_line); On 2017/03/16 22:42:05, csashi wrote: > On 2017/03/16 17:39:00, Evan Stade wrote: > > calling this before you change the value of selected_line_ is a little weird > > because it means |current_selected_line| will not match > > controller_->selected_line(). > > Done. > > > > > You might be able to get by without even adding any params to this function, > > assuming you call it after updating selected_line_. I'm not sure if scheduling > a > > paint in just the selected and unselected lines actually paints less than if > you > > repainted the whole popup. > > Perhaps - but seems better if the view can decide this - no? > Maybe, but then again in this CL the view is still deciding to make its implementation more elaborate. I'd envision: AutofillPopupViewViews::OnSelectedRowChanged() { SchedulePaint(); if (controller_->selected_line()) child_at(*controller_->selected_line())->NotifyAccessibilityEvent( ui::AX_EVENT_SELECTION, true); } https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:126: previous_row_selection < controller_->GetLineCount()) { On 2017/03/16 22:42:06, csashi wrote: > On 2017/03/16 17:39:00, Evan Stade wrote: > > why is this second check required? > > Because the previous_row_selection could be for a row that has been deleted? I > ported this check from the check that was present in the controller > (https://codesearch.chromium.org/chromium/src/chrome/browser/ui/autofill/autof...) > but that check looks like it turned into a dcheck in the controller https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:137: DCHECK_EQ(child->GetClassName(), AutofillPopupChildView::kViewClassName); On 2017/03/16 22:42:06, csashi wrote: > On 2017/03/16 17:39:00, Evan Stade wrote: > > not sure this is necessary > > Acknowledged. A previous reviewer asked that I add it in case other types of > child views were added. Seemed reasonable to check this. I mean, lots of things would break then right, like the size/child count checks. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:139: static_cast<AutofillPopupChildView*>(child); nit: do you need this cast?
Hi Evan, Can you take a look? Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:398: view_->OnSelectedRowChanged(selected_line_, selected_line); On 2017/03/17 14:11:54, Evan Stade wrote: > On 2017/03/16 22:42:05, csashi wrote: > > On 2017/03/16 17:39:00, Evan Stade wrote: > > > calling this before you change the value of selected_line_ is a little weird > > > because it means |current_selected_line| will not match > > > controller_->selected_line(). > > > > Done. > > > > > > > > You might be able to get by without even adding any params to this function, > > > assuming you call it after updating selected_line_. I'm not sure if > scheduling > > a > > > paint in just the selected and unselected lines actually paints less than if > > you > > > repainted the whole popup. > > > > Perhaps - but seems better if the view can decide this - no? > > > > Maybe, but then again in this CL the view is still deciding to make its > implementation more elaborate. I'd envision: > > AutofillPopupViewViews::OnSelectedRowChanged() { > SchedulePaint(); > if (controller_->selected_line()) > child_at(*controller_->selected_line())->NotifyAccessibilityEvent( > ui::AX_EVENT_SELECTION, true); > } Assuming SchedulePaintInRect on 2 rectangles is not faster than SchedulePaint, this is clearly simpler. We are currently using https://developer.apple.com/reference/uikit/uiview/1622587-setneedsdisplayinrect to redraw specific rectangles. I could change the rect parameter that we pass in https://codesearch.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill... to the rect for the whole frame (https://developer.apple.com/reference/appkit/nsview/1483458-initwithframe) - I expect this will have a similar effect as changing SchedulePaintInRect to SchedulePaint, but I don't have a way to build on Mac to verify this. Can you confirm that using the rect for the whole frame on Mac is a safe change? https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:126: previous_row_selection < controller_->GetLineCount()) { On 2017/03/17 14:11:55, Evan Stade wrote: > On 2017/03/16 22:42:06, csashi wrote: > > On 2017/03/16 17:39:00, Evan Stade wrote: > > > why is this second check required? > > > > Because the previous_row_selection could be for a row that has been deleted? I > > ported this check from the check that was present in the controller > > > (https://codesearch.chromium.org/chromium/src/chrome/browser/ui/autofill/autof...) > > > > but that check looks like it turned into a dcheck in the controller The dcheck in the controller is for the newly selected line (which needs to be < |GetLineCount()|. IIUC, the previous selection can be >= |GetLineCount()| and we don't have a dcheck for that. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:137: DCHECK_EQ(child->GetClassName(), AutofillPopupChildView::kViewClassName); On 2017/03/17 14:11:55, Evan Stade wrote: > On 2017/03/16 22:42:06, csashi wrote: > > On 2017/03/16 17:39:00, Evan Stade wrote: > > > not sure this is necessary > > > > Acknowledged. A previous reviewer asked that I add it in case other types of > > child views were added. Seemed reasonable to check this. > > I mean, lots of things would break then right, like the size/child count checks. Done. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:139: static_cast<AutofillPopupChildView*>(child); On 2017/03/17 14:11:55, Evan Stade wrote: > nit: do you need this cast? Done.
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... 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 2017/03/17 14:11:54, Evan Stade wrote: > > On 2017/03/16 22:42:05, csashi wrote: > > > On 2017/03/16 17:39:00, Evan Stade wrote: > > > > calling this before you change the value of selected_line_ is a little > weird > > > > because it means |current_selected_line| will not match > > > > controller_->selected_line(). > > > > > > Done. > > > > > > > > > > > You might be able to get by without even adding any params to this > function, > > > > assuming you call it after updating selected_line_. I'm not sure if > > scheduling > > > a > > > > paint in just the selected and unselected lines actually paints less than > if > > > you > > > > repainted the whole popup. > > > > > > Perhaps - but seems better if the view can decide this - no? > > > > > > > Maybe, but then again in this CL the view is still deciding to make its > > implementation more elaborate. I'd envision: > > > > AutofillPopupViewViews::OnSelectedRowChanged() { > > SchedulePaint(); > > if (controller_->selected_line()) > > child_at(*controller_->selected_line())->NotifyAccessibilityEvent( > > ui::AX_EVENT_SELECTION, true); > > } > > Assuming SchedulePaintInRect on 2 rectangles is not faster than SchedulePaint, > this is clearly simpler. > > We are currently using > https://developer.apple.com/reference/uikit/uiview/1622587-setneedsdisplayinrect > to redraw specific rectangles. I could change the rect parameter that we pass in > https://codesearch.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill... > to the rect for the whole frame > (https://developer.apple.com/reference/appkit/nsview/1483458-initwithframe) - I > expect this will have a similar effect as changing SchedulePaintInRect to > SchedulePaint, but I don't have a way to build on Mac to verify this. Can you > confirm that using the rect for the whole frame on Mac is a safe change? I'm only talking about the Views implementation right now, not cocoa. I'm pretty sure that in Views, schedulepaintinrect winds up repainting the whole view (to my surprise, but when I tried to fix that it got real complicated real fast). https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:126: previous_row_selection < controller_->GetLineCount()) { On 2017/03/17 16:30:35, csashi wrote: > On 2017/03/17 14:11:55, Evan Stade wrote: > > On 2017/03/16 22:42:06, csashi wrote: > > > On 2017/03/16 17:39:00, Evan Stade wrote: > > > > why is this second check required? > > > > > > Because the previous_row_selection could be for a row that has been deleted? > I > > > ported this check from the check that was present in the controller > > > > > > (https://codesearch.chromium.org/chromium/src/chrome/browser/ui/autofill/autof...) > > > > > > > but that check looks like it turned into a dcheck in the controller > > The dcheck in the controller is for the newly selected line (which needs to be < > |GetLineCount()|. IIUC, the previous selection can be >= |GetLineCount()| and we > don't have a dcheck for that. fair enough, but I think somewhere in Show() you should be resetting the line selection to none as necessary, and it should be the controller's job to make sure previous_row_selection is a valid row. https://codereview.chromium.org/2727233003/diff/520001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/520001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:398: const base::Optional<size_t> previous_selected_line(selected_line_); auto previous_selected_line = selected_line_;
Hi Evan, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:398: view_->OnSelectedRowChanged(selected_line_, selected_line); On 2017/03/17 19:08:54, Evan Stade wrote: > On 2017/03/17 16:30:35, csashi wrote: > > On 2017/03/17 14:11:54, Evan Stade wrote: > > > On 2017/03/16 22:42:05, csashi wrote: > > > > On 2017/03/16 17:39:00, Evan Stade wrote: > > > > > calling this before you change the value of selected_line_ is a little > > weird > > > > > because it means |current_selected_line| will not match > > > > > controller_->selected_line(). > > > > > > > > Done. > > > > > > > > > > > > > > You might be able to get by without even adding any params to this > > function, > > > > > assuming you call it after updating selected_line_. I'm not sure if > > > scheduling > > > > a > > > > > paint in just the selected and unselected lines actually paints less > than > > if > > > > you > > > > > repainted the whole popup. > > > > > > > > Perhaps - but seems better if the view can decide this - no? > > > > > > > > > > Maybe, but then again in this CL the view is still deciding to make its > > > implementation more elaborate. I'd envision: > > > > > > AutofillPopupViewViews::OnSelectedRowChanged() { > > > SchedulePaint(); > > > if (controller_->selected_line()) > > > child_at(*controller_->selected_line())->NotifyAccessibilityEvent( > > > ui::AX_EVENT_SELECTION, true); > > > } > > > > Assuming SchedulePaintInRect on 2 rectangles is not faster than SchedulePaint, > > this is clearly simpler. > > > > We are currently using > > > https://developer.apple.com/reference/uikit/uiview/1622587-setneedsdisplayinrect > > to redraw specific rectangles. I could change the rect parameter that we pass > in > > > https://codesearch.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill... > > to the rect for the whole frame > > (https://developer.apple.com/reference/appkit/nsview/1483458-initwithframe) - > I > > expect this will have a similar effect as changing SchedulePaintInRect to > > SchedulePaint, but I don't have a way to build on Mac to verify this. Can you > > confirm that using the rect for the whole frame on Mac is a safe change? > > I'm only talking about the Views implementation right now, not cocoa. I'm pretty > sure that in Views, schedulepaintinrect winds up repainting the whole view (to > my surprise, but when I tried to fix that it got real complicated real fast). Do you mean you want to have two function signatures for OnSelectedRowChanged? - one that takes the previous_row (and the current_row) and one for Views implementation? Both Cocoa and Views implementation implement the interface defined in chrome/browser/ui/autofill/autofill_popup_view.h. view_ is an implementation of that interface. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views.cc:126: previous_row_selection < controller_->GetLineCount()) { On 2017/03/17 19:08:54, Evan Stade wrote: > On 2017/03/17 16:30:35, csashi wrote: > > On 2017/03/17 14:11:55, Evan Stade wrote: > > > On 2017/03/16 22:42:06, csashi wrote: > > > > On 2017/03/16 17:39:00, Evan Stade wrote: > > > > > why is this second check required? > > > > > > > > Because the previous_row_selection could be for a row that has been > deleted? > > I > > > > ported this check from the check that was present in the controller > > > > > > > > > > (https://codesearch.chromium.org/chromium/src/chrome/browser/ui/autofill/autof...) > > > > > > > > > > but that check looks like it turned into a dcheck in the controller > > > > The dcheck in the controller is for the newly selected line (which needs to be > < > > |GetLineCount()|. IIUC, the previous selection can be >= |GetLineCount()| and > we > > don't have a dcheck for that. > > fair enough, but I think somewhere in Show() you should be resetting the line > selection to none as necessary, and it should be the controller's job to make > sure previous_row_selection is a valid row. Done.
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Hi Evan, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/400001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:398: view_->OnSelectedRowChanged(selected_line_, selected_line); On 2017/03/17 21:55:21, csashi wrote: > On 2017/03/17 19:08:54, Evan Stade wrote: > > On 2017/03/17 16:30:35, csashi wrote: > > > On 2017/03/17 14:11:54, Evan Stade wrote: > > > > On 2017/03/16 22:42:05, csashi wrote: > > > > > On 2017/03/16 17:39:00, Evan Stade wrote: > > > > > > calling this before you change the value of selected_line_ is a little > > > weird > > > > > > because it means |current_selected_line| will not match > > > > > > controller_->selected_line(). > > > > > > > > > > Done. > > > > > > > > > > > > > > > > > You might be able to get by without even adding any params to this > > > function, > > > > > > assuming you call it after updating selected_line_. I'm not sure if > > > > scheduling > > > > > a > > > > > > paint in just the selected and unselected lines actually paints less > > than > > > if > > > > > you > > > > > > repainted the whole popup. > > > > > > > > > > Perhaps - but seems better if the view can decide this - no? > > > > > > > > > > > > > Maybe, but then again in this CL the view is still deciding to make its > > > > implementation more elaborate. I'd envision: > > > > > > > > AutofillPopupViewViews::OnSelectedRowChanged() { > > > > SchedulePaint(); > > > > if (controller_->selected_line()) > > > > child_at(*controller_->selected_line())->NotifyAccessibilityEvent( > > > > ui::AX_EVENT_SELECTION, true); > > > > } > > > > > > Assuming SchedulePaintInRect on 2 rectangles is not faster than > SchedulePaint, > > > this is clearly simpler. > > > > > > We are currently using > > > > > > https://developer.apple.com/reference/uikit/uiview/1622587-setneedsdisplayinrect > > > to redraw specific rectangles. I could change the rect parameter that we > pass > > in > > > > > > https://codesearch.chromium.org/chromium/src/chrome/browser/ui/cocoa/autofill... > > > to the rect for the whole frame > > > (https://developer.apple.com/reference/appkit/nsview/1483458-initwithframe) > - > > I > > > expect this will have a similar effect as changing SchedulePaintInRect to > > > SchedulePaint, but I don't have a way to build on Mac to verify this. Can > you > > > confirm that using the rect for the whole frame on Mac is a safe change? > > > > I'm only talking about the Views implementation right now, not cocoa. I'm > pretty > > sure that in Views, schedulepaintinrect winds up repainting the whole view (to > > my surprise, but when I tried to fix that it got real complicated real fast). > > Do you mean you want to have two function signatures for OnSelectedRowChanged? - > one that takes the previous_row (and the current_row) and one for Views > implementation? > Both Cocoa and Views implementation implement the interface defined in > chrome/browser/ui/autofill/autofill_popup_view.h. view_ is an implementation of > that interface. I kept the function signature to pass both previous and current selection, but simplified Views implementation.
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:114: if (selected_line_ && *selected_line_ >= suggestions_.size()) { nit: no curlies https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:376: return selected_line_ && index == static_cast<int>(*selected_line_) since some things are of type size_t and some are int, we have a few annoying casts. I don't think we should be using size_t --- I used to think it was a good idea to use it for things that shouldn't be negative, but that's explicitly mentioned in the style guide as the wrong reason to use size_t. Therefore everything should probably be int. https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:429: int new_selected_line; nit: int new_selected_line = selected_line_.value_or(0) - 1; (this is both shorter than what you have and it doesn't break when the last line is unselectable, which is often the case) https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc (right): https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:128: autofill_popup_view_views_->OnSelectedRowChanged(i - 1, i); hmmm... if this implicit cast to optional works, then there are lots of places you can get rid of explicit base::Optional<>() https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:138: i ? i - 1 : kNumInitialSuggestions - 1, i); I don't understand what this is testing really (a comment about what behavior this loop is designed to test would be nice) but wouldn't you want this to just be kNumInitialSuggestions https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:148: i ? kNumInitialSuggestions : i - 1, i); I don't understand this one either. kNumInitialSuggestions is now out of range given the reduced number of suggestions.
Hi Evan, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:114: if (selected_line_ && *selected_line_ >= suggestions_.size()) { On 2017/03/18 00:47:03, Evan Stade wrote: > nit: no curlies Done. https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:376: return selected_line_ && index == static_cast<int>(*selected_line_) On 2017/03/18 00:47:03, Evan Stade wrote: > since some things are of type size_t and some are int, we have a few annoying > casts. I don't think we should be using size_t --- I used to think it was a good > idea to use it for things that shouldn't be negative, but that's explicitly > mentioned in the style guide as the wrong reason to use size_t. Therefore > everything should probably be int. Acknowledged. There are many places where we are using size_t to refer to a row (examples: GetSuggestionAt, GetElidedValueAt). There are some places where we are using int (examples: GetBackgroundColorIDForRow, RemoveSuggestion). When you say "everything should probably be int", I assumed you wanted every place where we are using size_t to mean an int that is >= 0. https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:429: int new_selected_line; On 2017/03/18 00:47:03, Evan Stade wrote: > nit: int new_selected_line = selected_line_.value_or(0) - 1; > > (this is both shorter than what you have and it doesn't break when the last line > is unselectable, which is often the case) Done. Did you mean selected_line_.value_or(GetLineCount()) - 1? https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/view... File chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc (right): https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:128: autofill_popup_view_views_->OnSelectedRowChanged(i - 1, i); On 2017/03/18 00:47:04, Evan Stade wrote: > hmmm... if this implicit cast to optional works, then there are lots of places > you can get rid of explicit base::Optional<>() Done. https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:138: i ? i - 1 : kNumInitialSuggestions - 1, i); On 2017/03/18 00:47:04, Evan Stade wrote: > I don't understand what this is testing really (a comment about what behavior > this loop is designed to test would be nice) but wouldn't you want this to just > be kNumInitialSuggestions Sorry about that - I haven't been updating this test to reflect changes to the interfaces. Added more comments. https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/view... chrome/browser/ui/views/autofill/autofill_popup_view_views_browsertest.cc:148: i ? kNumInitialSuggestions : i - 1, i); On 2017/03/18 00:47:04, Evan Stade wrote: > I don't understand this one either. kNumInitialSuggestions is now out of range > given the reduced number of suggestions. Done.
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm(!) with last two issues addressed https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:376: return selected_line_ && index == static_cast<int>(*selected_line_) On 2017/03/18 02:25:09, csashi wrote: > On 2017/03/18 00:47:03, Evan Stade wrote: > > since some things are of type size_t and some are int, we have a few annoying > > casts. I don't think we should be using size_t --- I used to think it was a > good > > idea to use it for things that shouldn't be negative, but that's explicitly > > mentioned in the style guide as the wrong reason to use size_t. Therefore > > everything should probably be int. > > Acknowledged. There are many places where we are using size_t to refer to a row > (examples: GetSuggestionAt, GetElidedValueAt). There are some places where we > are using int (examples: GetBackgroundColorIDForRow, RemoveSuggestion). When you > say "everything should probably be int", I assumed you wanted every place where > we are using size_t to mean an int that is >= 0. Yea, when changing things from size_t to int they will still be non-negative. https://codereview.chromium.org/2727233003/diff/580001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/580001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:429: int new_selected_line = selected_line_.value_or(GetLineCount()) - 1; I guess either 0 or GetLineCount() here is effectively the same. But now that you've rewritten it, it looks like there's a bug either way --- if we wrap around the beginning of the suggestions to the end, we don't check if that last suggestion is actually acceptable. Couldn't the last one be an insecure form warning or something? This might warrant adding a test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Hi Evan, Please take a look. Thanks! -sashi. https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/560001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:376: return selected_line_ && index == static_cast<int>(*selected_line_) On 2017/03/18 02:37:48, Evan Stade wrote: > On 2017/03/18 02:25:09, csashi wrote: > > On 2017/03/18 00:47:03, Evan Stade wrote: > > > since some things are of type size_t and some are int, we have a few > annoying > > > casts. I don't think we should be using size_t --- I used to think it was a > > good > > > idea to use it for things that shouldn't be negative, but that's explicitly > > > mentioned in the style guide as the wrong reason to use size_t. Therefore > > > everything should probably be int. > > > > Acknowledged. There are many places where we are using size_t to refer to a > row > > (examples: GetSuggestionAt, GetElidedValueAt). There are some places where we > > are using int (examples: GetBackgroundColorIDForRow, RemoveSuggestion). When > you > > say "everything should probably be int", I assumed you wanted every place > where > > we are using size_t to mean an int that is >= 0. > > Yea, when changing things from size_t to int they will still be non-negative. Acknowledged. https://codereview.chromium.org/2727233003/diff/580001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/580001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:429: int new_selected_line = selected_line_.value_or(GetLineCount()) - 1; On 2017/03/18 02:37:48, Evan Stade wrote: > I guess either 0 or GetLineCount() here is effectively the same. IIUC 0 preserves existing behavior, so switched to 0. > > But now that you've rewritten it, it looks like there's a bug either way --- if > we wrap around the beginning of the suggestions to the end, we don't check if > that last suggestion is actually acceptable. Couldn't the last one be an > insecure form warning or something? This might warrant adding a test. SetSelectedLine resets selected_line_ if we can't accept the last suggestion. Not sure if it would be better to wrap around and keep looking for an acceptable suggestion. But suppose there is only 1 acceptable suggestion - do we want to reset selected_line_ if we wrap and are back at original selection? IIUC, this will not be an issue for insecure form warning because I believe there are no acceptable suggestions (we just show the warning). Not sure about POPUP_ITEM_ID_TITLE though. In any event, since this is a change in behavior, I am reluctant to make that change as part of this CL but I have added a test.
Hi Scott, Can you approve for chrome/browser/ui ? Thanks, -sashi.
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... 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, so you shouldn't need it here. https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h:56: base::Optional<int> current_row_selection) override {} Don't inline overrides (see chromium style guide). https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.h (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.h:14: #include "base/optional.h" Similar comment as to other header. https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.h:52: base::Optional<int> current_row_selection) override {} Similar comment about override.
Hi Scott, Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h:15: #include "base/optional.h" On 2017/03/20 15:50:32, sky wrote: > The use of optional comes from chrome/browser/ui/autofill/autofill_popup_view.h, > so you shouldn't need it here. Can you confirm we don't do IWYU in chrome? """ For symbols used by bar.h, bar.h must #include or forward-declare what it uses. """ https://sites.google.com/a/google.com/iwyu-fixit/?pli=1 https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h:56: base::Optional<int> current_row_selection) override {} On 2017/03/20 15:50:32, sky wrote: > Don't inline overrides (see chromium style guide). Done. https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_popup_view_android.h (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.h:14: #include "base/optional.h" On 2017/03/20 15:50:32, sky wrote: > Similar comment as to other header. Can you confirm we don't use IWYU? https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_popup_view_android.h:52: base::Optional<int> current_row_selection) override {} On 2017/03/20 15:50:32, sky wrote: > Similar comment about override. Done.
Did you upload a new patch? https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h:15: #include "base/optional.h" On 2017/03/20 18:11:42, csashi wrote: > On 2017/03/20 15:50:32, sky wrote: > > The use of optional comes from > chrome/browser/ui/autofill/autofill_popup_view.h, > > so you shouldn't need it here. > > Can you confirm we don't do IWYU in chrome? > > """ > For symbols used by bar.h, bar.h must #include or forward-declare what it uses. > """ > https://sites.google.com/a/google.com/iwyu-fixit/?pli=1 When I asked the other members of my office of this specific case, they felt IWYU does not apply (which I agree with). Your are overriding a function whose signature is defined in the super-class, you should not need to include all the headers the super-class includes as well. Similarly you wouldn't forward declare all the declarations of the super-class that you have to override as well in the subclass.
sky@chromium.org changed reviewers: + groby@chromium.org
+groby for the cocoa files
Sorry for repeated comments :( I'm still uncomfortable with the API change, because it pushes logic from the controller into several independent view classes, replicating it there. https://codereview.chromium.org/2727233003/diff/240001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/240001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:30: const auto& kNoSelection = AutofillPopupView::kNoSelection; Why define a new constant here? (And if we must, please use constexpr) https://codereview.chromium.org/2727233003/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h (left): https://codereview.chromium.org/2727233003/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:52: void InvalidateRow(size_t row) override; Why change away from InvalidateRow() - which doesn't require optional types - to OnSelectedRowChanged, which does? https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_view.h (left): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_view.h:28: virtual void InvalidateRow(size_t row) = 0; Why does using child views necessitate an API change? https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_view.h (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_view.h:13: #include "ui/accessibility/ax_enums.h" Why are ax_enums.h included? https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:48: void AutofillPopupViewBridge::OnSelectedRowChanged( The docs[1] suggest not using Optional<> as a function parameter. [1] https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:45: index:(int)index If we absolutely must change to int (and if we keep the old API, we don't), please take it all the way for Cocoa and make the parameter NSInteger
> I'm still uncomfortable with the API change, because it pushes logic from the > controller into several independent view classes Specifically, what logic are you worried about replicating? The a11y logic is being added here, and it is view/toolkit specific. https://codereview.chromium.org/2727233003/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h (left): https://codereview.chromium.org/2727233003/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:52: void InvalidateRow(size_t row) override; On 2017/03/20 20:42:55, groby wrote: > Why change away from InvalidateRow() - which doesn't require optional types - to > OnSelectedRowChanged, which does? I asked for this change. The optional change is to get rid of kNoSelection. The API change is so that we only have to call it once, and because you're not just invalidating, you're also sending an a11y event. The options are to modify this API call or to add a new one. https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:48: void AutofillPopupViewBridge::OnSelectedRowChanged( On 2017/03/20 20:42:55, groby wrote: > The docs[1] suggest not using Optional<> as a function parameter. > > [1] https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md I asked for this. See discussion above. Cocoa will want to send an a11y event which it does not currently do.
On 2017/03/20 20:10:50, sky wrote: > Did you upload a new patch? Oops. Please take a look. > > https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... > File chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h > (right): > > https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/andr... > chrome/browser/ui/android/autofill/autofill_keyboard_accessory_view.h:15: > #include "base/optional.h" > On 2017/03/20 18:11:42, csashi wrote: > > On 2017/03/20 15:50:32, sky wrote: > > > The use of optional comes from > > chrome/browser/ui/autofill/autofill_popup_view.h, > > > so you shouldn't need it here. > > > > Can you confirm we don't do IWYU in chrome? > > > > """ > > For symbols used by bar.h, bar.h must #include or forward-declare what it > uses. > > """ > > https://sites.google.com/a/google.com/iwyu-fixit/?pli=1 > > When I asked the other members of my office of this specific case, they felt > IWYU does not apply (which I agree with). Your are overriding a function whose > signature is defined in the super-class, you should not need to include all the > headers the super-class includes as well. Similarly you wouldn't forward declare > all the declarations of the super-class that you have to override as well in the > subclass. Would be useful to include this guideline in the style guide. Thanks!
https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:48: void AutofillPopupViewBridge::OnSelectedRowChanged( On 2017/03/20 21:36:57, Evan Stade wrote: > On 2017/03/20 20:42:55, groby wrote: > > The docs[1] suggest not using Optional<> as a function parameter. > > > > [1] https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md > > I asked for this. See discussion above. > > Cocoa will want to send an a11y event which it does not currently do. also, I think that those docs need to be updated. base::Optional has good ergonomics --- you can base base::nullopt or you can pass a value and it implicitly makes an Optional.
Hi, My response to a couple of the comments crossed Evan's. Please take a look. Thanks, -sashi. https://codereview.chromium.org/2727233003/diff/240001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/240001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:30: const auto& kNoSelection = AutofillPopupView::kNoSelection; On 2017/03/20 20:42:55, groby wrote: > Why define a new constant here? (And if we must, please use constexpr) I removed this a few patches ago. Can you please check the latest patch? https://codereview.chromium.org/2727233003/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h (left): https://codereview.chromium.org/2727233003/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:52: void InvalidateRow(size_t row) override; On 2017/03/20 20:42:55, groby wrote: > Why change away from InvalidateRow() - which doesn't require optional types - to > OnSelectedRowChanged, which does? FWIW, this was a result of a previous review. I agree that this particular change is not needed for the bug I am fixing and that InvalidateRow did not need an optional type, but the review recommendation seemed reasonable, so I implemented it. As I see it, new interface is better because: 1. OnSelectedRowChanged better describes what happened. 2. We need some interface change, if not this one, then another way to indicate what happened. Please see https://codereview.chromium.org/2670003002/patch/440001/450008 and https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... for two other possibilities that I had implemented previously. 3. The view implementation in autofill_popup_view_views.cc is simpler (perhaps, slightly faster?) because we schedule a single paint. I don't know if the optimization applies to Cocoa. In order to reduce the scope of the CL I made only the minimum required modifications to Cocoa. https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_view.h (left): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_view.h:28: virtual void InvalidateRow(size_t row) = 0; On 2017/03/20 20:42:55, groby wrote: > Why does using child views necessitate an API change? Using child views does not necessitate an API change, but accessibility supported needs some API change. Please look at response in https://codereview.chromium.org/2727233003/diff/300001/chrome/browser/ui/coco... https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/auto... File chrome/browser/ui/autofill/autofill_popup_view.h (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_view.h:13: #include "ui/accessibility/ax_enums.h" On 2017/03/20 20:42:55, groby wrote: > Why are ax_enums.h included? Done. https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:48: void AutofillPopupViewBridge::OnSelectedRowChanged( On 2017/03/20 20:42:55, groby wrote: > The docs[1] suggest not using Optional<> as a function parameter. > > [1] https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md Discussed in https://codereview.chromium.org/2727233003/#msg71. Would be useful if we have a strict guideline, one way or another. https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.mm:45: index:(int)index On 2017/03/20 20:42:55, groby wrote: > If we absolutely must change to int (and if we keep the old API, we don't), > please take it all the way for Cocoa and make the parameter NSInteger Done.
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The android and other random bits LGTM - wait for groby@ on the cocoa side though.
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/auto... File chrome/browser/ui/autofill/autofill_popup_controller_impl.cc (right): https://codereview.chromium.org/2727233003/diff/240001/chrome/browser/ui/auto... chrome/browser/ui/autofill/autofill_popup_controller_impl.cc:30: const auto& kNoSelection = AutofillPopupView::kNoSelection; On 2017/03/20 21:41:51, csashi wrote: > On 2017/03/20 20:42:55, groby wrote: > > Why define a new constant here? (And if we must, please use constexpr) > > I removed this a few patches ago. Can you please check the latest patch? Acknowledged. https://codereview.chromium.org/2727233003/diff/300001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h (left): https://codereview.chromium.org/2727233003/diff/300001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:52: void InvalidateRow(size_t row) override; On 2017/03/20 21:41:51, csashi wrote: > On 2017/03/20 20:42:55, groby wrote: > > Why change away from InvalidateRow() - which doesn't require optional types - > to > > OnSelectedRowChanged, which does? > > FWIW, this was a result of a previous review. I agree that this particular > change is not needed for the bug I am fixing and that InvalidateRow did not need > an optional type, but the review recommendation seemed reasonable, so I > implemented it. As I see it, new interface is better because: > > 1. OnSelectedRowChanged better describes what happened. > > 2. We need some interface change, if not this one, then another way to indicate > what happened. Please see > https://codereview.chromium.org/2670003002/patch/440001/450008 and > https://codereview.chromium.org/2727233003/diff/220001/chrome/browser/ui/view... > for two other possibilities that I had implemented previously. > > 3. The view implementation in autofill_popup_view_views.cc is simpler (perhaps, > slightly faster?) because we schedule a single paint. I don't know if the > optimization applies to Cocoa. In order to reduce the scope of the CL I made > only the minimum required modifications to Cocoa. Thanks for the explanation - I wasn't aware of the views implication. https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:48: void AutofillPopupViewBridge::OnSelectedRowChanged( On 2017/03/20 21:41:51, csashi wrote: > On 2017/03/20 20:42:55, groby wrote: > > The docs[1] suggest not using Optional<> as a function parameter. > > > > [1] https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md > > Discussed in https://codereview.chromium.org/2727233003/#msg71. Would be useful > if we have a strict guideline, one way or another. Fine with it for this CL. Would love it if you could raise the general issue on cr-dev. https://codereview.chromium.org/2727233003/diff/660001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h (right): https://codereview.chromium.org/2727233003/diff/660001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h:18: #include "ui/accessibility/ax_enums.h" Probably not needed. https://codereview.chromium.org/2727233003/diff/660001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h (right): https://codereview.chromium.org/2727233003/diff/660001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h:36: - (void)invalidateRow:(int)row; NSInteger please.
https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:48: void AutofillPopupViewBridge::OnSelectedRowChanged( On 2017/03/20 22:53:10, groby wrote: > On 2017/03/20 21:41:51, csashi wrote: > > On 2017/03/20 20:42:55, groby wrote: > > > The docs[1] suggest not using Optional<> as a function parameter. > > > > > > [1] https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md > > > > Discussed in https://codereview.chromium.org/2727233003/#msg71. Would be > useful > > if we have a strict guideline, one way or another. > > Fine with it for this CL. Would love it if you could raise the general issue on > cr-dev. The docs and impl were written by mlamouri so I sent him a CL/question.
https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm (right): https://codereview.chromium.org/2727233003/diff/640001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm:48: void AutofillPopupViewBridge::OnSelectedRowChanged( On 2017/03/20 22:57:30, Evan Stade wrote: > On 2017/03/20 22:53:10, groby wrote: > > On 2017/03/20 21:41:51, csashi wrote: > > > On 2017/03/20 20:42:55, groby wrote: > > > > The docs[1] suggest not using Optional<> as a function parameter. > > > > > > > > [1] > https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md > > > > > > Discussed in https://codereview.chromium.org/2727233003/#msg71. Would be > > useful > > > if we have a strict guideline, one way or another. > > > > Fine with it for this CL. Would love it if you could raise the general issue > on > > cr-dev. > > The docs and impl were written by mlamouri so I sent him a CL/question. Thank you!
https://codereview.chromium.org/2727233003/diff/660001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.h (right): https://codereview.chromium.org/2727233003/diff/660001/chrome/browser/ui/coco... 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 not needed. Done. https://codereview.chromium.org/2727233003/diff/660001/chrome/browser/ui/coco... File chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h (right): https://codereview.chromium.org/2727233003/diff/660001/chrome/browser/ui/coco... chrome/browser/ui/cocoa/autofill/autofill_popup_view_cocoa.h:36: - (void)invalidateRow:(int)row; On 2017/03/20 22:53:10, groby wrote: > NSInteger please. Done.
The CQ bit was checked by csashi@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by csashi@google.com
The CQ bit was checked by csashi@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from mathp@chromium.org, estade@chromium.org, groby@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2727233003/#ps680001 (title: "(int) to NSInteger")
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": 680001, "attempt_start_ts": 1490057849388760, "parent_rev": "1ad3437632699fb98fdd46a3ba6ad8d63cf10ee5", "commit_rev": "d9c8ac4a549d3a365b60fc6abf7d23adfe4c389f"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/d9c8ac4a549d3a365b60fc6abf7d... ==========
Message was sent while issue was closed.
Committed patchset #35 (id:680001) as https://chromium.googlesource.com/chromium/src/+/d9c8ac4a549d3a365b60fc6abf7d... |