[webkit-reviews] review denied: [Bug 63301] Rename ThreadableLoaderOptions and use it down to ResourceLoader : [Attachment 105093] Addressing levin's comments

[bugzilla-daemon at webkit.org]

David Levin <levin at chromium.org> has denied Nate Chapin <japhet at chromium.org>'s
request for review:
Bug 63301: Rename ThreadableLoaderOptions and use it down to ResourceLoader
https://bugs.webkit.org/show_bug.cgi?id=63301

Attachment 105093: Addressing levin's comments
https://bugs.webkit.org/attachment.cgi?id=105093&action=review

------- Additional Comments from David Levin <levin at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=105093&action=review


Just a few issues.

> Source/WebCore/loader/ResourceLoaderOptions.h:44
> +const ResourceLoaderOptions& defaultResourceOptions();

I find it confusing that we have some called defaultResourceOptions which is
different from what the default constructor does.

What makes this more "default" than the "default" constructor?

Ultimately, what I'm getting at is that I think this is a bad name (and I don't
know what a better name is because I don't understand it).

> Source/WebCore/loader/ResourceLoader.h:183
> +	   ResourceLoaderOptions m_options;

What does it meant that ResourceLoaderOptions has this extra field that wasn't
in ResourceLoader before?
    bool allowCredentials; // Whether HTTP credentials and cookies are sent
with the request.

It seems odd to allow a new option to be passed in which isn't supported.

> Source/WebCore/loader/ThreadableLoader.h:67
> +	   ThreadableLoaderOptions() : ResourceLoaderOptions(),
preflightPolicy(ConsiderPreflight),
crossOriginRequestPolicy(DenyCrossOriginRequests) { }

I'm pretty sure you don't need to list "ResourceLoaderOptions()" here.

> Source/WebCore/loader/ResourceLoaderOptions.cpp:38
> +    static bool s_optionsInitialized = false;

This isn't thread-safe. Please add an ASSERT(isMainThread()); here.