[webkit-reviews] review granted: [Bug 186130] Move animated resize into the layer tree transaction, and make it asynchronous : [Attachment 341717] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Tim Horton
<thorton at apple.com>'s request for review:
Bug 186130: Move animated resize into the layer tree transaction, and make it
asynchronous
https://bugs.webkit.org/show_bug.cgi?id=186130

Attachment 341717: Patch

https://bugs.webkit.org/attachment.cgi?id=341717&action=review




--- Comment #7 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 341717
  --> https://bugs.webkit.org/attachment.cgi?id=341717
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341717&action=review

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreeTransaction.h:274
> +    std::optional<uint64_t> dynamicViewportSizeUpdateID() const { return
m_dynamicViewportSizeUpdateID; }

Why do we not have typedefs for uint64_t things.

> Source/WebKit/Shared/RemoteLayerTree/RemoteLayerTreeTransaction.h:312
> +    std::optional<uint64_t> m_dynamicViewportSizeUpdateID { std::nullopt };

No need to initialize to nullopt. Same with the one above.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:352
> +    BOOL _waitingForEndAnimatedResize;
> +    BOOL _waitingForCommitAfterAnimatedResize;

Do these get cleared in "the web process did crash" code path?

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1758
> +- (void)_didCommitLayerTreeDuringAnimatedResize:(const
WebKit::RemoteLayerTreeTransaction&)layerTreeTransaction

Things kinda sounds like it should return a BOOL, but I can't think of a better
name.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:2917
> +    if (!_lastSentViewLayoutSize || newViewLayoutSize !=
_lastSentViewLayoutSize.value())
> +	   [self _dispatchSetViewLayoutSize:newViewLayoutSize];
> +    if (!_lastSentMaximumUnobscuredSize || newMaximumUnobscuredSize !=
_lastSentMaximumUnobscuredSize.value())
> +	   [self
_dispatchSetMaximumUnobscuredSize:WebCore::FloatSize(newMaximumUnobscuredSize)]
;
> +    if (!_lastSentDeviceOrientation || newOrientation !=
_lastSentDeviceOrientation.value())
> +	   [self _dispatchSetDeviceOrientation:newOrientation];

Some blank lines between these would be nice.

> Source/WebKit/UIProcess/ios/WebPageProxyIOS.mm:316
> +   
m_process->send(Messages::WebPage::DynamicViewportSizeUpdate(viewLayoutSize,
maximumUnobscuredSize, targetExposedContentRect, targetUnobscuredRect,
targetUnobscuredRectInScrollViewCoordinates, unobscuredSafeAreaInsets,
targetScale, deviceOrientation, dynamicViewportSizeUpdateID), m_pageID);

Maybe unwrap this.

> Source/WebKit/WebProcess/WebPage/WebPage.h:1658
>      uint64_t m_currentAssistedNodeIdentifier { 0 };

Oh look a uint64_t that's something totally different.