[webkit-reviews] review granted: [Bug 45306] Media elements should derive from ActiveDOMObjects : [Attachment 66739] proposed patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Eric Carlson
<eric.carlson at apple.com>'s request for review:
Bug 45306: Media elements should derive from ActiveDOMObjects
https://bugs.webkit.org/show_bug.cgi?id=45306

Attachment 66739: proposed patch
https://bugs.webkit.org/attachment.cgi?id=66739&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
Do we already have tests that cover a media element with pending events where
we force the last reference to be garbage collected?

> +    // ActiveDOMObject methods.

I prefer that we use the C++ terminology. So "functions" rather than "methods".


> +    virtual bool canSuspend() const { return true; }

There doesn’t seem to be significant benefit to putting this definition in the
header, since it’s a virtual function that will only be called through a
function pointer. I suggest putting it in the .cpp file instead.

It's too bad that the stop function is calling another virtual function,
suspend, doing an unnecessary virtual dispatch. Not a major problem, but
slightly unpleasant. Instead it could call HTMLMediaElement::suspend(), but
that would strange enough that we probably shouldn't do it.

The headers in HTMLMediaElement.h should stay sorted alphabetically. I bet that
is what our stylebot is complaining about.