[webkit-reviews] review granted: [Bug 101877] Refactor Media Control Elements (second half of bug 88871) : [Attachment 176896] Fix for Android

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has granted Silvia Pfeiffer
<silviapf at chromium.org>'s request for review:
Bug 101877: Refactor Media Control Elements (second half of bug 88871)
https://bugs.webkit.org/show_bug.cgi?id=101877

Attachment 176896: Fix for Android
https://bugs.webkit.org/attachment.cgi?id=176896&action=review

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


> Source/WebCore/html/shadow/MediaControlElementTypes.cpp:56
> +#if ENABLE(VIDEO_TRACK)
> +static const int textTracksIndexNotFound = -2;
> +#endif

Please don't define this in two files in the same directory, add a static
method to a class accessible to all places it is used.

> Source/WebCore/html/shadow/MediaControlElementTypes.h:120
> +

Nit: blank line isn't necessary.

> Source/WebCore/html/shadow/MediaControlElementTypes.h:139
> +    virtual void setCurrentValue(float);
> +    virtual float currentValue() const { return m_currentValue; }

Dot these need to be virtual?

> Source/WebCore/html/shadow/MediaControlElementTypes.h:180
> +    virtual void setActive(bool /*flag*/ = true, bool /*pause*/ = false);

OVERRIDE

> Source/WebCore/html/shadow/MediaControlElementTypes.h:185
> +    virtual void startTimer();
> +    virtual void stopTimer();
> +    virtual float nextRate() const;
> +    virtual void seekTimerFired(Timer<MediaControlSeekButtonElement>*);

Do these need to be virtual?

> Source/WebCore/html/shadow/MediaControlElementTypes.h:202
> +    virtual void setVolume(float);
> +    virtual void setClearMutedOnUserInteraction(bool);

Ditto.