[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 #14 from Nick Young <nicholas.young at nokia.com>  2010-02-10 18:22:18 PST ---
(In reply to comment #13)
> 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:

Your partial review is still much appreciated :)

> 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.

Yep. Good Point.

> 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));

On this, I will need more convincing. My question would be this: where is it
said that KURL is better at parsing than QUrl?

In either case, the url parameter passed to this method has already been parsed
by KURL and checked by security policies, before being converted back to a
String. See HTMLMediaElement::loadResource().

Aside: the Media Player interface should probably specify a KURL parameter for
this load method, rather than a String.

Bottom line: I don't see how this conversion could ever be unsafe.
Perhaps removing the implicit conversion would be sufficient:

const QUrl rUrl(QString(url));

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

It should be "Referer". Neither of us was right ;)

> 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 :)

I had a look at the file you specified, as well as some of the related
networking code. Looks like I could replace this entire block of code
with:

    // Construct the media content with a network request if the resource is
http[s]
    if (scheme == "http" || scheme == "https") {
        const QNetworkRequest request =
ResourceRequest(rurl).toNetworkRequest();
        m_mediaPlayer->setMedia(QMediaContent(request));
    } else {
        // Otherwise, just use the URL
        m_mediaPlayer->setMedia(QMediaContent(rUrl));
    }

Would that work?
I certainly appreciate your guidance here.


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

Fixed :)

> Is it really possible that m_mediaPlayer is null?

Yeah, this check is probably unnecessary.
Technically, if "new QMediaPlayer" fails, this could happen - but new rarely
fails, even when out of memory.

I've removed the check in both of the places where it appears :)

> Style nitpick: No space before * here.

Yep.

> 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.

Done.

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

Whoops. I missed that when moving that class in to Webkit. Removed.

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

Sure.

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

Yep.

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

Indeed they do. Fixed.

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

The phonon backend gains a current time display and a functional volume
control. The controls have been tested for both backends :)

That part in RenderThemeQt which has a version guard is to account for the fact
that the phonon backend neither implements the buffered() method nor the
maxTimeSeekable() method. If you would prefer, I could try and implement one of
those methods on the phonon backend so that the media controls are on a truly
equal footing.

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