Bug 72704 - [Qt] Use QEvent for dispatchFunctionsFromMainThread()
: [Qt] Use QEvent for dispatchFunctionsFromMainThread()
Status: RESOLVED FIXED
: WebKit
WebKit Qt
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
: Qt
:
:
  Show dependency treegraph
 
Reported: 2011-11-18 01:48 PST by
Modified: 2011-11-24 05:28 PST (History)


Attachments
Patch (2.47 KB, patch)
2011-11-18 01:59 PST, Patrick R. Gansterer
no flags Review Patch | Details | Formatted Diff | Diff
Patch (2.50 KB, patch)
2011-11-18 06:19 PST, Patrick R. Gansterer
no flags Review Patch | Details | Formatted Diff | Diff
Patch (2.57 KB, patch)
2011-11-23 15:21 PST, Patrick R. Gansterer
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-11-18 01:48:08 PST
[Qt] Use QEvent for dispatchFunctionsFromMainThread()
------- Comment #1 From 2011-11-18 01:59:07 PST -------
Created an attachment (id=115767) [details]
Patch
------- Comment #2 From 2011-11-18 06:19:35 PST -------
Created an attachment (id=115795) [details]
Patch
------- Comment #3 From 2011-11-21 00:19:57 PST -------
(From update of attachment 115795 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=115795&action=review

r=me. QMetaObject::invokeMethod is implemented using posted events behind the scenes and this patch removes the unnecessary layer of indirection (which does things like setting the current QObject::sender, which we don't need). It would be nice if you could fix the nitpicks before landing :)

> Source/JavaScriptCore/wtf/qt/MainThreadQt.cpp:36
> +#include <QtCore/QEvent>

In general we don't need #include <ModuelName/ClassName> includes and we should just use #include <classname> or #include <header.h>.
The only place where we _need_ to use this pattern is in header files used for the public API in Qt (ask me on IRC if you're curious :)

> Source/JavaScriptCore/wtf/qt/MainThreadQt.cpp:52
> +    s_mainThreadInvokerEventType = QEvent::registerEventType();

Nitpick: Technically it's QCoreEvent::registerEventType ;)

> Source/JavaScriptCore/wtf/qt/MainThreadQt.cpp:58
> +    if (e->type() == s_mainThreadInvokerEventType)
> +        dispatchFunctionsFromMainThread();

Nitpick: Missing {} with "return" in the body? (no need to call QObject::event for _our_ event type)
------- Comment #4 From 2011-11-23 15:21:06 PST -------
Created an attachment (id=116441) [details]
Patch
------- Comment #5 From 2011-11-24 01:59:57 PST -------
(From update of attachment 115795 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=115795&action=review

>> Source/JavaScriptCore/wtf/qt/MainThreadQt.cpp:36
>> +#include <QtCore/QEvent>
> 
> In general we don't need #include <ModuelName/ClassName> includes and we should just use #include <classname> or #include <header.h>.
> The only place where we _need_ to use this pattern is in header files used for the public API in Qt (ask me on IRC if you're curious :)

Copy&Paste ;-)

>> Source/JavaScriptCore/wtf/qt/MainThreadQt.cpp:52
>> +    s_mainThreadInvokerEventType = QEvent::registerEventType();
> 
> Nitpick: Technically it's QCoreEvent::registerEventType ;)

I don't think so! ;-) It's defined in qcoreevent.h, but the class is still QEvent!
------- Comment #6 From 2011-11-24 03:37:46 PST -------
(From update of attachment 116441 [details])
Clearing flags on attachment: 116441

Committed r101131: <http://trac.webkit.org/changeset/101131>
------- Comment #7 From 2011-11-24 03:37:50 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #8 From 2011-11-24 04:12:38 PST -------
It broke _all_ http/tests/appcache test with "FAIL: Timed out waiting for notifyDone to be called" timeout. Could you check it please as soon as possible? (Now all of our tester bot is broken because of this regression.)
------- Comment #9 From 2011-11-24 05:28:14 PST -------
Fix landed in http://trac.webkit.org/changeset/101134