[webkit-reviews] review denied: [Bug 36460] Implement Show/Hide Controls command for <video> in chromium : [Attachment 51378] Patch

[bugzilla-daemon at webkit.org]

Dmitry Titov <dimich at chromium.org> has denied Sergey Ulanov
<sergeyu at chromium.org>'s request for review:
Bug 36460: Implement Show/Hide Controls command for <video> in chromium
https://bugs.webkit.org/show_bug.cgi?id=36460

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

------- Additional Comments from Dmitry Titov <dimich at chromium.org>
Almost there. Just couple of nits:

> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> +	   Changes needed to implement Show/Hide Controls command for <video>
in
> +	   chrome.

WebKit does not have 80-col rule, so it's better not to wrap the line for a
single word.

> +	   (WebKit::WebContextMenuData::): Added MediaHasVideo and
MediaControls.

That's not exactly the type of info I was looking for. It is obvious from the
code that these two are added. But, for example, why MediaHasVideo was added?
Is it somehow necessary to be able to toggle controls? If it's just independent
addition, it could be noted as well.
Sorry for what looks like nits, but a few months later someone else will look
at the code and since there is not much comments in it, will probably search in
trac and wish there were more actual info, especially on 'why'.

> +	   (WebKit::WebViewImpl::performMediaPlayerAction): Controls action
> +	   handler.

Also not a necessary wrapping.

Another small iteration?