Bug 108482

Created attachment 185782 [details] Patch

Hi again Jon! :) This one is a crasher that happens with WK1 clients when enabling NOTIFICATIONS. I am working exclusively on adding notifications support in wk2 for the GTK port and clients still using WK1 should not crash, even if notifications are enabled.

Comment on attachment 185782 [details] Patch There is another unchecked use of NotificationController::from in Notification::permission. Should it be fixed, too?

(In reply to comment #3) > (From update of attachment 185782 [details]) > There is another unchecked use of NotificationController::from in Notification::permission. Should it be fixed, too? I see. So far I can't tell whether it's an issue but I guess it wouldn't hurt to ensure it won't crash.

Comment on attachment 185782 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=185782&action=review Unofficial r=me. > Source/WebCore/Modules/notifications/Notification.cpp:291 > + if (client) It would be better to define it in the if clause: if (NotificationClient* client = ...)

Actually, doesn't it make more sense to not call provideNotification(), and check for the existence of a controller?

(In reply to comment #6) > Actually, doesn't it make more sense to not call provideNotification(), and check for the existence of a controller? Actually the caller of Notification::requestPermission() seems to be JSNotification::requestPermission(). Not sure I understand what you mean?

(In reply to comment #7) > (In reply to comment #6) > > Actually, doesn't it make more sense to not call provideNotification(), and check for the existence of a controller? > > Actually the caller of Notification::requestPermission() seems to be JSNotification::requestPermission(). Not sure I understand what you mean? If you don't have a notification client, then you shouldn't be adding a NotificationController as a supplement to the Page. So I think you should be able to check for a null NotificationController, rather than checking for a null NotificationClient.

Comment on attachment 185782 [details] Patch Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.