Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2051253002:
Start autoplay muted videos with autoplay attribute when they are visible. (Closed)
|
Created:
4 years, 6 months ago by mlamouri (slow - plz ping) Modified:
4 years, 6 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, eric.carlson_apple.com, feature-media-reviews_chromium.org, fs, gasubic, mlamouri+watch-blink_chromium.org, rwlbuis, sof, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionStart autoplay muted videos with autoplay attribute when they are visible.
The behaviour is linked to the autoplay-muted-videos flag.
This is introducing a ElementVisibilityObserver helper that is behind the
scene using the IntersectionObserver.
BUG=618301
Committed: https://crrev.com/0c33d8c3ae4c8f1fc2d58574c128725389864d95
Cr-Commit-Position: refs/heads/master@{#401242}
Patch Set 1 #
Total comments: 4
Patch Set 2 : cleanup and tests #
Total comments: 8
Patch Set 3 : review comments #Patch Set 4 : add cl dependency #Patch Set 5 : fix test #
Total comments: 4
Patch Set 6 : review comments #
Total comments: 16
Patch Set 7 : review comments #Patch Set 8 : reduce #Depends on Patchset: Messages
Total messages: 60 (19 generated)
mlamouri@chromium.org changed reviewers: + esprehn@chromium.org, ojan@chromium.org, szager@chromium.org
This is a bit rough around the edges: I didn't clean up the CL and I took a couple of shortcuts. Also it is missing tests. However, the intent is to have some early feedback so maybe I can iterate on it tomorrow when I clean up the CL and write tests.
mlamouri@chromium.org changed reviewers: + skyostil@chromium.org
Seems reasonable to me. Do we need to clear the observer if playback starts through other means? https://codereview.chromium.org/2051253002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp (right): https://codereview.chromium.org/2051253002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:50: Node* root = nullptr; DCHECK(!m_intersectionObserver)? https://codereview.chromium.org/2051253002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:60: { DCHECK(m_intersectionObserver)?
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051253002/20001
mlamouri@chromium.org changed reviewers: + zqzhang@chromium.org
Applied skyostil's comments. +zqzhang@ for another round of local review. esprehn@, ojan@ and szager@, PTAL. https://codereview.chromium.org/2051253002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp (right): https://codereview.chromium.org/2051253002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:50: Node* root = nullptr; On 2016/06/10 at 12:42:25, Sami wrote: > DCHECK(!m_intersectionObserver)? Good idea! https://codereview.chromium.org/2051253002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:60: { On 2016/06/10 at 12:42:25, Sami wrote: > DCHECK(m_intersectionObserver)? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The VisibilityObserver is nice, I think we'll want to use that to also not _load_ media until it's visible -- this avoid spinning up resources to preload and decode the data until necessary.
LGTM
Sorry I'm late to the game here, but I think there's too much code here. You
don't need an ElementVisibilityObserver class, you just need a subclass of
IntersectionObserverCallback (and I think it should be specific to
HTMLMediaElement); something like this:
class HTMLMediaElement ... {
private:
class VisibilityCallback : public IntersectionObserverCallback {
public:
VisibilityCallback(HTMLMediaElement* element);
void handleEvent (...) override;
private:
Member<HTMLMedialElement> m_element;
};
}
HTMLMediaElement.cpp should just instantiate IntersectionObserver directly, with
an instance of HTMLMediaElement::VisibilityCallback.
Does that make sense? Sorry, this is a bit rushed, I'm kind of running out the
door.
+1 to Stefan's feedback. -- You received this message because you are subscribed to the Google Groups "Blink Reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to blink-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/2051253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/2051253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:182: observe(target); Infinite recursion.
https://codereview.chromium.org/2051253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/2051253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:231: unobserve(target); Infinite recursion.
https://codereview.chromium.org/2051253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/2051253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:182: observe(target); On 2016/06/10 21:07:17, szager1 wrote: > Infinite recursion. Oh never mind, I see what you did here. I would prefer that you just use an IgnorableExceptionState: IgnorableExceptionState exceptionState; observer->observe(target, exceptionState); https://codereview.chromium.org/2051253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:231: unobserve(target); On 2016/06/10 21:07:49, szager1 wrote: > Infinite recursion. Same comment.
+1 to Stefan's feedback. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Lets make ElementVisibilityObserver look like a js library, once that's done I think this is fine. We need this kind of "framework" features for web modules, and this is a great start! https://codereview.chromium.org/2051253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp (right): https://codereview.chromium.org/2051253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:13: class ElementVisibilityObserver::ElementVisibilityCallback : public IntersectionObserverCallback { move into IntersectionObserver constructor for C++ function callbacks https://codereview.chromium.org/2051253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:60: m_intersectionObserver = new IntersectionObserver(*m_intersectionObserverCallback, *root, Vector<Length>(), Vector<float>({0.f})); Please add IntersectionObserver::create() or constructor for C++ that takes a WTF::Function<void(Vector<IntersectionRecord>)> https://codereview.chromium.org/2051253002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObserver.h (right): https://codereview.chromium.org/2051253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObserver.h:34: void observe(Element*, ExceptionState&); void observe(Element*, ExceptionState& = ASSERT_NO_EXCEPTION); void unobserve(Element*, ExceptionState& = ASSERT_NO_EXCEPTION); https://codereview.chromium.org/2051253002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObserver.h:46: void unobserve(Element*); remove
esprehn@ comments applied. PTAL.
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051253002/60001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051253002/80001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2051253002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp (right): https://codereview.chromium.org/2051253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:50: bool isVisible = entries[0]->intersectionRatio() > 0.f; Unfortunately, this will not work. IntersectionObserver treats a zero threshold specially: it signifies a transition between two states: "not intersecting at all", and "intersecting by any amount, including a zero-area intersection when root and target have a coincident corner or edge." So intersectionRatio() can be zero even if the notification indicates a transition from not-intersecting to intersecting. This is a known gotcha with IntersectionObserver, and we are open to ways to make this less obscure. For the time being, you might be better off creating an IntersectionObserver with Vector<float>({std::numeric_limits<float>::FLT_MIN}) for the thresholds argument. If you do that, then this line will work as intended. Otherwise, you'll have to keep a boolean state variable around, initialized to "not intersecting", and flip it on every notification. https://codereview.chromium.org/2051253002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/2051253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:33: // Internal implementation of IntersectionObserverCallback when usig s/usig/using/
https://codereview.chromium.org/2051253002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp (right): https://codereview.chromium.org/2051253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:50: bool isVisible = entries[0]->intersectionRatio() > 0.f; On 2016/06/17 at 07:32:36, szager1 wrote: > Unfortunately, this will not work. IntersectionObserver treats a zero threshold specially: it signifies a transition between two states: "not intersecting at all", and "intersecting by any amount, including a zero-area intersection when root and target have a coincident corner or edge." > > So intersectionRatio() can be zero even if the notification indicates a transition from not-intersecting to intersecting. > > This is a known gotcha with IntersectionObserver, and we are open to ways to make this less obscure. For the time being, you might be better off creating an IntersectionObserver with Vector<float>({std::numeric_limits<float>::FLT_MIN}) for the thresholds argument. If you do that, then this line will work as intended. > > Otherwise, you'll have to keep a boolean state variable around, initialized to "not intersecting", and flip it on every notification. Done. https://codereview.chromium.org/2051253002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/2051253002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:33: // Internal implementation of IntersectionObserverCallback when usig On 2016/06/17 at 07:32:36, szager1 wrote: > s/usig/using/ Done.
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051253002/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
szager@, friendly ping :) This CL blocks us from being feature complete on the autoplay changes for m53.
Mostly looks good, a couple of details... https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp (right): https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:47: DCHECK_EQ(entries.size(), 1u); This is a bad assumption; observations are taken every frame, but may be delivered up to 100ms later. If multiple visibility events happened during that 100ms, they will be bundled here. The entries are ordered by the time they occurred, earliest to latest, so you can get rid of the DCHECK and just: bool isVisible = entries.last()->intersectionRatio() > 0.f; https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:60: Member<ExecutionContext> m_context; WeakMember<ExecutionContext> m_context;
Comments applied. PTAL :) https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp (right): https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:47: DCHECK_EQ(entries.size(), 1u); On 2016/06/21 at 21:18:05, szager1 wrote: > This is a bad assumption; observations are taken every frame, but may be delivered up to 100ms later. If multiple visibility events happened during that 100ms, they will be bundled here. > > The entries are ordered by the time they occurred, earliest to latest, so you can get rid of the DCHECK and just: > > bool isVisible = entries.last()->intersectionRatio() > 0.f; Good to know! These small things comforts me in having this ElementVisibilityObserver layer :) https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/dom/IntersectionObserver.cpp (right): https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/dom/IntersectionObserver.cpp:60: Member<ExecutionContext> m_context; On 2016/06/21 at 21:18:05, szager1 wrote: > WeakMember<ExecutionContext> m_context; Done.
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051253002/120001
This is really exciting, thanks for doing it! https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/autoplay-muted.html (right): https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/autoplay-muted.html:3: <head> you don't need <head> or <html>, just add <body>. https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/autoplay-muted.html:11: <script> <body> is all you need here, right before the <script> with the test itself. https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/autoplay-muted.html:109: </html> remove https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/autoplay-when-visible.html (right): https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/autoplay-when-visible.html:3: <head> ditto https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/autoplay-when-visible.html:6: <body> put the body after the media-file.js, right before your script https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/autoplay-when-visible.html:102: </html> no need for these https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1652: m_autoplayVisibilityObserver = new ElementVisibilityObserver(this, WTF::bind<bool>(&HTMLMediaElement::onVisibilityChangedForAutoplay, this)); do you actually need to pass the <bool> like this? rarely does anyone specify type like this.
https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/autoplay-muted.html (right): https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/autoplay-muted.html:3: <head> On 2016/06/21 at 21:28:41, esprehn wrote: > you don't need <head> or <html>, just add <body>. I could but having <title> outside of <head> isn't proper HTML. What's the benefit of removing the rest? https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/autoplay-when-visible.html (right): https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/autoplay-when-visible.html:6: <body> On 2016/06/21 at 21:28:41, esprehn wrote: > put the body after the media-file.js, right before your script What's the intent? https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/html/HTMLMediaElement.cpp (right): https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/html/HTMLMediaElement.cpp:1652: m_autoplayVisibilityObserver = new ElementVisibilityObserver(this, WTF::bind<bool>(&HTMLMediaElement::onVisibilityChangedForAutoplay, this)); On 2016/06/21 at 21:28:41, esprehn wrote: > do you actually need to pass the <bool> like this? rarely does anyone specify type like this. AFAIK, this is required to compile.
https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/autoplay-muted.html (right): https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/autoplay-muted.html:3: <head> On 2016/06/21 at 21:32:11, Mounir Lamouri (slow) wrote: > On 2016/06/21 at 21:28:41, esprehn wrote: > > you don't need <head> or <html>, just add <body>. > > I could but having <title> outside of <head> isn't proper HTML. What's the benefit of removing the rest? The <title> won't be outside the head, just like it wasn't before. Everything ends up in the <head> until we hit the first element that's not allowed there. https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/media/autoplay-when-visible.html (right): https://codereview.chromium.org/2051253002/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/media/autoplay-when-visible.html:6: <body> On 2016/06/21 at 21:32:11, Mounir Lamouri (slow) wrote: > On 2016/06/21 at 21:28:41, esprehn wrote: > > put the body after the media-file.js, right before your script > > What's the intent? It's the most minimal thing needed to make your test work. All the rest of this is just boilerplate that the parser will do for you anyway.
I've reduced the tests to no longer have <html> and <head>. I'm not a big fan of doing implicit instead of explicit but given that WPT recommends this, I will go with the flow. I still need <body> for body.appendChild() though.
I'm happy as soon as szager is. :)
The CQ bit was checked by mlamouri@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051253002/140001
lgtm, thanks!
lgtm
The CQ bit was unchecked by mlamouri@chromium.org
The CQ bit was checked by mlamouri@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zqzhang@chromium.org Link to the patchset: https://codereview.chromium.org/2051253002/#ps140001 (title: "reduce")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051253002/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by mlamouri@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2051253002/140001
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== Start autoplay muted videos with autoplay attribute when they are visible. The behaviour is linked to the autoplay-muted-videos flag. This is introducing a ElementVisibilityObserver helper that is behind the scene using the IntersectionObserver. BUG=618301 ========== to ========== Start autoplay muted videos with autoplay attribute when they are visible. The behaviour is linked to the autoplay-muted-videos flag. This is introducing a ElementVisibilityObserver helper that is behind the scene using the IntersectionObserver. BUG=618301 Committed: https://crrev.com/0c33d8c3ae4c8f1fc2d58574c128725389864d95 Cr-Commit-Position: refs/heads/master@{#401242} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/0c33d8c3ae4c8f1fc2d58574c128725389864d95 Cr-Commit-Position: refs/heads/master@{#401242} |
