[webkit-reviews] review denied: [Bug 80477] Add Notification constructor : [Attachment 138055] Patch

[bugzilla-daemon at webkit.org]

Jian Li <jianli at chromium.org> has denied Jon Lee <jonlee at apple.com>'s request
for review:
Bug 80477: Add Notification constructor
https://bugs.webkit.org/show_bug.cgi?id=80477

Attachment 138055: Patch
https://bugs.webkit.org/attachment.cgi?id=138055&action=review

------- Additional Comments from Jian Li <jianli at chromium.org>
I notice that there're a lot of QT-specific icon-loading logic in
Notification.*. Some of them are guarded while other are not. This makes
Notification implementation quite complex and fragmented. I am thinking
probably we can move all icon-loading logic out of Notification class and put
it into something like NotificationIconLoader. Certainly, this is not part of
this patch. Could you please file a bug on this and assign it to QT guy?


View in context: https://bugs.webkit.org/attachment.cgi?id=138055&action=review


> Source/WebCore/notifications/Notification.cpp:188
>      if (m_state == Idle && m_notificationCenter->client()) {

It seems that we can merge the logic for MAC platform and other non-QT
platform. Since NotificationClient::show could return false for an error, why
not also checking it before setting state to Showing, as what we do in non-QT
platform.

> Source/WebCore/notifications/Notification.cpp:304
>      unsetPendingActivity(this);

For non-mac platform, where do we call unsetPendingActivity?

> Source/WebCore/notifications/Notification.h:165
>      void finishLoading();

finishLoading is also only related to icon loading in QT. Could you please also
suffix it with Icon?