Bug 117621

Summary: [Mac] clicking caption track glyph should dismiss menu
Product: WebKit Reporter: Antoine Quint <graouts>
Component: MediaAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, eric.carlson, esprehn+autocc, glenn, jer.noble, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Antoine Quint 2013-06-13 18:52:36 PDT
Clicking on the caption glyph shows the caption menu if it is not visible. Clicking on it when the caption menu is already visible does nothing, it should dismiss the menu.
Comment 1 Antoine Quint 2013-06-13 18:52:46 PDT
<rdar://problem/14143083>
Comment 2 Antoine Quint 2013-06-13 19:09:10 PDT
Created attachment 204662 [details]
Patch
Comment 3 Joseph Pecoraro 2013-06-13 23:47:19 PDT
Comment on attachment 204662 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=204662&action=review

> Source/WebCore/html/shadow/MediaControlsApple.cpp:600
>      if (event->type() == eventNames().mousewheelEvent && event->hasInterface(eventNames().interfaceForWheelEvent)) {

Nit (not your code, but): This if can be an else if since if the top case is true, this case can never be true. Or early return here and no need to else if.
Comment 4 Eric Carlson 2013-06-14 06:44:51 PDT
Comment on attachment 204662 [details]
Patch

r=me, although it would be nice to address Joe's nit before landing.
Comment 5 Antoine Quint 2013-06-14 07:16:37 PDT
Created attachment 204711 [details]
Patch for landing
Comment 6 Antoine Quint 2013-06-14 07:21:17 PDT
(In reply to comment #3)
> (From update of attachment 204662 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=204662&action=review
> 
> > Source/WebCore/html/shadow/MediaControlsApple.cpp:600
> >      if (event->type() == eventNames().mousewheelEvent && event->hasInterface(eventNames().interfaceForWheelEvent)) {
> 
> Nit (not your code, but): This if can be an else if since if the top case is true, this case can never be true. Or early return here and no need to else if.

Actually, very much my code from a previous patch, fixed in patch for landing.
Comment 7 WebKit Commit Bot 2013-06-14 07:43:13 PDT
Comment on attachment 204711 [details]
Patch for landing

Clearing flags on attachment: 204711

Committed r151592: <http://trac.webkit.org/changeset/151592>
Comment 8 WebKit Commit Bot 2013-06-14 07:43:15 PDT
All reviewed patches have been landed.  Closing bug.