Bug 26661 – Fullscreen button should be hidden when the backend doesn't support it.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Pierre d'Herbemont]

Blocked by: https://bugs.webkit.org/show_bug.cgi?id=26659

[Pierre d'Herbemont]

Created attachment 31739 [details]
patch 1/2 - v1.

[Pierre d'Herbemont]

Created attachment 31740 [details]
patch 2/2 - v1.

[Pierre d'Herbemont]

Created attachment 31750 [details]
patch 1/2 - v2.

With ChangeLog.

[Pierre d'Herbemont]

Created attachment 31751 [details]
patch 2/2 - v2.

With ChangeLog.

[Eric Seidel (no email)]

Comment on attachment 31750 [details]
patch 1/2 - v2.

I bet there are still ways to test this... but ok.

[Eric Seidel (no email)]

Comment on attachment 31751 [details]
patch 2/2 - v2.

Looks OK.

[Eric Seidel (no email)]

Pierre will you commit these yourself, or does someone need to commit them for you?

[Pierre d'Herbemont]

(In reply to comment #7)
> Pierre will you commit these yourself, or does someone need to commit them for
> you?

I don't have commit access... So I still need someone to commit them.

[Pierre d'Herbemont]

Created attachment 31863 [details]
patch 1/2 - v3.

supportsFullscreen() shouldn't be pure virtual. Return false by default.

[Pierre d'Herbemont]

Comment on attachment 31863 [details]
patch 1/2 - v3.

I think it's still r+ by Eric Seidel, even after this change. Yet. Marquing as r?

[Simon Fraser (smfr)]

Comment on attachment 31863 [details]
patch 1/2 - v3.


> diff --git a/WebCore/html/HTMLMediaElement.h b/WebCore/html/HTMLMediaElement.h
> index 2a08e3c..18b263a 100644
> --- a/WebCore/html/HTMLMediaElement.h
> +++ b/WebCore/html/HTMLMediaElement.h
> @@ -65,7 +65,9 @@ public:
>      
>      virtual bool isVideo() const { return false; }
>      virtual bool hasVideo() const { return false; }
> -    
> +
> +    virtual bool supportsFullscreen() const { return false; }

Try to avoid changing whitespace.

> +void MediaControlFullscreenButtonElement::update()
> +{
> +    setHidden(!m_mediaElement->supportsFullscreen());
> +    MediaControlInputElement::update();

Will need to change when setHidden goes away.

> diff --git a/WebCore/rendering/MediaControlElements.h b/WebCore/rendering/MediaControlElements.h
> index 0c95976..4d8647e 100644
> --- a/WebCore/rendering/MediaControlElements.h
> +++ b/WebCore/rendering/MediaControlElements.h
> @@ -154,6 +154,7 @@ class MediaControlFullscreenButtonElement : public MediaControlInputElement {
>  public:
>      MediaControlFullscreenButtonElement(Document*, HTMLMediaElement*);
>      virtual void defaultEventHandler(Event*);
> +    void update();

You'll need to make update() virtual on MediaControlInputElement, then declare if virtual here.

OK with these changes.

[Pierre d'Herbemont]

Created attachment 31886 [details]
patch 1/2 - v4.

With Simon's comment.

[Eric Seidel (no email)]

Comment on attachment 31886 [details]
patch 1/2 - v4.

No argument name needed:
virtual bool rendererIsNeeded(RenderStyle* style);
according to WK style.

I think simon should review this.

[Simon Fraser (smfr)]

Comment on attachment 31886 [details]
patch 1/2 - v4.

> +bool MediaControlFullscreenButtonElement::rendererIsNeeded(RenderStyle* style)
> +{
> +    return m_mediaElement->supportsFullscreen() && HTMLInputElement::rendererIsNeeded(style);

Should just call up to the superclass here, not skip a generation.

[Simon Fraser (smfr)]

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