[Webkit-unassigned] [Bug 136611] Pass certificate info as part of ResourceResponse

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=136611


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #237774|review?                     |review+
               Flag|                            |




--- Comment #12 from Darin Adler <darin at apple.com>  2014-09-08 09:55:05 PST ---
(From update of attachment 237774)
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\ResourceResponseBase.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?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.