Bug 84672 - [META] Chrome video controls visual update
: [META] Chrome video controls visual update
Status: ASSIGNED
: WebKit
Media Elements
: 528+ (Nightly build)
: All Unspecified
: P2 Normal
Assigned To:
:
: WebExposed
: 87683 87835 88297 88623 88724 88743 88818 89050 89093 89284 89344 89416 89508 91368 91802 91811 93222 94395 95395 96339
:
  Show dependency treegraph
 
Reported: 2012-04-23 23:52 PST by
Modified: 2012-11-26 19:32 PST (History)


Attachments
Patch (919.94 KB, patch)
2012-04-24 00:08 PST, Silvia Pfeiffer
no flags Review Patch | Details | Formatted Diff | Diff
removed No new tests line (919.91 KB, patch)
2012-04-24 00:45 PST, Silvia Pfeiffer
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (6.20 MB, application/zip)
2012-04-24 14:27 PST, WebKit Review Bot
no flags Details
Patch - not to be committed before 7 May (1.93 MB, patch)
2012-05-04 08:26 PST, Silvia Pfeiffer
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (2.57 MB, application/zip)
2012-05-04 09:42 PST, WebKit Review Bot
no flags Details
Includes shadowPseudoId fix (2.15 MB, patch)
2012-05-16 17:33 PST, Silvia Pfeiffer
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.48 MB, application/zip)
2012-05-16 18:23 PST, WebKit Review Bot
no flags Details
Fixed Linux tests, removed other tests (590.71 KB, patch)
2012-05-18 01:43 PST, Silvia Pfeiffer
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-02 (1.74 MB, application/zip)
2012-05-18 03:16 PST, WebKit Review Bot
no flags Details
Flexbox changed in https://bugs.webkit.org/show_bug.cgi?id=86529 (590.77 KB, patch)
2012-05-19 05:03 PST, Silvia Pfeiffer
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (1.20 MB, application/zip)
2012-05-19 08:35 PST, WebKit Review Bot
no flags Details
On Linux only pass image tests in test_expectations (493.89 KB, patch)
2012-05-20 16:00 PST, Silvia Pfeiffer
eric.carlson: review-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-23 23:52:10 PST
Chrome video controls visual update
------- Comment #1 From 2012-04-24 00:08:01 PST -------
Created an attachment (id=138506) [details]
Patch
------- Comment #2 From 2012-04-24 00:18:41 PST -------
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.
------- Comment #3 From 2012-04-24 00:45:54 PST -------
Created an attachment (id=138510) [details]
removed No new tests line
------- Comment #4 From 2012-04-24 11:37:50 PST -------
(From update of attachment 138510 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=138510&action=review

Publishing these comments now because I won't be able to look at this again today.

> Source/WebCore/ChangeLog:17
> +        (audio::-webkit-media-controls-panel, video::-webkit-media-controls-panel):
> +        (audio::-webkit-media-controls-play-button, video::-webkit-media-controls-play-button):
> +        (audio::-webkit-media-controls-timeline, video::-webkit-media-controls-timeline):
> +        (input[type="range"]::-webkit-slider-container):
> +        (input[type="range"]::-webkit-slider-thumb):
> +        (audio::-webkit-media-controls-time-remaining-display, video::-webkit-media-controls-time-remaining-display):

I think it is really helpful, especially on a patch that is this big, to have comments on each line of the ChangeLog so a reviewer can see a summary of the changes in one place.

> Source/WebCore/css/mediaControlsChromium.css:97
> +/* FIXME this might break other usage of input in shadow DOMs: however, this doesn't work:
> +video::-webkit-media-controls input[type="range"]::-webkit-slider-container
> +*/

Nit: FIXMEs should associated with a bug.

> Source/WebCore/html/shadow/MediaControlElements.cpp:334
> +#if !PLATFORM(CHROMIUM)
>      element->hide();
> +#endif

Instead of littering the code with #ifs, I would prefer if the "hide or not" behavior was defined by the theme - like we do for the position of the volume slider, the duration of the fade, etc.

> Source/WebCore/html/shadow/MediaControlElements.cpp:353
> +#if !PLATFORM(CHROMIUM)
>      hide();
> +#endif

Ditto.

> Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:409
> -}
> +} // namespace WebCore

Nit: drive-by cleanups like this are generally frowned upon as part of a bigger patch.

