Implement a new SPI to inform clients about AppSSO.
<rdar://problem/50028246>
Created attachment 372598 [details] Patch
GTK build error: Source/WebKit/CMakeFiles/WebKit.dir/__/__/DerivedSources/WebKit/VisitedLinkStoreMessageReceiver.cpp.o -c /home/ews/gtk-wk2-ews/WebKit/WebKitBuild/Release/DerivedSources/WebKit/VisitedLinkStoreMessageReceiver.cpp In file included from ../../Source/WebKit/UIProcess/VisitedLinkStore.h:31, from /home/ews/gtk-wk2-ews/WebKit/WebKitBuild/Release/DerivedSources/WebKit/VisitedLinkStoreMessageReceiver.cpp:27: ../../Source/WebKit/UIProcess/WebPageProxy.h:44:10: fatal error: SOAuthorizationLoadPolicy.h: No such file or directory #include "SOAuthorizationLoadPolicy.h" ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~ compilation terminated.
Comment on attachment 372598 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372598&action=review This seems reasonable, but we need a WK API review (ggaren?), and please fix the build issues I highlighted. r- to fix the build. > Source/WebKit/UIProcess/API/APINavigationClient.h:35 > +#include "SOAuthorizationLoadPolicy.h" This needs to be protected with HAVE(APP_SSO). > Source/WebKit/UIProcess/WebPageProxy.h:44 > +#include "SOAuthorizationLoadPolicy.h" This needs to be protected by HAVE(APP_SSO)
(In reply to Brent Fulgham from comment #4) > Comment on attachment 372598 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372598&action=review > > This seems reasonable, but we need a WK API review (ggaren?), and please fix > the build issues I highlighted. r- to fix the build. > > > Source/WebKit/UIProcess/API/APINavigationClient.h:35 > > +#include "SOAuthorizationLoadPolicy.h" > > This needs to be protected with HAVE(APP_SSO). > > > Source/WebKit/UIProcess/WebPageProxy.h:44 > > +#include "SOAuthorizationLoadPolicy.h" > > This needs to be protected by HAVE(APP_SSO) Thanks Brent for reviewing the patch. I was working on another branch locally, and therefore couldn't make this change timely. Also, I might just remove the extension name in this SPI as we might not be able to get that information before the session start. Also, since the UI can be shown out of process, it might not event make sense to misguide clients to show that name. Therefore, I might need a separate SPI to tell clients what extension is showing when the UI is actually handled by the UI process.
Created attachment 372662 [details] Patch
Comment on attachment 372662 [details] 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.
Comment on attachment 372662 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372662&action=review Thanks Geoff for r+ this patch. >> 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 Fixed. >> Source/WebKit/ChangeLog:12 >> + pass along a human readable name for the extension such that clients could do whatever they > > could do => can do Fixed. >> Source/WebKit/UIProcess/Cocoa/SOAuthorization/SOAuthorizationSession.mm:109 >> + 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. Ultimately, the website datastore will keep it alive. I don't know why we would want to use a strong reference to extend the lifetime of the current SOAuthorizationSession if the corresponding datastore dies or another SOAuthorizationSession suppresses the current one. Therefore, a weakPtr seems appropriate. With regard to adding a protector like strong reference to the lambda scope, I feel like it is not necessary. If this becomes null at any point of the execution of this lambda, that means there is a bug at any of those method this lambda tries to call. Adding a protector would effectively hide the real bug. For readability, I could capture this as well. And then, we could just use the weakThis once at the very beginning of the lambda. >> 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. Fixed. >> Tools/TestWebKitAPI/Tests/WebKitCocoa/TestSOAuthorization.mm:189 >> + } > > 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. Sounds great. Added.
Created attachment 372894 [details] Patch for landing
Comment on attachment 372894 [details] Patch for landing Clearing flags on attachment: 372894 Committed r246829: <https://trac.webkit.org/changeset/246829>