Bug 39357

Summary: [QT] QT_BEARER flag is not enabled on S60 properly
Product: WebKit Reporter: Mahesh Kulkarni <maheshk>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: CLOSED FIXED    
Severity: Normal CC: commit-queue, hausmann, laszlo.gombos, webkit.review.bot, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: S60 Hardware   
OS: Other   
Bug Depends on:    
Bug Blocks: 39724    
Attachments:
Description Flags
proposed solution
none
solution as suggest by Simon
none
solution as suggest by Simon
none
fix build warning kenneth: review+, laszlo.gombos: commit-queue-

Description Mahesh Kulkarni 2010-05-19 06:45:46 PDT
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!
Comment 1 Yael 2010-05-19 06:50:47 PDT
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.
Comment 2 Mahesh Kulkarni 2010-05-19 07:32:54 PDT
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.
Comment 3 Mahesh Kulkarni 2010-05-19 07:36:31 PDT
(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.
Comment 4 Laszlo Gombos 2010-05-19 09:14:28 PDT
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).
Comment 5 Mahesh Kulkarni 2010-05-19 09:47:20 PDT
Thank you for your comments. Marking the bug resolved.
Comment 6 Simon Hausmann 2010-05-26 04:38:36 PDT
(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).
Comment 7 Simon Hausmann 2010-05-26 04:40:51 PDT
(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 :)
Comment 8 Laszlo Gombos 2010-05-26 05:47:19 PDT
(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.
Comment 9 Laszlo Gombos 2010-05-26 05:47:53 PDT
Comment on attachment 56488 [details]
proposed solution

Putting back the r? as this patch is now being reconsidered.
Comment 10 Simon Hausmann 2010-05-26 08:02:31 PDT
(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.
Comment 11 Simon Hausmann 2010-06-04 06:08:22 PDT
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)
}
Comment 12 Mahesh Kulkarni 2010-06-07 05:51:07 PDT
re-opening the bug to use solution Simon suggested
Comment 13 Mahesh Kulkarni 2010-06-07 06:05:55 PDT
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.
Comment 14 WebKit Review Bot 2010-06-07 06:07:04 PDT
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.
Comment 15 Mahesh Kulkarni 2010-06-07 06:18:11 PDT
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 :(
Comment 16 Simon Hausmann 2010-06-07 07:12:26 PDT
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
Comment 17 WebKit Commit Bot 2010-06-07 07:29:22 PDT
Comment on attachment 58023 [details]
solution as suggest by Simon

Clearing flags on attachment: 58023

Committed r60780: <http://trac.webkit.org/changeset/60780>
Comment 18 WebKit Commit Bot 2010-06-07 07:29:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 19 Laszlo Gombos 2010-06-08 08:26:26 PDT
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
         }
Comment 20 Laszlo Gombos 2010-06-08 08:40:18 PDT
Created attachment 58142 [details]
fix build warning
Comment 21 Simon Hausmann 2010-06-10 03:33:06 PDT
Revision r60780 cherry-picked into qtwebkit-2.0 with commit f3dc6deef5a85d6c7ae13c6c7b37e7da50e20233
Comment 22 Simon Hausmann 2010-06-10 03:46:13 PDT
(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
Comment 23 Mahesh Kulkarni 2010-06-10 04:37:53 PDT
(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.
Comment 24 Laszlo Gombos 2010-06-10 09:19:29 PDT
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.