Bug 94307 – Author Shadow DOM for <video>

• Component: DOM
• Status: RESOLVED WONTFIX

[Hajime Morrita]

<video> should accept Shadow DOM attachment.

[Hajime Morrita]

Created attachment 160362 [details]
WIP, needs more test.

[WebKit Review Bot]

Comment on attachment 160362 [details]
WIP, needs more test.

Attachment 160362 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13601050

New failing tests:
fast/dom/shadow/shadowdom-for-media.html

[WebKit Review Bot]

Created attachment 160384 [details]
Archive of layout-test-results from gce-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Dimitri Glazkov (Google)]

Comment on attachment 160362 [details]
WIP, needs more test.

View in context: https://bugs.webkit.org/attachment.cgi?id=160362&action=review

> Source/WebCore/html/HTMLMediaElement.cpp:123
> +class MediaInnerElement : public HTMLElement {

This is begging to be named more descriptively. Perhaps MediaSurfaceElement or something like this?

[Eric Carlson]

+    if (!thisRenderer || thisRenderer->isMedia())
+        return toRenderMedia(thisRenderer);

toRenderMedia() ASSERTs when passed 0.

[Hajime Morrita]

Created attachment 160943 [details]
Patch

[Hajime Morrita]

Eric, Dimitri, thanks for taking a look. I addressed your points and added a set of tests.
Could you take another look?

[Dimitri Glazkov (Google)]

Comment on attachment 160943 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=160943&action=review

From the overall approach perspective, could we possibly get away with just always having the structure that uses <webkitmediasurface> element? In case of <img>, it was necessary because images are aplenty in the Web, and we had to be sensitive to memory use concerns. In case of video element, it's unlikely that we will have any significant impact with one extra element. In return, we'll have a much simpler logic.

WDYT?

R- due to a multitude spleeelling errors :P

> Source/WebCore/ChangeLog:10
> +        then add new called MediaSurfaceElement to back the RenderMeedia object which is originally backed by the host element.

Meedia -> Media, and "which _was_ originally".

> Source/WebCore/ChangeLog:18
> +                            - #shadow-root - ... author shdodow with <shadow> ...

shdodow -> shadow

> Source/WebCore/ChangeLog:27
> +        - Tells HTMLMediaElement::createRenderer() to create plain render objects instaed of RenderMedia when there is any author tree.

instaed -> instead.

> Source/WebCore/ChangeLog:36
> +        (audio, video): Set these elememnts display: inline-block. This matters when the element has any author shadows.

elememnts -> elements

> Source/WebCore/ChangeLog:91
> +        (WebCore::RenderVideo::updateIntrinsicSize): Repalced node() with videoElement().

Repalced -> Replaced

[Hajime Morrita]

(In reply to comment #8)
> (From update of attachment 160943 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160943&action=review
> 
> From the overall approach perspective, could we possibly get away with just always having the structure that uses <webkitmediasurface> element? In case of <img>, it was necessary because images are aplenty in the Web, and we had to be sensitive to memory use concerns. In case of video element, it's unlikely that we will have any significant impact with one extra element. In return, we'll have a much simpler logic.
> 
> WDYT?
Okay, let me try this way and see how it makes the code simpler.

> 
> R- due to a multitude spleeelling errors :P
> 
I should've never attempted to make any sentences in late ;-/

[Hajime Morrita]

Created attachment 162189 [details]
Patch

[Eric Carlson]

Comment on attachment 162189 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162189&action=review

> Source/WebCore/css/mediaControls.css:33
> +audio, video {
> +    display: inline-block;
> +}

I am not sure this is correct, the 'display' property of an audio element without controls should be 'none'. See bug #88105.

Why is this necessary?

> Source/WebCore/html/HTMLMediaElement.cpp:153
> +    DEFINE_STATIC_LOCAL(AtomicString, pseudoId, ("-webkit-media-surface"));

Should this use AtomicString::ConstructFromLiteral?

[Ryosuke Niwa]

We decided not to do this in V1 API.