2010-04-14 Aaron Boodman <aa@chromium.org>

Reviewed by David Levin. Support relative URLs for notifications on Chromium. They weren't working previously because WebCore was inserting the relative URL into a KURL instance, but when KURL is backed by GURL as it is on Chromium, relative URLs are unsupported. Fixed by resolving the relative URL first. https://bugs.webkit.org/show_bug.cgi?id=36623 Adding tests for this is difficult because we don't currently have DRT support for notifications on Mac, only Windows. * notifications/Notification.cpp: (WebCore::Notification::Notification): Accept resolved KURL instead of relative string. * notifications/Notification.h: (WebCore::Notification::create): Ditto. (WebCore::Notification::iconURL): Return resolved KURL instead of relative string. * notifications/NotificationCenter.h: (WebCore::NotificationCenter::createHTMLNotification): Immediately resolve URL instead of passing off relative string. (WebCore::NotificationCenter::createNotification): Ditto. * notifications/NotificationContents.h: (WebCore::NotificationContents::NotificationContents): Accept resolved KURL instead of relative string. (WebCore::NotificationContents::icon): Return resolved URL. 2010-04-14 Aaron Boodman <aa@chromium.org> Reviewed by David Levin. Support relative URLs for notifications on Chromium. They weren't working previously because WebCore was inserting the relative URL into a KURL instance, but when KURL is backed by GURL as it is on Chromium, relative URLs are unsupported. Fixed by resolving the relative URL first. https://bugs.webkit.org/show_bug.cgi?id=36623 Adding tests for this is difficult because we don't currently have DRT support for notifications on Mac, only Windows. * public/WebNotification.h: Remove deprecated icon() method. * src/WebNotification.cpp: Ditto. 2010-04-14 Aaron Boodman <aa@chromium.org> Reviewed by David Levin. Support relative URLs for notifications on Chromium. They weren't working previously because WebCore was inserting the relative URL into a KURL instance, but when KURL is backed by GURL as it is on Chromium, relative URLs are unsupported. Fixed by resolving the relative URL first. https://bugs.webkit.org/show_bug.cgi?id=36623 Adding tests for this is difficult because we don't currently have DRT support for notifications on Mac, only Windows. * WebCoreSupport/NotificationPresenterClientQt.cpp: (NotificationPresenterClientQt::show): Return type of NotificationContents::iconURL() changed. Canonical link: https://commits.webkit.org/48892@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@57604 268f45cc-cd09-0410-ab3c-d52691b4dbfc
WebKit · Apr 14, 2010 · c231f2cd45aede32ff2eb09b805c4ad52f5328d8 · c231f2c
1 parent f4ec7c8
commit c231f2cd45aede32ff2eb09b805c4ad52f5328d8
Showing 10 changed files with 73 additions and 27 deletions.
diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
@@ -1,3 +1,27 @@
+2010-04-14  Aaron Boodman  <aa@chromium.org>
+
+        Reviewed by David Levin.
+
+        Support relative URLs for notifications on Chromium. They weren't working previously because WebCore was inserting
+        the relative URL into a KURL instance, but when KURL is backed by GURL as it is on Chromium, relative URLs are
+        unsupported. Fixed by resolving the relative URL first.
+
+        https://bugs.webkit.org/show_bug.cgi?id=36623
+
+        Adding tests for this is difficult because we don't currently have DRT support for notifications on Mac, only Windows.
+
+        * notifications/Notification.cpp:
+        (WebCore::Notification::Notification): Accept resolved KURL instead of relative string.
+        * notifications/Notification.h:
+        (WebCore::Notification::create): Ditto.
+        (WebCore::Notification::iconURL): Return resolved KURL instead of relative string.
+        * notifications/NotificationCenter.h:
+        (WebCore::NotificationCenter::createHTMLNotification): Immediately resolve URL instead of passing off relative string.
+        (WebCore::NotificationCenter::createNotification): Ditto.
+        * notifications/NotificationContents.h:
+        (WebCore::NotificationContents::NotificationContents): Accept resolved KURL instead of relative string.
+        (WebCore::NotificationContents::icon): Return resolved URL.
+
 2010-04-14  Anders Carlsson  <andersca@apple.com>
 
         Reviewed by Sam Weinig.

