[webkit-reviews] review granted: [Bug 23328] Upstream remaining files from platform/chromium/ : [Attachment 26721] v1 patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Wed Jan 14 11:34:06 PST 2009


Eric Seidel <eric at webkit.org> has granted Darin Fisher (Google)
<darin at chromium.org>'s request for review:
Bug 23328: Upstream remaining files from platform/chromium/
https://bugs.webkit.org/show_bug.cgi?id=23328

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

------- Additional Comments from Eric Seidel <eric at webkit.org>
notImplemented()?
 IntSize dragImageSize(DragImageRef image)
 39 {
 40	return IntSize();
 41 }
 42 

Other code seems to prefer just "unsigned" over "unsigned int":
 36	int windowsKeyCodeForKeyEvent(unsigned int keycode);

It's a little funny just to wrap this one line:
2     ChromiumBridge::clipboardWriteSelection(html, url, plainText,
 83						canSmartCopyOrDelete);

"event" is not needed here:
78     virtual bool handleMouseDownEvent(const PlatformMouseEvent& event);
 79	virtual bool handleMouseMoveEvent(const PlatformMouseEvent& event);
 80	virtual bool handleMouseReleaseEvent(const PlatformMouseEvent& event);
 81	virtual bool handleWheelEvent(const PlatformWheelEvent& event);
 82	virtual bool handleKeyEvent(const PlatformKeyboardEvent& event);

Generally I've seen WebCore avoid this kind of construction:
894	m_originalIndex = m_selectedIndex = index;
But I'm not sure we have any specific style rules about it.

No longer required after your previous change:
// Required to use notImplemented() outside of the WebCore namespace.
 49 using namespace WebCore;

I have no substantive comments.  This looks fine as-is.


More information about the webkit-reviews mailing list