[webkit-reviews] review denied: [Bug 214816] SOAuthorizationSession::dismissViewController presentingWindow could be null : [Attachment 405261] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied Jiewen Tan <jiewen_tan at apple.com>'s
request for review:
Bug 214816: SOAuthorizationSession::dismissViewController presentingWindow
could be null
https://bugs.webkit.org/show_bug.cgi?id=214816

Attachment 405261: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 405261
  --> https://bugs.webkit.org/attachment.cgi?id=405261
Patch

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

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:300
>      if (m_page && m_page->platformWindow()) {

WebKit coding style proposes a style called early exit. Since this code already
has "return" statements in it, I think it’s a good candidate for that style.

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:301
> +	   if (auto *presentingWindow = m_page->platformWindow()) {

Why is it valuable to check this for null again? The line of code just above
this already checks it for null. This patch seems to be based on an incorrect
assumption that it’s not.