Bug 29161 – Implement default media UI for mac chromium.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Albert J. Wong]

The render theme code for mac chromium uses native mac widgets for media controls.  This is inconsistent with the chromium UI for other platforms.  Redo the render theme code in the style of the Windows and Linux ports.

[Albert J. Wong]

Created attachment 39393 [details]
Reimplement default media UI for Mac Chromium to match the style

of the Windows and Linux versions.  Also breaks the dependency
on the internal wk* functions that were previously used to
render the media controller widgets.
https://bugs.webkit.org/show_bug.cgi?id=29161

Patch by Albert J. Wong <[e-mail]> on 2009-09-10
Reviewed by NOBODY (OOPS!).

No media layout tests are currently enabled in Mac Chromium, so
nothing needs rebaselineing, etc.

* css/mediaControlsChromium.css:
* rendering/RenderThemeChromiumMac.h:
* rendering/RenderThemeChromiumMac.mm:
(WebCore::mediaElementParent):
(WebCore::RenderThemeChromiumMac::extraMediaControlsStyleSheet):
(WebCore::mediaSliderThumbImage):
(WebCore::mediaVolumeSliderThumbImage):
(WebCore::RenderThemeChromiumMac::paintSliderTrack):
(WebCore::RenderThemeChromiumMac::adjustSliderThumbSize):
(WebCore::RenderThemeChromiumMac::paintMediaButtonInternal):
(WebCore::RenderThemeChromiumMac::paintMediaPlayButton):
(WebCore::RenderThemeChromiumMac::paintMediaMuteButton):
(WebCore::RenderThemeChromiumMac::paintMediaSliderTrack):
(WebCore::RenderThemeChromiumMac::paintMediaVolumeSliderTrack):
(WebCore::RenderThemeChromiumMac::paintMediaSliderThumb):
(WebCore::RenderThemeChromiumMac::paintMediaVolumeSliderThumb):
(WebCore::RenderThemeChromiumMac::paintMediaControlsBackground):
* rendering/RenderThemeChromiumSkia.cpp:
(WebCore::RenderThemeChromiumSkia::adjustSliderThumbSize):
---
 5 files changed, 256 insertions(+), 99 deletions(-)

[Albert J. Wong]

Created attachment 39394 [details]
Reimplement default media UI for Mac Chromium to match the style

of the Windows and Linux versions.  Also breaks the dependency
on the internal wk* functions that were previously used to
render the media controller widgets.
https://bugs.webkit.org/show_bug.cgi?id=29161

Patch by Albert J. Wong <[e-mail]> on 2009-09-10
Reviewed by NOBODY (OOPS!).

No media layout tests are currently enabled in Mac Chromium, so
nothing needs rebaselineing, etc.

* css/mediaControlsChromium.css:
* rendering/RenderThemeChromiumMac.h:
* rendering/RenderThemeChromiumMac.mm:
(WebCore::mediaElementParent):
(WebCore::RenderThemeChromiumMac::extraMediaControlsStyleSheet):
(WebCore::mediaSliderThumbImage):
(WebCore::mediaVolumeSliderThumbImage):
(WebCore::RenderThemeChromiumMac::paintSliderTrack):
(WebCore::RenderThemeChromiumMac::adjustSliderThumbSize):
(WebCore::RenderThemeChromiumMac::paintMediaButtonInternal):
(WebCore::RenderThemeChromiumMac::paintMediaPlayButton):
(WebCore::RenderThemeChromiumMac::paintMediaMuteButton):
(WebCore::RenderThemeChromiumMac::paintMediaSliderTrack):
(WebCore::RenderThemeChromiumMac::paintMediaVolumeSliderTrack):
(WebCore::RenderThemeChromiumMac::paintMediaSliderThumb):
(WebCore::RenderThemeChromiumMac::paintMediaVolumeSliderThumb):
(WebCore::RenderThemeChromiumMac::paintMediaControlsBackground):
* rendering/RenderThemeChromiumSkia.cpp:
(WebCore::RenderThemeChromiumSkia::adjustSliderThumbSize):
---
 5 files changed, 255 insertions(+), 98 deletions(-)

[Albert J. Wong]

Created attachment 39395 [details]
Screenshot o new UI on white background.

[Albert J. Wong]

If this looks good, please also commit-queue+.

[Albert J. Wong]

Created attachment 39401 [details]
Reimplement default media UI for Mac Chromium to match the style

