We should add the proposed IsLoggedIn API as an experimental feature.
<rdar://problem/56095064>
Created attachment 380471 [details] Patch
(In reply to John Wilander from comment #0) > We should add the proposed IsLoggedIn API as an experimental feature. Where are these proposed? Can you add a link to the spec in the ChangeLog?
(In reply to Sam Weinig from comment #3) > (In reply to John Wilander from comment #0) > > We should add the proposed IsLoggedIn API as an experimental feature. > > Where are these proposed? Can you add a link to the spec in the ChangeLog? Proposed to the WebAppSec WG at TPAC. So far there is only an explainer posted to the mailing list: https://lists.w3.org/Archives/Public/public-webappsec/2019Sep/0004.html We're working on getting approval to add it to the WICG. I will add the mailing list link to the change log entry.
Created attachment 380477 [details] Patch
The wincairo error seems unrelated and that bot has been shaky for a while. The win tree may be red.
Created attachment 380488 [details] Patch
Created attachment 380499 [details] Patch
Comment on attachment 380499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380499&action=review I don't think the api-mac failure is related to this patch, but please double-check. It also looks like the patch does not apply cleanly to the tree. Otherwise, this seems fine though I would suggest removing the 'm_navigator' member of the class, since it is not used anywhere. > Source/WebCore/page/NavigatorIsLoggedIn.h:55 > + Navigator& m_navigator; I don't think you need to hold this reference to Navigator, since the entire API expects to receive the Navigator as an argument. > LayoutTests/ChangeLog:40 > + * http/tests/is-logged-in/unavailable-in-insecure-contexts.html: Added. Whoops! Double changelog!
Comment on attachment 380499 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=380499&action=review > Source/WebCore/ChangeLog:15 > + - Promise<void> setLoggedOut() Could this be a single method taking a boolean parameter instead of 2 separate ones? > Source/WebCore/page/NavigatorIsLoggedIn.cpp:80 > + promise->resolve<IDLBoolean>(true); Seems like this could rely on m_navigator.cookieEnabled() at least? like setLoggedIn(), no? > Source/WebCore/page/NavigatorIsLoggedIn.h:29 > +#include <wtf/WeakPtr.h> What is this for? > Source/WebCore/page/NavigatorIsLoggedIn.h:36 > +class NavigatorIsLoggedIn final : public Supplement<Navigator>, public CanMakeWeakPtr<NavigatorIsLoggedIn> { What is CanMakeWeakPtr for? Also, spacing seems wrong before the curly bracket. > Source/WebCore/page/NavigatorIsLoggedIn.idl:29 > +] partial interface Navigator { Should this be exposed to workers or no? > Source/WebCore/page/RuntimeEnabledFeatures.h:389 > + bool isLoggedInEnabled() const { return m_isLoggedInEnabled; } isLoggedInAPIEnabled would look better IMHO, with "API" in there. > Source/WebCore/page/RuntimeEnabledFeatures.h:390 > + void setIsLoggedInEnabled(bool isEnabled) { m_isLoggedInEnabled = isEnabled; } Unless this needs to be accessed from multiple threads, I believe we prefer new flags to be on Settings, not RuntimeEnabledFeatures.
(In reply to Brent Fulgham from comment #9) > Comment on attachment 380499 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380499&action=review > > I don't think the api-mac failure is related to this patch, but please > double-check. Yeah, I don't know what's up with that test. I'll do another EWS pass before putting the patch on the queue. > It also looks like the patch does not apply cleanly to the tree. Will fix. > Otherwise, this seems fine though I would suggest removing the 'm_navigator' > member of the class, since it is not used anywhere. Sure. > > Source/WebCore/page/NavigatorIsLoggedIn.h:55 > > + Navigator& m_navigator; > > I don't think you need to hold this reference to Navigator, since the entire > API expects to receive the Navigator as an argument. True. > > LayoutTests/ChangeLog:40 > > + * http/tests/is-logged-in/unavailable-in-insecure-contexts.html: Added. > > Whoops! Double changelog! Good catch. Thanks!
(In reply to Chris Dumez from comment #10) > Comment on attachment 380499 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=380499&action=review > > > Source/WebCore/ChangeLog:15 > > + - Promise<void> setLoggedOut() > > Could this be a single method taking a boolean parameter instead of 2 > separate ones? That was brought up early on but it's currently proposed as two different functions. Many things will be hashed out in the incubator group anyway so I'd like to keep the experimental feature work aligned with what we have documented. > > Source/WebCore/page/NavigatorIsLoggedIn.cpp:80 > > + promise->resolve<IDLBoolean>(true); > > Seems like this could rely on m_navigator.cookieEnabled() at least? like > setLoggedIn(), no? Absolutely. The cookieEnabled() part was just me adding a first little piece of functionality. > > Source/WebCore/page/NavigatorIsLoggedIn.h:29 > > +#include <wtf/WeakPtr.h> > > What is this for? This is in preparation for asynchronous calls into underlying things for the real functionality. We will need a weak pointer to the navigator for the completion handler. But I can remove it now and add it when needed. Is that better? > > Source/WebCore/page/NavigatorIsLoggedIn.h:36 > > +class NavigatorIsLoggedIn final : public Supplement<Navigator>, public CanMakeWeakPtr<NavigatorIsLoggedIn> { > > What is CanMakeWeakPtr for? > Also, spacing seems wrong before the curly bracket. > > > Source/WebCore/page/NavigatorIsLoggedIn.idl:29 > > +] partial interface Navigator { > > Should this be exposed to workers or no? Probably not. You mean I need an Exposed attribute? > > Source/WebCore/page/RuntimeEnabledFeatures.h:389 > > + bool isLoggedInEnabled() const { return m_isLoggedInEnabled; } > > isLoggedInAPIEnabled would look better IMHO, with "API" in there. Good idea. Will fix. > > Source/WebCore/page/RuntimeEnabledFeatures.h:390 > > + void setIsLoggedInEnabled(bool isEnabled) { m_isLoggedInEnabled = isEnabled; } > > Unless this needs to be accessed from multiple threads, I believe we prefer > new flags to be on Settings, not RuntimeEnabledFeatures. Aha. Will fix. Thanks!
Created attachment 380567 [details] Patch
Comment on attachment 380567 [details] Patch Brent, m_navigator is used. It's sent to the contractor when the supplement is created and then (currently) used to check that cookies are enabled. Waiting for EWS results before putting it on the queue.
Comment on attachment 380567 [details] Patch Clearing flags on attachment: 380567 Committed r250944: <https://trac.webkit.org/changeset/250944>
All reviewed patches have been landed. Closing bug.