Summary: | WTF::RunLoop should not depend on isMainThread() idiom. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | benjamin, cdumez, cmarcelo, dbates, ews-watchlist, fpizlo, hi, joepeck, keith_miller, msaboff, rmorisset, rniwa, saam, simon.fraser, tzagallo, webkit-bug-importer, ysuzuki | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: |
https://bugs.webkit.org/show_bug.cgi?id=202716 https://bugs.webkit.org/show_bug.cgi?id=203875 |
||||||||
Attachments: |
|
Description
Mark Lam
2019-11-05 17:49:02 PST
Created attachment 382873 [details]
proposed patch.
Comment on attachment 382873 [details]
proposed patch.
Need to fix non abstraction to also work for non USE(CF) ports.
Created attachment 382877 [details]
proposed patch.
Comment on attachment 382877 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=382877&action=review > Source/WTF/ChangeLog:7 > + Reviewed by NOBODY (OOPS!). can you also open a bug about the Date function which uses createCFString? Comment on attachment 382877 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=382877&action=review >> Source/WTF/ChangeLog:7 >> + Reviewed by NOBODY (OOPS!). > > can you also open a bug about the Date function which uses createCFString? Will do. (In reply to Mark Lam from comment #5) > Comment on attachment 382877 [details] > proposed patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382877&action=review > > >> Source/WTF/ChangeLog:7 > >> + Reviewed by NOBODY (OOPS!). > > > > can you also open a bug about the Date function which uses createCFString? > > Will do. See https://bugs.webkit.org/show_bug.cgi?id=203875 Looks good to me. Sorry for the regression. Comment on attachment 382877 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=382877&action=review r=me as well, nice fix! > Source/WTF/wtf/RunLoop.h:56 > +#if USE(CF) > +using RunLoopMode = CFStringRef; > +#define DefaultRunLoopMode kCFRunLoopDefaultMode > +#else > +using RunLoopMode = unsigned; > +#define DefaultRunLoopMode 0 > +#endif Should we move these to be inside `RunLoop`? ``` class RunLoop : public FunctionDispatcher { ... #if USE(CF) using Mode = CFStringRef; using DefaultMode = kCFRunLoopDefaultMode; #else using Mode = unsigned; using DefaultMode = 0; #endif ... WTF_EXPORT_PRIVATE CycleResult static cycle(Mode = DefaultMode); ... } ``` Comment on attachment 382877 [details] proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=382877&action=review >> Source/WTF/wtf/RunLoop.h:56 >> +#endif > > Should we move these to be inside `RunLoop`? > > ``` > class RunLoop : public FunctionDispatcher { > ... > > #if USE(CF) > using Mode = CFStringRef; > using DefaultMode = kCFRunLoopDefaultMode; > #else > using Mode = unsigned; > using DefaultMode = 0; > #endif > > ... > > WTF_EXPORT_PRIVATE CycleResult static cycle(Mode = DefaultMode); > > ... > } > ``` This unfortunately is not possible because you can't do "using DefaultMode = kCFRunLoopDefaultMode;". We can only use "using" for types, and kCFRunLoopDefaultMode is not a type. We also can't use "static constexpr DefaultMode = kCFRunLoopDefaultMode;" because kCFRunLoopDefaultMode is not a constexpr expression. That's why I settled on the current form that uses the #define for DefaultRunLoopMode. I'll stick with this unless we run into a namespace collision issue. Thanks for the reviews. Landed in r252124: <http://trac.webkit.org/r252124>. |