Bug 37442 – [PATCH] [Qt] Fix compilation with system tray disabled.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Qt

[Ismail Donmez]

Notification support should be disabled if QT_NO_SYSTEMTRAYICON is defined.

[Ismail Donmez]

Created attachment 53158 [details]
Compile if QT_NO_SYSTEMTRAYICON is defined

[Ismail Donmez]

Patch missing some more checks. I will fix it.

[Ismail Donmez]

WebCore::ChromeClient::notificationPresenter() is abstract, it should be implemented in all conditions.

[Ismail Donmez]

Created attachment 53159 [details]
Implement WebCore::ChromeClient::notificationPresenter() in all cases

[WebKit Review Bot]

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.

[Ismail Donmez]

Created attachment 53160 [details]
Fix style error

[Simon Hausmann]

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

[Laszlo Gombos]

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

[Laszlo Gombos]

*** Bug 37716 has been marked as a duplicate of this bug. ***

[Chang Shu]

(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.

[Laszlo Gombos]

(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.

[Chang Shu]

> > > 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.

[Chang Shu]

Created attachment 53847 [details]
fix patch

[Simon Hausmann]

Comment on attachment 53847 [details]
fix patch

Thanks Chang!

[WebKit Commit Bot]

Comment on attachment 53847 [details]
fix patch

Clearing flags on attachment: 53847

Committed r57972: <http://trac.webkit.org/changeset/57972>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Simon Hausmann]

(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?

[Laszlo Gombos]

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.

[Simon Hausmann]

(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*