Bug 84672 – [META] Chrome video controls visual update

• Status: RESOLVED
• Resolution: FIXED
• Component: Media

[Silvia Pfeiffer]

Chrome video controls visual update

[Silvia Pfeiffer]

Created attachment 138506 [details]
Patch

[WebKit Review Bot]

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.

[Silvia Pfeiffer]

Created attachment 138510 [details]
removed No new tests line

[Eric Carlson]

Comment on attachment 138510 [details]
removed No new tests line

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.

[WebKit Review Bot]

Comment on attachment 138510 [details]
removed No new tests line

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

[WebKit Review Bot]

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

[Silvia Pfeiffer]

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.

[Silvia Pfeiffer]

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

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.

[Silvia Pfeiffer]

Created attachment 140240 [details]
Patch - not to be committed before 7 May

[WebKit Review Bot]

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.

[WebKit Review Bot]

Comment on attachment 140240 [details]
Patch - not to be committed before 7 May

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

[WebKit Review Bot]

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

[Dominic Cooney]

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
> [phone]  Silvia Pfeiffer  <[e-mail]>

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?

[Silvia Pfeiffer]

(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
> > [phone]  Silvia Pfeiffer  <[e-mail]>
> 
> 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.

[Silvia Pfeiffer]

Created attachment 142379 [details]
Includes shadowPseudoId fix

Includes shadowPseudoId fix and feedback

[WebKit Review Bot]

Comment on attachment 142379 [details]
Includes shadowPseudoId fix

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

[WebKit Review Bot]

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

[Silvia Pfeiffer]

Created attachment 142662 [details]
Fixed Linux tests, removed other tests

corrected test_expectations

[WebKit Review Bot]

Comment on attachment 142662 [details]
Fixed Linux tests, removed other tests

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

[WebKit Review Bot]

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

[Silvia Pfeiffer]

Created attachment 142862 [details]
Flexbox changed in https://bugs.webkit.org/show_bug.cgi?id=86529

updated flexbox use

[WebKit Review Bot]

Comment on attachment 142862 [details]
Flexbox changed in https://bugs.webkit.org/show_bug.cgi?id=86529

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

[WebKit Review Bot]

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

[Silvia Pfeiffer]

Created attachment 142924 [details]
On Linux only pass image tests in test_expectations

Fail IMAGE+TEXT or TEXT only test expectations

[Eric Carlson]

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 .

[Silvia Pfeiffer]

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

[Eric Carlson]

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

[Silvia Pfeiffer]

(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?

[Eric Carlson]

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

[Silvia Pfeiffer]

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

[Eric Carlson]

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

[Silvia Pfeiffer]

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