AudioPlayer & VideoPlayer startup DOMContentLoaded could have fired before init |
|||||
Issue descriptionThe AudioPlayer uses DOMContentLoaded to init, but it is possible, given code moves and changes now and in future, that the DOM DOMContentLoaded event has fired before the listener is installed. We should fix AudioPlayer to do a DOMContentLoaded setup that accounts for the document "loading" ready state.
,
Sep 18
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bb9a744c80ca3187020a07168f7807f4f836ef2d commit bb9a744c80ca3187020a07168f7807f4f836ef2d Author: Noel Gordon <noel@chromium.org> Date: Tue Sep 18 04:34:21 2018 AudioPlayer initialization: improve DOMContentLoaded handling DOMContentLoaded can fire before the addEventListener is installed, so setup the listener per convention: test "document.readyState" and then install the event listener if needed. Per the HTML DOM spec, DOMContentLoaded fires on document, not window, so install the event listener on document. Test: browser_tests --gtest_filter="AudioPlayerBrowserTest*" Bug: 884963 Change-Id: I0c8168dd4869d16ccfdf7dd01513d3921fe45c9b Reviewed-on: https://chromium-review.googlesource.com/1229713 Reviewed-by: Luciano Pacheco <lucmult@chromium.org> Commit-Queue: Noel Gordon <noel@chromium.org> Cr-Commit-Position: refs/heads/master@{#591935} [modify] https://crrev.com/bb9a744c80ca3187020a07168f7807f4f836ef2d/ui/file_manager/audio_player/js/audio_player.js
,
Sep 18
,
Sep 19
The VideoPlayer has the same issue as the AudioPlayer. Changing title.
,
Sep 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/99d369035a5137fc3039aa8cf599545824f49892 commit 99d369035a5137fc3039aa8cf599545824f49892 Author: Noel Gordon <noel@chromium.org> Date: Thu Sep 20 05:48:05 2018 VideoPlayer initialization: await DOMContentLoaded Make the initialization promise await DOMContentLoaded and then apply i18n transforms to the video player DOM [1]. Minor: correct commentary, add commentary. Add assertions re document readyState when the i18n template strings are applied. [1] Applying i18n strings to an incomplete (readyState === 'loading') DOM document makes no sense at all. Test: browser_tests --gtest_filter="VideoPlayerBrowserTest*" Bug: 884963 Change-Id: Ie5a25763c54c56e949edde40c7e06b8eaad230c4 Reviewed-on: https://chromium-review.googlesource.com/1233394 Commit-Queue: Noel Gordon <noel@chromium.org> Reviewed-by: Trent Apted <tapted@chromium.org> Cr-Commit-Position: refs/heads/master@{#592690} [modify] https://crrev.com/99d369035a5137fc3039aa8cf599545824f49892/ui/file_manager/video_player/js/video_player.js
,
Sep 20
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/bf30bb4f3ce8301881a14a394383da5e5dc3c60d commit bf30bb4f3ce8301881a14a394383da5e5dc3c60d Author: Luciano Pacheco <lucmult@chromium.org> Date: Thu Sep 20 09:12:05 2018 Include utils for Polymer2 and remove /deep/ Bug: 875525 , 884963 Change-Id: I3b3d290e2f7cc9781b7af3d3caea4508330e21ff Reviewed-on: https://chromium-review.googlesource.com/1234118 Commit-Queue: Noel Gordon <noel@chromium.org> Reviewed-by: Noel Gordon <noel@chromium.org> Cr-Commit-Position: refs/heads/master@{#592729} [modify] https://crrev.com/bf30bb4f3ce8301881a14a394383da5e5dc3c60d/ui/file_manager/audio_player/audio_player.html [modify] https://crrev.com/bf30bb4f3ce8301881a14a394383da5e5dc3c60d/ui/file_manager/integration_tests/audio_player/open_audio_files.js
,
Sep 20
,
Sep 20
,
Sep 24
FTR, issue 887792 filed for remaining parts of FilesApp and DOMContentLoaded. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by noel@chromium.org
, Sep 18