Bug 37442 - [PATCH] [Qt] Fix compilation with system tray disabled.
Summary: [PATCH] [Qt] Fix compilation with system tray disabled.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Qt (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: Qt
: 37716 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-04-12 01:20 PDT by Ismail Donmez
Modified: 2010-04-26 09:07 PDT (History)
5 users (show)

See Also:


Attachments
Compile if QT_NO_SYSTEMTRAYICON is defined (2.56 KB, patch)
2010-04-12 01:22 PDT, Ismail Donmez
no flags Details | Formatted Diff | Diff
Implement WebCore::ChromeClient::notificationPresenter() in all cases (3.25 KB, patch)
2010-04-12 01:36 PDT, Ismail Donmez
no flags Details | Formatted Diff | Diff
Fix style error (3.24 KB, patch)
2010-04-12 01:38 PDT, Ismail Donmez
laszlo.gombos: review-
Details | Formatted Diff | Diff
fix patch (1.78 KB, patch)
2010-04-20 11:08 PDT, Chang Shu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ismail Donmez 2010-04-12 01:20:31 PDT
Notification support should be disabled if QT_NO_SYSTEMTRAYICON is defined.
Comment 1 Ismail Donmez 2010-04-12 01:22:04 PDT
Created attachment 53158 [details]
Compile if QT_NO_SYSTEMTRAYICON is defined
Comment 2 Ismail Donmez 2010-04-12 01:25:40 PDT
Patch missing some more checks. I will fix it.
Comment 3 Ismail Donmez 2010-04-12 01:34:44 PDT
WebCore::ChromeClient::notificationPresenter() is abstract, it should be implemented in all conditions.
Comment 4 Ismail Donmez 2010-04-12 01:36:05 PDT
Created attachment 53159 [details]
Implement WebCore::ChromeClient::notificationPresenter() in all cases
Comment 5 WebKit Review Bot 2010-04-12 01:37:50 PDT
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.
Comment 6 Ismail Donmez 2010-04-12 01:38:58 PDT
Created attachment 53160 [details]
Fix style error
Comment 7 Simon Hausmann 2010-04-13 06:58:49 PDT
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 8 Laszlo Gombos 2010-04-13 23:32:37 PDT
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
Comment 9 Laszlo Gombos 2010-04-18 21:33:39 PDT
*** Bug 37716 has been marked as a duplicate of this bug. ***
Comment 10 Chang Shu 2010-04-19 06:45:25 PDT
(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.
Comment 11 Laszlo Gombos 2010-04-20 09:49:53 PDT
(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.
Comment 12 Chang Shu 2010-04-20 09:59:35 PDT
> > > 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.
Comment 13 Chang Shu 2010-04-20 11:08:12 PDT
Created attachment 53847 [details]
fix patch
Comment 14 Simon Hausmann 2010-04-20 11:17:09 PDT
Comment on attachment 53847 [details]
fix patch

Thanks Chang!
Comment 15 WebKit Commit Bot 2010-04-21 06:08:22 PDT
Comment on attachment 53847 [details]
fix patch

Clearing flags on attachment: 53847

Committed r57972: <http://trac.webkit.org/changeset/57972>
Comment 16 WebKit Commit Bot 2010-04-21 06:08:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Simon Hausmann 2010-04-26 03:39:45 PDT
(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?
Comment 18 Laszlo Gombos 2010-04-26 06:50:14 PDT
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.
Comment 19 Simon Hausmann 2010-04-26 09:07:36 PDT
(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*