[Webkit-unassigned] [Bug 61140] [GTK] Add HTML5 Notifications support
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon May 23 15:10:12 PDT 2011
https://bugs.webkit.org/show_bug.cgi?id=61140
--- Comment #5 from Alexandre Mazari <scaroo at gmail.com> 2011-05-23 15:10:12 PST ---
Hi Martin,
Thanks for the review. I prematurely posted it to here to get this kind of feedbacks validating/dismissing the global approach and regarding code-style and convention I am still not familiar with :)
(In reply to comment #4)
> (From update of attachment 94111 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=94111&action=review
>
> Nice work overall, though I have a few concerns. The API is a bit confusing and it has no documentation. Be sure to add documentation in your next version of the patch.
Yup I'll document the public API (mostly the signals) in a following iteration.
Beforehand it would be nice to talk about how the API is used. Ideally it should be as similar to other ports as possible.
I tried to keep as close as possible to the Chrome an Qt API, themselves straight interpretations of the draft specification.
Here is basically the rules this patch tries to implement:
- js code request permission to show notifications from the current security domain using webkitNotifications.requestPermission()
- webkit-gtk, delegating to our NotificationPresenter impl, notifies the UA code using the "notification-request-permission" signal
- the UA ask the user and store its answers. Each security domain can have 3 states: allowed, declined and pending.
- when js instanciates a new notification, it checks implicitly the permission for the current security domain using NotificationPresenter::checkPermission
- this, in turn, delegates to the UA by sending the "notification-check-permission".
- if the UA forbid it, a security exception is thrown in the js context
- else the notification is now usable
- js code request the displaying of the notification
- we wrap the webkit notification in a GObject, and provide it to the UA by the mean of the "notification-show" signal
- the UA can decide to show it straight away or enqueue the notification for later use.
- at *actual* show-time, the "show" event is thrown in the js context (current implementation do not respect that, the "show" event is thrown at gsignal handlers return)
- in the case the notification couldnt be displayed, an "error" js event is fired.
- the js code can cancel the notification. If it is already show, the UA must hide it, if not, the notification must not be shown.
- if the system supports it, any click event on it must be forwarded to the js context. The proposed implementation provide the webkit_web_notification_clicked method for that purpose.
- webcore notifies us (callingceaNotificationPresenter::notificqtionObjectDestroyed when a kit'Notification object is dying. We use this oportunity to unref our GObject representation.
- A notification can replace a previous one. For that, its "replaceId" and security origin should equal a previously sent notification.
On a broader POV, I used signals to give control to the UA code in order to follow the surrounding conventions.
Though, in most cases, if not all, only one handler will be attached to each signal (1-1 rel). GObject signals are not type-safe, implie some overhead and do not enforce the webkit-gtk and client code to be logically segmented. See both WebKitWebView and EphyWebView for instance: huge inflation of signals/signals handlers.
I was wondering if your share this point of view and if in this case a more traditional delegation architecture, using an interface to be implemented by client and a setter in WebView would be fine to push at least for this feature.
>Careful with code style. Sorry if it seems like some of the following review comments are overly picky!
No pun taken, thanks again for the detailed review. Following a comment for each of your points that need precision. The left-outs do not require detailed explanation and will be corrected.
>
> > LayoutTests/platform/gtk/Skipped:263
> > +# Notification support needs improvement in unexpected conditions
> > +fast/notifications/notifications-document-close-crash.html
> > +fast/notifications/notifications-cancel-request-permission.html
> > +fast/notifications/notification-after-close.html
> > http/tests/notifications
>
> Why are these failing still?
>
Working on it, still one to go. Some of the tests seems to contradict with each-other and the draft spec.
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:81
> > +GList* DumpRenderTreeSupportGtk::s_notificationsAllowedOrigins = 0;
> > +GList* DumpRenderTreeSupportGtk::s_notifications = 0;
>
> Instead of using class static members here, please simply keep them static inside the file. I recommend using WTF::Vector here and WebCore Strings.
Ok. How do I define the comparator func so to compare on string content instead of address ?
>
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:821
> > +static gint matchNotificationByTitle(WebKitWebNotification* notification,
> > + char* title)
>
> Typically in WebKitGTK+ code we do not put newlines in argument lists.
Ok
>
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:823
> > + return g_strcmp0(webkit_web_notification_get_title(notification), title);
>
> I think it might be clearer to remove this helper.
Needed for the GList implementation, hopefully using Vector<String> will remove the need for them.
>
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:830
> > + return g_strcmp0(webkit_web_notification_get_replaceid(notification1),
> > + webkit_web_notification_get_replaceid(notification2));
>
> Ditto.
>
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:836
> > + g_list_free_full(s_notificationsAllowedOrigins, g_free);
> > + s_notificationsAllowedOrigins = 0;
>
> This would be much simpler if you used WTF::Vector for instance.
>
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:31
> > +
> > +
>
> This seems accidental.
>
> > Source/WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.h:136
> > + static void replaceDesktopNotificationWith(WebKitWebNotification*);
> > + static void resetDesktopNotifications();
> > + static void addDesktopNotification(WebKitWebNotification*);
> > + static void removeDesktopNotification(WebKitWebNotification*);
> > + static void resetDesktopNotificationPermissions();
> > + static void grantDesktopNotificationPermission(char*);
> > + static bool checkDesktopNotificationPermission(char*);
> > + static void ignoreDesktopNotificationPermissionRequests(gboolean);
> > + static bool areDesktopNotificationPermissionRequestsIgnored();
> > + static void simulateDesktopNotificationClick(char* title);
> > +
>
> This is a lot of private functions. Should some of these go in the public API?
Well this a tricky one: most of those methods are exposed by the *custom* Qt and Chromium LayoutTesControllers. Those two do not implement the common LTC that do not declare those APIs.
So I added them to the common LHT and made LayoutTestControllerGtk implementations delegates to DumpRenderTreeControllerGtk because it is AFAIK the only place where state can be shared between DRT and LTC and DRT needs to access some of the state (the list of authorized security origins, the ignore flags...).
>
> > Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:52
> > + webkit_web_notification_set_real(gnotification, notification);
>
> Wouldn't it be better to set this with g_object_new?
>
I used a private setter to forbid the client code to touch it. Might better use a constructor only prop instead indeed.
> > Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:58
> > + &isOk);
>
> Would isOkay be more accurately named if it was called permissionGranted?
>
Yup
> > Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:61
> > + event = Event::create("display", false, true);
>
> Does Event::create return a PassRefPtr or a raw Event*. If it's the latter, this is a memory leak, since RefPtr takes a reference.
>
Gotta check and ping you to explain me all the subtlety behind that.
> > Source/WebKit/gtk/WebCoreSupport/NotificationPresenterGtk.cpp:94
> > + gchar* origin = g_strdup(context->securityOrigin()->toString().utf8().data());
> > + g_signal_emit_by_name(m_webView, "notification-check-permission",
> > + origin, &permission);
> > + g_free(origin);
>
> Here you can just use context->securityOrigin()->toString().utf8().data() in the function call and avoid an entire string copy
Tried that initially but somehow the address was rewritten sometime; thus the copy. Maybe the JS/GC thread do its magic backstage.
> > Source/WebKit/gtk/webkit/webkitwebnotification.cpp:43
> > + char* url;
> > + char* icon_url;
> > + char* title;
> > + char* body;
> > + char* dir;
>
> Lately we've been making the private sections C++ objects which we call new and delete on manually (take a look at WebKitWebFrame, I believe -- or contact me via email for more information). If you did that you could change all these to be CString members and never have to worry about managing their memory.
>
A'ight.
> > Source/WebKit/gtk/webkit/webkitwebnotification.cpp:44
> > + char* replaceid;
>
> replaceId should be replaceId, though perhaps just id would make sense?
>
Well, the spec specifies the name replace id and this field is optional so 'id' might not convey the full intent.
>
> > Source/WebKit/gtk/webkit/webkitwebview.cpp:2755
> > + webkit_web_view_signals[NOTIFICATION_SHOW] = g_signal_new("notification-show",
>
> show-notification?
As you wish. I wanted to prefix the notification-related signals to "namespace them a bit.
> >> Source/WebKit/gtk/webkit/webkitwebview.h:85
> >> +{
> >
> > This { should be at the end of the previous line [whitespace/braces] [4]
>
> Please fix this.
>
The enums around do not put opening brace on the same line, so should I ?
BTW, do you think this enum should be located here or in webkiwebnotification.h ? I'd favor the 2nd solution.
> > Tools/DumpRenderTree/LayoutTestController.cpp:2202
> > +static JSValueRef ignoreDesktopNotificationPermissionRequestsCallback(JSContextRef context, JSObjectRef function, JSObjectRef thisObject, size_t argumentCount, const JSValueRef arguments[], JSValueRef* exception)
>
> Why is it necessary to change platform-independent code?
See above.
> > Tools/DumpRenderTree/LayoutTestController.cpp:2510
> > -void LayoutTestController::grantDesktopNotificationPermission(JSStringRef origin)
> > -{
> > - m_desktopNotificationAllowedOrigins.push_back(JSStringRetain(origin));
> > -}
> > -
> > -bool LayoutTestController::checkDesktopNotificationPermission(JSStringRef origin)
> > -{
> > - std::vector<JSStringRef>::iterator i;
> > - for (i = m_desktopNotificationAllowedOrigins.begin();
> > - i != m_desktopNotificationAllowedOrigins.end();
> > - ++i) {
> > - if (JSStringIsEqual(*i, origin))
> > - return true;
> > - }
> > - return false;
> > -}
> > +// void LayoutTestController::grantDesktopNotificationPermission(JSStringRef origin)
> > +// {
> > +// m_desktopNotificationAllowedOrigins.push_back(JSStringRetain(origin));
> > +// }
> > +
> > +// bool LayoutTestController::checkDesktopNotificationPermission(JSStringRef origin)
> > +// {
> > +// std::vector<JSStringRef>::iterator i;
> > +// for (i = m_desktopNotificationAllowedOrigins.begin();
> > +// i != m_desktopNotificationAllowedOrigins.end();
> > +// ++i) {
> > +// if (JSStringIsEqual(*i, origin))
> > +// return true;
> > +// }
> > +// return false;
> > +// }
> >
>
> I don't understand the purpose of this change. In general, patches shouldn't retain code commented out. It worries me that this code is commented out in platfrom-independent code though. This will likely break other ports.
See above. this code was not used by any port AFAIK. In later patch, I moved the implementation within the *Gtk and added empty methods in other ports (wx, windows, mac).
Still we could reuse those impl, but thqt would imply a gchar* -> JSStringRef conversion each time we check for a permission.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list