Move SOAuthorization from WebKitAdditions to WebKit.
<rdar://problem/47573431>
Created attachment 372158 [details] Patch
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.
The internal counterpart is on <rdar://problem/51768865>.
Created attachment 372164 [details] Patch
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.
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.
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.
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.
Created attachment 372265 [details] Patch for landing
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.
(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>.
Comment on attachment 372265 [details] Patch for landing Clearing flags on attachment: 372265 Committed r246514: <https://trac.webkit.org/changeset/246514>
http://trac.webkit.org/r246519
Another build fix: Committed r246527: <https://trac.webkit.org/changeset/246527>