Bug 36460 – Implement Show/Hide Controls command for <video> in chromium

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Sergey Ulanov]

Corresponding chromium bug http://code.google.com/p/chromium/issues/detail?id=19848 .

[Sergey Ulanov]

Created attachment 51339 [details]
Patch

[Sergey Ulanov]

Created attachment 51350 [details]
Patch

[Dmitry Titov]

Comment on attachment 51350 [details]
Patch

> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> +
> +        Show/Hide Controls command for <video>
> +
> +        https://bugs.webkit.org/show_bug.cgi?id=36460

these 2 lines usually do not have empty line between them.

Also, typically bugs and good ChangeLogs contain a bit more info, as to why and what was changed. It helps other folks when they 'scan' trac database or search bugzilla (or sometimes try to glean more information in an attempt to fix a bug that seems to be caused by the change). Look at existing entries that describe a change in a way that give you a good understanding of why and what has changed.

Since you are not a committer yet, I think you'd want to use a commit-bot. It runs compile and tests and lands the patch once it is approved. In order to use it, you flip the commit-queue flag to ?, so the reviewer knows you want to use it and flips it to + with r+.

Doing r- for now, looking forward for more info in ChangeLog, looks ok in general.

[Sergey Ulanov]

Created attachment 51378 [details]
Patch

[Dmitry Titov]

Comment on attachment 51378 [details]
Patch

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?

[Sergey Ulanov]

Created attachment 51444 [details]
Patch

[Dmitry Titov]

Comment on attachment 51444 [details]
Patch

r=me

Thanks!

[WebKit Commit Bot]

Comment on attachment 51444 [details]
Patch

Clearing flags on attachment: 51444

Committed r56415: <http://trac.webkit.org/changeset/56415>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.