> Source/WebCore/html/shadow/SliderThumbElement.cpp:69
> +#if PLATFORM(CHROMIUM)
> +    return sliderStyle->appearance() == SliderVerticalPart;
> +#else
>      return sliderStyle->appearance() == SliderVerticalPart || sliderStyle->appearance() == MediaVolumeSliderPart;
> +#endif

Can this logic also come from the theme instead of adding #ifs?

> Source/WebCore/html/shadow/SliderThumbElement.cpp:120
> +#if PLATFORM(CHROMIUM)
> +    bool isVertical = style()->appearance() == SliderThumbVerticalPart;
> +#else
>      bool isVertical = style()->appearance() == SliderThumbVerticalPart || style()->appearance() == MediaVolumeSliderThumbPart;
> +#endif

Ditto.
------- Comment #5 From 2012-04-24 14:27:18 PST -------
(From update of attachment 138510 [details])
Attachment 138510 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12517358

New failing tests:
fast/forms/range/slider-delete-while-dragging-thumb.html
media/media-controls-invalid-url.html
fast/dom/HTMLInputElement/input-slider-update.html
fast/forms/range/slider-thumb-shared-style.html
batterystatus/event-after-navigation.html
accessibility/aria-disabled.html
fast/frames/lots-of-objects.html
media/track/track-cue-rendering-snap-to-lines-not-set.html
fast/loader/text-document-wrapping.html
media/video-controls-rendering-toggle-display-none.html
fast/forms/range/input-appearance-range.html
fast/forms/range/slider-mouse-events.html
fast/canvas/webgl/shader-precision-format.html
fast/forms/range/slider-padding.html
fast/forms/input-appearance-height.html
fast/forms/range/slider-onchange-event.html
fast/dom/HTMLInputElement/input-slider-update-styled.html
fast/forms/range/slider-thumb-stylability.html
fullscreen/full-screen-stacking-context.html
http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html
fast/repaint/slider-thumb-drag-release.html
fast/css/unknown-pseudo-element-matching.html
fast/forms/box-shadow-override.html
fast/repaint/slider-thumb-float.html
fast/forms/range/range-thumb-height-percentage.html
media/controls-layout-direction.html
------- Comment #6 From 2012-04-24 14:27:26 PST -------
Created an attachment (id=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
------- Comment #7 From 2012-04-24 18:56:45 PST -------
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.
------- Comment #8 From 2012-04-29 23:06:04 PST -------
(In reply to comment #4)
> (From update of attachment 138510 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=138510&action=review
> 
> Publishing these comments now because I won't be able to look at this again today.
> 
> > Source/WebCore/ChangeLog:17
> > +        (audio::-webkit-media-controls-panel, video::-webkit-media-controls-panel):
> > +        (audio::-webkit-media-controls-play-button, video::-webkit-media-controls-play-button):
> > +        (audio::-webkit-media-controls-timeline, video::-webkit-media-controls-timeline):
> > +        (input[type="range"]::-webkit-slider-container):
> > +        (input[type="range"]::-webkit-slider-thumb):
> > +        (audio::-webkit-media-controls-time-remaining-display, video::-webkit-media-controls-time-remaining-display):
> 
> I think it is really helpful, especially on a patch that is this big, to have comments on each line of the ChangeLog so a reviewer can see a summary of the changes in one place.

Will do. We've put just the most basic features into this patch that we want with the visual update of the video controls for https://code.google.com/p/chromium/issues/detail?id=88489 . Unfortunately even that is a fair amount. There will be more patches coming with further features.


> > Source/WebCore/css/mediaControlsChromium.css:97
> > +/* FIXME this might break other usage of input in shadow DOMs: however, this doesn't work:
> > +video::-webkit-media-controls input[type="range"]::-webkit-slider-container
> > +*/
> 
> Nit: FIXMEs should associated with a bug.

Added.


> > Source/WebCore/html/shadow/MediaControlElements.cpp:334
> > +#if !PLATFORM(CHROMIUM)
> >      element->hide();
> > +#endif
> 
> Instead of littering the code with #ifs, I would prefer if the "hide or not" behavior was defined by the theme - like we do for the position of the volume slider, the duration of the fade, etc.

Done.


> > Source/WebCore/html/shadow/MediaControlElements.cpp:353
> > +#if !PLATFORM(CHROMIUM)
> >      hide();
> > +#endif
> 
> Ditto.

Done.


> > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:409
> > -}
> > +} // namespace WebCore
> 
> Nit: drive-by cleanups like this are generally frowned upon as part of a bigger patch.

Removed.


> > Source/WebCore/html/shadow/SliderThumbElement.cpp:69
> > +#if PLATFORM(CHROMIUM)
> > +    return sliderStyle->appearance() == SliderVerticalPart;
> > +#else
> >      return sliderStyle->appearance() == SliderVerticalPart || sliderStyle->appearance() == MediaVolumeSliderPart;
> > +#endif
> 
> Can this logic also come from the theme instead of adding #ifs?

Done.


> > Source/WebCore/html/shadow/SliderThumbElement.cpp:120
> > +#if PLATFORM(CHROMIUM)
> > +    bool isVertical = style()->appearance() == SliderThumbVerticalPart;
> > +#else
> >      bool isVertical = style()->appearance() == SliderThumbVerticalPart || style()->appearance() == MediaVolumeSliderThumbPart;
> > +#endif
> 
> Ditto.

Done.
------- Comment #9 From 2012-05-04 08:26:14 PST -------
Created an attachment (id=140240) [details]
Patch
------- Comment #10 From 2012-05-04 08:31:36 PST -------
Attachment 140240 [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
LayoutTests/ChangeLog:9:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:15:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:16:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:17:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
LayoutTests/ChangeLog:18:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:9:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:10:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:12:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:13:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:14:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:15:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:16:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:17:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Source/WebCore/ChangeLog:18:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 18 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #11 From 2012-05-04 09:42:40 PST -------
(From update of attachment 140240 [details])
Attachment 140240 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12631193

New failing tests:
fast/forms/range/slider-delete-while-dragging-thumb.html
media/audio-repaint.html
media/audio-controls-rendering.html
fast/css/unknown-pseudo-element-matching.html
http/tests/security/sandboxed-iframe-modify-self.html
fast/forms/range/slider-thumb-shared-style.html
accessibility/aria-disabled.html
fast/frames/lots-of-objects.html
fast/loader/text-document-wrapping.html
fast/forms/range/input-appearance-range.html
media/controls-after-reload.html
fast/forms/range/slider-mouse-events.html
fast/canvas/webgl/shader-precision-format.html
media/controls-styling.html
fast/forms/range/slider-padding.html
fast/forms/range/slider-onchange-event.html
fast/layers/video-layer.html
media/controls-strict.html
fast/dom/HTMLInputElement/input-slider-update-styled.html
fast/forms/range/slider-thumb-stylability.html
fullscreen/full-screen-stacking-context.html
http/tests/xmlhttprequest/xmlhttprequest-unsafe-redirect.html
fast/repaint/slider-thumb-drag-release.html
fast/loader/javascript-url-in-embed.html
fast/dom/HTMLInputElement/input-slider-update.html
fast/repaint/slider-thumb-float.html
fast/forms/range/range-thumb-height-percentage.html
media/controls-layout-direction.html
------- Comment #12 From 2012-05-04 09:42:47 PST -------
Created an attachment (id=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 #13 From 2012-05-15 21:16:38 PST -------
(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?

> 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?
------- Comment #14 From 2012-05-15 21:40:38 PST -------
(In reply to comment #13)
> (From update of attachment 140240 [details] [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.
------- Comment #15 From 2012-05-16 17:33:33 PST -------
Created an attachment (id=142379) [details]
Includes shadowPseudoId fix

Includes shadowPseudoId fix and feedback
------- Comment #16 From 2012-05-16 18:23:50 PST -------
(From update of attachment 142379 [details])
Attachment 142379 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12718400

New failing tests:
media/media-document-audio-repaint.html
media/video-no-audio.html
media/video-display-toggle.html
media/sources-fallback-codecs.html
media/video-zoom-controls.html
media/media-controls-clone.html
media/video-empty-source.html
media/video-playing-and-pause.html
------- Comment #17 From 2012-05-16 18:23:56 PST -------
Created an attachment (id=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
------- Comment #18 From 2012-05-18 01:43:46 PST -------
Created an attachment (id=142662) [details]
Fixed Linux tests, removed other tests

corrected test_expectations
------- Comment #19 From 2012-05-18 03:16:35 PST -------
(From update of attachment 142662 [details])
Attachment 142662 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12717813

New failing tests:
media/media-document-audio-repaint.html
fullscreen/full-screen-stacking-context.html
media/video-no-audio.html
media/controls-strict.html
media/controls-styling.html
media/video-display-toggle.html
media/audio-repaint.html
media/audio-controls-rendering.html
media/video-zoom-controls.html
media/controls-without-preload.html
media/media-controls-clone.html
fast/layers/video-layer.html
media/video-empty-source.html
media/video-playing-and-pause.html
media/controls-layout-direction.html
media/controls-after-reload.html
------- Comment #20 From 2012-05-18 03:16:41 PST -------
Created an attachment (id=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
------- Comment #21 From 2012-05-19 05:03:08 PST -------
Created an attachment (id=142862) [details]
Flexbox changed in https://bugs.webkit.org/show_bug.cgi?id=86529

updated flexbox use
------- Comment #22 From 2012-05-19 08:35:08 PST -------
(From update of attachment 142862 [details])
Attachment 142862 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12737118

New failing tests:
media/media-document-audio-repaint.html
fullscreen/full-screen-stacking-context.html
media/video-no-audio.html
media/controls-strict.html
media/controls-styling.html
media/video-display-toggle.html
media/audio-repaint.html
media/audio-controls-rendering.html
media/video-zoom-controls.html
media/controls-without-preload.html
media/media-controls-clone.html
fast/layers/video-layer.html
media/video-playing-and-pause.html
media/controls-layout-direction.html
media/controls-after-reload.html
------- Comment #23 From 2012-05-19 08:35:14 PST -------
Created an attachment (id=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 #24 From 2012-05-20 16:00:52 PST -------
Created an attachment (id=142924) [details]
On Linux only pass image tests in test_expectations

Fail IMAGE+TEXT or TEXT only test expectations
------- Comment #25 From 2012-05-20 19:56:32 PST -------
(From update of attachment 142924 [details])
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 .
------- Comment #26 From 2012-05-20 20:56:18 PST -------
(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.
------- Comment #27 From 2012-05-21 10:12:27 PST -------
(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.
------- Comment #28 From 2012-05-21 17:42:33 PST -------
(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?
------- Comment #29 From 2012-05-21 18:18:36 PST -------
(In reply to comment #28)
> 
> How do I do this? Create a new bug for each or attach them all to this bug?

A new bug for each issue is preferable.
------- Comment #30 From 2012-06-10 23:33:34 PST -------
[NOTE: patches are being delivered in linked bugs.]

(In reply to comment #25)
> 
> > 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.

Thanks, doing it now.


> > 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);

DONE, see bug 88724.


> > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:376
> > +    insertBefore(textDisplayContainer.release(), m_enclosure, ec, true);
> 
> Ditto.

I only did the rename from m_panel to m_enclosure. Want me to fix this drive-by?


> > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:82
> > +    if (!hasSource(mediaElement) || !mediaElement->hasAudio() || mediaElement->muted() || mediaElement->volume() <= 0.0f)
> 
> "0.0f" should be simly "0".

DONE, see bug 88297.


> > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:85
> > +    if (mediaElement->volume() <= 0.33f)
> 
> The "f" is unnecessary.

DONE, see bug 88297.


> > Source/WebCore/rendering/RenderMediaControlsChromium.cpp:88
> > +    if (mediaElement->volume() <= 0.66f)
> 
> Ditto.

DONE, see bug 88297.


> > 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 .

Thanks, doing it from now on.
------- Comment #31 From 2012-06-11 09:47:47 PST -------
(In reply to comment #30)
> > > 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);
> 
> DONE, see bug 88724.
> 
> 
> > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:376
> > > +    insertBefore(textDisplayContainer.release(), m_enclosure, ec, true);
> > 
> > Ditto.
> 
> I only did the rename from m_panel to m_enclosure. Want me to fix this drive-by?
> 
Yes, I think you may as well fix it while you change the name.
------- Comment #32 From 2012-06-11 18:06:41 PST -------
(In reply to comment #31)
> (In reply to comment #30)
> > > > Source/WebCore/html/shadow/MediaControlRootElementChromium.cpp:376
> > > > +    insertBefore(textDisplayContainer.release(), m_enclosure, ec, true);
> > > 
> > > Ditto.
> > 
> > I only did the rename from m_panel to m_enclosure. Want me to fix this drive-by?
> > 
> Yes, I think you may as well fix it while you change the name.

DONE - it's in the cq? patch for bug 87683 .