[Webkit-unassigned] [Bug 36460] Implement Show/Hide Controls command for <video> in chromium

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=36460


Dmitry Titov <dimich at chromium.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #51378|review?                     |review-
               Flag|                            |




--- Comment #5 from Dmitry Titov <dimich at chromium.org>  2010-03-22 19:32:40 PST ---
(From update of attachment 51378)
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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.