[webkit-reviews] review granted: [Bug 215218] [ iOS wk2 ] editing/pasteboard/paste-without-nesting.html is a flaky failure : [Attachment 406260] Remove flaky test expectation too

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Wenson Hsieh
<wenson_hsieh at apple.com>'s request for review:
Bug 215218: [ iOS wk2 ] editing/pasteboard/paste-without-nesting.html is a
flaky failure
https://bugs.webkit.org/show_bug.cgi?id=215218

Attachment 406260: Remove flaky test expectation too

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




--- Comment #22 from Darin Adler <darin at apple.com> ---
Comment on attachment 406260
  --> https://bugs.webkit.org/attachment.cgi?id=406260
Remove flaky test expectation too

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

> Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:457
> +    if (auto* process = webProcessProxyForConnection(connection))

Writing just auto instead of auto* seems better for refactoring if we ever make
this return a smart pointer instead of a raw pointer.

> Source/WebKit/UIProcess/Cocoa/WebPasteboardProxyCocoa.mm:458
> +	  
process->send(Messages::WebProcess::DidWriteToPasteboardAsynchronously(pasteboa
rdName), 0);

What gives us test coverage to make sure we didn’t miss any cases? I assume the
flaky test only exercises one of these.

> Source/WebKit/WebProcess/WebCoreSupport/WebPlatformStrategies.cpp:173
> +   
WebProcess::singleton().waitForPendingPasteboardWritesToFinish(pasteboardName);

What gives us test coverage to make sure we didn’t miss any cases? I assume the
flaky test only exercises one of these.

> Source/WebKit/WebProcess/WebProcess.h:658
> +    HashMap<String, unsigned> m_pendingPasteboardWriteCounts;

Should use HashCountedSet instead. It’s designed for this kind of use.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1095
> +    while (m_pendingPasteboardWriteCounts.get(pasteboardName)) {

This does unnecessary extra work and should just check contains. We never have
a hash table entry with a 0 in it.

(Will change if you use HashCountedSet.)

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1096
> +	   if
(!parentProcessConnection()->waitForAndDispatchImmediately<Messages::WebProcess
::DidWriteToPasteboardAsynchronously>(0, 1_s,
IPC::WaitForOption::InterruptWaitingIfSyncMessageArrives))

How did we select a 1 second timeout? Because of the way this is written it
will loop forever anyway, so it’s not clear why it’s OK to time out and what we
will do in that case. And this case is untested. Sending extra messages seems a
bit dangerous? Not sure.

> Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm:1099
> +    m_pendingPasteboardWriteCounts.remove(pasteboardName);

This seems like a no op. The count should already have been removed by
didWriteToPasteboardAsynchronously.