Attachment 138506[details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/full..." exit_code: 1
Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 21 files
If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 138644[details]
Archive of layout-test-results from ec2-cr-linux-03
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Thanks for your review (even though I committed explicitly with a no-review request ;-).
At this stage, I only wanted to get some of the tests running on all platforms to see what the visual impact would be, which I seem to have achieved.
Your input is much appreciated and I will address it, and also fix the remaining layout tests.

Created attachment 140256[details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 140240[details]
Patch - not to be committed before 7 May
View in context: https://bugs.webkit.org/attachment.cgi?id=140240&action=review> Source/WebCore/ChangeLog:1
> +2012-05-04 Silvia Pfeiffer <silviapf@chromium.org>
I guess the part that changes the shadow pseudo ID of the slider when it is used in a media element shadow is not part of this patch yet?
> Source/WebCore/html/shadow/MediaControlRootElementChromium.h:62
> +// ----------------------------
I don’t think we need ASCII art like this.
> Source/WebCore/rendering/RenderMediaControlsChromium.cpp:103
> + // Show disabled play button when resource URL doesn't work.
What does this mean?

(In reply to comment #13)
> (From update of attachment 140240[details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=140240&action=review
>
> > Source/WebCore/ChangeLog:1
> > +2012-05-04 Silvia Pfeiffer <silviapf@chromium.org>
>
> I guess the part that changes the shadow pseudo ID of the slider when it is used in a media element shadow is not part of this patch yet?
No, I am preparing the patch right now. Please review again then. Thanks!
> > Source/WebCore/html/shadow/MediaControlRootElementChromium.h:62
> > +// ----------------------------
>
> I don’t think we need ASCII art like this.
The .cpp file uses that as a separator. But ok, I'll remove it.
> > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:103
> > + // Show disabled play button when resource URL doesn't work.
>
> What does this mean?
A different play button resource will be shown when the resource cannot be loaded.
I'll remove this comment, too.

Created attachment 142385[details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 142677[details]
Archive of layout-test-results from ec2-cr-linux-02
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 142868[details]
Archive of layout-test-results from ec2-cr-linux-04
The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Comment on attachment 142924[details]
On Linux only pass image tests in test_expectations
View in context: https://bugs.webkit.org/attachment.cgi?id=142924&action=review> Source/WebCore/ChangeLog:20
> + - Replace images for play, pause, mute and volume buttons.
> + - Add scalable buttons to fix https://crbug.com/110304
> + - Update CSS and use the new flexbox model.
> + - Remove timeline and volume containers.
> + - Add new enclosure div element with 30px high controls plus 5px padding.
> + - Change volume slider from vertical to horizontal layout.
> + - Set volume slider to 0 when media element is muted.
> + - Introduce a duration display on top of current time display when video is not autoplay.
> + - Remove complex code for coloring on playback and volume slider ranges in
> + preparation for an upcoming patch that will do it all through CSS.
> + - Introduce a special shadowPseudoId for styling of input ranges in controls.
I know that you have been working on this for a while now, but this many changes really should be split into multiple patches/bugs. This makes it easier to review, but it also makes it easier to track down behavior changes and/or regressions later. It may be that some of these are interdependent, but at first glance it looks like at least the following can be split into separate patches:
- Replace images for play, pause, mute and volume buttons.
- Add scalable buttons to fix https://crbug.com/110304
- Change volume slider from vertical to horizontal layout.
- Set volume slider to 0 when media element is muted.
- Introduce a duration display on top of current time display when video is not autoplay.
- Introduce a special shadowPseudoId for styling of input ranges in controls.
> Source/WebCore/ChangeLog:45
> + * html/shadow/MediaControlElements.cpp: capture fade-out duration 0.
> + (WebCore::MediaControlPanelElement::makeTransparent):
> + * html/shadow/MediaControlRootElement.h: Add a duration element.
> + (WebCore):
> + * html/shadow/MediaControlRootElementChromium.cpp: The main visual update.
> + (WebCore::MediaControlChromiumEnclosureElement::MediaControlChromiumEnclosureElement):
> + (WebCore):
> + (WebCore::MediaControlChromiumEnclosureElement::create):
> + (WebCore::MediaControlChromiumEnclosureElement::shadowPseudoId):
I find it *extremely* helpful to have per-function comments in a ChangeLog. This makes it much easier to review a patch, and it also makes it simpler for someone looking at just the ChangeLog later to see exactly what changed.
> Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:219
> + ExceptionCode ec;
> + m_durationDisplay->setInnerText(page->theme()->formatMediaControlsTime(duration), ec);
Because you aren't using the exception code, this would be better as:
m_durationDisplay->setInnerText(page->theme()->formatMediaControlsTime(duration), ASSERT_NO_EXCEPTION);
> Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:376
> + insertBefore(textDisplayContainer.release(), m_enclosure, ec, true);
Ditto.
> Source/WebCore/rendering/RenderMediaControlsChromium.cpp:82
> + if (!hasSource(mediaElement) || !mediaElement->hasAudio() || mediaElement->muted() || mediaElement->volume() <= 0.0f)
"0.0f" should be simly "0".
> Source/WebCore/rendering/RenderMediaControlsChromium.cpp:85
> + if (mediaElement->volume() <= 0.33f)
The "f" is unnecessary.
> Source/WebCore/rendering/RenderMediaControlsChromium.cpp:88
> + if (mediaElement->volume() <= 0.66f)
Ditto.
> Source/WebCore/rendering/RenderThemeChromiumMac.mm:192
> + // duration defines the format of how the time is rendered
Comment should look like complete sentences by starting with a capital letter and ending with a period .

(In reply to comment #25)
>
> I know that you have been working on this for a while now, but this many
> changes really should be split into multiple patches/bugs. This makes it
> easier to review, but it also makes it easier to track down behavior changes
> and/or regressions later.
All of these together make the newly styled video controls work in the new fashion. I have another 5 patches waiting which I want to roll out in steps on top of this.
> It may be that some of these are interdependent, but at first glance it looks
> like at least the following can be split into separate patches:
>
> - Replace images for play, pause, mute and volume buttons.
I can't just replace the images for these - the controls would look hideous. The images and the CSS are a core requirement for the new layout.
> - Add scalable buttons to fix https://crbug.com/110304
The scalability of the buttons is a side effect of the new CSS, not a separate fix.
> - Change volume slider from vertical to horizontal layout.
The layout won't work with a vertical slider.
> - Set volume slider to 0 when media element is muted.
This is part of the new look.
> - Introduce a duration display on top of current time display when video is not autoplay.
This is part of the new look.
> - Introduce a special shadowPseudoId for styling of input ranges in controls.
This is a requirement for the new CSS to work - I can't roll this out selectively.

(In reply to comment #26)
> (In reply to comment #25)
> >
> > I know that you have been working on this for a while now, but this many
> > changes really should be split into multiple patches/bugs. This makes it
> > easier to review, but it also makes it easier to track down behavior changes
> > and/or regressions later.
>
> All of these together make the newly styled video controls work in the new fashion. I have another 5 patches waiting which I want to roll out in steps on top of this.
>
I wasn't suggesting that you try to land them separately, only that you consider breaking this into smaller pieces focusing on some of the changes you note it addresses. Once the separate patches have been reviewed, you can land them all simultaneously.
I actually think you might want to consider getting your other pending patches into the review pipeline because it looks like there will be some unwanted UI changes if this lands by itself, eg. in this patch paintMediaSlider doesn't appear to do anything useful.

(In reply to comment #27)
> I wasn't suggesting that you try to land them separately, only that you
> consider breaking this into smaller pieces focusing on some of the changes
> you note it addresses. Once the separate patches have been reviewed, you can
> land them all simultaneously.
How do I do this? Create a new bug for each or attach them all to this bug?