Bug 44593 – [Qt] Fix warnings in WebKit2 directory

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Csaba Osztrogonác]

Warnings on Qt Linux Release buildbot:
../../../WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:61: warning: unused variable ‘numberOfBytesRead’
../../../WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:73: warning: unused variable ‘numberOfBytesRead’
../../../WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:126: warning: unused variable ‘bytesWritten’
../../../WebKit2/Shared/qt/WebEventFactoryQt.cpp:176: warning: ‘state’ may be used uninitialized in this function
../../../WebKit2/WebProcess/Plugins/Netscape/qt/NetscapePluginQt.cpp:54: warning: ‘npEvent’ is used uninitialized in this function

Fix is coming soon.

[Csaba Osztrogonác]

Created attachment 65386 [details]
proposed fix

[Andras Becsi]

Comment on attachment 65386 [details]
proposed fix


WebKit2/WebProcess/Plugins/Netscape/qt/NetscapePluginQt.cpp: 
 +  
Just try npEvent = NPEvent(), else the new could leak. If this gets implemented this should be initialized.

WebKit2/Shared/qt/WebEventFactoryQt.cpp:200
 +              state = WebPlatformTouchPoint::TouchReleased;
This initialization should be at the beginning, I think this would avoid confusion. The ASSERT is needed however.

Otherwise LGTM, but I'm not a reviewer :)

[Kenneth Rohde Christiansen]

Comment on attachment 65386 [details]
proposed fix

WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp:62
 +              (void)numberOfBytesRead;
Isn't there a macro for that? Please check wtf. Qt normally uses Q_UNUSED

[Csaba Osztrogonác]

Created attachment 65398 [details]
proposed fix

Updated version based on comments of Andras and Kenneth.

[Early Warning System Bot]

Attachment 65398 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3769682

[Csaba Osztrogonác]

Comment on attachment 65398 [details]
proposed fix

It isn't my day. :) Fix is coming soon.

[Csaba Osztrogonác]

Created attachment 65402 [details]
proposed fix

Qt build fixed.

[Andras Becsi]

