[webkit-reviews] review denied: [Bug 32555] [Qt] support navigator.onLine and ononline/onoffline events. : [Attachment 45016] Patch.

[bugzilla-daemon at webkit.org]

Laszlo Gombos <laszlo.1.gombos at nokia.com> has denied Yael
<yael.aharon at nokia.com>'s request for review:
Bug 32555: [Qt] support navigator.onLine and ononline/onoffline events.
https://bugs.webkit.org/show_bug.cgi?id=32555

Attachment 45016: Patch.
https://bugs.webkit.org/attachment.cgi?id=45016&action=review

------- Additional Comments from Laszlo Gombos <laszlo.1.gombos at nokia.com>
Yael, thanks for taking on this, the core of the patch looks really good.

I think it would be a small extra effort to support not only Symbian but all
the platforms that Bearer Management library supports (most notably Maemo).

> +	   NetworkAccessAllowed

I'm not sure I understand the need for the new setting in QtWebKit. I would
think that onlineStateChanged fires if the network is turned off from the
"application UI". 

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

I think the library name should be QtBearer instead of QtBearer_tp (I think tp
stands for Technology Preview here).

AFAIK the convention for this type of build flag would be
QT_NO_BEARER_MANAGEMENT or QT_NO_BEARER. Also if bearer management library only
works with Qt 4.6 (or later) we should also check for Qt version (in the source
files not in the build file).

> +contains(DEFINES, ENABLE_QT_BEARER=1) {
> +    SOURCES += \
> +	   platform/network/qt/NetworkStateNotifierQt.cpp
> +
> +    HEADERS += \
> +	   platform/network/qt/NetworkStateNotifierPrivate.h
> +}

I prefer to always add the source and header files to the list of files to
build and add the guard to the header/cpp file.

Setting it to r- as I think it needs a bit more work, but as I said
fundamentally the change looks good.