Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2173353002:
Simplify ElementVisibilityObserver implementation. (Closed)
|
Created:
4 years, 5 months ago by sof Modified:
4 years, 4 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, nessy, vcarbune.chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSimplify ElementVisibilityObserver implementation.
Recast ElementVisibilityObserver's VisibilityCallback in a more
Blink-like manner by way of a Client interface. Thereby also addressing
on-off-heap cycle, a leak source.
Similarly, simplify the connection between ElementVisibilityObserver and
IntersectionObserver -- have the former directly implement the
IntersectionObserverCallback instead of indirectly using closure callbacks.
R=
BUG=627539
Committed: https://crrev.com/baf0ff22e47eb51e6cd4b0764d771151f7e0500b
Cr-Commit-Position: refs/heads/master@{#407450}
Patch Set 1 #Patch Set 2 : add reqd CORE_EXPORT #Patch Set 3 : simplify finalization + sync class comment #
Total comments: 2
Patch Set 4 : switch to IntersectionObserverCallback #Patch Set 5 : adjust visibility #
Messages
Total messages: 48 (25 generated)
The CQ bit was checked by sigbjornf@opera.com 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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by sigbjornf@opera.com 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...
The CQ bit was checked by sigbjornf@opera.com 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 ========== Simplify ElementVisibilityObserver client connection. R= BUG= ========== to ========== Simplify ElementVisibilityObserver client connection. Recast ElementVisibilityObserver's VisibilityCallback in a more Blink-like manner by way of a Client interface. Thereby also addressing on-off-heap cycle, a leak source. R= BUG= ==========
sigbjornf@opera.com changed reviewers: + haraken@chromium.org
please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
haraken@chromium.org changed reviewers: + szager@chromium.org, zqzhang@chromium.org
LGTM
LGTM. However I tried your patch locally, the flaky leak still exists if ElementVisibilityObserver.stop() is never called. But at least we know the leak source is not in ElementVisibilityObserver.
https://codereview.chromium.org/2173353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp (left): https://codereview.chromium.org/2173353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:26: WTF::bind(&ElementVisibilityObserver::onVisibilityChanged, wrapWeakPersistent(this))); I'd prefer not using a WeakPersistent<> here also.
The CQ bit was checked by sigbjornf@opera.com 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...
The CQ bit was checked by sigbjornf@opera.com 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...
https://codereview.chromium.org/2173353002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp (left): https://codereview.chromium.org/2173353002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/dom/ElementVisibilityObserver.cpp:26: WTF::bind(&ElementVisibilityObserver::onVisibilityChanged, wrapWeakPersistent(this))); On 2016/07/25 05:16:26, sof wrote: > I'd prefer not using a WeakPersistent<> here also. Now done - having two heap objects linked together by way of an off-heap callback (that additionally requires an on-heap intermediary + a WeakPersistent<>), seems unnecessary.
LGTM
On 2016/07/25 03:28:33, Zhiqiang Zhang (slow or OOO) wrote: > LGTM. > > However I tried your patch locally, the flaky leak still exists if > ElementVisibilityObserver.stop() is never called. But at least we know the leak > source is not in ElementVisibilityObserver. Thanks for trying it out locally! What tests are you seeing as failing? I cannot see them listed on the associated bug (nor can repro.)
On 2016/07/25 08:20:11, sof wrote: > On 2016/07/25 03:28:33, Zhiqiang Zhang (slow or OOO) wrote: > > LGTM. > > > > However I tried your patch locally, the flaky leak still exists if > > ElementVisibilityObserver.stop() is never called. But at least we know the > leak > > source is not in ElementVisibilityObserver. > > Thanks for trying it out locally! What tests are you seeing as failing? I cannot > see them listed on the associated bug (nor can repro.) In HTMLMediaElement, comment out the following lines: m_autoplayVisibilityObserver->stop(); m_autoplayVisibilityObserver = nullptr; Build target blink_tests, and run the following command: for i in `seq 20`; do third_party/WebKit/Tools/Scripts/run-webkit-tests -t Linux media/autoplay-muted.html --enable-leak-detection; done I'm seeing roughly 1/3 of the tests having memory leak.
On 2016/07/25 08:52:40, Zhiqiang Zhang (slow or OOO) wrote: > On 2016/07/25 08:20:11, sof wrote: > > On 2016/07/25 03:28:33, Zhiqiang Zhang (slow or OOO) wrote: > > > LGTM. > > > > > > However I tried your patch locally, the flaky leak still exists if > > > ElementVisibilityObserver.stop() is never called. But at least we know the > > leak > > > source is not in ElementVisibilityObserver. > > > > Thanks for trying it out locally! What tests are you seeing as failing? I > cannot > > see them listed on the associated bug (nor can repro.) > > In HTMLMediaElement, comment out the following lines: > m_autoplayVisibilityObserver->stop(); > m_autoplayVisibilityObserver = nullptr; > > Build target blink_tests, and run the following command: > > for i in `seq 20`; do third_party/WebKit/Tools/Scripts/run-webkit-tests -t Linux > media/autoplay-muted.html --enable-leak-detection; done > > I'm seeing roughly 1/3 of the tests having memory leak. Anyway, I'm fine if you can't fix the leak in this patch. I think it's not in ElementVisibilityObserver.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
mlamouri@chromium.org changed reviewers: + mlamouri@chromium.org
I find this change a bit unfortunate. It's easier for developers to use callbacks than having clients/interfaces for things like this. For example, one could use multiple instance of ElementVisibilityObservers depending on different contexts without having to keep track of the state: depending on which callback is called, the state will be clarified.
On 2016/07/25 10:43:02, Mounir Lamouri wrote: > I find this change a bit unfortunate. It's easier for developers to use > callbacks than having clients/interfaces for things like this. For example, one > could use multiple instance of ElementVisibilityObservers depending on different > contexts without having to keep track of the state: depending on which callback > is called, the state will be clarified. Closures provide one way to express that, but their convenience comes at a price in this context. Which doesn't seem warranted, where would you want to have multiple "visibility observers" for the same element? It seems like something an element itself could want to register, but who else?
On 2016/07/25 09:02:24, Zhiqiang Zhang (slow or OOO) wrote: > On 2016/07/25 08:52:40, Zhiqiang Zhang (slow or OOO) wrote: > > On 2016/07/25 08:20:11, sof wrote: > > > On 2016/07/25 03:28:33, Zhiqiang Zhang (slow or OOO) wrote: > > > > LGTM. > > > > > > > > However I tried your patch locally, the flaky leak still exists if > > > > ElementVisibilityObserver.stop() is never called. But at least we know the > > > leak > > > > source is not in ElementVisibilityObserver. > > > > > > Thanks for trying it out locally! What tests are you seeing as failing? I > > cannot > > > see them listed on the associated bug (nor can repro.) > > > > In HTMLMediaElement, comment out the following lines: > > m_autoplayVisibilityObserver->stop(); > > m_autoplayVisibilityObserver = nullptr; > > > > Build target blink_tests, and run the following command: > > > > for i in `seq 20`; do third_party/WebKit/Tools/Scripts/run-webkit-tests -t > Linux > > media/autoplay-muted.html --enable-leak-detection; done > > > > I'm seeing roughly 1/3 of the tests having memory leak. > > Anyway, I'm fine if you can't fix the leak in this patch. I think it's not in > ElementVisibilityObserver. Thanks, I can reproduce w/ that code modification. Unclear where the leakage really enters -- you can prevent it by weakifying the reference that HTMLMediaElement keeps to ElementVisibilityObserver (and vice versa in the other direction), but I think that papers over the real retention/leak source.
Description was changed from ========== Simplify ElementVisibilityObserver client connection. Recast ElementVisibilityObserver's VisibilityCallback in a more Blink-like manner by way of a Client interface. Thereby also addressing on-off-heap cycle, a leak source. R= BUG= ========== to ========== Simplify ElementVisibilityObserver implementation. Recast ElementVisibilityObserver's VisibilityCallback in a more Blink-like manner by way of a Client interface. Thereby also addressing on-off-heap cycle, a leak source. Similarly, simplify the connection between ElementVisibilityObserver and IntersectionObserver -- have the former directly implement the IntersectionObserverCallback instead of indirectly using closure callbacks. R= BUG=627539 ==========
The CQ bit was checked by sigbjornf@opera.com
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/2173353002/#ps80001 (title: "adjust visibility")
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.
Description was changed from ========== Simplify ElementVisibilityObserver implementation. Recast ElementVisibilityObserver's VisibilityCallback in a more Blink-like manner by way of a Client interface. Thereby also addressing on-off-heap cycle, a leak source. Similarly, simplify the connection between ElementVisibilityObserver and IntersectionObserver -- have the former directly implement the IntersectionObserverCallback instead of indirectly using closure callbacks. R= BUG=627539 ========== to ========== Simplify ElementVisibilityObserver implementation. Recast ElementVisibilityObserver's VisibilityCallback in a more Blink-like manner by way of a Client interface. Thereby also addressing on-off-heap cycle, a leak source. Similarly, simplify the connection between ElementVisibilityObserver and IntersectionObserver -- have the former directly implement the IntersectionObserverCallback instead of indirectly using closure callbacks. R= BUG=627539 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Simplify ElementVisibilityObserver implementation. Recast ElementVisibilityObserver's VisibilityCallback in a more Blink-like manner by way of a Client interface. Thereby also addressing on-off-heap cycle, a leak source. Similarly, simplify the connection between ElementVisibilityObserver and IntersectionObserver -- have the former directly implement the IntersectionObserverCallback instead of indirectly using closure callbacks. R= BUG=627539 ========== to ========== Simplify ElementVisibilityObserver implementation. Recast ElementVisibilityObserver's VisibilityCallback in a more Blink-like manner by way of a Client interface. Thereby also addressing on-off-heap cycle, a leak source. Similarly, simplify the connection between ElementVisibilityObserver and IntersectionObserver -- have the former directly implement the IntersectionObserverCallback instead of indirectly using closure callbacks. R= BUG=627539 Committed: https://crrev.com/baf0ff22e47eb51e6cd4b0764d771151f7e0500b Cr-Commit-Position: refs/heads/master@{#407450} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/baf0ff22e47eb51e6cd4b0764d771151f7e0500b Cr-Commit-Position: refs/heads/master@{#407450}
Message was sent while issue was closed.
On 2016/07/25 at 10:52:36, sigbjornf wrote: > On 2016/07/25 10:43:02, Mounir Lamouri wrote: > > I find this change a bit unfortunate. It's easier for developers to use > > callbacks than having clients/interfaces for things like this. For example, one > > could use multiple instance of ElementVisibilityObservers depending on different > > contexts without having to keep track of the state: depending on which callback > > is called, the state will be clarified. > > Closures provide one way to express that, but their convenience comes at a price in this context. Which doesn't seem warranted, where would you want to have multiple "visibility observers" for the same element? It seems like something an element itself could want to register, but who else? zqzhang@, I believe, has a CL that implements some UMA for autoplay where we measure multiple visibility aspects. If this CL doesn't fix the leak, I'm a bit confused by the benefit it provides. I find it unfortunate too that you didn't discuss the change with me given that I originally wrote ElementVisibilityObserver -- or any of the original reviewers of the class.
Message was sent while issue was closed.
On 2016/07/25 15:49:05, Mounir Lamouri wrote: > On 2016/07/25 at 10:52:36, sigbjornf wrote: > > On 2016/07/25 10:43:02, Mounir Lamouri wrote: > > > I find this change a bit unfortunate. It's easier for developers to use > > > callbacks than having clients/interfaces for things like this. For example, > one > > > could use multiple instance of ElementVisibilityObservers depending on > different > > > contexts without having to keep track of the state: depending on which > callback > > > is called, the state will be clarified. > > > > Closures provide one way to express that, but their convenience comes at a > price in this context. Which doesn't seem warranted, where would you want to > have multiple "visibility observers" for the same element? It seems like > something an element itself could want to register, but who else? > > zqzhang@, I believe, has a CL that implements some UMA for autoplay where we > measure multiple visibility aspects. I'm okay with reverting this CL. Sigbjorn? > > If this CL doesn't fix the leak, I'm a bit confused by the benefit it provides. > I find it unfortunate too that you didn't discuss the change with me given that > I originally wrote ElementVisibilityObserver -- or any of the original reviewers > of the class. Sorry about that :/ I was just cc-ing zqzhang@ and szager@ since they were in the loop of https://codereview.chromium.org/2151243002/.
Message was sent while issue was closed.
On 2016/07/29 11:21:47, haraken wrote: > On 2016/07/25 15:49:05, Mounir Lamouri wrote: > > On 2016/07/25 at 10:52:36, sigbjornf wrote: > > > On 2016/07/25 10:43:02, Mounir Lamouri wrote: > > > > I find this change a bit unfortunate. It's easier for developers to use > > > > callbacks than having clients/interfaces for things like this. For > example, > > one > > > > could use multiple instance of ElementVisibilityObservers depending on > > different > > > > contexts without having to keep track of the state: depending on which > > callback > > > > is called, the state will be clarified. > > > > > > Closures provide one way to express that, but their convenience comes at a > > price in this context. Which doesn't seem warranted, where would you want to > > have multiple "visibility observers" for the same element? It seems like > > something an element itself could want to register, but who else? > > > > zqzhang@, I believe, has a CL that implements some UMA for autoplay where we > > measure multiple visibility aspects. > > I'm okay with reverting this CL. Sigbjorn? > Why do we want to have all this complexity of an extra closure, extra GC-impedance matching object for EventCallbacks, a bunch of WeakPersistent<>s? I don't understand the reasoning being made of trade-offs, but do as you like if it helps somehow.
Message was sent while issue was closed.
On 2016/07/29 at 11:32:39, sigbjornf wrote: > On 2016/07/29 11:21:47, haraken wrote: > > On 2016/07/25 15:49:05, Mounir Lamouri wrote: > > > On 2016/07/25 at 10:52:36, sigbjornf wrote: > > > > On 2016/07/25 10:43:02, Mounir Lamouri wrote: > > > > > I find this change a bit unfortunate. It's easier for developers to use > > > > > callbacks than having clients/interfaces for things like this. For > > example, > > > one > > > > > could use multiple instance of ElementVisibilityObservers depending on > > > different > > > > > contexts without having to keep track of the state: depending on which > > > callback > > > > > is called, the state will be clarified. > > > > > > > > Closures provide one way to express that, but their convenience comes at a > > > price in this context. Which doesn't seem warranted, where would you want to > > > have multiple "visibility observers" for the same element? It seems like > > > something an element itself could want to register, but who else? > > > > > > zqzhang@, I believe, has a CL that implements some UMA for autoplay where we > > > measure multiple visibility aspects. > > > > I'm okay with reverting this CL. Sigbjorn? > > > > Why do we want to have all this complexity of an extra closure, extra GC-impedance matching object for EventCallbacks, a bunch of WeakPersistent<>s? > > I don't understand the reasoning being made of trade-offs, but do as you like if it helps somehow. My concern here is developer experience: using a Client interface is a worse developer experience than using a callback. I understand that it is a bit more complicated to handle on the memory side and Zhiqiang and I were actually looking into this issue. My understanding is that Blink is trying to improve its internal APIs and this change is not going in this direction in my opinion. However, I'm happy to defer to someone else in the architecture team.
Message was sent while issue was closed.
On 2016/07/29 14:05:23, Mounir Lamouri wrote: > On 2016/07/29 at 11:32:39, sigbjornf wrote: > > On 2016/07/29 11:21:47, haraken wrote: > > > On 2016/07/25 15:49:05, Mounir Lamouri wrote: > > > > On 2016/07/25 at 10:52:36, sigbjornf wrote: > > > > > On 2016/07/25 10:43:02, Mounir Lamouri wrote: > > > > > > I find this change a bit unfortunate. It's easier for developers to > use > > > > > > callbacks than having clients/interfaces for things like this. For > > > example, > > > > one > > > > > > could use multiple instance of ElementVisibilityObservers depending on > > > > different > > > > > > contexts without having to keep track of the state: depending on which > > > > callback > > > > > > is called, the state will be clarified. > > > > > > > > > > Closures provide one way to express that, but their convenience comes at > a > > > > price in this context. Which doesn't seem warranted, where would you want > to > > > > have multiple "visibility observers" for the same element? It seems like > > > > something an element itself could want to register, but who else? > > > > > > > > zqzhang@, I believe, has a CL that implements some UMA for autoplay where > we > > > > measure multiple visibility aspects. > > > > > > I'm okay with reverting this CL. Sigbjorn? > > > > > > > Why do we want to have all this complexity of an extra closure, extra > GC-impedance matching object for EventCallbacks, a bunch of WeakPersistent<>s? > > > > I don't understand the reasoning being made of trade-offs, but do as you like > if it helps somehow. > > My concern here is developer experience: using a Client interface is a worse > developer experience than using a callback. I understand that it is a bit more > complicated to handle on the memory side and Zhiqiang and I were actually > looking into this issue. My understanding is that Blink is trying to improve its > internal APIs and this change is not going in this direction in my opinion. > However, I'm happy to defer to someone else in the architecture team. It would be helpful to look at zqzhang@'s patch that adds multiple visibility observers for the same element. And consider what we can do there.
Message was sent while issue was closed.
On 2016/07/29 at 14:09:18, haraken wrote: > On 2016/07/29 14:05:23, Mounir Lamouri wrote: > > On 2016/07/29 at 11:32:39, sigbjornf wrote: > > > On 2016/07/29 11:21:47, haraken wrote: > > > > On 2016/07/25 15:49:05, Mounir Lamouri wrote: > > > > > On 2016/07/25 at 10:52:36, sigbjornf wrote: > > > > > > On 2016/07/25 10:43:02, Mounir Lamouri wrote: > > > > > > > I find this change a bit unfortunate. It's easier for developers to > > use > > > > > > > callbacks than having clients/interfaces for things like this. For > > > > example, > > > > > one > > > > > > > could use multiple instance of ElementVisibilityObservers depending on > > > > > different > > > > > > > contexts without having to keep track of the state: depending on which > > > > > callback > > > > > > > is called, the state will be clarified. > > > > > > > > > > > > Closures provide one way to express that, but their convenience comes at > > a > > > > > price in this context. Which doesn't seem warranted, where would you want > > to > > > > > have multiple "visibility observers" for the same element? It seems like > > > > > something an element itself could want to register, but who else? > > > > > > > > > > zqzhang@, I believe, has a CL that implements some UMA for autoplay where > > we > > > > > measure multiple visibility aspects. > > > > > > > > I'm okay with reverting this CL. Sigbjorn? > > > > > > > > > > Why do we want to have all this complexity of an extra closure, extra > > GC-impedance matching object for EventCallbacks, a bunch of WeakPersistent<>s? > > > > > > I don't understand the reasoning being made of trade-offs, but do as you like > > if it helps somehow. > > > > My concern here is developer experience: using a Client interface is a worse > > developer experience than using a callback. I understand that it is a bit more > > complicated to handle on the memory side and Zhiqiang and I were actually > > looking into this issue. My understanding is that Blink is trying to improve its > > internal APIs and this change is not going in this direction in my opinion. > > However, I'm happy to defer to someone else in the architecture team. > > It would be helpful to look at zqzhang@'s patch that adds multiple visibility observers for the same element. And consider what we can do there. Sorry for the delay. zqzhang@ is on holidays (he actually reviewed this while on holiady) Though, I think my point can be explained without a CL. There are various things we want to measure with regards to visibility: - how long after play() does the video becomes visible - how long after it becomes visible, does it hide - how long does it play hidden - same for autoplay attribute With the ::Client solution, we need to have a large method and keep states around to know in which one we are while with callbacks, we can easily create a callback that will trigger in some situation: play() has started, we create a callback that register when it will be visible, and will handle the visibility changes and same for autoplay attribute. We don't need to know which source started playback, we only need to set up the right callback when the state changes.
Message was sent while issue was closed.
On 2016/08/02 13:40:50, Mounir Lamouri wrote: > On 2016/07/29 at 14:09:18, haraken wrote: > > On 2016/07/29 14:05:23, Mounir Lamouri wrote: > > > On 2016/07/29 at 11:32:39, sigbjornf wrote: > > > > On 2016/07/29 11:21:47, haraken wrote: > > > > > On 2016/07/25 15:49:05, Mounir Lamouri wrote: > > > > > > On 2016/07/25 at 10:52:36, sigbjornf wrote: > > > > > > > On 2016/07/25 10:43:02, Mounir Lamouri wrote: > > > > > > > > I find this change a bit unfortunate. It's easier for developers > to > > > use > > > > > > > > callbacks than having clients/interfaces for things like this. For > > > > > example, > > > > > > one > > > > > > > > could use multiple instance of ElementVisibilityObservers > depending on > > > > > > different > > > > > > > > contexts without having to keep track of the state: depending on > which > > > > > > callback > > > > > > > > is called, the state will be clarified. > > > > > > > > > > > > > > Closures provide one way to express that, but their convenience > comes at > > > a > > > > > > price in this context. Which doesn't seem warranted, where would you > want > > > to > > > > > > have multiple "visibility observers" for the same element? It seems > like > > > > > > something an element itself could want to register, but who else? > > > > > > > > > > > > zqzhang@, I believe, has a CL that implements some UMA for autoplay > where > > > we > > > > > > measure multiple visibility aspects. > > > > > > > > > > I'm okay with reverting this CL. Sigbjorn? > > > > > > > > > > > > > Why do we want to have all this complexity of an extra closure, extra > > > GC-impedance matching object for EventCallbacks, a bunch of > WeakPersistent<>s? > > > > > > > > I don't understand the reasoning being made of trade-offs, but do as you > like > > > if it helps somehow. > > > > > > My concern here is developer experience: using a Client interface is a worse > > > developer experience than using a callback. I understand that it is a bit > more > > > complicated to handle on the memory side and Zhiqiang and I were actually > > > looking into this issue. My understanding is that Blink is trying to improve > its > > > internal APIs and this change is not going in this direction in my opinion. > > > However, I'm happy to defer to someone else in the architecture team. > > > > It would be helpful to look at zqzhang@'s patch that adds multiple visibility > observers for the same element. And consider what we can do there. > > Sorry for the delay. zqzhang@ is on holidays (he actually reviewed this while on > holiady) > > Though, I think my point can be explained without a CL. There are various things > we want to measure with regards to visibility: > - how long after play() does the video becomes visible > - how long after it becomes visible, does it hide > - how long does it play hidden > - same for autoplay attribute > > With the ::Client solution, we need to have a large method and keep states > around to know in which one we are while with callbacks, we can easily create a > callback that will trigger in some situation: play() has started, we create a > callback that register when it will be visible, and will handle the visibility > changes and same for autoplay attribute. We don't need to know which source > started playback, we only need to set up the right callback when the state > changes. You can have multiple client + element visibility observers, one for each of the above. We need to see actual code.
Message was sent while issue was closed.
Hmm, I think we should revert this. This was done very intentionally because the
API ergonomics are supposed to use callbacks and model JS.
ex.
addEventListener("foo", this.foo.bind(this))
historically in blink you'd need to write a CppEventHandler which is super
verbose and painful. Instead we're trying to modernize these interfaces as wee
use them from C++ so that they can be much closer to this.
addEventListener(EvenTypeNames::foo, bind(this, foo))
and so on. IntersectionObserver was done this way for that reason. It's fine if
you want to just vend an IntersectionObserverCallback to share the code paths,
but we need to preserve the Callback like interface.
The hope is to do this all over the API, ex. instead of having a delegate
interface for cc/ we're using base::Callbacks there too.
https://cs.chromium.org/chromium/src/cc/trees/proxy_main.cc?q=ThreadProxy&sq=...
and I hope we can generalize this to invert a bunch of responsibity and reduce
the boilerplate that's hurting us. :)
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
