Summary: | Use CoreAnimation fences instead of synchronous IPC to synchronize resize | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tim Horton <thorton> | ||||||||||||
Component: | New Bugs | Assignee: | Tim Horton <thorton> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | andersca, benjamin, cmarcelo, commit-queue, sam, simon.fraser | ||||||||||||
Priority: | P2 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | Unspecified | ||||||||||||||
OS: | Unspecified | ||||||||||||||
Attachments: |
|
Description
Tim Horton
2015-06-24 15:45:28 PDT
Created attachment 255521 [details]
Patch
Comment on attachment 255521 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255521&action=review Looks OK. Some problems that need to be fixed before landing. > Source/WebKit2/UIProcess/API/mac/WKView.mm:4438 > + // This SPI is only used on 10.9 and below, and is incompatible with the fence-based drawing area size synchronization in 10.10+. Maybe ASSERT_NOT_REACHED? > Source/WebKit2/UIProcess/API/mac/WKView.mm:4439 > +#if __MAC_OS_X_VERSION_MIN_REQUIRED <= 1090 Given the comment above, maybe #if !HAVE(COREANIMATION_FENCES) is a better thing to write here? > Source/WebKit2/UIProcess/API/mac/WKView.mm:4454 > + // This SPI is only used on 10.9 and below, and is incompatible with the fence-based drawing area size synchronization in 10.10+. Maybe ASSERT_NOT_REACHED? > Source/WebKit2/UIProcess/API/mac/WKView.mm:4455 > +#if __MAC_OS_X_VERSION_MIN_REQUIRED <= 1090 Given the comment above, maybe #if !HAVE(COREANIMATION_FENCES) is a better thing to write here? > Source/WebKit2/UIProcess/mac/TiledCoreAnimationDrawingAreaProxy.mm:168 > + m_webPageProxy.process().send(Messages::DrawingArea::UpdateGeometry(m_size, m_layerPosition, true, fencePort), m_webPageProxy.pageID()); > + > } Stray blank line here. I suggest omitting it. > Source/WebKit2/WebProcess/WebPage/DrawingArea.h:35 > +#include <WebCore/MachSendRight.h> This needs to be inside PLATFORM(COCOA) to avoid breaking GTK and EFL builds. I think it would be better just to forward declare as "class MachSendRight", since we only need to compile "const WebCore::MachSendRight&", and a forward declaration is sufficient for that. And those don’t necessarily need to be guarded with PLATFORM(COCOA). I don’t mind leaving the #if out, although some prefer that because they think it’s worth the extra complexity and ugliness to guarantee a compile time error if we try to use the type name at all. Created attachment 256467 [details]
Patch
> > Source/WebKit2/UIProcess/API/mac/WKView.mm:4439
> > +#if __MAC_OS_X_VERSION_MIN_REQUIRED <= 1090
>
> Given the comment above, maybe #if !HAVE(COREANIMATION_FENCES) is a better
> thing to write here?
I didn't do this because the code is really not used on 10.10, and leaving it this way means we'll get to delete it sooner :)
I made all your other changes, and added a lot more code re: the invalidation of the fences if a sync message arrives.
I think this needs an andersca review because of the IPC changes.
Attachment 256467 [details] did not pass style-queue:
ERROR: Source/WebKit2/Platform/IPC/Connection.h:203: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/Platform/IPC/Connection.h:310: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/Platform/IPC/Connection.cpp:708: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 3 in 20 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 256468 [details]
fix the build
Attachment 256468 [details] did not pass style-queue:
ERROR: Source/WebKit2/Platform/IPC/Connection.h:203: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/Platform/IPC/Connection.h:310: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/Platform/IPC/Connection.cpp:708: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 3 in 20 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 256471 [details]
Patch
Attachment 256471 [details] did not pass style-queue:
ERROR: Source/WebKit2/Platform/IPC/Connection.h:203: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/Platform/IPC/Connection.h:310: Extra space before ( in function call [whitespace/parens] [4]
ERROR: Source/WebKit2/Platform/IPC/Connection.cpp:708: Extra space before ( in function call [whitespace/parens] [4]
Total errors found: 3 in 20 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 256471 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=256471&action=review > Source/WebCore/platform/cocoa/MachSendRight.h:38 > + WEBCORE_EXPORT MachSendRight() = default; This doesn't need WEBCORE_EXPORT. > Source/WebKit2/Platform/IPC/Connection.cpp:221 > + , m_incomingSyncMessageCallbackQueue(WorkQueue::create("com.apple.WebKit.IPC.IncomingSyncMessageCallbackQueue")) This should be made on demand, there's no point in creating it for all the various connections that don't use this feature. > Source/WebKit2/Platform/IPC/Connection.cpp:713 > + std::lock_guard<std::mutex> lock(m_incomingSyncMessageCallbackMutex); > + m_incomingSyncMessageCallbacks.add(m_nextIncomingSyncMessageCallbackID, callback); > + > + return m_nextIncomingSyncMessageCallbackID++; Please change this to get the callback ID before incrementing it so you can make it start at 0. > Source/WebKit2/Platform/IPC/Connection.cpp:732 > + for (auto it = m_incomingMessages.begin(), end = m_incomingMessages.end(); it != end; ++it) { > + std::unique_ptr<MessageDecoder>& message = *it; > + if (message->isSyncMessage()) > + hasIncomingSyncMessage = true; > + } Modern for-loop please! > Source/WebKit2/Platform/IPC/Connection.h:312 > + uint64_t m_nextIncomingSyncMessageCallbackID { 1 }; Should start at zero here. Reopening to attach new patch. Created attachment 256617 [details]
tiny followup
|