Bug 205041 – IsLoggedIn: Abstract data type for IsLoggedIn state

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[John Wilander]

We need data representation for IsLoggedIn state.

[John Wilander]

<rdar://problem/56723904>

[John Wilander]

Created attachment 385217 [details]
Patch

[John Wilander]

The ios-wk2 bot failure is an unrelated text diff thing.

[Chris Dumez]

Comment on attachment 385217 [details]
Patch

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

> Source/WebCore/page/IsLoggedIn.cpp:33
> +    initialize(domain, username, tokenType, authType, m_authType == IsLoggedInAuthenticationType::Unmanaged ? LoggedInExpiryShort : LoggedInExpiryLong);

Using m_authType here is wrong.

> Source/WebCore/page/IsLoggedIn.cpp:41
> +void IsLoggedIn::initialize(const RegistrableDomain& domain, const String& username, IsLoggedInCredentialTokenType tokenType, IsLoggedInAuthenticationType authType, const Seconds& expiry)

Why don't we have one constructor call the other instead of having an initialize() method?

> Source/WebCore/page/IsLoggedIn.cpp:53
> +    for (unsigned i = 0; i < length; ++i) {

Seems like this could use String::find(CodeUnitMatchFunction)

> Source/WebCore/page/IsLoggedIn.cpp:71
> +void IsLoggedIn::setExpiry(const Seconds& expiry)

no need for const &.

> Source/WebCore/page/IsLoggedIn.cpp:76
> +    if (m_authType == IsLoggedInAuthenticationType::Unmanaged)

m_expiryTime = m_loggedInTime + std::min(expiry, m_authType == IsLoggedInAuthenticationType::Unmanaged ? LoggedInExpiryShort : LoggedInExpiryLong);

> Source/WebCore/page/IsLoggedIn.h:35
> +enum IsLoggedInCredentialTokenType : bool { LegacyCookie, HttpStateToken };

enum class
Http -> HTTP

> Source/WebCore/page/IsLoggedIn.h:36
> +enum IsLoggedInAuthenticationType : uint8_t { WebAuthn, PasswordManager, Unmanaged };

enum class

> Source/WebCore/page/IsLoggedIn.h:40
> +

We should probably drop this blank line.

> Source/WebCore/page/IsLoggedIn.h:51
> +    WEBCORE_EXPORT void setExpiry(const Seconds&);

It is confusing that expiry() returns a expiration date/time as a MonotonicTime, but setExpiry() sets a lifetime in seconds. Maybe this should be called setLifetime()?
?

> Source/WebCore/page/IsLoggedIn.h:54
> +    RegistrableDomain registrableDomain() const { return m_domain; }

const RegistrableDomain&

> Source/WebCore/page/IsLoggedIn.h:55
> +    String username() const { return m_username; }

const String&

[John Wilander]

(In reply to Chris Dumez from comment #4)
> Comment on attachment 385217 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385217&action=review
> 
> > Source/WebCore/page/IsLoggedIn.cpp:33
> > +    initialize(domain, username, tokenType, authType, m_authType == IsLoggedInAuthenticationType::Unmanaged ? LoggedInExpiryShort : LoggedInExpiryLong);
> 
> Using m_authType here is wrong.

You're right! Good catch.

> > Source/WebCore/page/IsLoggedIn.cpp:41
> > +void IsLoggedIn::initialize(const RegistrableDomain& domain, const String& username, IsLoggedInCredentialTokenType tokenType, IsLoggedInAuthenticationType authType, const Seconds& expiry)
> 
> Why don't we have one constructor call the other instead of having an
> initialize() method?

You mean delegating constructors. I thought of that but remember seeing other places where we seemed to prefer an initialize() function. But I'll try the delegating approach.

> > Source/WebCore/page/IsLoggedIn.cpp:53
> > +    for (unsigned i = 0; i < length; ++i) {
> 
> Seems like this could use String::find(CodeUnitMatchFunction)

Have not used that before. Will have a look.

> > Source/WebCore/page/IsLoggedIn.cpp:71
> > +void IsLoggedIn::setExpiry(const Seconds& expiry)
> 
> no need for const &.

Ah. There are a few instances of it in WebKit but not too many. Will fix.

> > Source/WebCore/page/IsLoggedIn.cpp:76
> > +    if (m_authType == IsLoggedInAuthenticationType::Unmanaged)
> 
> m_expiryTime = m_loggedInTime + std::min(expiry, m_authType ==
> IsLoggedInAuthenticationType::Unmanaged ? LoggedInExpiryShort :
> LoggedInExpiryLong);

Good suggestion.

> > Source/WebCore/page/IsLoggedIn.h:35
> > +enum IsLoggedInCredentialTokenType : bool { LegacyCookie, HttpStateToken };
> 
> enum class

Will fix.

> Http -> HTTP

Will fix.

> > Source/WebCore/page/IsLoggedIn.h:36
> > +enum IsLoggedInAuthenticationType : uint8_t { WebAuthn, PasswordManager, Unmanaged };
> 
> enum class

Will fix.

> > Source/WebCore/page/IsLoggedIn.h:40
> > +
> 
> We should probably drop this blank line.

Will fix.

> > Source/WebCore/page/IsLoggedIn.h:51
> > +    WEBCORE_EXPORT void setExpiry(const Seconds&);
> 
> It is confusing that expiry() returns a expiration date/time as a
> MonotonicTime, but setExpiry() sets a lifetime in seconds. Maybe this should
> be called setLifetime()?
> ?

Hmm. I'll think about it and get them aligned.

> > Source/WebCore/page/IsLoggedIn.h:54
> > +    RegistrableDomain registrableDomain() const { return m_domain; }
> 
> const RegistrableDomain&

Will fix.

> > Source/WebCore/page/IsLoggedIn.h:55
> > +    String username() const { return m_username; }
> 
> const String&

Will fix.

Thanks, Chris! Excellent feedback.

[John Wilander]

Created attachment 385327 [details]
Patch

[John Wilander]

Created attachment 385329 [details]
Patch

[John Wilander]

Forgot to update the change log with the fixed function name.

[John Wilander]

The api-ios test failure for TestWebKitAPI.WebKit.MediaCache is unrelated.

[Chris Dumez]

Comment on attachment 385329 [details]
Patch

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

> Source/WebCore/page/IsLoggedIn.cpp:48
> +    auto spaceOrNewline = username.find([](UChar ch) {

Wouldn't this work?
auto spaceOrNewline = username.find(isSpaceOrNewline);

> Source/WebCore/page/IsLoggedIn.h:35
> +enum class IsLoggedInCredentialTokenType : bool { LegacyCookie, HTTPStateToken };

Given that you have IsLoggedIn in the name of your enum, you might as well move it inside the class scope and drop the IsLoggedIn prefix.

> Source/WebCore/page/IsLoggedIn.h:36
> +enum class IsLoggedInAuthenticationType : uint8_t { WebAuthn, PasswordManager, Unmanaged };

Ditto.

> Source/WebCore/page/IsLoggedIn.h:38
> +class IsLoggedIn {

As discussed offline, IsLoggedIn is an off name for a class IMHO. I'd prefer something like LoggedInStatus

> Source/WebCore/page/IsLoggedIn.h:63
> +    IsLoggedInCredentialTokenType m_tokenType;

Your class has a default constructor but this does not have a default value.

> Source/WebCore/page/IsLoggedIn.h:64
> +    IsLoggedInAuthenticationType m_authType;

ditto.

> Source/WebCore/page/IsLoggedIn.h:66
> +    MonotonicTime m_expiryTime;

Note that if you are going to persist these to disk, then I do not think that using MonotonicTime is safe. IIRC, MonotonicTime gets reset on device restart.

> Source/WebCore/page/IsLoggedIn.h:73
> +template<> struct EnumTraits<WebCore::IsLoggedInAuthenticationType> {

Can you please check with Alex to see if we really still need EnumTraits?

[John Wilander]

Created attachment 385467 [details]
Patch

[John Wilander]

The style checker complained when I indented the initializer list for the constructor:

ERROR: Source/WebCore/page/LoggedInStatus.cpp:63:  Code inside a namespace should not be indented.  [whitespace/indent] [4]

That's why it's not indented. Looks ugly.

[John Wilander]

We still need EnumTraits according to Alex and I wasn’t able to use username.find(isSpaceOrNewline).

[Chris Dumez]

Comment on attachment 385467 [details]
Patch

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

> Source/WebCore/page/LoggedInStatus.cpp:62
> +: m_domain { domain }

Indentation problem.

> Source/WebCore/page/LoggedInStatus.h:39
> +class LoggedInStatus : public RefCounted<LoggedInStatus> {

Without seeing how this will be used, it is hard for me to comment on whether or not this should subclass RefCounted.

> Source/WebCore/page/LoggedInStatus.h:62
> +    using RefCounted::ref;

Why is this needed?

> Source/WebCore/page/LoggedInStatus.h:63
> +    using RefCounted::deref;

Ditto.

[John Wilander]

Created attachment 385617 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 385617 [details]
Patch for landing

Clearing flags on attachment: 385617

Committed r253489: <https://trac.webkit.org/changeset/253489>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[John Wilander]

(In reply to Chris Dumez from comment #14)
> Comment on attachment 385467 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385467&action=review
> 
> > Source/WebCore/page/LoggedInStatus.cpp:62
> > +: m_domain { domain }
> 
> Indentation problem.

Yeah, it was the style checker complaining. I fixed it though.

> > Source/WebCore/page/LoggedInStatus.h:39
> > +class LoggedInStatus : public RefCounted<LoggedInStatus> {
> 
> Without seeing how this will be used, it is hard for me to comment on
> whether or not this should subclass RefCounted.

As discussed offline, I switched to a unique ref.

> > Source/WebCore/page/LoggedInStatus.h:62
> > +    using RefCounted::ref;
> 
> Why is this needed?

No longer. It was some build issue I addressed with it but that might have been when I was figuring out the ISO heap allocation. Might have been a red herring.

> > Source/WebCore/page/LoggedInStatus.h:63
> > +    using RefCounted::deref;
> 
> Ditto.

No longer.

Thanks, Chris!