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

Issue 12302034: Always Close the Autofill UI through the same path (Closed)

Created:
7 years, 10 months ago by csharp
Modified:
7 years, 9 months ago
CC:
chromium-reviews, Raman Kakilate, tfarina, benquan, dhollowa+watch_chromium.org, ahutter, dbeam+watch-autofill_chromium.org, Dane Wallinga, dyu1, Albert Bodenhamer, estade+watch_chromium.org, Ilya Sherman
Base URL:
http://git.chromium.org/chromium/src.git@master
Visibility:
Public.

Description

Always Close the Autofill UI through the same path The autofill controller should always handle all hides, to ensure that everything is properly hidden. BUG=178564, 178504 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=186475

Patch Set 1 #

Patch Set 2 : Adding Tests #

Total comments: 20

Patch Set 3 : #

Total comments: 6

Patch Set 4 : #

Patch Set 5 : Fix android compile #

Patch Set 6 : Remove old (now incorrect) test #

Total comments: 6

Patch Set 7 : rebase #

Patch Set 8 : #

Patch Set 9 : Fix Test #

Total comments: 2

Patch Set 10 : Rebase #

Patch Set 11 : Fix Browser Test #

Patch Set 12 : Rebase #

Patch Set 13 : Fixing views test #

Patch Set 14 : #

Patch Set 15 : Rebasing #

Patch Set 16 : Disable new Test for Mac #

Unified diffs Side-by-side diffs Delta from patch set Stats (+178 lines, -32 lines) Patch
M chrome/browser/autofill/autofill_external_delegate_browsertest.cc View 1 2 3 4 5 6 7 3 chunks +7 lines, -0 lines 0 comments Download
M chrome/browser/ui/android/autofill/autofill_popup_view_android.cc View 1 2 3 4 5 6 7 8 9 10 2 chunks +3 lines, -3 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_controller.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -2 lines 0 comments Download
A chrome/browser/ui/autofill/autofill_popup_controller_browsertest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 1 chunk +112 lines, -0 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_controller_impl.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +2 lines, -6 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_controller_impl.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +2 lines, -11 lines 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_controller_unittest.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -1 line 0 comments Download
M chrome/browser/ui/autofill/autofill_popup_view.h View 1 2 3 4 5 6 7 8 9 10 2 chunks +11 lines, -2 lines 0 comments Download
A chrome/browser/ui/autofill/autofill_popup_view.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +15 lines, -0 lines 0 comments Download
M chrome/browser/ui/cocoa/autofill/autofill_popup_view_bridge.mm View 1 2 3 4 5 6 7 8 9 10 2 chunks +2 lines, -1 line 0 comments Download
M chrome/browser/ui/gtk/autofill/autofill_popup_view_gtk.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +3 lines, -2 lines 0 comments Download
M chrome/browser/ui/views/autofill/autofill_popup_view_views.cc View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +15 lines, -4 lines 0 comments Download
M chrome/chrome_browser_ui.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M chrome/chrome_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +1 line, -0 lines 0 comments Download

Messages

