Bug 76110 – Event Queue for HTMLMediaElement

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Victor Carbune]

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.

[Dimitri Glazkov (Google)]

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.

[Victor Carbune]

Created attachment 123272 [details]
Patch

[Early Warning System Bot]

Comment on attachment 123272 [details]
Patch

Attachment 123272 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11314006

[Eric Carlson]

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.

[Eric Carlson]

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.

[Eric Carlson]

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.

[Victor Carbune]

Created attachment 123407 [details]
Updated patch. Renamed the class in GenericQueueEvent under dom/ and hopefully fixed build issues.

[Victor Carbune]

Created attachment 123416 [details]
Updated the vcproj entry as it was not building on win-ews

[Victor Carbune]

Created attachment 123439 [details]
Patch

[Victor Carbune]

Created attachment 123441 [details]
Green patch

Fixed build issues. Patch should be ready for review.

[Eric Carlson]

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?

[Dominic Cooney]

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 **

[Eric Carlson]

http://trac.webkit.org/changeset/106156