[webkit-reviews] review granted: [Bug 136611] Pass certificate info as part of ResourceResponse : [Attachment 237774] patch 6

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Antti Koivisto <koivisto at iki.fi>'s
request for review:
Bug 136611: Pass certificate info as part of ResourceResponse
https://bugs.webkit.org/show_bug.cgi?id=136611

Attachment 237774: patch 6
https://bugs.webkit.org/attachment.cgi?id=237774&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=237774&action=review


> Source/WebCore/WebCore.xcodeproj/project.pbxproj:9202
> +		5F2DBBE8178E336900141486 /* CertificateInfo.h */ = {isa =
PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name =
CertificateInfo.h; path = ../mac/CertificateInfo.h; sourceTree = "<group>"; };

This seems strange. Can we move this in the mac group in the project too, so it
doesn’t have a path with ".." in it?

> Source/WebCore/platform/network/ResourceResponseBase.cpp:49
> +    , m_didInitializeCertificateInfo(false)

Shouldn’t this be true? Maybe we should use a dirty bit instead of a “did
initialize” bit?

> Source/WebCore/platform/network/ResourceResponseBase.cpp:76
> +    , m_didInitializeCertificateInfo(true)

If false is right for the empty constructor, not sure why true is right here.

> Source/WebCore/platform/network/ResourceResponseBase.cpp:220
> +CertificateInfo ResourceResponseBase::certificateInfo() const
> +{
> +    ASSERT(m_didInitializeCertificateInfo);
> +    return m_certificateInfo;
> +}

Seems like this should be inlined in the header. I wouldn’t put it in the class
definition, but I would put it at the bottom of the header. Hate to have
function call overhead for a simple getter like this.

> Source/WebCore/platform/network/ResourceResponseBase.h:30
> +#include "CertificateInfo.h"

Windows build error:

    
1>C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\network\ResourceRespon
seBase.h(30): fatal error C1083: Cannot open include file: 'CertificateInfo.h':
No such file or directory (..\html\canvas\WebGLRenderingContext.cpp)

> Source/WebCore/platform/network/ResourceResponseBase.h:96
> +    void initializeCertificateInfo() const;

I’m not entirely sure that initializeCertificateInfo is the right name for
this.

This function does something different from “initializing”. It effectively sets
a discretionary “should include certificate info” flag that allows someone to
get the certificate info from this object; implements a policy about which
responses should include certificate info. Very tricky concept, I think,
because you could always call initializeCertificateInfo at any time, except if
you serialize and deserialize an object, and then it would be too late for that
to work. The right name could make this easier to understand.

Maybe addCertificateInfo?

> Source/WebCore/platform/network/ResourceResponseBase.h:146
>      // The ResourceResponse subclass may "shadow" this method to lazily
initialize platform specific fields

Should say “these functions” instead of “this method” since there are now three
functions below.

> Source/WebCore/platform/network/ResourceResponseBase.h:149
> +    String platformSuggestedFileName() const { return String(); }

I don’t understand exactly why this was added. I don’t see any code that seems
to require it.

> Source/WebCore/platform/network/ResourceResponseBase.h:151
>      // The ResourceResponse subclass may "shadow" this method to compare
platform specific fields

This separate comment for one function isn’t great when the three functions
above share only a single comment. I suggest either a comment for every
function, or a single comment for all four functions.

> Source/WebCore/platform/network/ResourceResponseBase.h:162
> +    mutable bool m_didInitializeCertificateInfo;

I think this should just be m_includesCertificateInfo. Or maybe WTF::Optional
would be the best way to handle this.

> Source/WebCore/platform/network/mac/CertificateInfo.h:52
> +typedef int32_t CertificateInfo;

What is this about? A workaround to let use use the CertificateInfo type on
other platforms? Is the integer actually used?