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

Issue 2716593003: Fix ContentAutofillDriverBrowserTest.TestPageNavigationHidingAutofillPopup on Mac with PlzNavigate. (Closed)

Created:
3 years, 10 months ago by jam
Modified:
3 years, 10 months ago
CC:
chromium-reviews, vabr+watchlistpasswordmanager_chromium.org, rouslan+autofill_chromium.org, rogerm+autofillwatch_chromium.org, sebsg+autofillwatch_chromium.org, browser-components-watch_chromium.org, mathp+autofillwatch_chromium.org, vabr+watchlistautofill_chromium.org, estade+watch_chromium.org, gcasto+watchlist_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix ContentAutofillDriverBrowserTest.TestPageNavigationHidingAutofillPopup on Mac with PlzNavigate. The message loop was being quit too early in the test since it should have waited for committed navigations finishing. This passed on Windows/Linux because with Views the popup got close notification at test finish time when all the windows were closed, and that apparently isn't happening on Mac. The fix is to maintain the same behavior in DidFinishNavigation before it was converted in r381725. BUG=504347 Review-Url: https://codereview.chromium.org/2716593003 Cr-Commit-Position: refs/heads/master@{#452560} Committed: https://chromium.googlesource.com/chromium/src/+/72c9c6de085337d4d91546f8975a9c82319940c1

Patch Set 1 #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+8 lines, -0 lines) Patch
M chrome/browser/autofill/content_autofill_driver_browsertest.cc View 2 chunks +4 lines, -0 lines 0 comments Download
M chrome/browser/password_manager/password_manager_test_base.cc View 2 chunks +4 lines, -0 lines 1 comment Download

Messages

Total messages: 18 (12 generated)
jam
3 years, 10 months ago (2017-02-23 17:02:51 UTC) #6
jam
Switching to Evan https://codereview.chromium.org/2716593003/diff/1/chrome/browser/password_manager/password_manager_test_base.cc File chrome/browser/password_manager/password_manager_test_base.cc (right): https://codereview.chromium.org/2716593003/diff/1/chrome/browser/password_manager/password_manager_test_base.cc#newcode63 chrome/browser/password_manager/password_manager_test_base.cc:63: if (!navigation_handle->HasCommitted()) I saw this was ...
3 years, 10 months ago (2017-02-23 17:15:00 UTC) #10
Evan Stade
lgtm but it seems like vabr@ would have more context here.
3 years, 10 months ago (2017-02-23 17:53:14 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2716593003/1
3 years, 10 months ago (2017-02-23 18:12:38 UTC) #14
commit-bot: I haz the power
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/72c9c6de085337d4d91546f8975a9c82319940c1
3 years, 10 months ago (2017-02-23 18:31:16 UTC) #17
vabr (Chromium)
3 years, 10 months ago (2017-02-24 08:29:54 UTC) #18
Message was sent while issue was closed.
Thanks for detecting the issue, figuring the cause and landing this fix!
LGTM.
Cheers,
Vaclav

Powered by Google App Engine
This is Rietveld 408576698