(In reply to comment #7)
> Created an attachment (id=65402) [details]
> proposed fix
> 
> Qt build fixed.
LGTM.

[Antonio Gomes]

Comment on attachment 65402 [details]
proposed fix

Looks good, but need another round.

> diff --git a/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp b/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp
> index 3e77e46..aae977b 100644
> --- a/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp
> +++ b/WebKit2/Platform/CoreIPC/qt/ConnectionQt.cpp
> @@ -59,6 +59,7 @@ void Connection::readyReadHandler()
>      while (m_socket->bytesAvailable()) {
>          if (!m_currentMessageSize) {
>              size_t numberOfBytesRead = m_socket->read(reinterpret_cast<char*>(m_readBuffer.data()), sizeof(size_t));
> +            Q_UNUSED(numberOfBytesRead);
>              ASSERT(numberOfBytesRead);

combine both Q_UNUSED and ASSERT in a ASSERT_UNUSED

>              m_currentMessageSize = *reinterpret_cast<size_t*>(m_readBuffer.data());
>          }
> @@ -71,6 +72,7 @@ void Connection::readyReadHandler()
>          }
>  
>          size_t numberOfBytesRead = m_socket->read(reinterpret_cast<char*>(m_readBuffer.data()), m_currentMessageSize);
> +        Q_UNUSED(numberOfBytesRead);
>          ASSERT(numberOfBytesRead);

Ditto.

>          // The messageID is encoded at the end of the buffer.
> @@ -124,7 +126,7 @@ bool Connection::sendOutgoingMessage(MessageID messageID, PassOwnPtr<ArgumentEnc
>      m_socket->write(reinterpret_cast<char*>(&bufferSize), sizeof(bufferSize));
>  
>      qint64 bytesWritten = m_socket->write(reinterpret_cast<char*>(arguments->buffer()), arguments->bufferSize());
> -
> +    Q_UNUSED(bytesWritten);
>      ASSERT(bytesWritten == arguments->bufferSize());

Ditto.

>      return true;
>  }
> diff --git a/WebKit2/Shared/qt/WebEventFactoryQt.cpp b/WebKit2/Shared/qt/WebEventFactoryQt.cpp
> index bdd6a23..043b35e 100644
> --- a/WebKit2/Shared/qt/WebEventFactoryQt.cpp
> +++ b/WebKit2/Shared/qt/WebEventFactoryQt.cpp
> @@ -173,7 +173,7 @@ WebKeyboardEvent WebEventFactory::createWebKeyboardEvent(QKeyEvent* event)
>  WebTouchEvent WebEventFactory::createWebTouchEvent(QTouchEvent* event)
>  {
>      WebEvent::Type type  = webEventTypeForEvent(event);
> -    WebPlatformTouchPoint::TouchPointState state;
> +    WebPlatformTouchPoint::TouchPointState state = static_cast<WebPlatformTouchPoint::TouchPointState>(0);
>      unsigned int id;
>      WebEvent::Modifiers modifiers   = modifiersForEvent(event->modifiers());
>      double timestamp                = WTF::currentTime();
> @@ -196,6 +196,9 @@ WebTouchEvent WebEventFactory::createWebTouchEvent(QTouchEvent* event)
>          case Qt::TouchPointStationary: 
>              state = WebPlatformTouchPoint::TouchStationary; 
>              break;
> +        default:
> +            ASSERT_NOT_REACHED();
> +            break;
>          }
>  
>          m_touchPoints.append(WebPlatformTouchPoint(id, state, points.at(i).screenPos().toPoint().x(),
> diff --git a/WebKit2/WebProcess/Plugins/Netscape/qt/NetscapePluginQt.cpp b/WebKit2/WebProcess/Plugins/Netscape/qt/NetscapePluginQt.cpp
> index b8bbb87..3f2ae4f 100644
> --- a/WebKit2/WebProcess/Plugins/Netscape/qt/NetscapePluginQt.cpp
> +++ b/WebKit2/WebProcess/Plugins/Netscape/qt/NetscapePluginQt.cpp
> @@ -47,7 +47,7 @@ void NetscapePlugin::platformPaint(GraphicsContext* context, const IntRect& dirt
>  
>  NPEvent toNP(const WebMouseEvent& event)
>  {
> -    NPEvent npEvent;
> +    NPEvent npEvent = NPEvent();
>  
>      notImplemented();
>  

Strange it is need. I thought the default constructor would do this trick.

[Csaba Osztrogonác]

ASSERT_UNUSED != Q_UNUSED + ASSERT , because Q_UNUSED differs
for ICC and RVCT. Maybe ICC and RVCT don't like (void)x;
So I think Q_UNUSED would be better here.

1.)
#define ASSERT_UNUSED(variable, assertion) ((void)variable)

2.)
#if defined(Q_CC_INTEL) && !defined(Q_OS_WIN) || defined(Q_CC_RVCT)
template <typename T>
inline void qUnused(T &x) { (void)x; }
#  define Q_UNUSED(x) qUnused(x);
#else
#  define Q_UNUSED(x) (void)x;
#endif



>>  NPEvent toNP(const WebMouseEvent& event)
>>  {
>> -    NPEvent npEvent;
>> +    NPEvent npEvent = NPEvent();
>>  
>>      notImplemented();
>>  
> Strange it is need. I thought the default constructor would do this trick.
An NPEvent can be a QEvent (if XP_SYMBIAN), a struct (if XP_WIN or XP_OS2), XEvent (if XP_WIN) or a simple void* otherwise. Consequently we should initialize this local variable explicitly to make compilers happy.

[Csaba Osztrogonác]

Comment on attachment 65402 [details]
proposed fix

r? again, because I think Q_UNUSED is correct.

[Sam Weinig]

(In reply to comment #11)
> (From update of attachment 65402 [details])
> r? again, because I think Q_UNUSED is correct.

I don't see any reason to use Q_UNUSED, but it is your call. In most of platform specific code we use the WTF versions.

[Csaba Osztrogonác]

(In reply to comment #12)
> I don't see any reason to use Q_UNUSED, but it is your call. In most of platform specific code we use the WTF versions.

I found 64 occurences of Q_UNUSED mainly in Qt API layers.

[Csaba Osztrogonác]

ping?

Kenneth or Antonio, could you review my patch, please?
The question is ASSERT_UNUSED or (Q_UNUSED + ASSERT)?

[Balazs Kelemen]

(In reply to comment #14)
> ping?
> 
> Kenneth or Antonio, could you review my patch, please?
> The question is ASSERT_UNUSED or (Q_UNUSED + ASSERT)?

Definitely ASSERT_UNUSED.

[Antonio Gomes]

(In reply to comment #14)
> ping?
> 
> Kenneth or Antonio, could you review my patch, please?
> The question is ASSERT_UNUSED or (Q_UNUSED + ASSERT)?

Does not ASSERT_UNUSED build for you? I do not have a strong preference on it, but it looks like it was developed to cases like this.

[Balazs Kelemen]

(In reply to comment #15)
> (In reply to comment #14)
> > ping?
> > 
> > Kenneth or Antonio, could you review my patch, please?
> > The question is ASSERT_UNUSED or (Q_UNUSED + ASSERT)?
> 
> Definitely ASSERT_UNUSED.

Ok, I did not read the bug before writing this, however I think if it is necessary than we should rather fix ASSET_UNUSED for ICC and RVCT.

[Csaba Osztrogonác]

(In reply to comment #16)
> Does not ASSERT_UNUSED build for you? I do not have a strong preference on it, but it looks like it was developed to cases like this.

ASSERT_UNUSED build for me, but I can't test with ICC and RVCT. :(
I think there is a reason why are 64 occurences of Q_UNUSED in WebKit trunk.

[Csaba Osztrogonác]

(In reply to comment #17)
> Ok, I did not read the bug before writing this, however I think if it is necessary than we should rather fix ASSET_UNUSED for ICC and RVCT.

Good idea. ASSERT_UNUSED should call Q_UNUSED if PLATFORM(QT) is true.

[Csaba Osztrogonác]

Created attachment 65916 [details]
proposed fix

ASSERT_UNUSED will call Q_UNUSED, so we don't
need to call Q_UNUSED + ASSERT anymore:
https://bugs.webkit.org/show_bug.cgi?id=44870

[Early Warning System Bot]

Attachment 65916 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3871154

[Csaba Osztrogonác]

Comment on attachment 65916 [details]
proposed fix

one more round needed :S

[Csaba Osztrogonác]

Created attachment 65919 [details]
proposed fix again

[Csaba Osztrogonác]

Comment on attachment 65919 [details]
proposed fix again

Clearing flags on attachment: 65919

Committed r66495: <http://trac.webkit.org/changeset/66495>

[Csaba Osztrogonác]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

http://trac.webkit.org/changeset/66495 might have broken Leopard Intel Debug (Tests)
The following changes are on the blame list:
http://trac.webkit.org/changeset/66493
http://trac.webkit.org/changeset/66494
http://trac.webkit.org/changeset/66495