Bug 36623 - Relative URLs don't work for notifications in Chromium
Summary: Relative URLs don't work for notifications in Chromium
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-03-25 14:50 PDT by Aaron Boodman
Modified: 2010-04-14 13:52 PDT (History)
10 users (show)

See Also:


Attachments
Patch (8.15 KB, patch)
2010-04-09 17:46 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (7.85 KB, patch)
2010-04-09 18:08 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (8.07 KB, patch)
2010-04-09 18:39 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (7.98 KB, patch)
2010-04-09 18:49 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (9.89 KB, patch)
2010-04-13 15:54 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (9.62 KB, patch)
2010-04-13 17:56 PDT, Aaron Boodman
no flags Details | Formatted Diff | Diff
Patch (11.18 KB, patch)
2010-04-13 18:21 PDT, Aaron Boodman
levin: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aaron Boodman 2010-03-25 14:50:42 PDT
You can't do webkitNotifications.createHTMLNotification(<relative url>) in Chromium, because the implementation immediately stuffs the provided string into a KURL. Later, the KURL is resolved.

In Chromium, KURL is backed by GURL, which doesn't allow relative URLs, so the url becomes invalid and resolving it does nothing.
Comment 1 Dimitri Glazkov (Google) 2010-03-25 14:58:52 PDT
tkent@, this may be your opportunity to poke GURL::Resolve through to WebURL :)
Comment 2 Darin Fisher (:fishd, Google) 2010-03-25 20:01:20 PDT
Sounds like Document::completeURL should be used here.  We also have that exposed on WebDocument, but probably the resolution should occur within WebCore.
Comment 3 Aaron Boodman 2010-04-09 17:46:46 PDT
Created attachment 53024 [details]
Patch
Comment 4 John Gregg 2010-04-09 17:53:03 PDT
Comment on attachment 53024 [details]
Patch

