[Webkit-unassigned] [Bug 34631] [Qt] Switching from Phonon to QtMultimedia Backend for Qt 4.7

[bugzilla-daemon at webkit.org]

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





--- Comment #13 from Simon Hausmann <hausmann at webkit.org>  2010-02-10 14:32:51 PST ---
(From update of attachment 48460)
Nicholas, I'm not so familiar with this code myself, I think Tor Arne will be a
better reviewer. But there are a few things I can give feedback on:

> -# Disable HTML5 media compilation if phonon is unavailable
> -!contains(DEFINES, ENABLE_VIDEO=1) {
> -    !contains(QT_CONFIG, phonon) {
> -        DEFINES -= ENABLE_VIDEO=1
> -        DEFINES += ENABLE_VIDEO=0
> +# QtMultimedia since 4.7
> +greaterThan(QT_MINOR_VERSION, 6) {
> +    # Disable HTML5 media compilation if qtmultimedia is unavailable
> +    !contains(DEFINES, ENABLE_VIDEO=1) {
> +        !contains(QT_CONFIG, multimedia) {
> +            DEFINES -= ENABLE_VIDEO=1
> +            DEFINES += ENABLE_VIDEO=0
> +        }
> +    }
> +} else {
> +    # Disable HTML5 media compilation if phonon is unavailable
> +    !contains(DEFINES, ENABLE_VIDEO=1) {
> +        !contains(QT_CONFIG, phonon) {
> +            DEFINES -= ENABLE_VIDEO=1
> +            DEFINES += ENABLE_VIDEO=0
> +        }
>      }
>  }

Just a thought, but perhaps the logic of the above checks would be
simpler if reversed, i.e. _enable_ video only if we have either phonon
available or 4.7 and multimedia.

> +void MediaPlayerPrivate::load(const String& url)
[...]
> +    const QUrl rUrl(url);

I hope that conversion is safe. An alternate approach, slower but safer would
be something
along the lines of

const QUrl rUrl(KURL(ParsedURLString, url));

> +
> +            // Set the refferer
> +            request.setRawHeader("Refferer", document->documentURI().utf8().data());

That looks like a typo to me, shouldn't it be Referrer with only one f? :)

That said, there's another rule that's important to obey for referrers: When an
https site requests a non-http resources, the referrer should not be set. See
QNetworkReplyHandler.cpp.

Perhaps there's a way to re-use some more code from the networking layer here,
just a thought :)

> +MediaPlayerPrivateDefault::MediaPlayerPrivateDefault(MediaPlayer *player)

Coding style nitpick: The * position should be to the left.

> +    : MediaPlayerPrivate(player)
> +    , m_videoSurface(new QPainterVideoSurface(m_mediaPlayer))
> +{
> +    if (m_mediaPlayer) {

Is it really possible that m_mediaPlayer is null?

> +void MediaPlayerPrivateDefault::paint(GraphicsContext* context, const IntRect& rect)
> +{
> +    if (context->paintingDisabled())
> +        return;
> +
> +    if (!m_isVisible)
> +        return;
> +
> +    // Grab the painter
> +    QPainter* painter = context->platformContext();
> +
> +    if (!m_isActive) {
> +        // Attempt GL Setup
> +#if !defined(QT_NO_OPENGL) && !defined(QT_OPENGL_ES_1_CL) && !defined(QT_OPENGL_ES_1)
> +        if ((painter->paintEngine()->type() == QPaintEngine::OpenGL
> +             || painter->paintEngine()->type() == QPaintEngine::OpenGL2)) {
> +            // Set GL Context
> +            m_videoSurface->setGLContext(const_cast<QGLContext *>(QGLContext::currentContext()));

Style nitpick: No space before * here.


> +
> +class QVideoSurfacePainter {
> +public:

If this class is here only temporarily, then it would be best to add a comment
here
explaining that it'll be removed in the future. That would also help to
understand why
the coding style is different.


> +
> +class QVideoSurfacePainter;
> +class Q_AUTOTEST_EXPORT QPainterVideoSurface : public QAbstractVideoSurface {

There shouldn't be a Q_AUTOTEST_EXPORT macro here, not when building inside
WebKit.


> +bool RenderThemeQt::paintMediaCurrentTime(RenderObject* o, const RenderObject::PaintInfo& paintInfo, const IntRect& r)
> +{
> +    StylePainter p(this, paintInfo);
> +    if (!p.isValid())
> +        return true;
> +
> +    if (o->style()) {
> +        // Set the text color depending on platform
> +        QPalette pal = QApplication::palette();

I think it would be better to use setPaletteFromPageClientIfExists instead of
the application global palette.


> +    static const QColor highlightText = QApplication::palette().brush(QPalette::Active, QPalette::HighlightedText).color();
> +    static const QColor scaleColor(highlightText.red(), highlightText.green(), highlightText.blue(), mediaControlsBaselineOpacity() * 255);

I'd prefer if these weren't global variables but taken from the page client's
palette instead.


> +    #if QT_VERSION >= 0x040700
> +    if (MediaPlayer* player = mediaElement->player()) {
> +        // Get the buffered parts of the media
> +        PassRefPtr<TimeRanges> buffered = player->buffered();
> +        if (buffered->length() > 0 && player->duration() < std::numeric_limits<float>::infinity()) {
> +            // Set the transform and brush
> +            WorldMatrixTransformer transformer(p.painter, o, r);
> +            p.painter->setBrush(getMediaControlForegroundColor());
> +
> +            // Paint each buffered section
> +            ExceptionCode ex;
> +            for (int i = 0; i < buffered->length(); i++) {
> +                float startX = (buffered->start(i, ex) / player->duration()) * 100;
> +                float width = ((buffered->end(i, ex) / player->duration()) * 100) - startX;
> +                p.painter->drawRect(startX, 37, width, 26);
> +            }
> +        }
> +    }
> +    #endif

Usually preprocessor statements begin at the beginning of the line :)


How does the new RenderTheme media controls part affect the existing Phonon
backend?

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