[webkit-reviews] review granted: [Bug 71408] Implement MediaController. : [Attachment 114629] Patch

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 71408: Implement MediaController.
https://bugs.webkit.org/show_bug.cgi?id=71408

Attachment 114629: Patch
https://bugs.webkit.org/attachment.cgi?id=114629&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=114629&action=review


> Source/JavaScriptCore/wtf/Platform.h:1110
> -#if PLATFORM(MAC) || (PLATFORM(WIN) && !OS(WINCE) && !PLATFORM(WIN_CAIRO))
> +#if PLATFORM(MAC)

This didn't make it into a change log.


> Source/WebCore/bindings/js/JSHTMLMediaElementCustom.cpp:43
> +    // On setting, it must first remove the element's mediagroup attribute,
if any, 
> +    imp->setMediaGroup(String());
> +    // and then set the current media controller to the given value.
> +    imp->setController(toMediaController(value));

Nit: it would be helpful to have the spec section noted here.


> Source/WebCore/bindings/v8/custom/V8HTMLMediaElementCustom.cpp:50
> +    if (!controller) {
> +	   throwError("Value is not of type MediaController");
> +	   return;
> +    }
> +
> +    imp->setController(controller);

Doesn't this also need to clear the 'mediagroup' attribute as you do in JSC?


> Source/WebCore/html/HTMLMediaElement.cpp:142
> +typedef HashMap<Document*, HashSet<HTMLMediaElement*> > DocElementSetMap;

Might as well spell it out: Doc -> Document


> Source/WebCore/html/HTMLMediaElement.cpp:143
> +static DocElementSetMap& documentToElementSetMap()

None of these static functions made it into the ChangeLog.


> Source/WebCore/html/HTMLMediaElement.cpp:2288
> +	       //  then seek to the earliest possible position of the media
resource and abort these steps.
>	       seek(0, ignoredException);

m_player->startTime() is the "earliest possible position" (although in practice
I think it return 0 for all ports currently).


> Source/WebCore/html/HTMLMediaElement.cpp:3219
> +    // 2. Let m have no current media controller, if it currently has one.
> +    setController(0);
> +    // 3. If m's mediagroup attribute is being removed, then abort these
steps.

Nit: I think a blank line between code and the next comment make it easier to
read.


> Source/WebCore/html/HTMLMediaElement.h:427
> +    bool hasSource() const { return m_networkState != NETWORK_EMPTY ||
m_networkState != NETWORK_NO_SOURCE; }

I am not wild about "haveSource", I assumed it meant "have a 'src' attribute".


> Source/WebCore/html/MediaController.cpp:224
> +    m_clock->setPlayRate(rate);
> +
> +    // then queue a task to fire a simple event named ratechange at the
MediaController.
> +    scheduleEvent(eventNames().ratechangeEvent);
> +}

Don't you also need to set the rates of all attached elements?


> Source/WebCore/html/MediaController.cpp:376
> +	   // then queue a task that, if the MediaController object is a
playing media controller, and 
> +	   // all of the MediaController's slaved media elements have still
ended playback, and the 
> +	   // media controller playback rate is still positive or zero, 
> +	   if (!m_paused && hasEnded()) {
> +	       // changes the MediaController object to a paused media
controller
> +	       m_paused = true;
> +	       m_clock->stop();
> +
> +	       // and then fires a simple event named pause at the
MediaController object.
> +	       scheduleEvent(eventNames().pauseEvent);
> +	   }

"then queue a task ..." means the logic is supposed to happen after the current
run loop cycle ends. I think it would be fine to file a bug for this and fix it
in a follow up patch.


> Source/WebCore/html/MediaController.cpp:443
> +	   // or if any of its slaved media elements whose autoplaying flag is
true still have their 
> +	   // paused attribute set to true,
> +	   if (element->autoplay() && element->paused())
> +	       return true;

The "autoplaying flag" is not the same as the 'autoplay' attribute, t is a
private state kept in m_autoplaying.


> Source/WebCore/html/MediaController.cpp:456
> +    if (m_clock->playRate() <= 0)
> +	   return false;

This confused me when I read it, can you please include the spec text as you
have done above?


> Source/WebCore/html/MediaController.cpp:491
> +ScriptExecutionContext* MediaController::scriptExecutionContext() const
> +{
> +    if (m_mediaElements.isEmpty())
> +	   return 0;
> +    return m_mediaElements.first()->document();

This assumes that all elements are in the same document. Is this always true?


> LayoutTests/ChangeLog:1
> +2011-11-03  Jer Noble  <jer.noble at apple.com>

One...

> LayoutTests/ChangeLog:13
> +2011-11-03  Jer Noble  <jer.noble at apple.com>

Two...

> LayoutTests/ChangeLog:25
> +2011-11-02  Jer Noble  <jer.noble at apple.com>

Three times the fun!


> LayoutTests/ChangeLog:35
> +	   * media/media-controller-expected.txt: Added.
> +	   * media/media-controller-playback-expected.txt: Added.
> +	   * media/media-controller-playback.html: Added.
> +	   * media/media-controller.html: Added.

We definitely need more tests for this feature! Please file a master bug and we
can block others on it.


> LayoutTests/ChangeLog:36
> +

I think you also need to update fast/dom/Window/window-properties-expected.txt
and fast/js/global-constructors-expected.txt because of the changes to
DOMWindow.idl


> LayoutTests/media/media-controller-playback.html:16
> +	       video = document.getElementsByTagName('video')[0];
> +	       video2 = document.getElementsByTagName('video')[1];
> +	       run('controller = video.mediaController');
> +	       controller.addEventListener('canplaythrough', canplaythrough,
true);
> +	       setSrcByTagName('video', findMediaFile('video',
'content/test'));

Three calls to "document.getElementsByTagName('video')" in one function?


>
LayoutTests/platform/mac-lion/fast/regions/render-region-custom-style-mark-expe
cted.txt:1
> +layer at (0,0) size 800x600

Did you mean to change this result?