[Webkit-unassigned] [Bug 192736] HTTPS Upgrade: Use full sqlite upgrade list

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Sun Dec 16 15:40:32 PST 2018


Darin Adler <darin at apple.com> changed:

           What    |Removed                     |Added
                 CC|                            |darin at apple.com

--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 357378
  --> https://bugs.webkit.org/attachment.cgi?id=357378

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

A few comments, not a complete review

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:37
> +using namespace WebCore;

This is not allowed; it’s incompatible with unified builds.

Best solution would be to not us "using namespace" at all, or the using namespace could be put inside the WebKit namespace.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:42
> +    : m_isSetupComplete(false)

Typically better style is to initialize this in the data member definition in the class definition rather than hear.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:75
> +    if (this->m_isSetupComplete) {

No need for the "this->" here or in the many other places in this file. Please remove it wherever possible, everywhere if possible.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:132
> +    CFURLRef databaseURL = CFBundleCopyResourceURL(webKitBundle, CFSTR("HTTPSUpgradeList"), CFSTR("db"), nullptr);
> +    String path = CFURLGetString(databaseURL);
> +    CFRelease(databaseURL);
> +    return path;

Better style is to use adoptCF rather than an explicit CFRelease. For example:

    return CFURLGetString(adoptCF(CFBundleCopyResourceURL(webKitBundle, CFSTR("HTTPSUpgradeList"), CFSTR("db"), nullptr)).get());

Or, if you prefer:

    auto databaseURL = adoptCF(CFBundleCopyResourceURL(webKitBundle, CFSTR("HTTPSUpgradeList"), CFSTR("db"), nullptr));
    return CFURLGetString(databaseURL.get());

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:134
> +#endif // PLATFORM(COCOA)
> +    return { };

Need an else here. We don’t want double returns.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:137
> +const String NetworkHTTPSUpgradeChecker::queryDatabaseSQL()

The "const" here isn’t really meaningful and should be omitted.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.cpp:139
> +    return "SELECT host FROM hosts WHERE host = ?;";

Should use a _s suffix so we use ASCIILiteral here. But also, why have a function that returns this as a String?

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.h:33
> +#include <WebCore/ResourceRequest.h>
> +#include <WebCore/SQLiteDatabase.h>
> +#include <WebCore/SQLiteStatement.h>
> +#include <wtf/CompletionHandler.h>
> +#include <wtf/Ref.h>
> +#include <wtf/WorkQueue.h>

I don’t think we need complete includes of classes like SQLiteDatabase and SQLiteStatement. I believe we can compile with just forward declarations of most of these classes.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.h:48
> +    static const String databasePath();
> +    static const String queryDatabaseSQL();

The const here don’t mean anything and should be omitted. Not sure these need to be static class member functions; could just have them be free functions in the .cpp file.

> Source/WebKit/NetworkProcess/NetworkHTTPSUpgradeChecker.h:52
> +    std::atomic<bool> m_isSetupComplete;

I don’t think making this std::atomic is doing enough to synchronize here. To give one example, I think there is a race condition in ~NetworkHTTPSUpgradeChecker. It checks the boolean for true and then decides not to do work based on that, but the boolean could become true just after it checks. Doesn’t seem right.

You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20181216/c34b07df/attachment.html>

More information about the webkit-unassigned mailing list