Bug 26659

Summary: Support hidding an control bar element from the Media element controller (display:none does not work for media control elements)
Product: WebKit Reporter: Pierre d'Herbemont <pdherbemont>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: simon.fraser
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: OS X 10.5   
Bug Depends on: 26653    
Bug Blocks: 26661    
Attachments:
Description Flags
patch v1.
none
patch v2.
eric: review-
patch v3.
simon.fraser: review-
patch v4. simon.fraser: review+

Description Pierre d'Herbemont 2009-06-23 14:03:41 PDT
Support hidding an control bar element from the Media element controller.

We would then be able to remove the some buttons from <video controls> if the back end doesn't support those actions.

This is blocked by https://bugs.webkit.org/show_bug.cgi?id=26653
Comment 1 Pierre d'Herbemont 2009-06-23 14:09:51 PDT
Created attachment 31735 [details]
patch v1.

Attached a patch that should be applied after the blocker.
Comment 2 Pierre d'Herbemont 2009-06-23 15:28:15 PDT
Created attachment 31745 [details]
patch v2.

With ChangeLog.
Comment 3 Eric Seidel (no email) 2009-06-23 17:37:44 PDT
Comment on attachment 31745 [details]
patch v2.

You ChangeLog is missing the bug url.

Style:
+    if(m_isHidden)
+    if(!m_mediaElement || !m_mediaElement->renderer())
+    if (!renderer() && !m_isHidden && style->display() != NONE)
+    {
+        if (renderer)
+        {

When does this ever return NULL?
+        RenderObject* renderer = createRenderer(m_mediaElement->renderer()->renderArena(), style);

Why don't we just early-return in that case?

Why manually set attached instead of calling attach?
+            setAttached();
Comment 4 Pierre d'Herbemont 2009-06-24 16:40:54 PDT
(In reply to comment #3)
> (From update of attachment 31745 [details] [review])
> You ChangeLog is missing the bug url.

Thanks.

> Style:
> +    if(m_isHidden)
> +    if(!m_mediaElement || !m_mediaElement->renderer())
> +    if (!renderer() && !m_isHidden && style->display() != NONE)
> +    {
> +        if (renderer)
> +        {

Thanks.

> When does this ever return NULL?
> +        RenderObject* renderer =
> createRenderer(m_mediaElement->renderer()->renderArena(), style);

That's what previous code does.

I guess it will be NULL if malloc fails basically.

> Why don't we just early-return in that case?

ok.

> Why manually set attached instead of calling attach?
> +            setAttached();

That's what previous code was doing :)

Using attach() requires some changes. And I think it deserve a separate bug. (Basically we need to create a renderer with a specific style, so we need a to have styleForRenderer() virtual, along with other changes).
I have created https://bugs.webkit.org/show_bug.cgi?id=26697 for that.


Comment 5 Pierre d'Herbemont 2009-06-24 16:50:42 PDT
Created attachment 31817 [details]
patch v3.

ChangeLog with Bug number, coding style fixes.
Comment 6 Simon Fraser (smfr) 2009-06-25 11:00:38 PDT
*** Bug 26228 has been marked as a duplicate of this bug. ***
Comment 7 Simon Fraser (smfr) 2009-06-25 11:28:56 PDT
Comment on attachment 31817 [details]
patch v3.

Let's get rid of the m_isHidden on MediaControlInputElement, and a virtual method shouldBeVisible() on MediaControlInputElement that you can override to ask about fullscreen etc. updateStyle() will then need to handle the detach() case. Also need to clean up update() vs. updateStyle().
Comment 8 Pierre d'Herbemont 2009-06-25 15:55:02 PDT
Created attachment 31882 [details]
patch v4.

Updated with Simon's comment.
Comment 9 Simon Fraser (smfr) 2009-06-25 16:01:11 PDT
Comment on attachment 31882 [details]
patch v4.

> +    if (renderer() && !rendererIsNeeded(style))
> +        detach();
> +    else if (!renderer() && rendererIsNeeded(style)) {

It's probably worth changing this to call rendererIsNeeded() just once.

r=me
Comment 10 Simon Fraser (smfr) 2009-06-26 11:01:02 PDT
http://trac.webkit.org/changeset/45271