[webkit-reviews] review denied: [Bug 83143] MediaStream API: MediaStreams stops proper cleanup to take place during a page reload. : [Attachment 135553] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Tommy Widenflycht
<tommyw at google.com>'s request for review:
Bug 83143: MediaStream API: MediaStreams stops proper cleanup to take place
during a page reload.
https://bugs.webkit.org/show_bug.cgi?id=83143

Attachment 135553: Patch
https://bugs.webkit.org/attachment.cgi?id=135553&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=135553&action=review


This looks mostly fine, but I'm going to mark it R- because of the IDL /
ActiveDOMObject thing.	I've also left you a couple other things to think about
for the next iteration.

> Source/WebCore/Modules/mediastream/LocalMediaStream.h:41
> +    // ActiveDOMObject and idl

Woah.  Should we use [ImplementedAs] to avoid having this one function play
both roles?  It's fine if one calls the other, but it's better to do that
explicitly rather than having a name collision like this.

> Source/WebCore/Modules/mediastream/MediaStream.cpp:135
> +void MediaStream::stop()
> +{
> +}

If you don't need stop(), you can just use a ContextDestructionObserver rather
than ActiveDOMObject.