Bug 53122 – Change HTMLInputElement-derived parts of media element shadow DOM to use shadowPseudoId.

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Dimitri Glazkov (Google)]

Change HTMLInputElement-derived parts of media element shadow DOM to use shadowPseudoId.

[Dimitri Glazkov (Google)]

Created attachment 80115 [details]
Patch

[Darin Adler]

Comment on attachment 80115 [details]
Patch

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

Seems unfortunate that shadowPseudoId returns an AtomicString so has to thrash reference counts. Is there any caller that can’t return const AtomicString& instead? Might be nice to tighten that up again.

> Source/WebCore/rendering/MediaControlElements.cpp:468
> +    m_displayType = MediaMuteButton;

Why isn’t this a MediaControlInputElement constructor argument? It seems less than optimal to set this field in a base class directly instead of letting the constructor do it.

[Dimitri Glazkov (Google)]

Comment on attachment 80115 [details]
Patch

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

Good point on shadowPseudoId. I'll change to use const AtomicString& in separate patch.

>> Source/WebCore/rendering/MediaControlElements.cpp:468
>> +    m_displayType = MediaMuteButton;
> 
> Why isn’t this a MediaControlInputElement constructor argument? It seems less than optimal to set this field in a base class directly instead of letting the constructor do it.

I'll change to use a constructor argument, I like it.

[Dimitri Glazkov (Google)]

Turns out I also neglected to realize that since the elements now use proper selector pipeline, some initial CSS properties need to be explicitly set to avoid inheriting values from input[type=button] and input[type=range] definitions.

[Dimitri Glazkov (Google)]

Created attachment 80135 [details]
With fixups

[Dimitri Glazkov (Google)]

(In reply to comment #5)
> Created an attachment (id=80135) [details]
> With fixups

Darin, can you take another look?

[Dimitri Glazkov (Google)]

(In reply to comment #6)
> (In reply to comment #5)
> > Created an attachment (id=80135) [details] [details]
> > With fixups
> 
> Darin, can you take another look?

Since you r+ the first patch, I'll land this improvement with your silent agreement :) -- please yell at me if I'd done wrong.

[Kent Tamura]

Comment on attachment 80135 [details]
With fixups

Looks good.

[Dimitri Glazkov (Google)]

Committed r76719: <http://trac.webkit.org/changeset/76719>

[WebKit Review Bot]

http://trac.webkit.org/changeset/76719 might have broken Qt Linux Release

[Dimitri Glazkov (Google)]

Reverted r76719 for reason:

Broke a bunch of media tests in Chromium/Qt/GTK

Committed r76724: <http://trac.webkit.org/changeset/76724>

[Dimitri Glazkov (Google)]

(In reply to comment #11)
> Reverted r76719 for reason:
> 
> Broke a bunch of media tests in Chromium/Qt/GTK
> 
> Committed r76724: <http://trac.webkit.org/changeset/76724>

I think we may have to fix bug 52197 before proceeding further :(.

[WebKit Review Bot]

http://trac.webkit.org/changeset/76724 might have broken Leopard Intel Release (Tests)

[Dimitri Glazkov (Google)]

Committed r76951: <http://trac.webkit.org/changeset/76951>

[WebKit Review Bot]

http://trac.webkit.org/changeset/76951 might have broken Qt Linux Release
The following tests are not passing:
media/controls-without-preload.html