Notification support should be disabled if QT_NO_SYSTEMTRAYICON is defined.
Created attachment 53158 [details] Compile if QT_NO_SYSTEMTRAYICON is defined
Patch missing some more checks. I will fix it.
WebCore::ChromeClient::notificationPresenter() is abstract, it should be implemented in all conditions.
Created attachment 53159 [details] Implement WebCore::ChromeClient::notificationPresenter() in all cases
Attachment 53159 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WARNING: File exempt from style guide. Skipping: "WebKit/qt/Api/qwebpage.cpp" WebKit/qt/WebCoreSupport/ChromeClientQt.cpp:482: Use 0 instead of NULL. [readability/null] [5] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 53160 [details] Fix style error
Wouldn't it be better to disable ENABLE(NOTIFICATIONS) if there's no system tray icon class instead of adding another condition to every use of ENABLE(NOTIFICATIONS)? It may be easiest to do that in WebCore/config.h
Comment on attachment 53160 [details] Fix style error I agree with Simon's proposal, I'd like to give an r- this time. I similar mechanism exists to disable certain WebKit features if sqlite is not available. Would the following work .. contains(DEFINES, QT_NO_SYSTEMTRAYICON):DEFINES += ENABLE_NOTIFICATIONS=0
*** Bug 37716 has been marked as a duplicate of this bug. ***
(In reply to comment #8) > (From update of attachment 53160 [details]) > I agree with Simon's proposal, I'd like to give an r- this time. I similar > mechanism exists to disable certain WebKit features if sqlite is not available. > > Would the following work .. > > contains(DEFINES, QT_NO_SYSTEMTRAYICON):DEFINES += ENABLE_NOTIFICATIONS=0 I don't think the pri file is able to detect if QT_NO_SYSTEMTRAYICON is defined.
(In reply to comment #10) > (In reply to comment #8) > > (From update of attachment 53160 [details] [details]) > > I agree with Simon's proposal, I'd like to give an r- this time. I similar > > mechanism exists to disable certain WebKit features if sqlite is not available. > > > > Would the following work .. > > > > contains(DEFINES, QT_NO_SYSTEMTRAYICON):DEFINES += ENABLE_NOTIFICATIONS=0 > > I don't think the pri file is able to detect if QT_NO_SYSTEMTRAYICON is > defined. Chang, you're right that unless QT_NO_SYSTEMTRAYICON is explicitly listed in the DEFINES qmake will not pick it up as it is only defined in a Qt header. A compromise would be not to disable the ENABLE_NOTIFICATIONS feature but only guard the SysTray dependent code with SYSTEMTRAYICON. I think in the future we should allow QtWebKit clients to render the notification themselves.
> > > contains(DEFINES, QT_NO_SYSTEMTRAYICON):DEFINES += ENABLE_NOTIFICATIONS=0 > > > > I don't think the pri file is able to detect if QT_NO_SYSTEMTRAYICON is > > defined. > > Chang, you're right that unless QT_NO_SYSTEMTRAYICON is explicitly listed in > the DEFINES qmake will not pick it up as it is only defined in a Qt header. > > A compromise would be not to disable the ENABLE_NOTIFICATIONS feature but only > guard the SysTray dependent code with SYSTEMTRAYICON. I think in the future we > should allow QtWebKit clients to render the notification themselves. I agree to guard the systemtrayicon code as we don't have a general solution and in this case, it's even a better solution as you have indicated for future enhancement. I am working on a patch if you guys don't mind.
Created attachment 53847 [details] fix patch
Comment on attachment 53847 [details] fix patch Thanks Chang!
Comment on attachment 53847 [details] fix patch Clearing flags on attachment: 53847 Committed r57972: <http://trac.webkit.org/changeset/57972>
All reviewed patches have been landed. Closing bug.
(In reply to comment #15) > (From update of attachment 53847 [details]) > Clearing flags on attachment: 53847 > > Committed r57972: <http://trac.webkit.org/changeset/57972> This patch is scheduled for integration into the 2.0 branch, but the earlier patches that implement this feature are not part of the branch. Do we really need to include another feature in the release branch?
Simon, this is not needed for the 2.0 branch. If I read the history of the bug correctly, you added the 35784 blocker to this.
(In reply to comment #18) > Simon, this is not needed for the 2.0 branch. If I read the history of the bug > correctly, you added the 35784 blocker to this. Hehe, thanks *gets the brown paper bag*