Bug 94307

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

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

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

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?

+ if (!thisRenderer || thisRenderer->isMedia()) + return toRenderMedia(thisRenderer); toRenderMedia() ASSERTs when passed 0.

Created attachment 160943 [details] Patch

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

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

(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 ;-/

Created attachment 162189 [details] Patch

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?

We decided not to do this in V1 API.