2011-05-27 Adam Barth <abarth@webkit.org>

        Reviewed by Eric Seidel.

        HTMLVideoElement::currentSrc() should return a KURL
        https://bugs.webkit.org/show_bug.cgi?id=61578

        I suspect we got into this mess because the author of this code didn't
        know about the URL attribute in WebKit IDL, which is super useful!

        Bad news: The line of code in question seems to have another bug, which
        I've documented in a FIXME.  Let the yak shaving continue!

        * html/HTMLMediaElement.cpp:
        (WebCore::urlForLogging):
        (WebCore::HTMLMediaElement::loadResource):
        (WebCore::HTMLMediaElement::isSafeToLoadURL):
        (WebCore::HTMLMediaElement::selectNextSourceChild):
        (WebCore::HTMLMediaElement::getPluginProxyParams):
        * html/HTMLMediaElement.h:
        (WebCore::HTMLMediaElement::currentSrc):
        (WebCore::HTMLMediaElement::currentURL):
        * html/canvas/CanvasRenderingContext.cpp:
        (WebCore::CanvasRenderingContext::checkOrigin):
        * rendering/HitTestResult.cpp:
        (WebCore::HitTestResult::absoluteMediaURL):
            - This complete URL call was unnecessary because currentSrc is
              already absolute.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@87539 268f45cc-cd09-0410-ab3c-d52691b4dbfc

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
index 4889025..f226728 100644 (file)
--- a/Source/WebCore/ChangeLog
+++ b/Source/WebCore/ChangeLog
@@ -1,3 +1,32 @@
+2011-05-27  Adam Barth  <abarth@webkit.org>
+
+        Reviewed by Eric Seidel.
+
+        HTMLVideoElement::currentSrc() should return a KURL
+        https://bugs.webkit.org/show_bug.cgi?id=61578
+
+        I suspect we got into this mess because the author of this code didn't
+        know about the URL attribute in WebKit IDL, which is super useful!
+
+        Bad news: The line of code in question seems to have another bug, which
+        I've documented in a FIXME.  Let the yak shaving continue!
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::urlForLogging):
+        (WebCore::HTMLMediaElement::loadResource):
+        (WebCore::HTMLMediaElement::isSafeToLoadURL):
+        (WebCore::HTMLMediaElement::selectNextSourceChild):
+        (WebCore::HTMLMediaElement::getPluginProxyParams):
+        * html/HTMLMediaElement.h:
+        (WebCore::HTMLMediaElement::currentSrc):
+        (WebCore::HTMLMediaElement::currentURL):
+        * html/canvas/CanvasRenderingContext.cpp:
+        (WebCore::CanvasRenderingContext::checkOrigin):
+        * rendering/HitTestResult.cpp:
+        (WebCore::HitTestResult::absoluteMediaURL):
+            - This complete URL call was unnecessary because currentSrc is
+              already absolute.
+
 2011-05-27  Mikhail Naganov  <mnaganov@chromium.org>
 
         Reviewed by Pavel Feldman.

diff --git a/Source/WebCore/html/HTMLMediaElement.cpp b/Source/WebCore/html/HTMLMediaElement.cpp
index 9b5e35b..68029ff 100644 (file)
--- a/Source/WebCore/html/HTMLMediaElement.cpp
+++ b/Source/WebCore/html/HTMLMediaElement.cpp
@@ -85,16 +85,16 @@ using namespace std;
 namespace WebCore {
 
 #if !LOG_DISABLED
-static String urlForLogging(const String& url)
+static const char* urlForLogging(const KURL& url)
 {
     static const unsigned maximumURLLengthForLogging = 128;
 
-    if (url.length() < maximumURLLengthForLogging)
-        return url;
-    return url.substring(0, maximumURLLengthForLogging) + "...";
+    if (url.string().length() < maximumURLLengthForLogging)
+        return url.string().utf8().data();
+    return String(url.string().substring(0, maximumURLLengthForLogging) + "...").utf8().data();
 }
 
-static const char *boolString(bool val)
+static const char* boolString(bool val)
 {
     return val ? "true" : "false";
 }
@@ -457,11 +457,6 @@ void HTMLMediaElement::setSrc(const String& url)
     setAttribute(srcAttr, url);
 }
 
