Summary: | Change HTMLInputElement-derived parts of media element shadow DOM to use shadowPseudoId. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Dimitri Glazkov (Google) <dglazkov> | ||||||
Component: | DOM | Assignee: | 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
Dimitri Glazkov (Google)
2011-01-25 13:37:19 PST
Created attachment 80115 [details]
Patch
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 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. 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. Created attachment 80135 [details]
With fixups
(In reply to comment #5) > Created an attachment (id=80135) [details] > With fixups Darin, can you take another look? (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 on attachment 80135 [details]
With fixups
Looks good.
Committed r76719: <http://trac.webkit.org/changeset/76719> http://trac.webkit.org/changeset/76719 might have broken Qt Linux Release Reverted r76719 for reason: Broke a bunch of media tests in Chromium/Qt/GTK Committed r76724: <http://trac.webkit.org/changeset/76724> (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 :(. http://trac.webkit.org/changeset/76724 might have broken Leopard Intel Release (Tests) Committed r76951: <http://trac.webkit.org/changeset/76951> http://trac.webkit.org/changeset/76951 might have broken Qt Linux Release The following tests are not passing: media/controls-without-preload.html |