Bug 36460 - Implement Show/Hide Controls command for <video> in chromium
Summary: Implement Show/Hide Controls command for <video> in chromium
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2010-03-22 13:03 PDT by Sergey Ulanov
Modified: 2010-03-23 14:15 PDT (History)
1 user (show)

See Also:

Patch (2.99 KB, patch)
2010-03-22 13:25 PDT, Sergey Ulanov
no flags Details | Formatted Diff | Diff
Patch (2.99 KB, patch)
2010-03-22 14:04 PDT, Sergey Ulanov
no flags Details | Formatted Diff | Diff
Patch (3.17 KB, patch)
2010-03-22 18:31 PDT, Sergey Ulanov
no flags Details | Formatted Diff | Diff
Patch (3.27 KB, patch)
2010-03-23 12:12 PDT, Sergey Ulanov
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergey Ulanov 2010-03-22 13:03:02 PDT
Corresponding chromium bug http://code.google.com/p/chromium/issues/detail?id=19848 .
Comment 1 Sergey Ulanov 2010-03-22 13:25:13 PDT
Created attachment 51339 [details]
Comment 2 Sergey Ulanov 2010-03-22 14:04:02 PDT
Created attachment 51350 [details]
Comment 3 Dmitry Titov 2010-03-22 17:55:29 PDT
Comment on attachment 51350 [details]

> 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.
Comment 4 Sergey Ulanov 2010-03-22 18:31:51 PDT
Created attachment 51378 [details]
Comment 5 Dmitry Titov 2010-03-22 19:32:40 PDT
Comment on attachment 51378 [details]

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?
Comment 6 Sergey Ulanov 2010-03-23 12:12:01 PDT
Created attachment 51444 [details]
Comment 7 Dmitry Titov 2010-03-23 13:44:59 PDT
Comment on attachment 51444 [details]


Comment 8 WebKit Commit Bot 2010-03-23 14:15:46 PDT
Comment on attachment 51444 [details]

Clearing flags on attachment: 51444

Committed r56415: <http://trac.webkit.org/changeset/56415>
Comment 9 WebKit Commit Bot 2010-03-23 14:15:50 PDT
All reviewed patches have been landed.  Closing bug.