[webkit-reviews] review denied: [Bug 215460] [Cocoa] Avoid changing XPC target queue inside XPC event handler : [Attachment 406604] Patch

[bugzilla-daemon at webkit.org]

Chris Dumez <cdumez at apple.com> has denied Per Arne Vollan <pvollan at apple.com>'s
request for review:
Bug 215460: [Cocoa] Avoid changing XPC target queue inside XPC event handler
https://bugs.webkit.org/show_bug.cgi?id=215460

Attachment 406604: Patch

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




--- Comment #12 from Chris Dumez <cdumez at apple.com> ---
Comment on attachment 406604
  --> https://bugs.webkit.org/attachment.cgi?id=406604
Patch

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

This change seems wrong. What if the client app starts a load (the load gets
queued by your new logic) and then the client app cancels the load (calling
WebPageProxy::stopLoading()). It won't cancel the load anymore after your
change.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1589
> +	   WTFLogAlways("Launch Services database not updated when load
requested.");

I assume you did not mean to keep a WTFLogAlways() in your patch?

> Source/WebKit/WebProcess/WebPage/WebPage.h:2151
> +    Optional<Vector<LoadParameters>> m_pendingLoads;

Why do we need an Optional<Vector>? Cannot we simply use the fact that the
vector is empty as a hint?