[webkit-reviews] review denied: [Bug 43394] [Chromium] Full screen media player interface between WebMediaPlayer and WebWidgetClient : [Attachment 71515] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Andrew Scherkus
<scherkus at chromium.org>'s request for review:
Bug 43394: [Chromium] Full screen media player interface between WebMediaPlayer
and WebWidgetClient
https://bugs.webkit.org/show_bug.cgi?id=43394

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

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=71515&action=review

I'm worried that this patch introduces memory safety issues.  It might well be
fine, but I'm not sure it's correct.

> WebKit/chromium/src/ChromeClientImpl.cpp:836
> +    fullscreenClient->exitFullscreen();

When does the client get deleted?

> WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.cpp:43
> +namespace WebKit {

Missing blank line after this line.

> WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.cpp:46
> +    return new WebFullscreenMediaPlayerClientImpl(client);

Is this normal memory management for the Chromium API?	In WebCore, we'd use
something like a PassOwnPtr.

> WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.cpp:84
> +    delete this;

I see.	This seems like a dangerous pattern for memory management, but maybe
it's the norm?

> WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.cpp:94
> +    // Called when first created.

Can this be called multiple times?  I don't really understand this comment.

> WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.cpp:118
> +	 switch (static_cast<const WebKeyboardEvent&>(event).windowsKeyCode) {

Bad indent.

> WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.cpp:121
> +		 m_mediaElement->play(m_mediaElement->processingUserGesture());


Why are we asking the media element if its processing a user gesture?  Whether
we're processing a user gesture is static state.

> WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.h:103
> +    // Weak pointer to WebWidgetClient.

It's not technically a weak pointer, right?  There's no notification of when
the client destructs itself.

> WebKit/chromium/src/WebFullscreenMediaPlayerClientImpl.h:109
> +    // Pointer to the corresponding media element is saved
> +    // for properly exiting full screen as a result of user
> +    // action.
> +    WebCore::HTMLMediaElement* m_mediaElement;

We don't need to hold a reference to the element here?	What if it gets removed
from the DOM and deallocated?