[webkit-reviews] review denied: [Bug 63301] Rename ThreadableLoaderOptions and use it down to ResourceLoader : [Attachment 98437] Address ews'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 98437: Address ews's comments
https://bugs.webkit.org/attachment.cgi?id=98437&action=review

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

At a meta level, I wonder a few things:

1. Why do we need to move down the preflight stuff into ResourceLoader (which
adds more complexity at that level)? Does everything using ResourceLoader need
them? Can we people who need preflight to using DocumentThreadableLoader (and
perhaps rename *ThreadableLoader to *PreflightLoader or *CORSLoaders)?

2. Why not create a new struct ResourceLoaderOptions and derive
ThreadableLoaderOptions from that? Only move the fields into
ResourceLoaderOptions which actually do something. (With the current patch I
can fill in fields that do nothing which would be confusing.)

> Source/WebCore/notifications/Notification.cpp:172
> +    options.allowCredentials = true;

This is flipping the value from false to true. While this looks correct, it
would be good to do this as a separate bug instead of putting it in as a part
of this bigger change.

> Source/WebCore/loader/ResourceLoadOptions.h:50
> +struct ResourceLoadOptions {

Seems like this should be ResourceLoad*er*Options.

>> Source/WebCore/loader/ResourceLoadOptions.h:51
>> +	ResourceLoadOptions() : sendLoadCallbacks(true), sniffContent(true),
allowCredentials(true), forcePreflight(false),
crossOriginRequestPolicy(AllowCrossOriginRequests), shouldBufferData(true) { }
> 
> This should probably be split into multiple lines.

Changing the defaults from restrictive/conservations options to less
conservative options makes it too easy to introduce unintended security issues.


>From example:
  allowCredentials from false to true.
  crossOriginRequestPolicy from DenyCrossOriginRequests to
AllowCrossOriginRequests
etc.

Please change all of these back to what they were.  (Also this is addressing a
different issues from "Rename ThreadableLoaderOptions and use it down to
ResourceLoader").

> Source/WebCore/loader/ThreadableLoader.h:-48
> -    enum StoredCredentials {

This placement is an artifact of history, so it shouldn't move to the file with
ThreadableLoaderOptions but it should move elsewhere (possibly
Source/WebCore/platform/network/ResourceHandle.h).

Regardless this is a separate issue from the one being addressed in this patch
(of moving ThreadableLoaderOptions).