Total messages: 36 (0 generated)
csharp
I looked into adding a browser tests, but I could see any way to reliably ...
7 years, 10 months ago (2013-02-19 20:41:49 UTC) #1
Evan Stade
what is it you want the test to do?
7 years, 10 months ago (2013-02-20 03:17:17 UTC) #2
csharp
On 2013/02/20 03:17:17, Evan Stade wrote: > what is it you want the test to ...
7 years, 10 months ago (2013-02-20 14:50:47 UTC) #3
Evan Stade
On 2013/02/20 14:50:47, csharp wrote: > On 2013/02/20 03:17:17, Evan Stade wrote: > > what ...
7 years, 10 months ago (2013-02-20 21:59:16 UTC) #4
csharp
Added a test, didn't use WindowedNotificationObserver but it pointed me in the right direction.
7 years, 10 months ago (2013-02-21 21:37:32 UTC) #5
Evan Stade
lgtm
7 years, 10 months ago (2013-02-21 22:32:38 UTC) #6
Ilya Sherman
https://codereview.chromium.org/12302034/diff/6002/chrome/browser/ui/autofill/autofill_popup_controller.h File chrome/browser/ui/autofill/autofill_popup_controller.h (right): https://codereview.chromium.org/12302034/diff/6002/chrome/browser/ui/autofill/autofill_popup_controller.h#newcode28 chrome/browser/ui/autofill/autofill_popup_controller.h:28: virtual void ViewDestroyed() = 0; Is this method still ...
7 years, 10 months ago (2013-02-22 00:29:03 UTC) #7
csharp
https://codereview.chromium.org/12302034/diff/6002/chrome/browser/ui/autofill/autofill_popup_controller.h File chrome/browser/ui/autofill/autofill_popup_controller.h (right): https://codereview.chromium.org/12302034/diff/6002/chrome/browser/ui/autofill/autofill_popup_controller.h#newcode28 chrome/browser/ui/autofill/autofill_popup_controller.h:28: virtual void ViewDestroyed() = 0; On 2013/02/22 00:29:04, Ilya ...
7 years, 10 months ago (2013-02-22 15:41:31 UTC) #8
Ilya Sherman
https://codereview.chromium.org/12302034/diff/6002/chrome/browser/ui/autofill/autofill_popup_controller.h File chrome/browser/ui/autofill/autofill_popup_controller.h (right): https://codereview.chromium.org/12302034/diff/6002/chrome/browser/ui/autofill/autofill_popup_controller.h#newcode28 chrome/browser/ui/autofill/autofill_popup_controller.h:28: virtual void ViewDestroyed() = 0; On 2013/02/22 15:41:31, csharp ...
7 years, 10 months ago (2013-02-23 00:02:36 UTC) #9
csharp
https://codereview.chromium.org/12302034/diff/6002/chrome/browser/ui/autofill/autofill_popup_controller.h File chrome/browser/ui/autofill/autofill_popup_controller.h (right): https://codereview.chromium.org/12302034/diff/6002/chrome/browser/ui/autofill/autofill_popup_controller.h#newcode28 chrome/browser/ui/autofill/autofill_popup_controller.h:28: virtual void ViewDestroyed() = 0; On 2013/02/23 00:02:36, Ilya ...
7 years, 10 months ago (2013-02-25 20:29:29 UTC) #10
Ilya Sherman
https://codereview.chromium.org/12302034/diff/6002/chrome/browser/ui/autofill/autofill_popup_controller.h File chrome/browser/ui/autofill/autofill_popup_controller.h (right): https://codereview.chromium.org/12302034/diff/6002/chrome/browser/ui/autofill/autofill_popup_controller.h#newcode28 chrome/browser/ui/autofill/autofill_popup_controller.h:28: virtual void ViewDestroyed() = 0; On 2013/02/25 20:29:29, csharp ...
7 years, 10 months ago (2013-02-26 01:02:32 UTC) #11
csharp
https://codereview.chromium.org/12302034/diff/6002/chrome/browser/ui/autofill/autofill_popup_controller.h File chrome/browser/ui/autofill/autofill_popup_controller.h (right): https://codereview.chromium.org/12302034/diff/6002/chrome/browser/ui/autofill/autofill_popup_controller.h#newcode28 chrome/browser/ui/autofill/autofill_popup_controller.h:28: virtual void ViewDestroyed() = 0; On 2013/02/26 01:02:32, Ilya ...
7 years, 10 months ago (2013-02-26 18:33:31 UTC) #12
Ilya Sherman
LGTM. Note that you're going to have a merge conflict with [ https://chromiumcodereview.appspot.com/12340065/ ], depending ...
7 years, 10 months ago (2013-02-26 23:50:34 UTC) #13
csharp
sky@ for views owner's approval jcivelli@ for android ui owner's approval https://codereview.chromium.org/12302034/diff/6002/chrome/browser/ui/autofill/autofill_popup_controller.h File chrome/browser/ui/autofill/autofill_popup_controller.h (right): ...
7 years, 9 months ago (2013-02-27 18:18:13 UTC) #14
sky
LGTM
7 years, 9 months ago (2013-02-27 22:03:35 UTC) #15
Ilya Sherman
https://codereview.chromium.org/12302034/diff/14007/chrome/browser/ui/autofill/autofill_popup_controller_impl.h File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right): https://codereview.chromium.org/12302034/diff/14007/chrome/browser/ui/autofill/autofill_popup_controller_impl.h#newcode208 chrome/browser/ui/autofill/autofill_popup_controller_impl.h:208: bool hide_called_; I meant that you should do this ...
7 years, 9 months ago (2013-02-28 00:22:04 UTC) #16
csharp
https://codereview.chromium.org/12302034/diff/14007/chrome/browser/ui/autofill/autofill_popup_controller_impl.h File chrome/browser/ui/autofill/autofill_popup_controller_impl.h (right): https://codereview.chromium.org/12302034/diff/14007/chrome/browser/ui/autofill/autofill_popup_controller_impl.h#newcode208 chrome/browser/ui/autofill/autofill_popup_controller_impl.h:208: bool hide_called_; On 2013/02/28 00:22:04, Ilya Sherman wrote: > ...
7 years, 9 months ago (2013-03-04 22:22:03 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/12302034/23017
7 years, 9 months ago (2013-03-04 22:24:26 UTC) #18
Ilya Sherman
LGTM, thanks.
7 years, 9 months ago (2013-03-04 22:26:19 UTC) #19
commit-bot: I haz the power
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=21777
7 years, 9 months ago (2013-03-05 03:58:06 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/12302034/23017
7 years, 9 months ago (2013-03-05 04:26:38 UTC) #21
commit-bot: I haz the power
Retried try job too often on linux_aura for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura&number=21802
7 years, 9 months ago (2013-03-05 06:54:35 UTC) #22
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/12302034/47017
7 years, 9 months ago (2013-03-05 15:00:07 UTC) #23
commit-bot: I haz the power
Retried try job too often on win_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&number=118409
7 years, 9 months ago (2013-03-05 18:43:27 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/12302034/61002
7 years, 9 months ago (2013-03-05 20:18:27 UTC) #25
commit-bot: I haz the power
Step "update" is always a major failure. Look at the try server FAQ for more ...
7 years, 9 months ago (2013-03-05 20:27:34 UTC) #26
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/12302034/61002
7 years, 9 months ago (2013-03-05 22:48:35 UTC) #27
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=105554
7 years, 9 months ago (2013-03-06 00:12:06 UTC) #28
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/12302034/61002
7 years, 9 months ago (2013-03-06 00:25:52 UTC) #29
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) interactive_ui_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=105665
7 years, 9 months ago (2013-03-06 02:35:42 UTC) #30
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/12302034/61002
7 years, 9 months ago (2013-03-06 02:38:20 UTC) #31
commit-bot: I haz the power
Retried try job too often on mac_rel for step(s) browser_tests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&number=105750
7 years, 9 months ago (2013-03-06 05:08:14 UTC) #32
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/12302034/61002
7 years, 9 months ago (2013-03-06 09:04:16 UTC) #33
commit-bot: I haz the power
Failed to apply patch for chrome/browser/ui/autofill/autofill_popup_controller_impl.cc: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
7 years, 9 months ago (2013-03-06 09:04:20 UTC) #34
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/csharp@chromium.org/12302034/97003
7 years, 9 months ago (2013-03-06 16:03:20 UTC) #35
commit-bot: I haz the power
7 years, 9 months ago (2013-03-06 19:25:44 UTC) #36
Message was sent while issue was closed.
Change committed as 186475

Powered by Google App Engine
This is Rietveld 408576698