[webkit-reviews] review denied: [Bug 25903] Create a default UI theme for media controls in Chromium. : [Attachment 30569] Added ENABLE(VIDEO) ifdefs, and crated a toMediaElement()
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Thu May 21 17:16:22 PDT 2009
Eric Seidel <eric at webkit.org> has denied Albert J. Wong <ajwong at chromium.org>'s
request for review:
Bug 25903: Create a default UI theme for media controls in Chromium.
https://bugs.webkit.org/show_bug.cgi?id=25903
Attachment 30569: Added ENABLE(VIDEO) ifdefs, and crated a toMediaElement()
https://bugs.webkit.org/attachment.cgi?id=30569&action=review
------- Additional Comments from Eric Seidel <eric at webkit.org>
We use 0, not NULL in c++ code. See the WK style guide:
http://webkit.org/coding/coding-style.html
also { goes on a new line.
node ? node->shadowAncestorNode() : 0
would read better to me as:
if (!node)
return 0;
Node* mediaNode = node->shadowAncestorNode();
Since this is a function only written once, you can afford to take up a little
more space. :)
Also, I wonder if we shoudl call this toMediaElemetn since we're not casting
the passed in node. mediaElementParent(node) maybe?
Please file a bug about removing the copy/paste code. We really need to clean
this up.
r- for the style nits.
More information about the webkit-reviews
mailing list