Bug 53122

Summary: Change HTMLInputElement-derived parts of media element shadow DOM to use shadowPseudoId.
Product: WebKit Reporter: Dimitri Glazkov (Google) <dglazkov>
Component: DOMAssignee: Dimitri Glazkov (Google) <dglazkov>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, darin, eric, tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: OS X 10.5   
Bug Depends on: 52917, 53674    
Bug Blocks: 53020, 53136, 53314    
Attachments:
Description Flags
Patch
none
With fixups tkent: review+

Description Dimitri Glazkov (Google) 2011-01-25 13:37:19 PST
Change HTMLInputElement-derived parts of media element shadow DOM to use shadowPseudoId.
Comment 1 Dimitri Glazkov (Google) 2011-01-25 13:48:12 PST
Created attachment 80115 [details]
Patch
Comment 2 Darin Adler 2011-01-25 14:06:13 PST
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.
Comment 3 Dimitri Glazkov (Google) 2011-01-25 14:11:03 PST
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.
Comment 4 Dimitri Glazkov (Google) 2011-01-25 15:23:00 PST
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.
Comment 5 Dimitri Glazkov (Google) 2011-01-25 15:23:16 PST
Created attachment 80135 [details]
With fixups
Comment 6 Dimitri Glazkov (Google) 2011-01-25 15:23:51 PST
(In reply to comment #5)
> Created an attachment (id=80135) [details]
> With fixups

Darin, can you take another look?
Comment 7 Dimitri Glazkov (Google) 2011-01-26 13:59:50 PST
(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.
Comment 8 Kent Tamura 2011-01-26 14:09:04 PST
Comment on attachment 80135 [details]
With fixups

Looks good.
Comment 9 Dimitri Glazkov (Google) 2011-01-26 14:20:07 PST
Committed r76719: <http://trac.webkit.org/changeset/76719>
Comment 10 WebKit Review Bot 2011-01-26 14:52:30 PST
http://trac.webkit.org/changeset/76719 might have broken Qt Linux Release
Comment 11 Dimitri Glazkov (Google) 2011-01-26 14:54:46 PST
Reverted r76719 for reason:

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

Committed r76724: <http://trac.webkit.org/changeset/76724>
Comment 12 Dimitri Glazkov (Google) 2011-01-26 14:57:12 PST
(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 :(.
Comment 13 WebKit Review Bot 2011-01-26 15:51:04 PST
http://trac.webkit.org/changeset/76724 might have broken Leopard Intel Release (Tests)
Comment 14 Dimitri Glazkov (Google) 2011-01-28 09:53:08 PST
Committed r76951: <http://trac.webkit.org/changeset/76951>
Comment 15 WebKit Review Bot 2011-01-28 10:29:18 PST
http://trac.webkit.org/changeset/76951 might have broken Qt Linux Release
The following tests are not passing:
media/controls-without-preload.html