Bug 76110

Summary: Event Queue for HTMLMediaElement
Product: WebKit Reporter: Victor Carbune <vcarbune>
Component: MediaAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, dominicc, eric.carlson, rakuco, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 72171    
Attachments:
Description Flags
Patch
none
Updated patch. Renamed the class in GenericQueueEvent under dom/ and hopefully fixed build issues.
none
Updated the vcproj entry as it was not building on win-ews
none
Patch
none
Green patch eric.carlson: review+

Description Victor Carbune 2012-01-11 15:48:45 PST
The <track> element requires events to be dispatched asynchronously.

Even though the HTMLMediaElement provides a particular mechanism for firing such events using a timer and a queue, a generic class should be in place that handles both events that are dispatched by the media element and the track element.
Comment 1 Dimitri Glazkov (Google) 2012-01-16 08:37:53 PST
There's already a pretty nice EventQueue abstraction http://code.google.com/codesearch#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/dom/EventQueue.h&exact_package=chromium&q=EventQueue&type=cs&l=27. Might be worth taking a look.
Comment 2 Victor Carbune 2012-01-20 02:03:52 PST
Created attachment 123272 [details]
Patch
Comment 3 Early Warning System Bot 2012-01-20 02:22:37 PST
Comment on attachment 123272 [details]
Patch

Attachment 123272 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11314006
Comment 4 Eric Carlson 2012-01-20 10:13:57 PST
Comment on attachment 123272 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.h:581
> +    MediaEventQueue m_asyncEventQueue;

This shouldn't be inside of #if ENABLE(VIDEO_TRACK) as it is always used. This is causing the GTK and Qt bot failures.
Comment 5 Eric Carlson 2012-01-20 10:27:36 PST
Comment on attachment 123272 [details]
Patch

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

Marking r- only because it fails to compile on ports that don't define VIDEO_TRACK.

> Source/WebCore/html/MediaEventQueue.cpp:8
> +

Nit: This extra blank line is unnecessary.

> Source/WebCore/html/MediaEventQueue.cpp:70
> +    if (m_timer.isActive())

Nit: MediaEventQueue::close calls stop() without checking to see if the timer is active, to be consistent you should one or the other throughout the file. stop() is cheap so I would probably just call it but I don't have a strong preference.
Comment 6 Eric Carlson 2012-01-20 10:48:34 PST
Comment on attachment 123272 [details]
Patch

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

> Source/WebCore/html/MediaEventQueue.h:34
> +class MediaEventQueue : public EventQueue {
> +
> +public:
> +    MediaEventQueue();
> +    virtual ~MediaEventQueue();
> +
> +    // EventQueue
> +    virtual bool enqueueEvent(PassRefPtr<Event>) OVERRIDE;
> +    virtual bool cancelEvent(Event*) OVERRIDE;
> +    virtual void close() OVERRIDE;
> +
> +    void cancelAllEvents();
> +    bool hasPendingEvents() const;
> +
> +private:
> +    void timerFired(Timer<MediaEventQueue>*);
> +
> +    Vector<RefPtr<Event> > m_pendingEvents;
> +    Timer<MediaEventQueue> m_timer;
> +
> +    bool m_isClosed;

Why is this class called "MediaEventQueue"? We may end with a class that is specific to the media element at some point, but at this point it is just a generic event queue that could be used by other classes. 

I think this should a more generic name, GenericEventQueue or SimpleEventQueue perhaps, and it should be in WebCore/dom along with the abstract base class.
Comment 7 Victor Carbune 2012-01-20 16:33:16 PST
Created attachment 123407 [details]
Updated patch. Renamed the class in GenericQueueEvent under dom/ and hopefully fixed build issues.
Comment 8 Victor Carbune 2012-01-20 17:12:00 PST
Created attachment 123416 [details]
Updated the vcproj entry as it was not building on win-ews
Comment 9 Victor Carbune 2012-01-21 00:48:11 PST
Created attachment 123439 [details]
Patch
Comment 10 Victor Carbune 2012-01-21 01:43:26 PST
Created attachment 123441 [details]
Green patch

Fixed build issues. Patch should be ready for review.
Comment 11 Eric Carlson 2012-01-24 06:58:41 PST
Comment on attachment 123441 [details]
Green patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:-3181
> -    // Return true when we have pending events so we can't fire events after the JS 
> -    // object gets collected.

Why remove this comment?
Comment 12 Dominic Cooney 2012-01-25 11:00:14 PST
I tried building this before landing it, and this did not build for me on Mac. It looks like it needs symbols added to WebCore exports.

Here is the tail of the build output:

Ld /Volumes/HD2/WebKit2/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore normal x86_64
    cd /Volumes/HD2/WebKit2/Source/WebCore
    setenv MACOSX_DEPLOYMENT_TARGET 10.7
    /Developer/usr/bin/clang++ -arch x86_64 -dynamiclib -L/Volumes/HD2/WebKit2/WebKitBuild/Debug -F/Volumes/HD2/WebKit2/WebKitBuild/Debug -F/System/Library/Frameworks/Carbon.framework/Frameworks -F/System/Library/Frameworks/ApplicationServices.framework/Frameworks -F/System/Library/Frameworks/CoreServices.framework/Frameworks -F/System/Library/PrivateFrameworks -filelist /Volumes/HD2/WebKit2/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/x86_64/WebCore.LinkFileList -Xlinker --no-demangle -exported_symbols_list /Volumes/HD2/WebKit2/WebKitBuild/Debug/DerivedSources/WebCore/WebCore.LP64.exp -install_name /Volumes/HD2/WebKit2/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore -mmacosx-version-min=10.7 -lsqlite3 -lobjc -lANGLE -allowable_client WebCoreTestSupport -sub_library libobjc -umbrella WebKit -allowable_client WebKit2 -Xlinker -objc_gc_compaction -framework IOSurface -framework Accelerate -framework ApplicationServices -framework AudioToolbox -framework AudioUnit -framework Carbon -framework Cocoa -framework CoreAudio -framework IOKit -framework JavaScriptCore -licucore -lobjc -lxml2 -lz -framework OpenGL -framework QuartzCore -framework SystemConfiguration -single_module -compatibility_version 1 -current_version 535.19 -o /Volumes/HD2/WebKit2/WebKitBuild/Debug/WebCore.framework/Versions/A/WebCore
Undefined symbols for architecture x86_64:
  "__ZN7WebCore17GenericEventQueueC1Ev", referenced from:
      __ZN7WebCore16HTMLMediaElementC2ERKNS_13QualifiedNameEPNS_8DocumentEb in HTMLMediaElement.o
  "__ZN7WebCore17GenericEventQueueD1Ev", referenced from:
      __ZN7WebCore16HTMLMediaElementD2Ev in HTMLMediaElement.o
  "__ZN7WebCore17GenericEventQueue15cancelAllEventsEv", referenced from:
      __ZN7WebCore16HTMLMediaElement31cancelPendingEventsAndCallbacksEv in HTMLMediaElement.o
  "__ZNK7WebCore17GenericEventQueue16hasPendingEventsEv", referenced from:
      __ZNK7WebCore16HTMLMediaElement18hasPendingActivityEv in HTMLMediaElement.o
ld: symbol(s) not found for architecture x86_64
clang: error: linker command failed with exit code 1 (use -v to see invocation)


** BUILD FAILED **
Comment 13 Eric Carlson 2012-01-27 14:57:20 PST
http://trac.webkit.org/changeset/106156