[webkit-reviews] review granted: [Bug 199085] Implement a new SPI to inform clients about AppSSO : [Attachment 372662] Patch

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has granted Jiewen Tan
<jiewen_tan at apple.com>'s request for review:
Bug 199085: Implement a new SPI to inform clients about AppSSO
https://bugs.webkit.org/show_bug.cgi?id=199085

Attachment 372662: Patch

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




--- Comment #7 from Geoffrey Garen <ggaren at apple.com> ---
Comment on attachment 372662
  --> https://bugs.webkit.org/attachment.cgi?id=372662
Patch

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

r=me

> Source/WebKit/ChangeLog:10
> +	   navigations. Therefore, clients can make a informed decision whether
this is the right moment

make a informed decision whether => make an informed decision about whether

> Source/WebKit/ChangeLog:12
> +	   pass along a human readable name for the extension such that clients
could do whatever they

could do => can do

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:109
> +	   if (!weakThis)
> +	       return;

Why use a weak reference instead of a strong reference? What will keep the
SOAuthorizationSession alive if the call to decidePolicyForSOAuthorizationLoad
returns asynchronously?

If we do want to use a weak reference, after checking the weak reference, we
should put it into a strong reference and then use data members directly
instead of using weakThis->, for both readability and protection from null
pointer crashes.

Or, if we switch to capturing a strong reference, we can also use data members
directly.

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:119
> +	   // FIXME<rdar://problem/48909336>: Replace the below with AppSSO
constants.

FIXME: <rdar://problem/48909336> Replace the below with AppSSO constants.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/TestSOAuthorization.mm:189
> +    if (self.allowSOAuthorizationLoad) {
> +	   completionHandler(policy);
> +	   return;
> +    }

Seems like we should also have a test for invoking the completion handler on
the next tick of the runloop, in case that exposes any object lifetime issues.