Bug 146294 – Use CoreAnimation fences instead of synchronous IPC to synchronize resize

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Use CoreAnimation fences instead of synchronous IPC to synchronize resize

[Tim Horton]

Created attachment 255521 [details]
Patch

[Darin Adler]

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.

[Tim Horton]

Created attachment 256467 [details]
Patch

[Tim Horton]

> > 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.

[WebKit Commit Bot]

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.

[Tim Horton]

Created attachment 256468 [details]
fix the build

[WebKit Commit Bot]

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.

[Tim Horton]

Created attachment 256471 [details]
Patch

[WebKit Commit Bot]

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.

[Anders Carlsson]

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.

[Tim Horton]

http://trac.webkit.org/changeset/186662

[Tim Horton]

Reopening to attach new patch.

[Tim Horton]

Created attachment 256617 [details]
tiny followup

[Tim Horton]

And http://trac.webkit.org/changeset/186693