[webkit-reviews] review denied: [Bug 28689] Replace disabled media mute button with port-specific implementation. : [Attachment 39900] Second attempt

[bugzilla-daemon at webkit.org]

Eric Carlson <eric.carlson at apple.com> has denied Andrew Scherkus
<scherkus at chromium.org>'s request for review:
Bug 28689: Replace disabled media mute button with port-specific
implementation.
https://bugs.webkit.org/show_bug.cgi?id=28689

Attachment 39900: Second attempt
https://bugs.webkit.org/attachment.cgi?id=39900&action=review

------- Additional Comments from Eric Carlson <eric.carlson at apple.com>

> +	   Sample implementation for port-specific rendering of media controls.


"Sample implementation", what do you mean?


> +    // Alternatively we could pass in the theme for fine-tuned adjustment. 
If
> +    // we keep it as simply setting display to NONE then I might rename this
method
> +    // shouldDisplayMediaControlPart instead.
> +    if (!renderer->theme()->shouldRenderMediaControlPart(renderer))
> +	   style->setDisplay(NONE);

This shouldn't be necessary. I think if have both MediaControlElement::
rendererIsNeeded and MediaControlInputElement:: rendererIsNeeded call the theme

method it should just work.


>  bool MediaControlMuteButtonElement::rendererIsNeeded(RenderStyle* style)
>  {
> -    return MediaControlInputElement::rendererIsNeeded(style) && !disabled();

> +    // All subclasses will probably end up NOT implementing this method
anymore.
> +    return MediaControlInputElement::rendererIsNeeded(style);

Why not just remove the method completely?


> +bool RenderTheme::shouldRenderMediaControlPart(RenderObject* o)
> +{
> +    HTMLMediaElement* mediaElement =
static_cast<MediaControlInputElement*>(o->node())->mediaElement();
> +    switch (o->style()->appearance()) {
> +    case MediaMuteButtonPart:
> +	   return mediaElement->hasAudio();
> +    // More media parts can go here, but then we end up moving localized
logic
> +    // from the MediaControl elements into a big switch statement here,
which
> +    // kind of sucks.

"which kind of sucks" is probably not the most helpful comment to someone
coming along later.