diff --git a/WebCore/notifications/Notification.cpp b/WebCore/notifications/Notification.cpp
@@ -42,7 +42,7 @@
 
 namespace WebCore {
 
-Notification::Notification(const String& url, ScriptExecutionContext* context, ExceptionCode& ec, NotificationPresenter* provider)
+Notification::Notification(const KURL& url, ScriptExecutionContext* context, ExceptionCode& ec, NotificationPresenter* provider)
     : ActiveDOMObject(context, this)
     , m_isHTML(true)
     , m_isShowing(false)
@@ -54,11 +54,12 @@ Notification::Notification(const String& url, ScriptExecutionContext* context, E
         return;
     }
 
-    m_notificationURL = context->completeURL(url);
-    if (url.isEmpty() || !m_notificationURL.isValid()) {
+    if (url.isEmpty() || !url.isValid()) {
         ec = SYNTAX_ERR;
         return;
     }
+
+    m_notificationURL = url;
 }
 
 Notification::Notification(const NotificationContents& contents, ScriptExecutionContext* context, ExceptionCode& ec, NotificationPresenter* provider)
@@ -74,9 +75,7 @@ Notification::Notification(const NotificationContents& contents, ScriptExecution
         return;
     }
 
-    if (!contents.icon().isEmpty())
-        m_iconURL = context->completeURL(contents.icon());
-    if (!m_iconURL.isEmpty() && !m_iconURL.isValid()) {
+    if (!contents.icon().isEmpty() && !contents.icon().isValid()) {
         ec = SYNTAX_ERR;
         return;
     }

diff --git a/WebCore/notifications/Notification.h b/WebCore/notifications/Notification.h
@@ -55,7 +55,7 @@ namespace WebCore {
 
     class Notification : public RefCounted<Notification>, public ActiveDOMObject, public EventTarget { 
     public:
-        static Notification* create(const String& url, ScriptExecutionContext* context, ExceptionCode& ec, NotificationPresenter* provider) { return new Notification(url, context, ec, provider); }
+        static Notification* create(const KURL& url, ScriptExecutionContext* context, ExceptionCode& ec, NotificationPresenter* provider) { return new Notification(url, context, ec, provider); }
         static Notification* create(const NotificationContents& contents, ScriptExecutionContext* context, ExceptionCode& ec, NotificationPresenter* provider) { return new Notification(contents, context, ec, provider); }
 
         virtual ~Notification();
@@ -65,7 +65,7 @@ namespace WebCore {
 
         bool isHTML() { return m_isHTML; }
         KURL url() { return m_notificationURL; }
-        KURL iconURL() { return m_iconURL; }
+        KURL iconURL() { return m_contents.icon(); }
         NotificationContents& contents() { return m_contents; }
 
         DEFINE_ATTRIBUTE_EVENT_LISTENER(display);
@@ -80,8 +80,8 @@ namespace WebCore {
         virtual Notification* toNotification() { return this; }
 
     private:
-        Notification(const String& url, ScriptExecutionContext* context, ExceptionCode& ec, NotificationPresenter* provider);
-        Notification(const NotificationContents& fields, ScriptExecutionContext* context, ExceptionCode& ec, NotificationPresenter* provider);
+        Notification(const KURL&, ScriptExecutionContext*, ExceptionCode&, NotificationPresenter*);
+        Notification(const NotificationContents&, ScriptExecutionContext*, ExceptionCode&, NotificationPresenter*);
 
         // EventTarget interface
         virtual void refEventTarget() { ref(); }
@@ -91,7 +91,6 @@ namespace WebCore {
 
         bool m_isHTML;
         KURL m_notificationURL;
-        KURL m_iconURL;
         NotificationContents m_contents;
 
         bool m_isShowing;

diff --git a/WebCore/notifications/NotificationCenter.h b/WebCore/notifications/NotificationCenter.h
@@ -55,7 +55,11 @@ namespace WebCore {
                 ec = INVALID_STATE_ERR;
                 return 0;
             }
-            return Notification::create(KURL(ParsedURLString, URI), context(), ec, presenter());
+            if (URI.isEmpty()) {
+                ec = SYNTAX_ERR;
+                return 0;
+            }
+            return Notification::create(m_scriptExecutionContext->completeURL(URI), context(), ec, presenter());
         }
 
         Notification* createNotification(const String& iconURI, const String& title, const String& body, ExceptionCode& ec)
@@ -64,7 +68,7 @@ namespace WebCore {
                 ec = INVALID_STATE_ERR;
                 return 0;
             }
-            NotificationContents contents(iconURI, title, body);
+            NotificationContents contents(iconURI.isEmpty() ? KURL() : m_scriptExecutionContext->completeURL(iconURI), title, body);
             return Notification::create(contents, context(), ec, presenter());
         }
 

diff --git a/WebCore/notifications/NotificationContents.h b/WebCore/notifications/NotificationContents.h
@@ -38,17 +38,17 @@ namespace WebCore {
     class NotificationContents { 
     public:
         NotificationContents() {}
-        NotificationContents(const String& iconUrl, const String& title, const String& body) 
+        NotificationContents(const KURL& iconUrl, const String& title, const String& body)
             : m_icon(iconUrl)
             , m_title(title)
             , m_body(body) {}
 
-        String icon() const { return m_icon; }
+        KURL icon() const { return m_icon; }
         String title() const { return m_title; }
         String body() const { return m_body; }
 
     private:
-        String m_icon;
+        KURL m_icon;
         String m_title;
         String m_body;
     };

diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
@@ -1,3 +1,18 @@
+2010-04-14  Aaron Boodman  <aa@chromium.org>
+
+        Reviewed by David Levin.
+
+        Support relative URLs for notifications on Chromium. They weren't working previously because WebCore was inserting
+        the relative URL into a KURL instance, but when KURL is backed by GURL as it is on Chromium, relative URLs are
+        unsupported. Fixed by resolving the relative URL first.
+
+        https://bugs.webkit.org/show_bug.cgi?id=36623
+
+        Adding tests for this is difficult because we don't currently have DRT support for notifications on Mac, only Windows.
+
+        * public/WebNotification.h: Remove deprecated icon() method.
+        * src/WebNotification.cpp: Ditto.
+
 2010-04-14  Jay Civelli  <jcivelli@chromium.org>
 
         Reviewed by Dimitri Glazkov.

diff --git a/WebKit/chromium/public/WebNotification.h b/WebKit/chromium/public/WebNotification.h
@@ -71,9 +71,6 @@ class WebNotification {
     // If HTML, the URL which contains the contents of the notification.
     WEBKIT_API WebURL url() const;
 
-    // If not HTML, the parameters for the icon-title-text notification.
-    // FIXME: Deprecated; use iconURL() instead.
-    WEBKIT_API WebString icon() const;
     WEBKIT_API WebURL iconURL() const;
     WEBKIT_API WebString title() const;
     WEBKIT_API WebString body() const;

diff --git a/WebKit/chromium/src/WebNotification.cpp b/WebKit/chromium/src/WebNotification.cpp
@@ -76,13 +76,6 @@ WebURL WebNotification::url() const
     return m_private->url();
 }
 
-// FIXME: remove this deprecated function once all callers use iconURL()
-WebString WebNotification::icon() const
-{
-    ASSERT(!isHTML());
-    return m_private->contents().icon();
-}
-
 WebURL WebNotification::iconURL() const
 {
     ASSERT(!isHTML());

diff --git a/WebKit/qt/ChangeLog b/WebKit/qt/ChangeLog
@@ -1,3 +1,18 @@
+2010-04-14  Aaron Boodman  <aa@chromium.org>
+
+        Reviewed by David Levin.
+
+        Support relative URLs for notifications on Chromium. They weren't working previously because WebCore was inserting
+        the relative URL into a KURL instance, but when KURL is backed by GURL as it is on Chromium, relative URLs are
+        unsupported. Fixed by resolving the relative URL first.
+
+        https://bugs.webkit.org/show_bug.cgi?id=36623
+
+        Adding tests for this is difficult because we don't currently have DRT support for notifications on Mac, only Windows.
+
+        * WebCoreSupport/NotificationPresenterClientQt.cpp:
+        (NotificationPresenterClientQt::show): Return type of NotificationContents::iconURL() changed.
+
 2010-04-13  Timothy Hatcher  <timothy@apple.com>
 
         Rename SecurityOrigin::whiteListAccessFromOrigin to addOriginAccessWhitelistEntry.

diff --git a/WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp b/WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp
@@ -63,7 +63,7 @@ bool NotificationPresenterClientQt::show(Notification* notification)
             printf("DESKTOP NOTIFICATION: contents at %s\n", QString(notification->url().string()).toUtf8().constData());
         else {
             printf("DESKTOP NOTIFICATION: icon %s, title %s, text %s\n", 
-                QString(notification->contents().icon()).toUtf8().constData(), QString(notification->contents().title()).toUtf8().constData(), 
+                QString(notification->contents().icon().string()).toUtf8().constData(), QString(notification->contents().title()).toUtf8().constData(), 
                 QString(notification->contents().body()).toUtf8().constData());
         }
     }