[webkit-reviews] review denied: [Bug 66878] HTMLAudioElement can be garbage collected while it playing : [Attachment 110845] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Eugene Nalimov <enal at google.com>'s
request for review:
Bug 66878: HTMLAudioElement can be garbage collected while it playing
https://bugs.webkit.org/show_bug.cgi?id=66878

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

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=110845&action=review


I agree with Eric that the V8 bindings fix and the audio element fix should be
separated.

> Source/WebCore/html/HTMLAudioElement.h:43
> +    virtual bool hasPendingActivity() const { return isPlaying() ||
HTMLMediaElement::hasPendingActivity(); }

This should be private, not public.

> Source/WebCore/html/HTMLAudioElement.idl:29
> +	   EventTarget,

This is a bad idea.

> Source/WebCore/html/HTMLAudioElement.idl:40
> +	   // EventTarget interface			      
> +	   void addEventListener(in DOMString type,
> +				 in EventListener listener,
> +				 in [Optional] boolean useCapture);
> +	   void removeEventListener(in DOMString type,
> +				    in EventListener listener,
> +				    in [Optional] boolean useCapture);
> +	   boolean dispatchEvent(in Event event)
> +	       raises(EventException);	  

This is wrong. We never have to repeat the EventTarget interface like this in a
Node subclass. If this was necessary we’d have it tons of other classes too.

> Source/WebCore/html/HTMLMediaElement.h:61
> +class HTMLMediaElement : public HTMLElement, public MediaPlayerClient,
private MediaCanStartListener, public ActiveDOMObject {

This should stay private.

> Source/WebCore/html/HTMLMediaElement.h:208
> +    virtual bool hasPendingActivity() const;

This should be protected, not public.