|Summary:||Implement Show/Hide Controls command for <video> in chromium|
|Product:||WebKit||Reporter:||Sergey Ulanov <sergeyu>|
|Component:||WebKit Misc.||Assignee:||Nobody <webkit-unassigned>|
|Version:||528+ (Nightly build)|
Description Sergey Ulanov 2010-03-22 13:03:02 PDT
Corresponding chromium bug http://code.google.com/p/chromium/issues/detail?id=19848 .
Comment 3 Dmitry Titov 2010-03-22 17:55:29 PDT
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.
Comment 5 Dmitry Titov 2010-03-22 19:32:40 PDT
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?
Comment 7 Dmitry Titov 2010-03-23 13:44:59 PDT
Comment on attachment 51444 [details] Patch r=me Thanks!
Comment 8 WebKit Commit Bot 2010-03-23 14:15:46 PDT
Comment on attachment 51444 [details] Patch 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.