Bug 36623

Summary:

Relative URLs don't work for notifications in Chromium

Product:

WebKit

Reporter:

Aaron Boodman <aa>

Component:

WebCore Misc.

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, brettw, commit-queue, dglazkov, fishd, johnnyg, levin, tkent, webkit-ews, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	levin: review+, commit-queue: commit-queue-

Aaron Boodman

Reported 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.

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
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

Dimitri Glazkov (Google)

Comment 1 2010-03-25 14:58:52 PDT

tkent@, this may be your opportunity to poke GURL::Resolve through to WebURL :)

Darin Fisher (:fishd, Google)

Comment 2 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.

Aaron Boodman

Comment 3 2010-04-09 17:46:46 PDT

Created attachment 53024 [details] Patch

John Gregg

Comment 4 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());

Aaron Boodman

Comment 5 2010-04-09 18:08:18 PDT

Created attachment 53025 [details] Patch

Aaron Boodman

Comment 6 2010-04-09 18:39:03 PDT

Created attachment 53027 [details] Patch

Aaron Boodman

Comment 7 2010-04-09 18:49:21 PDT

Created attachment 53029 [details] Patch

Aaron Boodman

Comment 8 2010-04-12 09:23:13 PDT

ping

John Gregg

Comment 9 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.

Adam Barth

Comment 10 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.

Jeremy Orlow

Comment 11 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

David Levin

Comment 12 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

Aaron Boodman

Comment 13 2010-04-13 15:54:43 PDT

Created attachment 53292 [details] Patch

Aaron Boodman

Comment 14 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.

WebKit Review Bot

Comment 15 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

David Levin

Comment 16 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.

Aaron Boodman

Comment 17 2010-04-13 17:56:54 PDT

Created attachment 53303 [details] Patch

Early Warning System Bot

Comment 18 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

Aaron Boodman

Comment 19 2010-04-13 18:21:56 PDT

Created attachment 53305 [details] Patch

WebKit Commit Bot

Comment 20 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

Eric Seidel (no email)

Comment 21 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.

Aaron Boodman

Comment 22 2010-04-14 13:52:02 PDT

Committed r57604: <http://trac.webkit.org/changeset/57604>

Note You need to log in before you can comment on or make changes to this bug.