New issue
Advanced search Search tips

Issue 884963 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 20
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 3
Type: Bug

Blocking:
issue 882606



Sign in to add a comment

AudioPlayer & VideoPlayer startup DOMContentLoaded could have fired before init

Project Member Reported by noel@chromium.org, Sep 18

Issue description

The 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.

 
Labels: OS-Chrome
Project Member

Comment 2 by bugdroid1@chromium.org, 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

Owner: noel@chromium.org
Status: Started (was: Untriaged)
Summary: AudioPlayer & VideoPlayer startup DOMContentLoaded could have fired before init (was: AudioPlayer startup DOMContentLoaded could have fired before init)
The VideoPlayer has the same issue as the AudioPlayer.  Changing title.
Project Member

Comment 5 by bugdroid1@chromium.org, 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

Project Member

Comment 6 by bugdroid1@chromium.org, 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

Cc: lucmult@chromium.org
Status: Fixed (was: Started)
Blockedon: 882606
Blockedon: -882606
Blocking: 882606
FTR,  issue 887792  filed for remaining parts of FilesApp and DOMContentLoaded.

Sign in to add a comment