Move animated resize into the layer tree transaction, and make it asynchronous

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Move animated resize into the layer tree transaction, and make it asynchronous

[Tim Horton]

Created attachment 341649 [details]
Patch

[Tim Horton]

Not sure this is 100% done (and it definitely needs a changelog), but it’s getting there.

[Tim Horton]

Created attachment 341717 [details]
Patch

[Sam Weinig]

Very cool!

Given how complex this logic is, do you have any ideas about how we could add automated tests for this?

[Tim Horton]

Pretty sure we already have API tests for animated resize. I’m not sure it’s worth it to write an API test for “animated resize doesn’t synchronously block the UI process”, though.

[Tim Horton]

It is possible we could test some of the smaller changes, though.

[Simon Fraser (smfr)]

Comment on attachment 341717 [details]
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.

[Tim Horton]

Created attachment 342020 [details]
Patch

[Tim Horton]

Still have to make the crash case work right. There’s a bunch of preexisting problems, I’m just adding some more :)

[Tim Horton]

Created attachment 342036 [details]
Patch

[Tim Horton]

Created attachment 342037 [details]
Patch

[Tim Horton]

(In reply to Tim Horton from comment #9)
> Still have to make the crash case work right. There’s a bunch of preexisting
> problems, I’m just adding some more :)

Not true! It’s actually fine. But we should clean the existing mechanism up, because it duplicates a lot of code.

[Tim Horton]

<rdar://problem/38477288>

[EWS Watchlist]

Comment on attachment 342037 [details]
Patch

Attachment 342037 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/8027639

New failing tests:
storage/domstorage/storage-close-database-on-idle.html

[EWS Watchlist]

Created attachment 342041 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[WebKit Commit Bot]

Comment on attachment 342037 [details]
Patch

Clearing flags on attachment: 342037

Committed r232544: <https://trac.webkit.org/changeset/232544>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.