Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2763843002:
Use stricter checking in UrlOverridingTest (Closed)
DescriptionUse stricter checking in UrlOverridingTest
Check the callback counts in all the test cases, and wait for the
fallback navigation to avoid early abortion.
The failures with PlzNavigate are not fixed yet.
BUG=699388
Review-Url: https://codereview.chromium.org/2763843002
Cr-Commit-Position: refs/heads/master@{#459325}
Committed: https://chromium.googlesource.com/chromium/src/+/ffb0ffaccfcded76b0165c24c6ba0c8c4712089c
Patch Set 1 #
Total comments: 4
Patch Set 2 : simplify observer #
Total comments: 2
Patch Set 3 : simplify #Messages
Total messages: 17 (8 generated)
wychen@chromium.org changed reviewers: + mariakhomenko@chromium.org
PTAL
https://codereview.chromium.org/2763843002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/UrlOverridingTest.java (right): https://codereview.chromium.org/2763843002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/UrlOverridingTest.java:90: return; nit: make this a one-liner https://codereview.chromium.org/2763843002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/UrlOverridingTest.java:93: mLoadFailCallback.notifyCalled(); this not in main frame check is a bit weird. Shouldn't this be called in main frame too?
https://codereview.chromium.org/2763843002/diff/1/chrome/android/javatests/sr... File chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/UrlOverridingTest.java (right): https://codereview.chromium.org/2763843002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/UrlOverridingTest.java:90: return; On 2017/03/21 17:55:32, Maria wrote: > nit: make this a one-liner Done. https://codereview.chromium.org/2763843002/diff/1/chrome/android/javatests/sr... chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/UrlOverridingTest.java:93: mLoadFailCallback.notifyCalled(); On 2017/03/21 17:55:32, Maria wrote: > this not in main frame check is a bit weird. Shouldn't this be called in main > frame too? The main frame case is covered by onPageLoadFailed(). I'll move it here so it's less confusing.
https://codereview.chromium.org/2763843002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/UrlOverridingTest.java (right): https://codereview.chromium.org/2763843002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/UrlOverridingTest.java:181: CallbackHelper unused = isMainFrame ? loadFailCallback : pageFailCallback; So what's the difference between the pageFailCallback and loadFailCallback? Are there really two different callbacks being used for main vs non-main frame load? Could we just have one callback for this?
https://codereview.chromium.org/2763843002/diff/20001/chrome/android/javatest... File chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/UrlOverridingTest.java (right): https://codereview.chromium.org/2763843002/diff/20001/chrome/android/javatest... chrome/android/javatests/src/org/chromium/chrome/browser/externalnav/UrlOverridingTest.java:181: CallbackHelper unused = isMainFrame ? loadFailCallback : pageFailCallback; On 2017/03/22 03:51:30, Maria wrote: > So what's the difference between the pageFailCallback and loadFailCallback? Are > there really two different callbacks being used for main vs non-main frame load? > Could we just have one callback for this? They are for main and non-main frames respectively. We probably don't have to distinguish which.
The CQ bit was checked by wychen@chromium.org 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.
lgtm Thanks!
The CQ bit was checked by wychen@chromium.org
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": 40001, "attempt_start_ts": 1490316080195900,
"parent_rev": "57c2207020be6758153102c90902b4b49781744e", "commit_rev":
"ffb0ffaccfcded76b0165c24c6ba0c8c4712089c"}
Message was sent while issue was closed.
Description was changed from ========== Use stricter checking in UrlOverridingTest Check the callback counts in all the test cases, and wait for the fallback navigation to avoid early abortion. The failures with PlzNavigate are not fixed yet. BUG=699388 ========== to ========== Use stricter checking in UrlOverridingTest Check the callback counts in all the test cases, and wait for the fallback navigation to avoid early abortion. The failures with PlzNavigate are not fixed yet. BUG=699388 Review-Url: https://codereview.chromium.org/2763843002 Cr-Commit-Position: refs/heads/master@{#459325} Committed: https://chromium.googlesource.com/chromium/src/+/ffb0ffaccfcded76b0165c24c6ba... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/ffb0ffaccfcded76b0165c24c6ba...
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/2769413002/ by dgn@chromium.org. The reason for reverting is: Breaking tablet testers: [FAIL] org.chromium.chrome.browser.externalnav.UrlOverridingTest#testOpenWindowFromUserGesture: junit.framework.AssertionFailedError at org.chromium.chrome.browser.externalnav.UrlOverridingTest.loadUrlAndWaitForIntentUrl(UrlOverridingTest.java:174) at org.chromium.chrome.browser.externalnav.UrlOverridingTest.testOpenWindowFromUserGesture(UrlOverridingTest.java:332) at java.lang.reflect.Method.invokeNative(Native Method) at android.test.InstrumentationTestCase.runMethod(InstrumentationTestCase.java:214) at android.test.InstrumentationTestCase.runTest(InstrumentationTestCase.java:199) at android.test.ActivityInstrumentationTestCase2.runTest(ActivityInstrumentationTestCase2.java:192) at org.chromium.chrome.test.ChromeActivityTestCaseBase.runTest(ChromeActivityTestCaseBase.java:752) at org.chromium.base.test.BaseTestResult.runParameterized(BaseTestResult.java:161) at org.chromium.base.test.BaseTestResult.run(BaseTestResult.java:124) at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:191) at android.test.AndroidTestRunner.runTest(AndroidTestRunner.java:176) at android.test.InstrumentationTestRunner.onStart(InstrumentationTestRunner.java:554) at android.app.Instrumentation$InstrumentationThread.run(Instrumentation.java:1701) See bug for builder links.. |
