Bug 198874 – Move SOAuthorization from WebKitAdditions to WebKit

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

Move SOAuthorization from WebKitAdditions to WebKit.

[Jiewen Tan]

<rdar://problem/47573431>

[Jiewen Tan]

Created attachment 372158 [details]
Patch

[Jiewen Tan]

Comment on attachment 372158 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:110
> +

Deleted.

[Jiewen Tan]

The internal counterpart is on <rdar://problem/51768865>.

[Jiewen Tan]

Created attachment 372164 [details]
Patch

[Brent Fulgham]

Comment on attachment 372164 [details]
Patch

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

r=me. Nice to get it into Open Source finally.

> Source/WebKit/ChangeLog:10
> +        Besides, it replaces all the load optimizer nonsenses with SOAuthorization.

I'd say: "It also replaces the LoadOptimizer nonsense with the actual SOAuthorization API.

[Darin Adler]

Comment on attachment 372164 [details]
Patch

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

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.h:42
> +class NavigationSOAuthorizationSession : public WebViewDidMoveToWindowObserver, public SOAuthorizationSession {

Can either of the base classes be private or protected instead of pubic?

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.h:44
> +    using Callback = CompletionHandler<void(bool)>;

Can this be protected instead of public?

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.h:47
> +    using WebViewDidMoveToWindowObserver::weakPtrFactory;
> +    using WeakValueType = WebViewDidMoveToWindowObserver::WeakValueType;

Can either of these be protected or private?

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.h:52
> +    // WebViewDidMoveToWindowObserver
> +    void webViewDidMoveToWindow() final;

This could be private. Why public?

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.h:57
> +    void callback(bool intercepted) { m_callback(intercepted); }

The word "callback" is a noun, not a verb. So this function should ideally be named something like invokeCallback.

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/PopUpSOAuthorizationSession.h:47
> +    static Ref<SOAuthorizationSession> create(SOAuthorization *soAuthorization, WebPageProxy& page, Ref<API::NavigationAction>&& navigationAction, NewPageCallback&& newPageCallback, UIClientCallback&& uiClientCallback)

I think it’s a slightly better pattern to have the function body be in the .cpp file. Nicer to inline the constructor in the not-inlined create function instead of inlining the create function at the call site and calling the constructor out of line. If this is called more than once, it’s better for code size to share a single create function. Also a tiny bit better to have a little less code in headers.

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/PopUpSOAuthorizationSession.mm:40
> +@package

Why @package? This is private within a single file so it seems @private would be OK.

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/PopUpSOAuthorizationSession.mm:42
> +WeakPtr<WebKit::PopUpSOAuthorizationSession> _session;
> +BOOL _isFirstNavigation;

normally we would indent these.

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/PopUpSOAuthorizationSession.mm:53
> +    if (self = [super init]) {

Usually requires an extra set of parentheses in this line of code to avoid a warning. I’m surprised there is no warning!

> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationCoordinator.h:64
> +    inline bool canAuthorize(const URL&) const;

This use of "inline" is not valuable. I think you can omit it.

[Jiewen Tan]

Comment on attachment 372164 [details]
Patch

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

Thanks Brent for r+ this patch.

>> Source/WebKit/ChangeLog:10
>> +        Besides, it replaces all the load optimizer nonsenses with SOAuthorization.
> 
> I'd say: "It also replaces the LoadOptimizer nonsense with the actual SOAuthorization API.

Fixed.

[Jiewen Tan]

Comment on attachment 372164 [details]
Patch

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

Thanks Darin for reviewing the patch.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.h:42
>> +class NavigationSOAuthorizationSession : public WebViewDidMoveToWindowObserver, public SOAuthorizationSession {
> 
> Can either of the base classes be private or protected instead of pubic?

Moved WebViewDidMoveToWindowObserver to private and keep SOAuthorizationSession public in order to use RefCounted/WeakPtr.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.h:44
>> +    using Callback = CompletionHandler<void(bool)>;
> 
> Can this be protected instead of public?

Yes.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.h:47
>> +    using WeakValueType = WebViewDidMoveToWindowObserver::WeakValueType;
> 
> Can either of these be protected or private?

I don't think so as WebPageProxy::addObserver needs to make WeakPtrs for NavigationSOAuthorizationSession objects.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.h:52
>> +    void webViewDidMoveToWindow() final;
> 
> This could be private. Why public?

True.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/NavigationSOAuthorizationSession.h:57
>> +    void callback(bool intercepted) { m_callback(intercepted); }
> 
> The word "callback" is a noun, not a verb. So this function should ideally be named something like invokeCallback.

Fixed.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/PopUpSOAuthorizationSession.h:47
>> +    static Ref<SOAuthorizationSession> create(SOAuthorization *soAuthorization, WebPageProxy& page, Ref<API::NavigationAction>&& navigationAction, NewPageCallback&& newPageCallback, UIClientCallback&& uiClientCallback)
> 
> I think it’s a slightly better pattern to have the function body be in the .cpp file. Nicer to inline the constructor in the not-inlined create function instead of inlining the create function at the call site and calling the constructor out of line. If this is called more than once, it’s better for code size to share a single create function. Also a tiny bit better to have a little less code in headers.

Fixed.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/PopUpSOAuthorizationSession.mm:40
>> +@package
> 
> Why @package? This is private within a single file so it seems @private would be OK.

Fixed.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/PopUpSOAuthorizationSession.mm:42
>> +BOOL _isFirstNavigation;
> 
> normally we would indent these.

Fixed.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/PopUpSOAuthorizationSession.mm:53
>> +    if (self = [super init]) {
> 
> Usually requires an extra set of parentheses in this line of code to avoid a warning. I’m surprised there is no warning!

Added in case anything happens later.

>> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationCoordinator.h:64
>> +    inline bool canAuthorize(const URL&) const;
> 
> This use of "inline" is not valuable. I think you can omit it.

Removed.

[Jiewen Tan]

Created attachment 372265 [details]
Patch for landing

[Sam Weinig]

Is "SO" a common enough acronym that we should use it here? I assume it stands for "sign-on", but I am not totally sure.

[Jiewen Tan]

(In reply to Sam Weinig from comment #11)
> Is "SO" a common enough acronym that we should use it here? I assume it
> stands for "sign-on", but I am not totally sure.

Probably not. I followed the naming conversion on the SPI that I called, and therefore it is easier to connect them together instead of having our own names, for example, @interface WKSOAuthorizationDelegate : NSObject <SOAuthorizationDelegate>.

[WebKit Commit Bot]

Comment on attachment 372265 [details]
Patch for landing

Clearing flags on attachment: 372265

Committed r246514: <https://trac.webkit.org/changeset/246514>

[Alex Christensen]

http://trac.webkit.org/r246519

[Jiewen Tan]

Another build fix:
Committed r246527: <https://trac.webkit.org/changeset/246527>