Bug 61578 - HTMLVideoElement::currentSrc() should return a KURL
Summary: HTMLVideoElement::currentSrc() should return a KURL
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-05-26 16:29 PDT by WebKit Review Bot
Modified: 2011-05-27 12:54 PDT (History)
5 users (show)

See Also:

Attachments
Patch (8.18 KB, patch)
2011-05-27 00:43 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Patch for landing (9.89 KB, patch)
2011-05-27 10:51 PDT, Adam Barth 		no flags Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Review Bot 2011-05-26 16:29:46 PDT 
HTMLVideoElement::currentSrc() should return a KURL
Requested by abarth on #webkit.
Comment 1 Adam Barth 2011-05-27 00:43:52 PDT 
Created attachment 95132 [details]
Patch
Comment 2 Alexis Menard (darktears) 2011-05-27 05:11:44 PDT 
Comment on attachment 95132 [details]
Patch

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

> Source/WebCore/html/canvas/CanvasRenderingContext.cpp:75
> +    // HTMLMediaElement.currentSrc DOM API would leak redirect detinations!

Typo here.

> Source/WebCore/rendering/HitTestResult.cpp:315
> +        return mediaElt->currentSrc();

I believe this is the same code because currentSrc has already his baseURL set.
Comment 3 Eric Seidel 2011-05-27 07:00:12 PDT 
Comment on attachment 95132 [details]
Patch

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

> Source/WebCore/html/HTMLMediaElement.cpp:678
> +    LOG(Media, "HTMLMediaElement::loadResource(%s, %s)", urlForLogging(initialURL).utf8().data(), contentType.raw().utf8().data());

Ha.  Seems urlForLogging should return a char*...

>> Source/WebCore/rendering/HitTestResult.cpp:315
>> +        return mediaElt->currentSrc();
> 
> I believe this is the same code because currentSrc has already his baseURL set.

Is this tested anywhere?  How do I as a reviewer know it's already absolute. :)
Comment 4 Adam Barth 2011-05-27 10:45:34 PDT 
(In reply to comment #3)
> (From update of attachment 95132 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95132&action=review
> 
> > Source/WebCore/html/HTMLMediaElement.cpp:678
> > +    LOG(Media, "HTMLMediaElement::loadResource(%s, %s)", urlForLogging(initialURL).utf8().data(), contentType.raw().utf8().data());
> 
> Ha.  Seems urlForLogging should return a char*...

Very true.

> >> Source/WebCore/rendering/HitTestResult.cpp:315
> >> +        return mediaElt->currentSrc();
> > 
> > I believe this is the same code because currentSrc has already his baseURL set.
> 
> Is this tested anywhere?  How do I as a reviewer know it's already absolute. :)

I'm not sure whether this is tested, but AFAIK the KURL type only holds absolute URLs, so the C++ type system (such as it is) can tell you that.  :)
Comment 5 Adam Barth 2011-05-27 10:51:56 PDT 
Created attachment 95190 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2011-05-27 12:53:59 PDT 
Comment on attachment 95190 [details]
Patch for landing

Clearing flags on attachment: 95190

Committed r87539: <http://trac.webkit.org/changeset/87539>
Comment 7 WebKit Commit Bot 2011-05-27 12:54:04 PDT 
All reviewed patches have been landed.  Closing bug.