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

Issue 2365063002: Fix CaptivePortalBrowserTest browser tests with PlzNavigate. (Closed)

Created:
4 years, 2 months ago by jam
Modified:
4 years, 2 months ago
Reviewers:
mmenke
CC:
chromium-reviews
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix CaptivePortalBrowserTest browser tests with PlzNavigate. CaptivePortalBrowserTest.InterstitialTimerNavigateAwayWhileLoading CaptivePortalBrowserTest.InterstitialTimerNavigateWhileLoading_EndWithCaptivePortalInterstitial CaptivePortalBrowserTest.InterstitialTimerNavigateWhileLoading_EndWithSSLInterstitial CaptivePortalBrowserTest.InterstitialTimerReloadWhileLoading: The following tests just need an adjustment in how many navigations they wait for. With PlzNavigate, when the renderer is navigated it doesn't send a FrameHostMsg_DidStopLoading IPC if it's loading. The tests were depending on that implementation detail, so they're updated now to handle both cases. CaptivePortalBrowserTest.SSLCertErrorLogin: With PlzNavigate, CaptivePortalTabReloader::OnLoadStart is called synchronously when a navigation occurs because of differences in the implementation details of NavigatorImpl::NavigateToEntry. Again, update the test to handle both cases. BUG=504347 Committed: https://crrev.com/d9001a3e575a7d399509f621773d05bcb6fa20ae Cr-Commit-Position: refs/heads/master@{#420777}

Patch Set 1 #

Total comments: 4

Patch Set 2 : review comments #

Unified diffs Side-by-side diffs Delta from patch set Stats (+21 lines, -11 lines) Patch
M chrome/browser/captive_portal/captive_portal_browsertest.cc View 1 8 chunks +21 lines, -11 lines 0 comments Download

Messages

Total messages: 16 (10 generated)
jam
4 years, 2 months ago (2016-09-23 21:29:34 UTC) #4
mmenke
LGTM https://codereview.chromium.org/2365063002/diff/1/chrome/browser/captive_portal/captive_portal_browsertest.cc File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/2365063002/diff/1/chrome/browser/captive_portal/captive_portal_browsertest.cc#newcode2180 chrome/browser/captive_portal/captive_portal_browsertest.cc:2180: // WIthout PlzNavigate: expect two navigations: First one ...
4 years, 2 months ago (2016-09-23 21:41:18 UTC) #5
jam
https://codereview.chromium.org/2365063002/diff/1/chrome/browser/captive_portal/captive_portal_browsertest.cc File chrome/browser/captive_portal/captive_portal_browsertest.cc (right): https://codereview.chromium.org/2365063002/diff/1/chrome/browser/captive_portal/captive_portal_browsertest.cc#newcode2180 chrome/browser/captive_portal/captive_portal_browsertest.cc:2180: // WIthout PlzNavigate: expect two navigations: First one for ...
4 years, 2 months ago (2016-09-23 21:49:22 UTC) #8
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/2365063002/20001
4 years, 2 months ago (2016-09-23 21:51:51 UTC) #12
commit-bot: I haz the power
Committed patchset #2 (id:20001)
4 years, 2 months ago (2016-09-23 23:19:06 UTC) #14
commit-bot: I haz the power
4 years, 2 months ago (2016-09-23 23:21:50 UTC) #16
Message was sent while issue was closed.
Patchset 2 (id:??) landed as
https://crrev.com/d9001a3e575a7d399509f621773d05bcb6fa20ae
Cr-Commit-Position: refs/heads/master@{#420777}

Powered by Google App Engine
This is Rietveld 408576698