[webkit-reviews] review denied: [Bug 213415] [AppSSO] Use protectedThis for dismissViewController() : [Attachment 402343] 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 213415: [AppSSO] Use protectedThis for dismissViewController()
https://bugs.webkit.org/show_bug.cgi?id=213415

Attachment 402343: Patch

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




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

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

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:305
> -		   dismissViewController();
> +		   protectedThis->dismissViewController();

There are tons of cases where we capture "this" in protectedThis, but then use
"this" relying on the protection indirectly. Why is it important to change this
one? I see no reason to make this change, but to leave the code two lines later
using this to null out m_presentingWindowDidDeminiaturizeObserver.

I like the idea of always using protectedThis, but I would do this and not even
capture this, and even then, I am not sure we need to make a change. It’s a
repeating pattern in a lot of different code in WebKit.