-String HTMLMediaElement::currentSrc() const
-{
-    return m_currentSrc;
-}
-
 HTMLMediaElement::NetworkState HTMLMediaElement::networkState() const
 {
     return m_networkState;
@@ -680,7 +675,7 @@ void HTMLMediaElement::loadResource(const KURL& initialURL, ContentType& content
 {
     ASSERT(isSafeToLoadURL(initialURL, Complain));
 
-    LOG(Media, "HTMLMediaElement::loadResource(%s, %s)", urlForLogging(initialURL.string()).utf8().data(), contentType.raw().utf8().data());
+    LOG(Media, "HTMLMediaElement::loadResource(%s, %s)", urlForLogging(initialURL), contentType.raw().utf8().data());
 
     Frame* frame = document()->frame();
     if (!frame)
@@ -695,7 +690,7 @@ void HTMLMediaElement::loadResource(const KURL& initialURL, ContentType& content
 
     m_currentSrc = url;
 
-    LOG(Media, "HTMLMediaElement::loadResource - m_currentSrc -> %s", urlForLogging(m_currentSrc).utf8().data());
+    LOG(Media, "HTMLMediaElement::loadResource - m_currentSrc -> %s", urlForLogging(m_currentSrc));
 
     if (m_sendProgressEvents) 
         startProgressEventTimer();
@@ -712,7 +707,7 @@ void HTMLMediaElement::loadResource(const KURL& initialURL, ContentType& content
     m_player->setPreservesPitch(m_webkitPreservesPitch);
     updateVolume();
 
-    m_player->load(m_currentSrc, contentType);
+    m_player->load(m_currentSrc.string(), contentType);
 
     // If there is no poster to display, allow the media engine to render video frames as soon as
     // they are available.
@@ -725,7 +720,7 @@ void HTMLMediaElement::loadResource(const KURL& initialURL, ContentType& content
 bool HTMLMediaElement::isSafeToLoadURL(const KURL& url, InvalidSourceAction actionIfInvalid)
 {
     if (!url.isValid()) {
-        LOG(Media, "HTMLMediaElement::isSafeToLoadURL(%s) -> FALSE because url is invalid", urlForLogging(url.string()).utf8().data());
+        LOG(Media, "HTMLMediaElement::isSafeToLoadURL(%s) -> FALSE because url is invalid", urlForLogging(url));
         return false;
     }
 
@@ -733,7 +728,7 @@ bool HTMLMediaElement::isSafeToLoadURL(const KURL& url, InvalidSourceAction acti
     if (!frame || !document()->securityOrigin()->canDisplay(url)) {
         if (actionIfInvalid == Complain)
             FrameLoader::reportLocalLoadFailed(frame, url.string());
-        LOG(Media, "HTMLMediaElement::isSafeToLoadURL(%s) -> FALSE rejected by SecurityOrigin", urlForLogging(url.string()).utf8().data());
+        LOG(Media, "HTMLMediaElement::isSafeToLoadURL(%s) -> FALSE rejected by SecurityOrigin", urlForLogging(url));
         return false;
     }
 
@@ -1736,7 +1731,7 @@ KURL HTMLMediaElement::selectNextSourceChild(ContentType *contentType, InvalidSo
         mediaURL = source->getNonEmptyURLAttribute(srcAttr);
 #if !LOG_DISABLED
         if (shouldLog)
-            LOG(Media, "HTMLMediaElement::selectNextSourceChild - 'src' is %s", urlForLogging(mediaURL).utf8().data());
+            LOG(Media, "HTMLMediaElement::selectNextSourceChild - 'src' is %s", urlForLogging(mediaURL));
 #endif
         if (mediaURL.isEmpty())
             goto check_again;
@@ -1787,7 +1782,7 @@ check_again:
 
 #if !LOG_DISABLED
     if (shouldLog)
-        LOG(Media, "HTMLMediaElement::selectNextSourceChild -> %p, %s", m_currentSourceNode, canUse ? urlForLogging(mediaURL.string()).utf8().data() : "");
+        LOG(Media, "HTMLMediaElement::selectNextSourceChild -> %p, %s", m_currentSourceNode, canUse ? urlForLogging(mediaURL) : "");
 #endif
     return canUse ? mediaURL : KURL();
 }
@@ -1799,7 +1794,7 @@ void HTMLMediaElement::sourceWasAdded(HTMLSourceElement* source)
 #if !LOG_DISABLED
     if (source->hasTagName(sourceTag)) {
         KURL url = source->getNonEmptyURLAttribute(srcAttr);
-        LOG(Media, "HTMLMediaElement::sourceWasAdded - 'src' is %s", urlForLogging(url).utf8().data());
+        LOG(Media, "HTMLMediaElement::sourceWasAdded - 'src' is %s", urlForLogging(url));
     }
 #endif
     
@@ -1846,7 +1841,7 @@ void HTMLMediaElement::sourceWillBeRemoved(HTMLSourceElement* source)
 #if !LOG_DISABLED
     if (source->hasTagName(sourceTag)) {
         KURL url = source->getNonEmptyURLAttribute(srcAttr);
-        LOG(Media, "HTMLMediaElement::sourceWillBeRemoved - 'src' is %s", urlForLogging(url).utf8().data());
+        LOG(Media, "HTMLMediaElement::sourceWillBeRemoved - 'src' is %s", urlForLogging(url));
     }
 #endif
 
@@ -2450,10 +2445,10 @@ void HTMLMediaElement::getPluginProxyParams(KURL& url, Vector<String>& names, Ve
     if (!isSafeToLoadURL(url, Complain))
         url = selectNextSourceChild(0, DoNothing);
 
-    m_currentSrc = url.string();
+    m_currentSrc = url;
     if (url.isValid() && frame && frame->loader()->willLoadMediaElementURL(url)) {
         names.append("_media_element_src_");
-        values.append(m_currentSrc);
+        values.append(m_currentSrc.string());
     }
 }
 

diff --git a/Source/WebCore/html/HTMLMediaElement.h b/Source/WebCore/html/HTMLMediaElement.h
index 7e0b81d..bdb8df0 100644 (file)
--- a/Source/WebCore/html/HTMLMediaElement.h
+++ b/Source/WebCore/html/HTMLMediaElement.h
@@ -86,7 +86,7 @@ public:
 
 // network state
     void setSrc(const String&);
-    String currentSrc() const;
+    const KURL& currentSrc() const { return m_currentSrc; }
 
     enum NetworkState { NETWORK_EMPTY, NETWORK_IDLE, NETWORK_LOADING, NETWORK_NO_SOURCE };
     NetworkState networkState() const;
@@ -347,8 +347,8 @@ private:
     NetworkState m_networkState;
     ReadyState m_readyState;
     ReadyState m_readyStateMaximum;
-    String m_currentSrc;
-    
+    KURL m_currentSrc;
+
     RefPtr<MediaError> m_error;
 
     float m_volume;

diff --git a/Source/WebCore/html/HTMLMediaElement.idl b/Source/WebCore/html/HTMLMediaElement.idl
index c7e6b07..0bca569 100644 (file)
--- a/Source/WebCore/html/HTMLMediaElement.idl
+++ b/Source/WebCore/html/HTMLMediaElement.idl
@@ -31,7 +31,7 @@ interface [Conditional=VIDEO] HTMLMediaElement : HTMLElement {
 
     // network state
     attribute [Reflect, URL] DOMString src;
-    readonly attribute DOMString currentSrc;
+    readonly attribute [URL] DOMString currentSrc;
     
     const unsigned short NETWORK_EMPTY = 0;
     const unsigned short NETWORK_IDLE = 1;

diff --git a/Source/WebCore/html/canvas/CanvasRenderingContext.cpp b/Source/WebCore/html/canvas/CanvasRenderingContext.cpp
index 701a492..7ba6c02 100644 (file)
--- a/Source/WebCore/html/canvas/CanvasRenderingContext.cpp
+++ b/Source/WebCore/html/canvas/CanvasRenderingContext.cpp
@@ -69,9 +69,11 @@ void CanvasRenderingContext::checkOrigin(const HTMLImageElement* image)
 void CanvasRenderingContext::checkOrigin(const HTMLVideoElement* video)
 {
 #if ENABLE(VIDEO)
-    // FIXME: HTMLVideoElement::currentSrc() should return a KURL.
-    // https://bugs.webkit.org/show_bug.cgi?id=61578
-    checkOrigin(KURL(ParsedURLString, video->currentSrc()));
+    // FIXME: This check is likely wrong when a redirect is involved. We need
+    // to test the finalURL. Please be careful when fixing this issue not to
+    // make currentSrc be the final URL because then the
+    // HTMLMediaElement.currentSrc DOM API would leak redirect destinations!
+    checkOrigin(video->currentSrc());
     if (canvas()->originClean() && video && !video->hasSingleSecurityOrigin())
         canvas()->setOriginTainted();
 #endif

diff --git a/Source/WebCore/rendering/HitTestResult.cpp b/Source/WebCore/rendering/HitTestResult.cpp
index 47f7a51..6330943 100644 (file)
--- a/Source/WebCore/rendering/HitTestResult.cpp
+++ b/Source/WebCore/rendering/HitTestResult.cpp
@@ -312,7 +312,7 @@ KURL HitTestResult::absoluteMediaURL() const
 {
 #if ENABLE(VIDEO)
     if (HTMLMediaElement* mediaElt = mediaElement())
-        return m_innerNonSharedNode->document()->completeURL(stripLeadingAndTrailingHTMLSpaces(mediaElt->currentSrc()));
+        return mediaElt->currentSrc();
     return KURL();
 #else
     return KURL();