> diff --git a/LayoutTests/fast/notifications/notifications-with-permission.html-disabled b/LayoutTests/fast/notifications/notifications-with-permission.html-disabled
> index a22717b..7ba887f 100644
> --- a/LayoutTests/fast/notifications/notifications-with-permission.html-disabled
> +++ b/LayoutTests/fast/notifications/notifications-with-permission.html-disabled
> @@ -6,7 +6,7 @@
>          {
>              document.getElementById("result").innerHTML += message + "<br>";
>          }
> -        
> +
>          function runTests()
>          {
>              if (window.layoutTestController) {
> @@ -17,10 +17,13 @@
>              if (!window.webkitNotifications) {
>                  log("FAIL: No webkitNotifications interface!");
>              }
> -            
> +
>              var N = window.webkitNotifications.createHTMLNotification("http://localhost/my_notification.html");
>              N.show();
> -            
> +
> +            var N = window.webkitNotifications.createHTMLNotification("resources/notification.html");
> +            N.show();

Even though this test is disabled you should change the corresponding expected.txt file.

> +
>              var M = window.webkitNotifications.createNotification("http://localhost/my_icon.png", "New E-mail", "Meet me tonight at 8!");
>              M.show();
>          }
> @@ -28,8 +31,8 @@
>  </head>
>  <body>
>  <p>Sending notifications with permission...</p>
> -    
> -    
> +
> +
>  <script type="text/javascript">
>  runTests();
>  </script>
> diff --git a/LayoutTests/fast/notifications/resources/notification.html b/LayoutTests/fast/notifications/resources/notification.html
> new file mode 100644
> index 0000000..273f256
> --- /dev/null
> +++ b/LayoutTests/fast/notifications/resources/notification.html
> @@ -0,0 +1 @@
> +Hello there. I am a notification.
> diff --git a/WebCore/notifications/Notification.cpp b/WebCore/notifications/Notification.cpp
> index 0c44db6..182f713 100644
> --- 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
> index c2ba5d9..ff56edc 100644
> --- 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,7 +80,7 @@ namespace WebCore {
>          virtual Notification* toNotification() { return this; }
>  
>      private:
> -        Notification(const String& url, ScriptExecutionContext* context, ExceptionCode& ec, NotificationPresenter* provider);
> +        Notification(const KURL& url, ScriptExecutionContext* context, ExceptionCode& ec, NotificationPresenter* provider);
>          Notification(const NotificationContents& fields, ScriptExecutionContext* context, ExceptionCode& ec, NotificationPresenter* provider);
>  
>          // EventTarget interface
> diff --git a/WebCore/notifications/NotificationCenter.h b/WebCore/notifications/NotificationCenter.h
> index ae3dc02..80ab6b7 100644
> --- a/WebCore/notifications/NotificationCenter.h
> +++ b/WebCore/notifications/NotificationCenter.h
> @@ -55,7 +55,7 @@ namespace WebCore {
>                  ec = INVALID_STATE_ERR;
>                  return 0;
>              }
> -            return Notification::create(KURL(ParsedURLString, URI), context(), ec, presenter());
> +            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 +64,7 @@ namespace WebCore {
>                  ec = INVALID_STATE_ERR;
>                  return 0;
>              }
> -            NotificationContents contents(iconURI, title, body);
> +            NotificationContents contents(m_scriptExecutionContext->completeURL(iconURI), title, body);

This is going to be a problem.  If iconURI the string is empty, we don't want to resolve it to the current script's URL, which is what will happen (I recently fixed this bug).  We need to make an empty URL.

>              return Notification::create(contents, context(), ec, presenter());
>          }
>  
> diff --git a/WebCore/notifications/NotificationContents.h b/WebCore/notifications/NotificationContents.h
> index ebdc514..2319980 100644
> --- 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/public/WebNotification.h b/WebKit/chromium/public/WebNotification.h
> index b63dd20..9d64e2a 100644
> --- a/WebKit/chromium/public/WebNotification.h
> +++ b/WebKit/chromium/public/WebNotification.h
> @@ -71,9 +71,6 @@ public:
>      // 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
> index 26fd4bc..5200d17 100644
> --- 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());
Comment 5 Aaron Boodman 2010-04-09 18:08:18 PDT
Created attachment 53025 [details]
Patch
Comment 6 Aaron Boodman 2010-04-09 18:39:03 PDT
Created attachment 53027 [details]
Patch
Comment 7 Aaron Boodman 2010-04-09 18:49:21 PDT
Created attachment 53029 [details]
Patch
Comment 8 Aaron Boodman 2010-04-12 09:23:13 PDT
ping
Comment 9 John Gregg 2010-04-12 10:05:21 PDT
(In reply to comment #8)
> ping

My comments have been addressed, but you'll need to find a reviewer to signoff.
Comment 10 Adam Barth 2010-04-12 10:49:17 PDT
I wanted to review your patch, but I don't understand this code.  Also, I noticed that your patch doesn't have any tests.
Comment 11 Jeremy Orlow 2010-04-12 11:16:40 PDT
Comment on attachment 53029 [details]
Patch

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 6f8e025..72aa8d5 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,13 @@
> +2010-04-09  Aaron Boodman  <aa@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Support relative URLs for notifiications on Chromium.
> +        https://bugs.webkit.org/show_bug.cgi?id=36623

Please add a few more details of exactly what you're doing here.


All comments seem to have been addressed.  Looks fine to me.  And the domain expert on this did an informal review.  r=me
Comment 12 David Levin 2010-04-12 11:20:51 PDT
It looks like jorlow r+'ed this.

Anyway, I think the following items should be addressed.

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 6f8e025..72aa8d5 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,13 @@
> +2010-04-09  Aaron Boodman  <aa@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Support relative URLs for notifiications on Chromium.

notifiications sp

> +        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.
> +        John is going to add this soon and will add a test for this at that time.


There are no functions listed here as is typical in changelogs.

> diff --git a/WebCore/notifications/Notification.cpp b/WebCore/notifications/Notification.cpp
> diff --git a/WebCore/notifications/Notification.h b/WebCore/notifications/Notification.h

> +        Notification(const KURL& url, ScriptExecutionContext* context, ExceptionCode& ec, NotificationPresenter* provider);

(I know this isn't your fault but you're changing the line, so) I don't think any of these parameter names add anything except maybe "provider", so you should remove them.

m_iconURL isn't used anymore as far as I can tell but you didn't remove it from the class.


> diff --git a/WebKit/chromium/ChangeLog b/WebKit/chromium/ChangeLog
> +2010-04-09  Aaron Boodman  <aa@chromium.org>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Support relative URLs for notifiications on Chromium.

notifiications sp
Comment 13 Aaron Boodman 2010-04-13 15:54:43 PDT
Created attachment 53292 [details]
Patch
Comment 14 Aaron Boodman 2010-04-13 16:12:35 PDT
(In reply to comment #11)
> Please add a few more details of exactly what you're doing here.

Done.

(In reply to comment #12)
> notifiications sp

Done.

> There are no functions listed here as is typical in changelogs.

Fixed.

> > +        Notification(const KURL& url, ScriptExecutionContext* context, ExceptionCode& ec, NotificationPresenter* provider);
> 
> (I know this isn't your fault but you're changing the line, so) I don't think
> any of these parameter names add anything except maybe "provider", so you
> should remove them.

Done.

> m_iconURL isn't used anymore as far as I can tell but you didn't remove it from
> the class.

Good catch, thanks. Done.

> notifiications sp

Done.
Comment 15 WebKit Review Bot 2010-04-13 16:35:36 PDT
Attachment 53292 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1581464
Comment 16 David Levin 2010-04-13 17:07:50 PDT
Comment on attachment 53292 [details]
Patch

r- due t chromium build break (which is due to the removal of arg names for the "static Notification* create" methods).

r+ other than that.
Comment 17 Aaron Boodman 2010-04-13 17:56:54 PDT
Created attachment 53303 [details]
Patch
Comment 18 Early Warning System Bot 2010-04-13 18:06:08 PDT
Attachment 53303 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1584469
Comment 19 Aaron Boodman 2010-04-13 18:21:56 PDT
Created attachment 53305 [details]
Patch
Comment 20 WebKit Commit Bot 2010-04-14 02:32:23 PDT
Comment on attachment 53305 [details]
Patch

Rejecting patch 53305 from commit-queue.

Failed to run "['git', 'svn', 'dcommit']" exit_code: 1
Last 500 characters of output:
ebKit/chromium/src/WebNotification.cpp
	M	WebKit/qt/ChangeLog
	M	WebKit/qt/WebCoreSupport/NotificationPresenterClientQt.cpp
A repository hook failed: MERGE request failed on '/repository/webkit/trunk': Commit blocked by pre-commit hook (exit code 1) with output:
svnlook: Can't write to stream: Broken pipe

    The following ChangeLog files contain OOPS:

        trunk/WebKit/qt/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
 at /usr/local/git/libexec/git-core/git-svn line 570


Full output: http://webkit-commit-queue.appspot.com/results/1633422
Comment 21 Eric Seidel (no email) 2010-04-14 12:24:37 PDT
Comment on attachment 53305 [details]
Patch

 5         Need a short description and bug URL (OOPS!)
in the Qt ChangeLOG caused the failure.
Comment 22 Aaron Boodman 2010-04-14 13:52:02 PDT
Committed r57604: <http://trac.webkit.org/changeset/57604>