Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2604133002:
Fix TabCaptureApiTest.FullscreenEvents flakiness (Closed)
|
Created:
3 years, 11 months ago by miu Modified:
3 years, 11 months ago Reviewers:
xjz CC:
chromium-reviews, chromium-apps-reviews_chromium.org, xjz+watch_chromium.org, extensions-reviews_chromium.org, miu+watch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix TabCaptureApiTest.FullscreenEvents flakiness
Fixes flakiness caused by a subtle distinction in document fullscreen
state change: The tabCapture.onStatusChanged API can sometimes report a
fullscreen state change before the document itself is aware the change
occurred. This is because the tabCapture API's listener reports on the
browser's fullscreen state, whereas the document is only aware of its
own internal fullscreen state (which is updated only after the browser
notifies it of a change).
The flakiness was due to the C++ side of the test being notified that
the page had entered fullscreen before the page itself was aware of it.
This caused the simulated mouse click (to toggle fullscreen mode) to be
made early, and then both the C++ and JS sides of the test became
out-of-sync and froze, each waiting on the other to do something.
This change fixes the issue by always using the document's fullscreen
change listeners, to ensure the out-of-sync condition cannot occur.
This change also "freshens up" the JavaScript code in this test, using
newer ES6 syntax.
BUG=675851
Committed: https://crrev.com/c52ce18fd8f6db97c62737f0924bd61a2727c5d3
Cr-Commit-Position: refs/heads/master@{#441252}
Patch Set 1 #
Total comments: 4
Patch Set 2 : nits #
Messages
Total messages: 18 (11 generated)
The CQ bit was checked by miu@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...
Description was changed from ========== Fix TabCaptureApiTest.FullscreenEvents flakiness Fixes flakiness caused by a subtle distinction in document fullscreen state change: The tabCapture.onStatusChanged API can sometimes report a fullscreen state change before the document itself is aware the change occurred. This is because the tabCapture API's listener reports on the browser's fullscreen state, whereas the document is only aware of its own internal fullscreen state (which is updated only after the browser notifies it of a change). The flakiness was due to the C++ side of the test being notified that the page had entered fullscreen before the page itself was aware of it. This caused the simulated mouse click (to toggle fullscreen mode) to be made early, and then both the C++ and JS sides of the test became out-of-sync and froze, each waiting on the other to do something. BUG=675851 ========== to ========== Fix TabCaptureApiTest.FullscreenEvents flakiness Fixes flakiness caused by a subtle distinction in document fullscreen state change: The tabCapture.onStatusChanged API can sometimes report a fullscreen state change before the document itself is aware the change occurred. This is because the tabCapture API's listener reports on the browser's fullscreen state, whereas the document is only aware of its own internal fullscreen state (which is updated only after the browser notifies it of a change). The flakiness was due to the C++ side of the test being notified that the page had entered fullscreen before the page itself was aware of it. This caused the simulated mouse click (to toggle fullscreen mode) to be made early, and then both the C++ and JS sides of the test became out-of-sync and froze, each waiting on the other to do something. This change fixes the issue by always using the document's fullscreen change listeners, to ensure the out-of-sync condition cannot occur. This change also "freshens up" the JavaScript code in this test, using newer ES6 syntax. BUG=675851 ==========
miu@chromium.org changed reviewers: + xjz@chromium.org
xjz: PTAL.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/2604133002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/tab_capture/fullscreen_test.js (right): https://codereview.chromium.org/2604133002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/tab_capture/fullscreen_test.js:16: if (!!document.fullscreenElement) ooc: Is this explicit conversion required? Can it just be if (document.fullscreenElement)? https://codereview.chromium.org/2604133002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/tab_capture/fullscreen_test.js:21: if (!!document.webkitFullscreenElement) ditto: if (document.webkitFullscreenElement)?
https://codereview.chromium.org/2604133002/diff/1/chrome/test/data/extensions... File chrome/test/data/extensions/api_test/tab_capture/fullscreen_test.js (right): https://codereview.chromium.org/2604133002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/tab_capture/fullscreen_test.js:16: if (!!document.fullscreenElement) On 2017/01/03 18:14:06, xjz wrote: > ooc: Is this explicit conversion required? Can it just be if > (document.fullscreenElement)? Done. https://codereview.chromium.org/2604133002/diff/1/chrome/test/data/extensions... chrome/test/data/extensions/api_test/tab_capture/fullscreen_test.js:21: if (!!document.webkitFullscreenElement) On 2017/01/03 18:14:06, xjz wrote: > ditto: if (document.webkitFullscreenElement)? Done.
The CQ bit was checked by miu@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from xjz@chromium.org Link to the patchset: https://codereview.chromium.org/2604133002/#ps20001 (title: "nits")
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": 20001, "attempt_start_ts": 1483476820258460,
"parent_rev": "3e5ae85322869054837e5f56dcef2e4215583392", "commit_rev":
"c76f4e5686a5ea89a7695acb353e494f18df9505"}
Message was sent while issue was closed.
Description was changed from ========== Fix TabCaptureApiTest.FullscreenEvents flakiness Fixes flakiness caused by a subtle distinction in document fullscreen state change: The tabCapture.onStatusChanged API can sometimes report a fullscreen state change before the document itself is aware the change occurred. This is because the tabCapture API's listener reports on the browser's fullscreen state, whereas the document is only aware of its own internal fullscreen state (which is updated only after the browser notifies it of a change). The flakiness was due to the C++ side of the test being notified that the page had entered fullscreen before the page itself was aware of it. This caused the simulated mouse click (to toggle fullscreen mode) to be made early, and then both the C++ and JS sides of the test became out-of-sync and froze, each waiting on the other to do something. This change fixes the issue by always using the document's fullscreen change listeners, to ensure the out-of-sync condition cannot occur. This change also "freshens up" the JavaScript code in this test, using newer ES6 syntax. BUG=675851 ========== to ========== Fix TabCaptureApiTest.FullscreenEvents flakiness Fixes flakiness caused by a subtle distinction in document fullscreen state change: The tabCapture.onStatusChanged API can sometimes report a fullscreen state change before the document itself is aware the change occurred. This is because the tabCapture API's listener reports on the browser's fullscreen state, whereas the document is only aware of its own internal fullscreen state (which is updated only after the browser notifies it of a change). The flakiness was due to the C++ side of the test being notified that the page had entered fullscreen before the page itself was aware of it. This caused the simulated mouse click (to toggle fullscreen mode) to be made early, and then both the C++ and JS sides of the test became out-of-sync and froze, each waiting on the other to do something. This change fixes the issue by always using the document's fullscreen change listeners, to ensure the out-of-sync condition cannot occur. This change also "freshens up" the JavaScript code in this test, using newer ES6 syntax. BUG=675851 Review-Url: https://codereview.chromium.org/2604133002 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix TabCaptureApiTest.FullscreenEvents flakiness Fixes flakiness caused by a subtle distinction in document fullscreen state change: The tabCapture.onStatusChanged API can sometimes report a fullscreen state change before the document itself is aware the change occurred. This is because the tabCapture API's listener reports on the browser's fullscreen state, whereas the document is only aware of its own internal fullscreen state (which is updated only after the browser notifies it of a change). The flakiness was due to the C++ side of the test being notified that the page had entered fullscreen before the page itself was aware of it. This caused the simulated mouse click (to toggle fullscreen mode) to be made early, and then both the C++ and JS sides of the test became out-of-sync and froze, each waiting on the other to do something. This change fixes the issue by always using the document's fullscreen change listeners, to ensure the out-of-sync condition cannot occur. This change also "freshens up" the JavaScript code in this test, using newer ES6 syntax. BUG=675851 Review-Url: https://codereview.chromium.org/2604133002 ========== to ========== Fix TabCaptureApiTest.FullscreenEvents flakiness Fixes flakiness caused by a subtle distinction in document fullscreen state change: The tabCapture.onStatusChanged API can sometimes report a fullscreen state change before the document itself is aware the change occurred. This is because the tabCapture API's listener reports on the browser's fullscreen state, whereas the document is only aware of its own internal fullscreen state (which is updated only after the browser notifies it of a change). The flakiness was due to the C++ side of the test being notified that the page had entered fullscreen before the page itself was aware of it. This caused the simulated mouse click (to toggle fullscreen mode) to be made early, and then both the C++ and JS sides of the test became out-of-sync and froze, each waiting on the other to do something. This change fixes the issue by always using the document's fullscreen change listeners, to ensure the out-of-sync condition cannot occur. This change also "freshens up" the JavaScript code in this test, using newer ES6 syntax. BUG=675851 Committed: https://crrev.com/c52ce18fd8f6db97c62737f0924bd61a2727c5d3 Cr-Commit-Position: refs/heads/master@{#441252} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/c52ce18fd8f6db97c62737f0924bd61a2727c5d3 Cr-Commit-Position: refs/heads/master@{#441252}
Message was sent while issue was closed.
A revert of this CL (patchset #2 id:20001) has been created in https://codereview.chromium.org/2611593005/ by samuong@chromium.org. The reason for reverting is: TabCaptureApiTest.FullscreenEvents is now timing out consistently on the Win7 Tests (dbg)(1) bot (build #56178 and later) so I don't think we're ready to re-enable it yet: https://build.chromium.org/p/chromium.win/builders/Win7%20Tests%20%28dbg%29%2.... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
