Bug 202707 – IsLoggedIn: Add as experimental feature

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

We should add the proposed IsLoggedIn API as an experimental feature.

[Radar WebKit Bug Importer]

<rdar://problem/56095064>

[John Wilander]

Created attachment 380471 [details]
Patch

[Sam Weinig]

(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?

[John Wilander]

(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.

[John Wilander]

Created attachment 380477 [details]
Patch

[John Wilander]

The wincairo error seems unrelated and that bot has been shaky for a while. The win tree may be red.

[John Wilander]

Created attachment 380488 [details]
Patch

[John Wilander]

Created attachment 380499 [details]
Patch

[Brent Fulgham]

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!

[Chris Dumez]

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.

[John Wilander]

(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!

[John Wilander]

(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!

[John Wilander]

Created attachment 380567 [details]
Patch

[John Wilander]

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.

[WebKit Commit Bot]

Comment on attachment 380567 [details]
Patch

Clearing flags on attachment: 380567

Committed r250944: <https://trac.webkit.org/changeset/250944>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.