Bug 26661 - Fullscreen button should be hidden when the backend doesn't support it.
Summary: Fullscreen button should be hidden when the backend doesn't support it.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Pierre d'Herbemont
URL:
Keywords:
Depends on: 26659
Blocks: 26742
  Show dependency treegraph
 
Reported: 2009-06-23 14:16 PDT by Pierre d'Herbemont
Modified: 2009-06-26 11:28 PDT (History)
0 users

See Also:


Attachments
patch 1/2 - v1. (5.35 KB, patch)
2009-06-23 14:33 PDT, Pierre d'Herbemont
no flags Details | Formatted Diff | Diff
patch 2/2 - v1. (1.58 KB, patch)
2009-06-23 14:34 PDT, Pierre d'Herbemont
no flags Details | Formatted Diff | Diff
patch 1/2 - v2. (6.72 KB, patch)
2009-06-23 15:54 PDT, Pierre d'Herbemont
eric: review+
Details | Formatted Diff | Diff
patch 2/2 - v2. (1.89 KB, patch)
2009-06-23 15:55 PDT, Pierre d'Herbemont
eric: review+
Details | Formatted Diff | Diff
patch 1/2 - v3. (6.52 KB, patch)
2009-06-25 10:25 PDT, Pierre d'Herbemont
simon.fraser: review+
Details | Formatted Diff | Diff
patch 1/2 - v4. (5.08 KB, patch)
2009-06-25 16:40 PDT, Pierre d'Herbemont
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Pierre d'Herbemont 2009-06-23 14:16:58 PDT
Blocked by: https://bugs.webkit.org/show_bug.cgi?id=26659
Comment 1 Pierre d'Herbemont 2009-06-23 14:33:34 PDT
Created attachment 31739 [details]
patch 1/2 - v1.
Comment 2 Pierre d'Herbemont 2009-06-23 14:34:25 PDT
Created attachment 31740 [details]
patch 2/2 - v1.
Comment 3 Pierre d'Herbemont 2009-06-23 15:54:30 PDT
Created attachment 31750 [details]
patch 1/2 - v2.

With ChangeLog.
Comment 4 Pierre d'Herbemont 2009-06-23 15:55:16 PDT
Created attachment 31751 [details]
patch 2/2 - v2.

With ChangeLog.
Comment 5 Eric Seidel (no email) 2009-06-23 17:34:24 PDT
Comment on attachment 31750 [details]
patch 1/2 - v2.

I bet there are still ways to test this... but ok.
Comment 6 Eric Seidel (no email) 2009-06-23 17:35:28 PDT
Comment on attachment 31751 [details]
patch 2/2 - v2.

Looks OK.
Comment 7 Eric Seidel (no email) 2009-06-24 18:50:50 PDT
Pierre will you commit these yourself, or does someone need to commit them for you?
Comment 8 Pierre d'Herbemont 2009-06-24 19:20:07 PDT
(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.
Comment 9 Pierre d'Herbemont 2009-06-25 10:25:29 PDT
Created attachment 31863 [details]
patch 1/2 - v3.

supportsFullscreen() shouldn't be pure virtual. Return false by default.
Comment 10 Pierre d'Herbemont 2009-06-25 10:26:33 PDT
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?
Comment 11 Simon Fraser (smfr) 2009-06-25 11:36:50 PDT
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.
Comment 12 Pierre d'Herbemont 2009-06-25 16:40:02 PDT
Created attachment 31886 [details]
patch 1/2 - v4.

With Simon's comment.
Comment 13 Eric Seidel (no email) 2009-06-25 17:07:19 PDT
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.
Comment 14 Simon Fraser (smfr) 2009-06-26 11:14:50 PDT
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.
Comment 15 Simon Fraser (smfr) 2009-06-26 11:28:55 PDT
http://trac.webkit.org/changeset/45273