Bug 39357 – [QT] QT_BEARER flag is not enabled on S60 properly

• Component: Tools / Tests
• Status: CLOSED FIXED

[Mahesh Kulkarni]

QT_BEARER is right now only enabled for symbian (on 4.6) and enabled for all 4.7 releases. 

There is though one problem with the way the flag has been defined. 


    symbian: {
        exists($${EPOCROOT}epoc32/release/winscw/udeb/QtBearer.lib)| \
        exists($${EPOCROOT}epoc32/release/armv5/lib/QtBearer.lib) {
            DEFINES += ENABLE_QT_BEARER=1
        }

Mobility builds only after Qt is built so the condition above never satisfies. After talking to vihria, ekoppen, tronical and SaMUELnEVALA on IRC, 

exists($$[QMAKE_MKSPECS]/features/mobility.prf) can be used to check if qtmobility is built. QtMobility 1.0.0 (public release) has bearer by default on all platform so no need to check for library either. 


PS: I will attach a patch on the same and may be not with symbian flag as well!

[Yael]

This is the same issue I reported in https://bugs.webkit.org/show_bug.cgi?id=36617.
At the time, I was assured that there was a solution outside of webkit.

[Mahesh Kulkarni]

Created attachment 56488 [details]
proposed solution

Enable QT_BEARER flag based on qtmobility availability. As qtmobility 1.0.0 includes bearer manager for all platform no need of symbian check.

[Mahesh Kulkarni]

(In reply to comment #1)
> This is the same issue I reported in https://bugs.webkit.org/show_bug.cgi?id=36617.
> At the time, I was assured that there was a solution outside of webkit.

Yael, On S60, there were two issues, one the flag not being defined (which was enabled outside webkit), second, navigator.onLine is not modified at all on any connection change. I'm working on the second issue right now

Another patch I'm working is on enabling geolocation which again uses qtmobility. This patch sets a base for using qtmobility.

[Laszlo Gombos]

Comment on attachment 56488 [details]
proposed solution

>  !CONFIG(QTDIR_build):!contains(DEFINES, ENABLE_QT_BEARER=.) {
> -    symbian: {
> -        exists($${EPOCROOT}epoc32/release/winscw/udeb/QtBearer.lib)| \
> -        exists($${EPOCROOT}epoc32/release/armv5/lib/QtBearer.lib) {
> -            DEFINES += ENABLE_QT_BEARER=1
> -        }

I find this existing build logic useful to continue building WebKit trunk for Symbian devices with QtNetwork 4.6 and a separate QtBearer module. We can maybe add an extra guard around this logic to state explicitly that this only make sense if Qt version is less than 4.7, but I prefer not to remove this code.

> +    exists($$[QMAKE_MKSPECS]/features/mobility.prf) {
> +       DEFINES += ENABLE_QT_BEARER=1
>      }

This might make sense for other QtMobility modules (like Geolocation), but it does not make sense to me for BearerMgmt as BearerMgmt is being rolled into QtNetwork and it will not be a separate QtMobility module going forward.

We should check for Qt version to see if QtNetwork module supports BearerMgmt, but that is already part of the build system.

# Bearer management is part of Qt 4.7
!lessThan(QT_MINOR_VERSION, 7):!contains(DEFINES, ENABLE_QT_BEARER=.):DEFINES += ENABLE_QT_BEARER=1

r- for taking this feature discovery approach for BearerMgmt; this will make more sense to mo for other QtMobility modules (e.g. Geolocation).

[Mahesh Kulkarni]

Thank you for your comments. Marking the bug resolved.

[Simon Hausmann]

(In reply to comment #4)
> (From update of attachment 56488 [details])
> >  !CONFIG(QTDIR_build):!contains(DEFINES, ENABLE_QT_BEARER=.) {
> > -    symbian: {
> > -        exists($${EPOCROOT}epoc32/release/winscw/udeb/QtBearer.lib)| \
> > -        exists($${EPOCROOT}epoc32/release/armv5/lib/QtBearer.lib) {
> > -            DEFINES += ENABLE_QT_BEARER=1
> > -        }
> 
> I find this existing build logic useful to continue building WebKit trunk for Symbian devices with QtNetwork 4.6 and a separate QtBearer module. We can maybe add an extra guard around this logic to state explicitly that this only make sense if Qt version is less than 4.7, but I prefer not to remove this code.

The problem with the existing logic is that it breaks the build in this setup:

    1) Symbian^3 (coming with Qt and the bearer libs in the above location)
    2) You compile Qt yourself (in say \dev\myqt)
    3) Then you try to compile WebKit and it thinks that Qt is configured with mobility support, which it actually isn't.

I think Mahesh's detection is correct here.

> > +    exists($$[QMAKE_MKSPECS]/features/mobility.prf) {
> > +       DEFINES += ENABLE_QT_BEARER=1
> >      }
> 
> This might make sense for other QtMobility modules (like Geolocation), but it does not make sense to me for BearerMgmt as BearerMgmt is being rolled into QtNetwork and it will not be a separate QtMobility module going forward.
> 
> We should check for Qt version to see if QtNetwork module supports BearerMgmt, but that is already part of the build system.

Right, we do that already below. But shouldn't we keep the existing support for Qt 4.6 + bearer management around, too, for Symbian^3?

I'd like to refine the existing check to work better (more reliable and cross-platform) and I think we should use the same check in general for future modules provided by the Qt Mobility project.
 
> # Bearer management is part of Qt 4.7
> !lessThan(QT_MINOR_VERSION, 7):!contains(DEFINES, ENABLE_QT_BEARER=.):DEFINES += ENABLE_QT_BEARER=1
> 
> r- for taking this feature discovery approach for BearerMgmt; this will make more sense to mo for other QtMobility modules (e.g. Geolocation).

[Simon Hausmann]

(In reply to comment #2)
> Created an attachment (id=56488) [details]
> proposed solution
> 
> Enable QT_BEARER flag based on qtmobility availability. As qtmobility 1.0.0 includes bearer manager for all platform no need of symbian check.

I think we should take this patch in. At the very least it fixes the build issues I outlined earlier (a not unusual setup for Qt + WebKit development on Symbian^3 IMHO) and it shouldn't break anything either :)

[Laszlo Gombos]

(In reply to comment #7)
> (In reply to comment #2)
> > Created an attachment (id=56488) [details] [details]
> > proposed solution
> > 
> > Enable QT_BEARER flag based on qtmobility availability. As qtmobility 1.0.0 includes bearer manager for all platform no need of symbian check.
> 
> I think we should take this patch in. At the very least it fixes the build issues I outlined earlier (a not unusual setup for Qt + WebKit development on Symbian^3 IMHO) and it shouldn't break anything either :)

But how can we detect BearerMgmt support reliably ? My understanding is that one can have mobility.prf without BearerMgmt support (as that can be turned off at configure time).

The reliably way seems to me to actually look inside mobility.prf to see if BearerMgmt was enabled.

[Laszlo Gombos]

Comment on attachment 56488 [details]
proposed solution

Putting back the r? as this patch is now being reconsidered.

[Simon Hausmann]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #2)
> > > Created an attachment (id=56488) [details] [details] [details]
> > > proposed solution
> > > 
> > > Enable QT_BEARER flag based on qtmobility availability. As qtmobility 1.0.0 includes bearer manager for all platform no need of symbian check.
> > 
> > I think we should take this patch in. At the very least it fixes the build issues I outlined earlier (a not unusual setup for Qt + WebKit development on Symbian^3 IMHO) and it shouldn't break anything either :)
> 
> But how can we detect BearerMgmt support reliably ? My understanding is that one can have mobility.prf without BearerMgmt support (as that can be turned off at configure time).

You're right, that's currently not possible. Oops. I'll see if that can be fixed.

[Simon Hausmann]

Ok, Alex fixed this upstream:

http://qt.gitorious.org/qt-mobility/qt-mobility/commit/480959c68fb1071bd77b503988b63f4e80387489

load(mobilityconfig)
contains(MOBILITY_CONFIG, bearer) {
    message(Bearer API available)
    CONFIG+=mobility
    MOBILITY+=bearer
} else {
    message(Bearer API NOT available)
}

[Mahesh Kulkarni]

re-opening the bug to use solution Simon suggested

[Mahesh Kulkarni]

Created attachment 58019 [details]
solution as suggest by Simon

enable bearer flag based on mobility configuration OR enable for all qt4.7+
Load mobility only for older qt versions as per the current changes.

[WebKit Review Bot]

Attachment 58019 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:9:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Mahesh Kulkarni]

Created attachment 58023 [details]
solution as suggest by Simon

previous one had a tab in patch as result of log thanks to failure of prepare-changelog script and had to manually edit change log :(

[Simon Hausmann]

Comment on attachment 58023 [details]
solution as suggest by Simon

r=me, this certainly improves the situation :)

(QtMobility 1.0.1 is required for that to work)

A next step would be to always do the load(mobilityconfig) at the top of WebKit.pri and then use it throughout the other places I guess

[WebKit Commit Bot]

Comment on attachment 58023 [details]
solution as suggest by Simon

Clearing flags on attachment: 58023

Committed r60780: <http://trac.webkit.org/changeset/60780>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Laszlo Gombos]

This created a warning when QtMobility is not available. Following change would fix it:

Index: WebCore/WebCore.pri
===================================================================
--- WebCore/WebCore.pri (revision 60842)
+++ WebCore/WebCore.pri (working copy)
@@ -115,7 +115,7 @@
      !lessThan(QT_MINOR_VERSION, 7) {
         DEFINES += ENABLE_QT_BEARER=1
      } else {
-        load(mobilityconfig)
+        exists($$[QMAKE_MKSPECS]/features/mobility.prf): load(mobilityconfig)
         contains(MOBILITY_CONFIG, bearer) {
             DEFINES += ENABLE_QT_BEARER=1
         }

[Laszlo Gombos]

Created attachment 58142 [details]
fix build warning

[Simon Hausmann]

Revision r60780 cherry-picked into qtwebkit-2.0 with commit f3dc6deef5a85d6c7ae13c6c7b37e7da50e20233

[Simon Hausmann]

(In reply to comment #19)
> This created a warning when QtMobility is not available. Following change would fix it:
> 
> Index: WebCore/WebCore.pri
> ===================================================================
> --- WebCore/WebCore.pri (revision 60842)
> +++ WebCore/WebCore.pri (working copy)
> @@ -115,7 +115,7 @@
>       !lessThan(QT_MINOR_VERSION, 7) {
>          DEFINES += ENABLE_QT_BEARER=1
>       } else {
> -        load(mobilityconfig)
> +        exists($$[QMAKE_MKSPECS]/features/mobility.prf): load(mobilityconfig)
>          contains(MOBILITY_CONFIG, bearer) {
>              DEFINES += ENABLE_QT_BEARER=1
>          }

A better fix is to replace

load(mobilityconfig)

with

load(mobilityconfig,true)

where the second argument (true) means: ignore errors when loading

[Mahesh Kulkarni]

(In reply to comment #22)
> (In reply to comment #19)
> > This created a warning when QtMobility is not available. Following change would fix it:
> > 
> > Index: WebCore/WebCore.pri
> > ===================================================================
> > --- WebCore/WebCore.pri (revision 60842)
> > +++ WebCore/WebCore.pri (working copy)
> > @@ -115,7 +115,7 @@
> >       !lessThan(QT_MINOR_VERSION, 7) {
> >          DEFINES += ENABLE_QT_BEARER=1
> >       } else {
> > -        load(mobilityconfig)
> > +        exists($$[QMAKE_MKSPECS]/features/mobility.prf): load(mobilityconfig)
> >          contains(MOBILITY_CONFIG, bearer) {
> >              DEFINES += ENABLE_QT_BEARER=1
> >          }
> 
> A better fix is to replace
> 
> load(mobilityconfig)
> 
> with
> 
> load(mobilityconfig,true)
> 
> where the second argument (true) means: ignore errors when loading

Nice! Thanks 
patch from 39724 covers the change for this case as well as we will be loading mobilityconfig only once.

[Laszlo Gombos]

Comment on attachment 58142 [details]
fix build warning

Marking obsolete as a better solution is on its way (see bug 39724) and it is not time critical to resolve this.