[webkit-reviews] review denied: [Bug 28310] Chromium: Show a "Mute Disabled" button on audio error. : [Attachment 34864] Fix

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Aug 14 15:14:18 PDT 2009


Eric Seidel <eric at webkit.org> has denied Kyle Prete <kylep at chromium.org>'s
request for review:
Bug 28310: Chromium: Show a "Mute Disabled" button on audio error.
https://bugs.webkit.org/show_bug.cgi?id=28310

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
I'm not a media expert, but this patch looks wrong.  Mostly because the methods
look out of place:

One of these does not look like the other:
     virtual bool isVideo() const { return false; }
     virtual bool hasVideo() const { return false; }
+    virtual bool hasAudio() const { return player() && player()->hasAudio(); }


And again:
     virtual bool hasVideo() const = 0;
+    virtual bool hasAudio() const { return true; }

It seems maybe you're not following the design of the rest of the media
classes?

Please explain.


More information about the webkit-reviews mailing list