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

[bugzilla-daemon at webkit.org]

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

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
Patch

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>