[webkit-reviews] review denied: [Bug 84672] Chrome video controls visual update : [Attachment 142924] On Linux only pass image tests in test_expectations

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Silvia Pfeiffer
<silviapf at chromium.org>'s request for review:
Bug 84672: Chrome video controls visual update
https://bugs.webkit.org/show_bug.cgi?id=84672

Attachment 142924: On Linux only pass image tests in test_expectations
https://bugs.webkit.org/attachment.cgi?id=142924&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>
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::MediaControlChromiumEnclosureEl
ement):
> +	   (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 .