Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2415723002:
[Blink, RemotePlayback] watchAvailability() implementation. (Closed)
|
Created:
4 years, 2 months ago by whywhat Modified:
4 years, 2 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews, eric.carlson_apple.com, feature-media-reviews_chromium.org, mlamouri+watch-blink_chromium.org, Srirama Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Blink, RemotePlayback] watchAvailability() implementation.
BUG=655233
TEST=new layout tests + https://avayvod.github.io/remote-playback/test.html
Committed: https://crrev.com/6ab7f8f63e07b15373ad84ff677244d70f5059e1
Cr-Commit-Position: refs/heads/master@{#425891}
Patch Set 1 #Patch Set 2 : Cleanup after RemotePlaybackAvailability #
Total comments: 12
Patch Set 3 : Addressing comments #
Total comments: 9
Patch Set 4 : setWrapperReferences and save ScriptState #Patch Set 5 : Added layout test for callback gc #Messages
Total messages: 47 (15 generated)
Cleanup after RemotePlaybackAvailability
The CQ bit was checked by avayvod@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...
avayvod@chromium.org changed reviewers: + bashi@chromium.org, mlamouri@chromium.org
Please, take a look. Mounir for overall review Kenichi-san for bindings/modules/v8/generated.gni
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:81: } while (!m_availabilityCallbacks.add(id, callback).isNewEntry); |callback| only has a weak reference to v8::Function and it could be garbage-collected unless there is no owner of the function. This means that when you keep |callback| in m_avaiabilityCallbacks, you need to set the v8 function as a private property of the wrapper object of RemotePlayback. You can do that by using [Custom=CallPrologue] or [Custom=CallEplilogue]. https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... In a custom binding, you will call V8PrivateProperty::getXXX().set() like V8IDBObserver::constructorCustom() This is a workaround to avoid circular dependency between v8 and blink objects. When crbug.com/468240 is resolved we should be able to remove such workaround. Sorry for inconvenience :(
mlamouri@chromium.org changed reviewers: + zqzhang@chromium.org
Zhiqiang, can you have a look at this? :)
lgtm w/ nits. I haven't done any promise-related stuffs in Blink, may miss some errors. https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/media/remoteplayback/disable-remote-playback-cancel-watch-availability-throws.html (right): https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/media/remoteplayback/disable-remote-playback-cancel-watch-availability-throws.html:38: }, 'Test that calling watchAvailability() when disableRemotePlayback attribute is set throws an exception.'); s/watchAvailability/cancelWatchAvailability https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:77: // TODO(avayvod): implement steps 4 and 5 of the algorithm. add a bug number? https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:84: m_scriptState = scriptState; Is this the same scriptState every time? Maybe yes or we don't care since it's in the same ExecutionContext. Not sure. https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h (right): https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.h:74: // Need a void() method to post it as a task. s/Need a/Need to be a
On 2016/10/13 at 00:45:46, bashi wrote: > https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): > > https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:81: } while (!m_availabilityCallbacks.add(id, callback).isNewEntry); > |callback| only has a weak reference to v8::Function and it could be garbage-collected unless there is no owner of the function. This means that when you keep |callback| in m_avaiabilityCallbacks, you need to set the v8 function as a private property of the wrapper object of RemotePlayback. You can do that by using [Custom=CallPrologue] or [Custom=CallEplilogue]. > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... > > In a custom binding, you will call V8PrivateProperty::getXXX().set() like V8IDBObserver::constructorCustom() > > This is a workaround to avoid circular dependency between v8 and blink objects. When crbug.com/468240 is resolved we should be able to remove such workaround. Sorry for inconvenience :( Thanks. Will try to do so.
https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:77: // TODO(avayvod): implement steps 4 and 5 of the algorithm. On 2016/10/13 at 15:23:11, Zhiqiang Zhang wrote: > add a bug number? Done. https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:81: } while (!m_availabilityCallbacks.add(id, callback).isNewEntry); On 2016/10/13 at 00:45:46, bashi1 wrote: > |callback| only has a weak reference to v8::Function and it could be garbage-collected unless there is no owner of the function. This means that when you keep |callback| in m_avaiabilityCallbacks, you need to set the v8 function as a private property of the wrapper object of RemotePlayback. You can do that by using [Custom=CallPrologue] or [Custom=CallEplilogue]. > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... > > In a custom binding, you will call V8PrivateProperty::getXXX().set() like V8IDBObserver::constructorCustom() > > This is a workaround to avoid circular dependency between v8 and blink objects. When crbug.com/468240 is resolved we should be able to remove such workaround. Sorry for inconvenience :( I think I need more info... Should I add Prologue or Epilogue? I imagine I need to add custom handling to both adding and removing callbacks? V8IDBObserver has plenty of code in it which I have little idea about :) Is copy/paste reliable? How do I test it actually works? How do I pass the last callback I added to the HeapHashMap into that custom method to bind it to the V8 callback? Lastly, if I chose to implement the callback class myself, would I be able to avoid this hassle? :) https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:84: m_scriptState = scriptState; On 2016/10/13 at 15:23:11, Zhiqiang Zhang wrote: > Is this the same scriptState every time? Maybe yes or we don't care since it's in the same ExecutionContext. Not sure. That's something I'm not 100% sure about. Other places using callbacks and passing scriptState just acquire the value in their constructor (since it's passed there) and keep it.
Addressing comments
haraken@chromium.org changed reviewers: + haraken@chromium.org
https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:84: m_scriptState = scriptState; On 2016/10/14 19:58:19, whywhat wrote: > On 2016/10/13 at 15:23:11, Zhiqiang Zhang wrote: > > Is this the same scriptState every time? Maybe yes or we don't care since it's > in the same ExecutionContext. Not sure. > > That's something I'm not 100% sure about. Other places using callbacks and > passing scriptState just acquire the value in their constructor (since it's > passed there) and keep it. m_scriptState must be equal to scriptState. You can just remove this code, or add DCHECK(m_scriptState == scriptState). https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:94: wrapPersistent(this), id), Would you help me understand why you need to post a task? https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl (right): https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl:25: [CallWith=ScriptState, Custom=CallPrologue] Promise<void> cancelWatchAvailability(optional long id); Where is the custom bindings written?
lgtm if haraken@ is happy :) https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:94: wrapPersistent(this), id), On 2016/10/15 at 01:52:45, haraken wrote: > Would you help me understand why you need to post a task? I'm not avayvod@ but I think this is because the spec says to queue a task. https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:121: NotFoundError, "A callback with the given id is not found.")); s/is not found/was not found/ ? https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:136: if (m_mediaElement->fastHasAttribute(HTMLNames::disableremoteplaybackAttr)) { Here and above, there is an issue where we don't cancel the callbacks when `disableRemotePlayback` is set but we do prevent cancelling. I've open a spec bug. No need to change this CL because we should address this on the spec side first. Likely, we will cancel all the callbacks if `disableRemotePlayback` is set to true.
https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:84: m_scriptState = scriptState; On 2016/10/15 at 01:52:45, haraken wrote: > On 2016/10/14 19:58:19, whywhat wrote: > > On 2016/10/13 at 15:23:11, Zhiqiang Zhang wrote: > > > Is this the same scriptState every time? Maybe yes or we don't care since it's > > in the same ExecutionContext. Not sure. > > > > That's something I'm not 100% sure about. Other places using callbacks and > > passing scriptState just acquire the value in their constructor (since it's > > passed there) and keep it. > > m_scriptState must be equal to scriptState. > > You can just remove this code, or add DCHECK(m_scriptState == scriptState). Do you mean I should remove the if and just always set m_scriptState to scriptState or leave the if and add a DCHECK()? I wish I could get ScriptState in the ctor and just save it there. Perhaps I need to try to pass it in create(). https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:94: wrapPersistent(this), id), On 2016/10/15 at 18:14:00, mlamouri wrote: > On 2016/10/15 at 01:52:45, haraken wrote: > > Would you help me understand why you need to post a task? > > I'm not avayvod@ but I think this is because the spec says to queue a task. There's also an issue of calling the callback before the promise is resolved, I think the developers would expect the callback to fire after and could do some setup in the promise resolution. https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:136: if (m_mediaElement->fastHasAttribute(HTMLNames::disableremoteplaybackAttr)) { On 2016/10/15 at 18:14:00, mlamouri wrote: > Here and above, there is an issue where we don't cancel the callbacks when `disableRemotePlayback` is set but we do prevent cancelling. I've open a spec bug. No need to change this CL because we should address this on the spec side first. Likely, we will cancel all the callbacks if `disableRemotePlayback` is set to true. Right, thanks for filing the spec bug. Cancelling the callbacks sgtm. https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl (right): https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl:25: [CallWith=ScriptState, Custom=CallPrologue] Promise<void> cancelWatchAvailability(optional long id); On 2016/10/15 at 01:52:45, haraken wrote: > Where is the custom bindings written? Nowhere yet as I'm not even sure how to write it. See my earlier reply to bashi@'s suggestion.
https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:81: } while (!m_availabilityCallbacks.add(id, callback).isNewEntry); On 2016/10/14 19:58:19, whywhat wrote: > On 2016/10/13 at 00:45:46, bashi1 wrote: > > |callback| only has a weak reference to v8::Function and it could be > garbage-collected unless there is no owner of the function. This means that when > you keep |callback| in m_avaiabilityCallbacks, you need to set the v8 function > as a private property of the wrapper object of RemotePlayback. You can do that > by using [Custom=CallPrologue] or [Custom=CallEplilogue]. > > > > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... > > > > In a custom binding, you will call V8PrivateProperty::getXXX().set() like > V8IDBObserver::constructorCustom() > > > > This is a workaround to avoid circular dependency between v8 and blink > objects. When crbug.com/468240 is resolved we should be able to remove such > workaround. Sorry for inconvenience :( > > I think I need more info... > > Should I add Prologue or Epilogue? I imagine I need to add custom handling to > both adding and removing callbacks? Maybe either is fine. As for removing, it depend on the relationship between the owner (RemotePlayback?) and callbacks. If removal occurs regardless of the lifetime of the owner, you may need to unregister callbacks. However it seems we don't provide a way to do that. yukishiino@ ? > V8IDBObserver has plenty of code in it which I have little idea about :) Is > copy/paste reliable? How do I test it actually works? I think you just need to call V8PrivateProperty::getXXX().set(). It's hard to test as we can't get v8 reference graphs from blink, but at least we should be able to have layout tests. > How do I pass the last callback I added to the HeapHashMap into that custom > method to bind it to the V8 callback? I don't think you need to do it. The custom binding code is just for adding owner -> callback reference in V8, and V8PrivateProperty does the job. > > Lastly, if I chose to implement the callback class myself, would I be able to > avoid this hassle? :) Unfortunately no. You will end up writing everything which the code generator can generate... https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl (right): https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl:25: [CallWith=ScriptState, Custom=CallPrologue] Promise<void> cancelWatchAvailability(optional long id); On 2016/10/16 03:41:40, whywhat wrote: > On 2016/10/15 at 01:52:45, haraken wrote: > > Where is the custom bindings written? > > Nowhere yet as I'm not even sure how to write it. See my earlier reply to > bashi@'s suggestion. You will need to add a new file under Source/bindings/modules/v8/custom. The new file will be called V8RemotePlaybackCustom.cpp and it would have: - void V8RemotePlayback::watchAvailabilityMethodEpilogueCustom(const v8::FunctionCallbackInfo<v8::Value>&, RemotePlayback*) - void V8RemotePlayback::cancelAvailabilityMethodPrologueCustom(const v8::FunctionCallbackInfo<v8::Value>&, RemotePlayback*)
bashi@chromium.org changed reviewers: + yukishiino@chromium.org
+yukishiino@
https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp (right): https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:84: m_scriptState = scriptState; On 2016/10/16 03:41:40, whywhat wrote: > On 2016/10/15 at 01:52:45, haraken wrote: > > On 2016/10/14 19:58:19, whywhat wrote: > > > On 2016/10/13 at 15:23:11, Zhiqiang Zhang wrote: > > > > Is this the same scriptState every time? Maybe yes or we don't care since > it's > > > in the same ExecutionContext. Not sure. > > > > > > That's something I'm not 100% sure about. Other places using callbacks and > > > passing scriptState just acquire the value in their constructor (since it's > > > passed there) and keep it. > > > > m_scriptState must be equal to scriptState. > > > > You can just remove this code, or add DCHECK(m_scriptState == scriptState). > > Do you mean I should remove the if and just always set m_scriptState to > scriptState or leave the if and add a DCHECK()? > I wish I could get ScriptState in the ctor and just save it there. Perhaps I > need to try to pass it in create(). Yes. You can save the ScriptState in the constructor and stop passing ScriptState to these methods.
On 2016/10/17 00:11:05, bashi1 wrote: > https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp > (right): > > https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:81: } while > (!m_availabilityCallbacks.add(id, callback).isNewEntry); > On 2016/10/14 19:58:19, whywhat wrote: > > On 2016/10/13 at 00:45:46, bashi1 wrote: > > > |callback| only has a weak reference to v8::Function and it could be > > garbage-collected unless there is no owner of the function. This means that > when > > you keep |callback| in m_avaiabilityCallbacks, you need to set the v8 function > > as a private property of the wrapper object of RemotePlayback. You can do that > > by using [Custom=CallPrologue] or [Custom=CallEplilogue]. > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... > > > > > > In a custom binding, you will call V8PrivateProperty::getXXX().set() like > > V8IDBObserver::constructorCustom() > > > > > > This is a workaround to avoid circular dependency between v8 and blink > > objects. When crbug.com/468240 is resolved we should be able to remove such > > workaround. Sorry for inconvenience :( > > > > I think I need more info... > > > > Should I add Prologue or Epilogue? I imagine I need to add custom handling to > > both adding and removing callbacks? > Maybe either is fine. > > As for removing, it depend on the relationship between the owner > (RemotePlayback?) and callbacks. If removal occurs regardless of the lifetime of > the owner, you may need to unregister callbacks. However it seems we don't > provide a way to do that. yukishiino@ ? > > > V8IDBObserver has plenty of code in it which I have little idea about :) Is > > copy/paste reliable? How do I test it actually works? > I think you just need to call V8PrivateProperty::getXXX().set(). It's hard to > test as we can't get v8 reference graphs from blink, but at least we should be > able to have layout tests. > > How do I pass the last callback I added to the HeapHashMap into that custom > > method to bind it to the V8 callback? > I don't think you need to do it. The custom binding code is just for adding > owner -> callback reference in V8, and V8PrivateProperty does the job. > > > > Lastly, if I chose to implement the callback class myself, would I be able to > > avoid this hassle? :) > > Unfortunately no. You will end up writing everything which the code generator > can generate... > > https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl > (right): > > https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl:25: > [CallWith=ScriptState, Custom=CallPrologue] Promise<void> > cancelWatchAvailability(optional long id); > On 2016/10/16 03:41:40, whywhat wrote: > > On 2016/10/15 at 01:52:45, haraken wrote: > > > Where is the custom bindings written? > > > > Nowhere yet as I'm not even sure how to write it. See my earlier reply to > > bashi@'s suggestion. > > You will need to add a new file under Source/bindings/modules/v8/custom. The new > file will be called V8RemotePlaybackCustom.cpp and it would have: > - void V8RemotePlayback::watchAvailabilityMethodEpilogueCustom(const > v8::FunctionCallbackInfo<v8::Value>&, RemotePlayback*) > - void V8RemotePlayback::cancelAvailabilityMethodPrologueCustom(const > v8::FunctionCallbackInfo<v8::Value>&, RemotePlayback*) An easier way of doing this would be to use [Custom=visitDOMWrapper]. You can write a visitDOMWrapper function to visit from a RemotePlayback's wrapper to RemotePlaybackCallbacks. bashi@, yukishiino@: That said, it's not nice that developers need to write custom bindings every time they want to use callback functions. Would it be an option to make XXXCallback::create take a wrapper parameter and add a strong hidden reference from XXXCallback to the wrapper in the generated code? (I'm assuming that callback functions should be associated with a wrapper object in common cases, but correct me if I'm wrong.)
On 2016/10/17 00:32:37, haraken wrote: > On 2016/10/17 00:11:05, bashi1 wrote: > > > https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp > > (right): > > > > > https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:81: } > while > > (!m_availabilityCallbacks.add(id, callback).isNewEntry); > > On 2016/10/14 19:58:19, whywhat wrote: > > > On 2016/10/13 at 00:45:46, bashi1 wrote: > > > > |callback| only has a weak reference to v8::Function and it could be > > > garbage-collected unless there is no owner of the function. This means that > > when > > > you keep |callback| in m_avaiabilityCallbacks, you need to set the v8 > function > > > as a private property of the wrapper object of RemotePlayback. You can do > that > > > by using [Custom=CallPrologue] or [Custom=CallEplilogue]. > > > > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... > > > > > > > > In a custom binding, you will call V8PrivateProperty::getXXX().set() like > > > V8IDBObserver::constructorCustom() > > > > > > > > This is a workaround to avoid circular dependency between v8 and blink > > > objects. When crbug.com/468240 is resolved we should be able to remove such > > > workaround. Sorry for inconvenience :( > > > > > > I think I need more info... > > > > > > Should I add Prologue or Epilogue? I imagine I need to add custom handling > to > > > both adding and removing callbacks? > > Maybe either is fine. > > > > As for removing, it depend on the relationship between the owner > > (RemotePlayback?) and callbacks. If removal occurs regardless of the lifetime > of > > the owner, you may need to unregister callbacks. However it seems we don't > > provide a way to do that. yukishiino@ ? > > > > > V8IDBObserver has plenty of code in it which I have little idea about :) Is > > > copy/paste reliable? How do I test it actually works? > > I think you just need to call V8PrivateProperty::getXXX().set(). It's hard to > > test as we can't get v8 reference graphs from blink, but at least we should be > > able to have layout tests. > > > How do I pass the last callback I added to the HeapHashMap into that custom > > > method to bind it to the V8 callback? > > I don't think you need to do it. The custom binding code is just for adding > > owner -> callback reference in V8, and V8PrivateProperty does the job. > > > > > > Lastly, if I chose to implement the callback class myself, would I be able > to > > > avoid this hassle? :) > > > > Unfortunately no. You will end up writing everything which the code generator > > can generate... > > > > > https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl > > (right): > > > > > https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl:25: > > [CallWith=ScriptState, Custom=CallPrologue] Promise<void> > > cancelWatchAvailability(optional long id); > > On 2016/10/16 03:41:40, whywhat wrote: > > > On 2016/10/15 at 01:52:45, haraken wrote: > > > > Where is the custom bindings written? > > > > > > Nowhere yet as I'm not even sure how to write it. See my earlier reply to > > > bashi@'s suggestion. > > > > You will need to add a new file under Source/bindings/modules/v8/custom. The > new > > file will be called V8RemotePlaybackCustom.cpp and it would have: > > - void V8RemotePlayback::watchAvailabilityMethodEpilogueCustom(const > > v8::FunctionCallbackInfo<v8::Value>&, RemotePlayback*) > > - void V8RemotePlayback::cancelAvailabilityMethodPrologueCustom(const > > v8::FunctionCallbackInfo<v8::Value>&, RemotePlayback*) > > An easier way of doing this would be to use [Custom=visitDOMWrapper]. You can > write a visitDOMWrapper function to visit from a RemotePlayback's wrapper to > RemotePlaybackCallbacks. > > bashi@, yukishiino@: That said, it's not nice that developers need to write > custom bindings every time they want to use callback functions. Would it be an > option to make XXXCallback::create take a wrapper parameter and add a strong > hidden reference from XXXCallback to the wrapper in the generated code? (I'm > assuming that callback functions should be associated with a wrapper object in > common cases, but correct me if I'm wrong.) I was assuming that visitDOMWrapper is not available yet. If it's ready to use, that's would be the right solution. Callback creation and referencing are not directly related IMO. Rather than adding a parameter to create(), I'd prefer to add SetOwner() method to callbacks if visitDOMWrapper isn't an option.
On 2016/10/17 00:43:43, bashi1 wrote: > On 2016/10/17 00:32:37, haraken wrote: > > On 2016/10/17 00:11:05, bashi1 wrote: > > > > > > https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp > > > (right): > > > > > > > > > https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:81: } > > while > > > (!m_availabilityCallbacks.add(id, callback).isNewEntry); > > > On 2016/10/14 19:58:19, whywhat wrote: > > > > On 2016/10/13 at 00:45:46, bashi1 wrote: > > > > > |callback| only has a weak reference to v8::Function and it could be > > > > garbage-collected unless there is no owner of the function. This means > that > > > when > > > > you keep |callback| in m_avaiabilityCallbacks, you need to set the v8 > > function > > > > as a private property of the wrapper object of RemotePlayback. You can do > > that > > > > by using [Custom=CallPrologue] or [Custom=CallEplilogue]. > > > > > > > > > > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... > > > > > > > > > > In a custom binding, you will call V8PrivateProperty::getXXX().set() > like > > > > V8IDBObserver::constructorCustom() > > > > > > > > > > This is a workaround to avoid circular dependency between v8 and blink > > > > objects. When crbug.com/468240 is resolved we should be able to remove > such > > > > workaround. Sorry for inconvenience :( > > > > > > > > I think I need more info... > > > > > > > > Should I add Prologue or Epilogue? I imagine I need to add custom handling > > to > > > > both adding and removing callbacks? > > > Maybe either is fine. > > > > > > As for removing, it depend on the relationship between the owner > > > (RemotePlayback?) and callbacks. If removal occurs regardless of the > lifetime > > of > > > the owner, you may need to unregister callbacks. However it seems we don't > > > provide a way to do that. yukishiino@ ? > > > > > > > V8IDBObserver has plenty of code in it which I have little idea about :) > Is > > > > copy/paste reliable? How do I test it actually works? > > > I think you just need to call V8PrivateProperty::getXXX().set(). It's hard > to > > > test as we can't get v8 reference graphs from blink, but at least we should > be > > > able to have layout tests. > > > > How do I pass the last callback I added to the HeapHashMap into that > custom > > > > method to bind it to the V8 callback? > > > I don't think you need to do it. The custom binding code is just for adding > > > owner -> callback reference in V8, and V8PrivateProperty does the job. > > > > > > > > Lastly, if I chose to implement the callback class myself, would I be able > > to > > > > avoid this hassle? :) > > > > > > Unfortunately no. You will end up writing everything which the code > generator > > > can generate... > > > > > > > > > https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl > > > (right): > > > > > > > > > https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl:25: > > > [CallWith=ScriptState, Custom=CallPrologue] Promise<void> > > > cancelWatchAvailability(optional long id); > > > On 2016/10/16 03:41:40, whywhat wrote: > > > > On 2016/10/15 at 01:52:45, haraken wrote: > > > > > Where is the custom bindings written? > > > > > > > > Nowhere yet as I'm not even sure how to write it. See my earlier reply to > > > > bashi@'s suggestion. > > > > > > You will need to add a new file under Source/bindings/modules/v8/custom. The > > new > > > file will be called V8RemotePlaybackCustom.cpp and it would have: > > > - void V8RemotePlayback::watchAvailabilityMethodEpilogueCustom(const > > > v8::FunctionCallbackInfo<v8::Value>&, RemotePlayback*) > > > - void V8RemotePlayback::cancelAvailabilityMethodPrologueCustom(const > > > v8::FunctionCallbackInfo<v8::Value>&, RemotePlayback*) > > > > An easier way of doing this would be to use [Custom=visitDOMWrapper]. You can > > write a visitDOMWrapper function to visit from a RemotePlayback's wrapper to > > RemotePlaybackCallbacks. > > > > bashi@, yukishiino@: That said, it's not nice that developers need to write > > custom bindings every time they want to use callback functions. Would it be an > > option to make XXXCallback::create take a wrapper parameter and add a strong > > hidden reference from XXXCallback to the wrapper in the generated code? (I'm > > assuming that callback functions should be associated with a wrapper object in > > common cases, but correct me if I'm wrong.) > > I was assuming that visitDOMWrapper is not available yet. If it's ready to use, > that's would be the right solution. traceWrapper is not available but visitDOMWrapper is avaialable. visitDOMWrapper is going to be replaced with traceWrapper. > Callback creation and referencing are not directly related IMO. Rather than > adding a parameter to create(), I'd prefer to add SetOwner() method to callbacks > if visitDOMWrapper isn't an option. Per the spec, I think you're right. I'm wondering if there is any case where we don't want to associate a callback function with a wrapper. FWIW, we're unconditionally adding a strong reference from a wrapper to its event listeners.
On 2016/10/17 00:50:55, haraken wrote: > On 2016/10/17 00:43:43, bashi1 wrote: > > On 2016/10/17 00:32:37, haraken wrote: > > > On 2016/10/17 00:11:05, bashi1 wrote: > > > > > > > > > > https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:81: } > > > while > > > > (!m_availabilityCallbacks.add(id, callback).isNewEntry); > > > > On 2016/10/14 19:58:19, whywhat wrote: > > > > > On 2016/10/13 at 00:45:46, bashi1 wrote: > > > > > > |callback| only has a weak reference to v8::Function and it could be > > > > > garbage-collected unless there is no owner of the function. This means > > that > > > > when > > > > > you keep |callback| in m_avaiabilityCallbacks, you need to set the v8 > > > function > > > > > as a private property of the wrapper object of RemotePlayback. You can > do > > > that > > > > > by using [Custom=CallPrologue] or [Custom=CallEplilogue]. > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... > > > > > > > > > > > > In a custom binding, you will call V8PrivateProperty::getXXX().set() > > like > > > > > V8IDBObserver::constructorCustom() > > > > > > > > > > > > This is a workaround to avoid circular dependency between v8 and blink > > > > > objects. When crbug.com/468240 is resolved we should be able to remove > > such > > > > > workaround. Sorry for inconvenience :( > > > > > > > > > > I think I need more info... > > > > > > > > > > Should I add Prologue or Epilogue? I imagine I need to add custom > handling > > > to > > > > > both adding and removing callbacks? > > > > Maybe either is fine. > > > > > > > > As for removing, it depend on the relationship between the owner > > > > (RemotePlayback?) and callbacks. If removal occurs regardless of the > > lifetime > > > of > > > > the owner, you may need to unregister callbacks. However it seems we don't > > > > provide a way to do that. yukishiino@ ? > > > > > > > > > V8IDBObserver has plenty of code in it which I have little idea about :) > > Is > > > > > copy/paste reliable? How do I test it actually works? > > > > I think you just need to call V8PrivateProperty::getXXX().set(). It's hard > > to > > > > test as we can't get v8 reference graphs from blink, but at least we > should > > be > > > > able to have layout tests. > > > > > How do I pass the last callback I added to the HeapHashMap into that > > custom > > > > > method to bind it to the V8 callback? > > > > I don't think you need to do it. The custom binding code is just for > adding > > > > owner -> callback reference in V8, and V8PrivateProperty does the job. > > > > > > > > > > Lastly, if I chose to implement the callback class myself, would I be > able > > > to > > > > > avoid this hassle? :) > > > > > > > > Unfortunately no. You will end up writing everything which the code > > generator > > > > can generate... > > > > > > > > > > > > > > https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... > > > > File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl > > > > (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... > > > > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl:25: > > > > [CallWith=ScriptState, Custom=CallPrologue] Promise<void> > > > > cancelWatchAvailability(optional long id); > > > > On 2016/10/16 03:41:40, whywhat wrote: > > > > > On 2016/10/15 at 01:52:45, haraken wrote: > > > > > > Where is the custom bindings written? > > > > > > > > > > Nowhere yet as I'm not even sure how to write it. See my earlier reply > to > > > > > bashi@'s suggestion. > > > > > > > > You will need to add a new file under Source/bindings/modules/v8/custom. > The > > > new > > > > file will be called V8RemotePlaybackCustom.cpp and it would have: > > > > - void V8RemotePlayback::watchAvailabilityMethodEpilogueCustom(const > > > > v8::FunctionCallbackInfo<v8::Value>&, RemotePlayback*) > > > > - void V8RemotePlayback::cancelAvailabilityMethodPrologueCustom(const > > > > v8::FunctionCallbackInfo<v8::Value>&, RemotePlayback*) > > > > > > An easier way of doing this would be to use [Custom=visitDOMWrapper]. You > can > > > write a visitDOMWrapper function to visit from a RemotePlayback's wrapper to > > > RemotePlaybackCallbacks. > > > > > > bashi@, yukishiino@: That said, it's not nice that developers need to write > > > custom bindings every time they want to use callback functions. Would it be > an > > > option to make XXXCallback::create take a wrapper parameter and add a strong > > > hidden reference from XXXCallback to the wrapper in the generated code? (I'm > > > assuming that callback functions should be associated with a wrapper object > in > > > common cases, but correct me if I'm wrong.) > > > > I was assuming that visitDOMWrapper is not available yet. If it's ready to > use, > > that's would be the right solution. > > traceWrapper is not available but visitDOMWrapper is avaialable. visitDOMWrapper > is going to be replaced with traceWrapper. > > > Callback creation and referencing are not directly related IMO. Rather than > > adding a parameter to create(), I'd prefer to add SetOwner() method to > callbacks > > if visitDOMWrapper isn't an option. > > Per the spec, I think you're right. I'm wondering if there is any case where we > don't want to associate a callback function with a wrapper. > > FWIW, we're unconditionally adding a strong reference from a wrapper to its > event listeners. I think this should be solved by traceWrapper. IMHO, what we are doing now doesn't mean it's a better way. Callback itself should be independent from referencing.
On 2016/10/17 01:15:33, bashi1 wrote: > On 2016/10/17 00:50:55, haraken wrote: > > On 2016/10/17 00:43:43, bashi1 wrote: > > > On 2016/10/17 00:32:37, haraken wrote: > > > > On 2016/10/17 00:11:05, bashi1 wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:81: > } > > > > while > > > > > (!m_availabilityCallbacks.add(id, callback).isNewEntry); > > > > > On 2016/10/14 19:58:19, whywhat wrote: > > > > > > On 2016/10/13 at 00:45:46, bashi1 wrote: > > > > > > > |callback| only has a weak reference to v8::Function and it could be > > > > > > garbage-collected unless there is no owner of the function. This means > > > that > > > > > when > > > > > > you keep |callback| in m_avaiabilityCallbacks, you need to set the v8 > > > > function > > > > > > as a private property of the wrapper object of RemotePlayback. You can > > do > > > > that > > > > > > by using [Custom=CallPrologue] or [Custom=CallEplilogue]. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... > > > > > > > > > > > > > > In a custom binding, you will call V8PrivateProperty::getXXX().set() > > > like > > > > > > V8IDBObserver::constructorCustom() > > > > > > > > > > > > > > This is a workaround to avoid circular dependency between v8 and > blink > > > > > > objects. When crbug.com/468240 is resolved we should be able to remove > > > such > > > > > > workaround. Sorry for inconvenience :( > > > > > > > > > > > > I think I need more info... > > > > > > > > > > > > Should I add Prologue or Epilogue? I imagine I need to add custom > > handling > > > > to > > > > > > both adding and removing callbacks? > > > > > Maybe either is fine. > > > > > > > > > > As for removing, it depend on the relationship between the owner > > > > > (RemotePlayback?) and callbacks. If removal occurs regardless of the > > > lifetime > > > > of > > > > > the owner, you may need to unregister callbacks. However it seems we > don't > > > > > provide a way to do that. yukishiino@ ? > > > > > > > > > > > V8IDBObserver has plenty of code in it which I have little idea about > :) > > > Is > > > > > > copy/paste reliable? How do I test it actually works? > > > > > I think you just need to call V8PrivateProperty::getXXX().set(). It's > hard > > > to > > > > > test as we can't get v8 reference graphs from blink, but at least we > > should > > > be > > > > > able to have layout tests. > > > > > > How do I pass the last callback I added to the HeapHashMap into that > > > custom > > > > > > method to bind it to the V8 callback? > > > > > I don't think you need to do it. The custom binding code is just for > > adding > > > > > owner -> callback reference in V8, and V8PrivateProperty does the job. > > > > > > > > > > > > Lastly, if I chose to implement the callback class myself, would I be > > able > > > > to > > > > > > avoid this hassle? :) > > > > > > > > > > Unfortunately no. You will end up writing everything which the code > > > generator > > > > > can generate... > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... > > > > > File third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl > > > > > (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... > > > > > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl:25: > > > > > [CallWith=ScriptState, Custom=CallPrologue] Promise<void> > > > > > cancelWatchAvailability(optional long id); > > > > > On 2016/10/16 03:41:40, whywhat wrote: > > > > > > On 2016/10/15 at 01:52:45, haraken wrote: > > > > > > > Where is the custom bindings written? > > > > > > > > > > > > Nowhere yet as I'm not even sure how to write it. See my earlier reply > > to > > > > > > bashi@'s suggestion. > > > > > > > > > > You will need to add a new file under Source/bindings/modules/v8/custom. > > The > > > > new > > > > > file will be called V8RemotePlaybackCustom.cpp and it would have: > > > > > - void V8RemotePlayback::watchAvailabilityMethodEpilogueCustom(const > > > > > v8::FunctionCallbackInfo<v8::Value>&, RemotePlayback*) > > > > > - void V8RemotePlayback::cancelAvailabilityMethodPrologueCustom(const > > > > > v8::FunctionCallbackInfo<v8::Value>&, RemotePlayback*) > > > > > > > > An easier way of doing this would be to use [Custom=visitDOMWrapper]. You > > can > > > > write a visitDOMWrapper function to visit from a RemotePlayback's wrapper > to > > > > RemotePlaybackCallbacks. > > > > > > > > bashi@, yukishiino@: That said, it's not nice that developers need to > write > > > > custom bindings every time they want to use callback functions. Would it > be > > an > > > > option to make XXXCallback::create take a wrapper parameter and add a > strong > > > > hidden reference from XXXCallback to the wrapper in the generated code? > (I'm > > > > assuming that callback functions should be associated with a wrapper > object > > in > > > > common cases, but correct me if I'm wrong.) > > > > > > I was assuming that visitDOMWrapper is not available yet. If it's ready to > > use, > > > that's would be the right solution. > > > > traceWrapper is not available but visitDOMWrapper is avaialable. > visitDOMWrapper > > is going to be replaced with traceWrapper. > > > > > Callback creation and referencing are not directly related IMO. Rather than > > > adding a parameter to create(), I'd prefer to add SetOwner() method to > > callbacks > > > if visitDOMWrapper isn't an option. > > > > Per the spec, I think you're right. I'm wondering if there is any case where > we > > don't want to associate a callback function with a wrapper. > > > > FWIW, we're unconditionally adding a strong reference from a wrapper to its > > event listeners. > > I think this should be solved by traceWrapper. Yeah, agreed. traceWrapper will solve this. At the moment we can just go with [Custom=visitDOMWrapper] and then replace it with traceWrapper. > IMHO, what we are doing now > doesn't mean it's a better way. Callback itself should be independent from > referencing. Per the long discussion about event listeners (https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/oY...), I think that the conclusion is that in almost all cases the spec requires to add a strong reference from a wrapper to the callback function/event listeners. So we'll end up with adding the strong reference using traceWrapper though.
On 2016/10/17 01:22:44, haraken wrote: > On 2016/10/17 01:15:33, bashi1 wrote: > > On 2016/10/17 00:50:55, haraken wrote: > > > On 2016/10/17 00:43:43, bashi1 wrote: > > > > On 2016/10/17 00:32:37, haraken wrote: > > > > > On 2016/10/17 00:11:05, bashi1 wrote: > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... > > > > > > File > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2415723002/diff/20001/third_party/WebKit/Sour... > > > > > > > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.cpp:81: > > } > > > > > while > > > > > > (!m_availabilityCallbacks.add(id, callback).isNewEntry); > > > > > > On 2016/10/14 19:58:19, whywhat wrote: > > > > > > > On 2016/10/13 at 00:45:46, bashi1 wrote: > > > > > > > > |callback| only has a weak reference to v8::Function and it could > be > > > > > > > garbage-collected unless there is no owner of the function. This > means > > > > that > > > > > > when > > > > > > > you keep |callback| in m_avaiabilityCallbacks, you need to set the > v8 > > > > > function > > > > > > > as a private property of the wrapper object of RemotePlayback. You > can > > > do > > > > > that > > > > > > > by using [Custom=CallPrologue] or [Custom=CallEplilogue]. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > https://chromium.googlesource.com/chromium/src/+/master/third_party/WebKit/So... > > > > > > > > > > > > > > > > In a custom binding, you will call > V8PrivateProperty::getXXX().set() > > > > like > > > > > > > V8IDBObserver::constructorCustom() > > > > > > > > > > > > > > > > This is a workaround to avoid circular dependency between v8 and > > blink > > > > > > > objects. When crbug.com/468240 is resolved we should be able to > remove > > > > such > > > > > > > workaround. Sorry for inconvenience :( > > > > > > > > > > > > > > I think I need more info... > > > > > > > > > > > > > > Should I add Prologue or Epilogue? I imagine I need to add custom > > > handling > > > > > to > > > > > > > both adding and removing callbacks? > > > > > > Maybe either is fine. > > > > > > > > > > > > As for removing, it depend on the relationship between the owner > > > > > > (RemotePlayback?) and callbacks. If removal occurs regardless of the > > > > lifetime > > > > > of > > > > > > the owner, you may need to unregister callbacks. However it seems we > > don't > > > > > > provide a way to do that. yukishiino@ ? > > > > > > > > > > > > > V8IDBObserver has plenty of code in it which I have little idea > about > > :) > > > > Is > > > > > > > copy/paste reliable? How do I test it actually works? > > > > > > I think you just need to call V8PrivateProperty::getXXX().set(). It's > > hard > > > > to > > > > > > test as we can't get v8 reference graphs from blink, but at least we > > > should > > > > be > > > > > > able to have layout tests. > > > > > > > How do I pass the last callback I added to the HeapHashMap into that > > > > custom > > > > > > > method to bind it to the V8 callback? > > > > > > I don't think you need to do it. The custom binding code is just for > > > adding > > > > > > owner -> callback reference in V8, and V8PrivateProperty does the job. > > > > > > > > > > > > > > Lastly, if I chose to implement the callback class myself, would I > be > > > able > > > > > to > > > > > > > avoid this hassle? :) > > > > > > > > > > > > Unfortunately no. You will end up writing everything which the code > > > > generator > > > > > > can generate... > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... > > > > > > File > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl > > > > > > (right): > > > > > > > > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2415723002/diff/40001/third_party/WebKit/Sour... > > > > > > > third_party/WebKit/Source/modules/remoteplayback/RemotePlayback.idl:25: > > > > > > [CallWith=ScriptState, Custom=CallPrologue] Promise<void> > > > > > > cancelWatchAvailability(optional long id); > > > > > > On 2016/10/16 03:41:40, whywhat wrote: > > > > > > > On 2016/10/15 at 01:52:45, haraken wrote: > > > > > > > > Where is the custom bindings written? > > > > > > > > > > > > > > Nowhere yet as I'm not even sure how to write it. See my earlier > reply > > > to > > > > > > > bashi@'s suggestion. > > > > > > > > > > > > You will need to add a new file under > Source/bindings/modules/v8/custom. > > > The > > > > > new > > > > > > file will be called V8RemotePlaybackCustom.cpp and it would have: > > > > > > - void V8RemotePlayback::watchAvailabilityMethodEpilogueCustom(const > > > > > > v8::FunctionCallbackInfo<v8::Value>&, RemotePlayback*) > > > > > > - void V8RemotePlayback::cancelAvailabilityMethodPrologueCustom(const > > > > > > v8::FunctionCallbackInfo<v8::Value>&, RemotePlayback*) > > > > > > > > > > An easier way of doing this would be to use [Custom=visitDOMWrapper]. > You > > > can > > > > > write a visitDOMWrapper function to visit from a RemotePlayback's > wrapper > > to > > > > > RemotePlaybackCallbacks. > > > > > > > > > > bashi@, yukishiino@: That said, it's not nice that developers need to > > write > > > > > custom bindings every time they want to use callback functions. Would it > > be > > > an > > > > > option to make XXXCallback::create take a wrapper parameter and add a > > strong > > > > > hidden reference from XXXCallback to the wrapper in the generated code? > > (I'm > > > > > assuming that callback functions should be associated with a wrapper > > object > > > in > > > > > common cases, but correct me if I'm wrong.) > > > > > > > > I was assuming that visitDOMWrapper is not available yet. If it's ready to > > > use, > > > > that's would be the right solution. > > > > > > traceWrapper is not available but visitDOMWrapper is avaialable. > > visitDOMWrapper > > > is going to be replaced with traceWrapper. > > > > > > > Callback creation and referencing are not directly related IMO. Rather > than > > > > adding a parameter to create(), I'd prefer to add SetOwner() method to > > > callbacks > > > > if visitDOMWrapper isn't an option. > > > > > > Per the spec, I think you're right. I'm wondering if there is any case where > > we > > > don't want to associate a callback function with a wrapper. > > > > > > FWIW, we're unconditionally adding a strong reference from a wrapper to its > > > event listeners. > > > > I think this should be solved by traceWrapper. > Yeah, agreed. traceWrapper will solve this. At the moment we can just go with > [Custom=visitDOMWrapper] and then replace it with traceWrapper. > > > IMHO, what we are doing now > > doesn't mean it's a better way. Callback itself should be independent from > > referencing. > > Per the long discussion about event listeners > (https://groups.google.com/a/chromium.org/d/topic/platform-architecture-dev/oY...), > I think that the conclusion is that in almost all cases the spec requires to add > a strong reference from a wrapper to the callback function/event listeners. So > we'll end up with adding the strong reference using traceWrapper though. I think we are on the same page. I don't think this is a right place to discuss callback referencing.
On 2016/10/17 00:32:37, haraken wrote: > An easier way of doing this would be to use [Custom=visitDOMWrapper]. You can > write a visitDOMWrapper function to visit from a RemotePlayback's wrapper to > RemotePlaybackCallbacks. I think we should use [Custom=visitDOMWrapper] in this case. > bashi@, yukishiino@: That said, it's not nice that developers need to write > custom bindings every time they want to use callback functions. Would it be an > option to make XXXCallback::create take a wrapper parameter and add a strong > hidden reference from XXXCallback to the wrapper in the generated code? (I'm > assuming that callback functions should be associated with a wrapper object in > common cases, but correct me if I'm wrong.) IIUC, in this case, you can add as many callback functions as you want, so [Custom=visitDOMWrapper] works better than private properties in my opinion.
On 2016/10/17 at 05:15:14, yukishiino wrote: > On 2016/10/17 00:32:37, haraken wrote: > > An easier way of doing this would be to use [Custom=visitDOMWrapper]. You can > > write a visitDOMWrapper function to visit from a RemotePlayback's wrapper to > > RemotePlaybackCallbacks. > > I think we should use [Custom=visitDOMWrapper] in this case. > > > bashi@, yukishiino@: That said, it's not nice that developers need to write > > custom bindings every time they want to use callback functions. Would it be an > > option to make XXXCallback::create take a wrapper parameter and add a strong > > hidden reference from XXXCallback to the wrapper in the generated code? (I'm > > assuming that callback functions should be associated with a wrapper object in > > common cases, but correct me if I'm wrong.) > > IIUC, in this case, you can add as many callback functions as you want, so [Custom=visitDOMWrapper] works better than private properties in my opinion. Ok, what I understood from reading this thread is I need to use [Custom=visitDOMWrapper], expose the callbacks map via a public method so visitDOMWrapperCustom could enumerate them and set reference from group (like the intersection and mutation observers do).
On 2016/10/17 15:25:44, whywhat wrote: > On 2016/10/17 at 05:15:14, yukishiino wrote: > > On 2016/10/17 00:32:37, haraken wrote: > > > An easier way of doing this would be to use [Custom=visitDOMWrapper]. You > can > > > write a visitDOMWrapper function to visit from a RemotePlayback's wrapper to > > > RemotePlaybackCallbacks. > > > > I think we should use [Custom=visitDOMWrapper] in this case. > > > > > bashi@, yukishiino@: That said, it's not nice that developers need to write > > > custom bindings every time they want to use callback functions. Would it be > an > > > option to make XXXCallback::create take a wrapper parameter and add a strong > > > hidden reference from XXXCallback to the wrapper in the generated code? (I'm > > > assuming that callback functions should be associated with a wrapper object > in > > > common cases, but correct me if I'm wrong.) > > > > IIUC, in this case, you can add as many callback functions as you want, so > [Custom=visitDOMWrapper] works better than private properties in my opinion. > > Ok, what I understood from reading this thread is I need to use > [Custom=visitDOMWrapper], expose the callbacks map via a public method so > visitDOMWrapperCustom could enumerate them and set reference from group (like > the intersection and mutation observers do). Right. (Sorry about a lot of noise here :/)
setWrapperReferences and save ScriptState
The CQ bit was checked by avayvod@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...
PTAL I hope the last patch addresses the issue of v8 garbage collecting the callbacks. I also simplified acquiring the script state in RemotePlayback by passing it to the constructor.
LGTM Can you add a test to check that the callback is called even after the wrapper is garbage-collected? You can do something like: ...; wrapper = null; gc(); ...; // Make sure that the callback is still fired.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
On 2016/10/17 at 22:50:52, haraken wrote: > LGTM > > Can you add a test to check that the callback is called even after the wrapper is garbage-collected? > > You can do something like: > > ...; > wrapper = null; > gc(); > ...; // Make sure that the callback is still fired. Hm, what is wrapper here? Is that RemotePlayback object? It can't be null as it's a readonly propery on HTMLMediaElement. I could "delete" the media element I guess but does it need to fire the callback then?
Added layout test for callback gc
On 2016/10/17 at 22:50:52, haraken wrote: > LGTM > > Can you add a test to check that the callback is called even after the wrapper is garbage-collected? > > You can do something like: > > ...; > wrapper = null; > gc(); > ...; // Make sure that the callback is still fired. I think I've got it :)
The CQ bit was checked by avayvod@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from zqzhang@chromium.org, mlamouri@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2415723002/#ps80001 (title: "Added layout test for callback gc")
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.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== [Blink, RemotePlayback] watchAvailability() implementation. BUG=655233 TEST=new layout tests + https://avayvod.github.io/remote-playback/test.html ========== to ========== [Blink, RemotePlayback] watchAvailability() implementation. BUG=655233 TEST=new layout tests + https://avayvod.github.io/remote-playback/test.html Committed: https://crrev.com/6ab7f8f63e07b15373ad84ff677244d70f5059e1 Cr-Commit-Position: refs/heads/master@{#425891} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/6ab7f8f63e07b15373ad84ff677244d70f5059e1 Cr-Commit-Position: refs/heads/master@{#425891} |
