Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2085813003:
Fix setUp failure in ImeTest (Closed)
|
Created:
4 years, 6 months ago by yabinh Modified:
4 years, 5 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jochen+watch_chromium.org, mkwst+moarreviews-shell_chromium.org, mlamouri+watch-content_chromium.org, Peter Beverloo Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix setUp failure in ImeTest
Sometimes setUp fails in ImeTest.
When we call "getElementById()" to reach "input_text", it returns null. It's
because the page is not ready yet.
We should make sure the page is ready before reaching its element.
BUG=580419
Committed: https://crrev.com/c5f27175f60bdeb0a7c42f3a3f564649c5d9cfaf
Cr-Commit-Position: refs/heads/master@{#402131}
Patch Set 1 #
Total comments: 6
Patch Set 2 : rebase #
Messages
Total messages: 16 (5 generated)
yabinh@chromium.org changed reviewers: + aelias@chromium.org, changwan@chromium.org
Before applying this patch, the failure rate is about 1/100 ~ 1/200. After applying it, it never fails with 5000+ times running. https://codereview.chromium.org/2085813003/diff/1/content/shell/android/javat... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestBase.java (left): https://codereview.chromium.org/2085813003/diff/1/content/shell/android/javat... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestBase.java:123: } It seems that we have Shell#isLoading() to make sure the page is ready. However, this function doesn't work at all. Here is the reason: When we call Shell#isLoading(), it will return |mLoading|. |mLoading| is supposed to indicate the page loading state. But it is initiated to be false at first, and keep false all the time, because the only function that can change it(Shell#setIsLoading()) is never used.
https://codereview.chromium.org/2085813003/diff/1/content/public/android/java... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (left): https://codereview.chromium.org/2085813003/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:78: assertWaitForPageScaleFactorMatch(1); What is the reason for this deletion? Is it needed to deflake? https://codereview.chromium.org/2085813003/diff/1/content/shell/android/javat... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestBase.java (left): https://codereview.chromium.org/2085813003/diff/1/content/shell/android/javat... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestBase.java:123: } OK. Can you also delete isLoading/setIsLoading/mLoading in Shell.java since you have confirmed it's dead code?
https://codereview.chromium.org/2085813003/diff/1/content/shell/android/javat... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestBase.java (left): https://codereview.chromium.org/2085813003/diff/1/content/shell/android/javat... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestBase.java:123: } On 2016/06/21 05:25:00, yabinh wrote: > > It seems that we have Shell#isLoading() to make sure the page is ready. However, > this function doesn't work at all. > > Here is the reason: > When we call Shell#isLoading(), it will return |mLoading|. |mLoading| is > supposed to indicate the page loading state. But it is initiated to be false at > first, and keep false all the time, because the only function that can change > it(Shell#setIsLoading()) is never used. I like the change, but I'm curious if it's Shell#isLoading() that actually needs fixing. Failing that, how about removing Shell#isLoading() and change the name of this function to something like waitForActiveShellToBeReady().
https://codereview.chromium.org/2085813003/diff/1/content/public/android/java... File content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java (left): https://codereview.chromium.org/2085813003/diff/1/content/public/android/java... content/public/android/javatests/src/org/chromium/content/browser/input/ImeTest.java:78: assertWaitForPageScaleFactorMatch(1); On 2016/06/21 05:42:50, aelias wrote: > What is the reason for this deletion? Is it needed to deflake? We can keep it, but it is unnecessary. So I deleted it.
https://codereview.chromium.org/2085813003/diff/1/content/shell/android/javat... File content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestBase.java (left): https://codereview.chromium.org/2085813003/diff/1/content/shell/android/javat... content/shell/android/javatests/src/org/chromium/content_shell_apk/ContentShellTestBase.java:123: } Shell#isLoading() can't work fine, but the reason I said is wrong. Shell#setIsLoading() is called by native code. Shell#isLoading() can't work fine because of race condition. Here is a case when it succeeds: Step0: |mLoading| is initiated to be false. Step1: WebContentsImpl::DidStartLoading is called -> |mLoading| is true. Step2: WebContentsImpl::DidStopLoading is called -> |mLoading| is false. Step3: WebContentsImpl::DidStartLoading is called -> |mLoading| is true. Step4: WebContentsImpl::DidStopLoading is called -> |mLoading| is false. Only when Step4 finishs, the page is fully loaded. Sometimes it fails between Step2 and Step3. Most of the time, it takes about 1 ms for |mLoading| to change from false to true. In rare case,it can take 2 or even 7 ms. (By the way, the criteria checking interval is 50 ms.) If we happne to check the criteria(ContentShellTestBase#waitForActiveShellToBeDoneLoading) during this time, we will get it wrong. We think the page is fully loaded but it's not. I think we should use shell.getContentViewCore().getWebContents().isReady() instead of shell.isLoading().
aelias@chromium.org changed reviewers: + sievers@chromium.org
OK, lgtm. Adding sievers@ for OWNERS review.
lgtm
Talked with changwan@ offline. I prefer not to change the function's name.
The CQ bit was checked by yabinh@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, sievers@chromium.org Link to the patchset: https://codereview.chromium.org/2085813003/#ps20001 (title: "rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix setUp failure in ImeTest Sometimes setUp fails in ImeTest. When we call "getElementById()" to reach "input_text", it returns null. It's because the page is not ready yet. We should make sure the page is ready before reaching its element. BUG=580419 ========== to ========== Fix setUp failure in ImeTest Sometimes setUp fails in ImeTest. When we call "getElementById()" to reach "input_text", it returns null. It's because the page is not ready yet. We should make sure the page is ready before reaching its element. BUG=580419 Committed: https://crrev.com/c5f27175f60bdeb0a7c42f3a3f564649c5d9cfaf Cr-Commit-Position: refs/heads/master@{#402131} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c5f27175f60bdeb0a7c42f3a3f564649c5d9cfaf Cr-Commit-Position: refs/heads/master@{#402131} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
