[Webkit-unassigned] [Bug 84672] Chrome video controls visual update

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=84672





--- Comment #4 from Eric Carlson <eric.carlson at apple.com>  2012-04-24 11:37:50 PST ---
(From update of attachment 138510)
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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.