Bug 79236 - [GStreamer] webkitwebsrc: use HTTP referer provided by MediaPlayer
Summary: [GStreamer] webkitwebsrc: use HTTP referer provided by MediaPlayer
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philippe Normand
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-02-22 06:40 PST by Philippe Normand
Modified: 2012-02-22 11:12 PST (History)
1 user (show)

See Also:


Attachments
Patch (5.04 KB, patch)
2012-02-22 06:55 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (5.31 KB, patch)
2012-02-22 06:59 PST, Philippe Normand
no flags Details | Formatted Diff | Diff
Patch (5.80 KB, patch)
2012-02-22 11:02 PST, Philippe Normand
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philippe Normand 2012-02-22 06:40:59 PST
MediaPlayer::referrer() was recently added. We should use it.
Comment 1 Philippe Normand 2012-02-22 06:55:32 PST
Created attachment 128205 [details]
Patch
Comment 2 Philippe Normand 2012-02-22 06:59:05 PST
Created attachment 128207 [details]
Patch
Comment 3 Martin Robinson 2012-02-22 08:50:09 PST
Comment on attachment 128207 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=128207&action=review

Looks fine. What caused you to make this change? Is it tracked by any tests? r- for the minor fixes below.

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:358
>          priv->frame.release();

This should be priv->frame.clear()

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:427
> +        if (priv->player)
> +            request.setHTTPReferrer(priv->player->referrer());
> +

It seems this should be outside of the if block now.

I think you can simplify this a bit to be:
FrameLoader* loader = priv->frame ? priv->frame->loader() : 0;
if (loader) {
    ....
}

if (priv->player)
    request.setHTTPReferrer(priv->player->referrer());

> Source/WebCore/platform/graphics/gstreamer/WebKitWebSourceGStreamer.cpp:752
> +

Extra newline here
Comment 4 Martin Robinson 2012-02-22 08:51:05 PST
(In reply to comment #3)

> Looks fine. What caused you to make this change? Is it tracked by any tests? r- for the minor fixes below.

Oh, I see now that ::refererer() is a recent addition.
Comment 5 Philippe Normand 2012-02-22 10:57:49 PST
(In reply to comment #4)
> (In reply to comment #3)
> 
> > Looks fine. What caused you to make this change? Is it tracked by any tests? r- for the minor fixes below.
> 
> Oh, I see now that ::refererer() is a recent addition.

Yes, we had code to deal with it already but I believe it makes more sense to use this new method of the player :)
Comment 6 Philippe Normand 2012-02-22 11:02:53 PST
Created attachment 128249 [details]
Patch
Comment 7 Philippe Normand 2012-02-22 11:04:06 PST
The test that should still pass is http/tests/media/video-referer.html
Comment 8 Philippe Normand 2012-02-22 11:12:39 PST
Committed r108524: <http://trac.webkit.org/changeset/108524>