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+

Dimitri Glazkov (Google)
Reported 2011-01-25 13:37:19 PST
Change HTMLInputElement-derived parts of media element shadow DOM to use shadowPseudoId.
Attachments
Patch (35.56 KB, patch)
2011-01-25 13:48 PST, Dimitri Glazkov (Google) 		no flags
DetailsFormatted DiffDiff
With fixups (38.54 KB, patch)
2011-01-25 15:23 PST, Dimitri Glazkov (Google) 		tkent: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dimitri Glazkov (Google)
Comment 1 2011-01-25 13:48:12 PST
Created attachment 80115 [details] Patch
Darin Adler
Comment 2 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.
Dimitri Glazkov (Google)
Comment 3 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.
Dimitri Glazkov (Google)
Comment 4 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.
Dimitri Glazkov (Google)
Comment 5 2011-01-25 15:23:16 PST
Created attachment 80135 [details] With fixups
Dimitri Glazkov (Google)
Comment 6 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?
Dimitri Glazkov (Google)
Comment 7 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.
Kent Tamura
Comment 8 2011-01-26 14:09:04 PST
Comment on attachment 80135 [details] With fixups Looks good.
Dimitri Glazkov (Google)
Comment 9 2011-01-26 14:20:07 PST
Committed r76719: <http://trac.webkit.org/changeset/76719>
WebKit Review Bot
Comment 10 2011-01-26 14:52:30 PST
http://trac.webkit.org/changeset/76719 might have broken Qt Linux Release
Dimitri Glazkov (Google)
Comment 11 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>
Dimitri Glazkov (Google)
Comment 12 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 :(.
WebKit Review Bot
Comment 13 2011-01-26 15:51:04 PST
http://trac.webkit.org/changeset/76724 might have broken Leopard Intel Release (Tests)
Dimitri Glazkov (Google)
Comment 14 2011-01-28 09:53:08 PST
Committed r76951: <http://trac.webkit.org/changeset/76951>
WebKit Review Bot
Comment 15 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
Note You need to log in before you can comment on or make changes to this bug.