of the Windows and Linux versions.  Also breaks the dependency
on the internal wk* functions that were previously used to
render the media controller widgets.
https://bugs.webkit.org/show_bug.cgi?id=29161

Patch by Albert J. Wong <[e-mail]> on 2009-09-10
Reviewed by NOBODY (OOPS!).

No media layout tests are currently enabled in Mac Chromium, so
nothing needs rebaselineing, etc.

* css/mediaControlsChromium.css:
* rendering/RenderThemeChromiumMac.h:
* rendering/RenderThemeChromiumMac.mm:
(WebCore::mediaElementParent):
(WebCore::RenderThemeChromiumMac::extraMediaControlsStyleSheet):
(WebCore::mediaSliderThumbImage):
(WebCore::mediaVolumeSliderThumbImage):
(WebCore::RenderThemeChromiumMac::paintSliderTrack):
(WebCore::RenderThemeChromiumMac::adjustSliderThumbSize):
(WebCore::RenderThemeChromiumMac::paintMediaButtonInternal):
(WebCore::RenderThemeChromiumMac::paintMediaPlayButton):
(WebCore::RenderThemeChromiumMac::paintMediaMuteButton):
(WebCore::RenderThemeChromiumMac::paintMediaSliderTrack):
(WebCore::RenderThemeChromiumMac::paintMediaVolumeSliderTrack):
(WebCore::RenderThemeChromiumMac::paintMediaSliderThumb):
(WebCore::RenderThemeChromiumMac::paintMediaVolumeSliderThumb):
(WebCore::RenderThemeChromiumMac::paintMediaControlsBackground):
* rendering/RenderThemeChromiumSkia.cpp:
(WebCore::RenderThemeChromiumSkia::adjustSliderThumbSize):
---
 5 files changed, 252 insertions(+), 98 deletions(-)

[Albert J. Wong]

FYI, [e-mail] also looked this over and thinks it is sane. He did default UI implementation for Windows/Linux.

[Eric Seidel (no email)]

You can always set cq=? if you want folks to know to cq+ when reviewing.

[Albert J. Wong]

Ah, didn't realize that.  cq=? set.

[David Levin]

Comment on attachment 39401 [details]
Reimplement default media UI for Mac Chromium to match the style


Just a few nits to fix up and then commit.

> diff --git a/WebCore/rendering/RenderThemeChromiumMac.mm b/WebCore/rendering/RenderThemeChromiumMac.mm

> +#if ENABLE(VIDEO)
> +// Attempt to retrieve a HTMLMediaElement from a Node. Returns NULL if one cannot be found.

Use 0 instead of NULL. check-webkit-style would have flagged this :)


> +    static Image* soundFull = Image::loadPlatformResource("mediaSoundFull").releaseRef();
> +    static Image* soundNone = Image::loadPlatformResource("mediaSoundNone").releaseRef();
> +    static Image* soundDisabled = Image::loadPlatformResource("mediaSoundDisabled").releaseRef();
> +
> +    if (mediaElement->networkState() == HTMLMediaElement::NETWORK_NO_SOURCE || !mediaElement->hasAudio())
> +        return paintMediaButtonInternal(paintInfo.context, rect, soundDisabled);
> +
> +    return paintMediaButtonInternal(paintInfo.context, rect, mediaElement->muted() ? soundNone: soundFull);

Add a space before the :


> +    if (bufferedRect.width() > 0 && bufferedRect.height() > 0) {
> +        FloatPoint p0 = bufferedRect.location();
> +        FloatPoint p1 = p0;

Can you think of more meaningful names than p0 and p1? (sliderTop, sliderBottom?)

[Albert J. Wong]

Committed r48438: <http://trac.webkit.org/changeset/48438>

[Brett Wilson (Google)]

I rolled back this change because it did not compile.

WebCore/rendering/RenderThemeChromiumMac.mm: In member function 'virtual bool WebCore::RenderThemeChromiumMac::paintMediaSliderTrack(WebCore::RenderObject*, const WebCore::RenderObject::PaintInfo&, const WebCore::IntRect&)':
/b/slave/webkit-rel-mac-webkit-org/build/src/webkit/../third_party/WebKit/WebCore/rendering/RenderThemeChromiumMac.mm:1966: error: 'p1' was not declared in this scope

[Eric Seidel (no email)]

Comment on attachment 39401 [details]
Reimplement default media UI for Mac Chromium to match the style

Marking this change as r- since it was rolled out.

[Albert J. Wong]

Resolving bug since it has been committed again.