Bug 26659 - Support hidding an control bar element from the Media element controller (display:none does not work for media control elements)
Summary: Support hidding an control bar element from the Media element controller (dis...
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: Nobody
URL:
Keywords:
: 26228 (view as bug list)
Depends on: 26653
Blocks: 26661
  Show dependency treegraph
 
Reported: 2009-06-23 14:03 PDT by Pierre d'Herbemont
Modified: 2009-06-26 11:01 PDT (History)
1 user (show)

See Also:


Attachments
patch v1. (3.22 KB, patch)
2009-06-23 14:09 PDT, Pierre d'Herbemont
no flags Details | Formatted Diff | Diff
patch v2. (3.81 KB, patch)
2009-06-23 15:28 PDT, Pierre d'Herbemont
eric: review-
Details | Formatted Diff | Diff
patch v3. (3.73 KB, patch)
2009-06-24 16:50 PDT, Pierre d'Herbemont
simon.fraser: review-
Details | Formatted Diff | Diff
patch v4. (3.70 KB, patch)
2009-06-25 15:55 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: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