Summary: | Fullscreen button should be hidden when the backend doesn't support it. | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Pierre d'Herbemont <pdherbemont> | ||||||||||||||
Component: | New Bugs | Assignee: | Pierre d'Herbemont <pdherbemont> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | ||||||||||||||||
Priority: | P2 | ||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||
Hardware: | Mac | ||||||||||||||||
OS: | OS X 10.5 | ||||||||||||||||
Bug Depends on: | 26659 | ||||||||||||||||
Bug Blocks: | 26742 | ||||||||||||||||
Attachments: |
|
Description
Pierre d'Herbemont
2009-06-23 14:16:58 PDT
Created attachment 31739 [details]
patch 1/2 - v1.
Created attachment 31740 [details]
patch 2/2 - v1.
Created attachment 31750 [details]
patch 1/2 - v2.
With ChangeLog.
Created attachment 31751 [details]
patch 2/2 - v2.
With ChangeLog.
Comment on attachment 31750 [details]
patch 1/2 - v2.
I bet there are still ways to test this... but ok.
Comment on attachment 31751 [details]
patch 2/2 - v2.
Looks OK.
Pierre will you commit these yourself, or does someone need to commit them for you? (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. Created attachment 31863 [details]
patch 1/2 - v3.
supportsFullscreen() shouldn't be pure virtual. Return false by default.
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 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. Created attachment 31886 [details]
patch 1/2 - v4.
With Simon's comment.
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 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. |