Summary: | HTMLVideoElement::currentSrc() should return a KURL | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | WebKit Review Bot <webkit.review.bot> | ||||||
Component: | New Bugs | Assignee: | Adam Barth <abarth> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | abarth, commit-queue, darin, eric.carlson, eric | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
WebKit Review Bot
2011-05-26 16:29:46 PDT
Created attachment 95132 [details]
Patch
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 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. :) (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. :) Created attachment 95190 [details]
Patch for landing
Comment on attachment 95190 [details] Patch for landing Clearing flags on attachment: 95190 Committed r87539: <http://trac.webkit.org/changeset/87539> All reviewed patches have been landed. Closing bug. |