Bug 159417 - Remove crossOriginRequestPolicy from ThreadableLoaderOptions
Summary: Remove crossOriginRequestPolicy from ThreadableLoaderOptions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords:
Depends on:
Blocks: 151937
  Show dependency treegraph
 
Reported: 2016-07-05 06:03 PDT by youenn fablet
Modified: 2016-07-20 23:13 PDT (History)
6 users (show)

See Also:


Attachments
Patch (20.94 KB, patch)
2016-07-05 06:17 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Updating change log (20.91 KB, patch)
2016-07-20 05:54 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-07-05 06:03:02 PDT
This is redundant with FetchOptions::Mode.
Comment 1 youenn fablet 2016-07-05 06:17:22 PDT
Created attachment 282779 [details]
Patch
Comment 2 Alex Christensen 2016-07-05 11:23:23 PDT
Comment on attachment 282779 [details]
Patch

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

FetchOptions::Mode has 4 values and CrossOriginRequestPolicy has 3.  Is FetchOptions::Mode::Navigate not used?

> Source/WebCore/ChangeLog:3
> +        Fix 159417

This isn't necessary.  The bug link is already in the changelog.

> Source/WebCore/loader/ThreadableLoader.cpp:46
> +    mode = FetchOptions::Mode::SameOrigin;

Things like this and the growing list of things in the ResourceLoaderOptions constructor make me think we should be calling superclass constructors instead of manually setting member variables in constructors.  That might be best done in another patch, though.
Comment 3 youenn fablet 2016-07-05 13:24:00 PDT
(In reply to comment #2)
> Comment on attachment 282779 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=282779&action=review
> 
> FetchOptions::Mode has 4 values and CrossOriginRequestPolicy has 3.  Is
> FetchOptions::Mode::Navigate not used?

Navigate is triggered by user interaction like page loads and so on.
Fetch API does not expose it, setting it will trigger a TypeError when constructing q Request. So it is not used in WebKit yet.

I think it becomes useful with service worker, although we could try to use it if doing some additional refactoring to better align with fetch algorith.

> > Source/WebCore/ChangeLog:3
> > +        Fix 159417
> 
> This isn't necessary.  The bug link is already in the changelog.

Sure, that is a left over, I'll remove it.

> > Source/WebCore/loader/ThreadableLoader.cpp:46
> > +    mode = FetchOptions::Mode::SameOrigin;
> 
> Things like this and the growing list of things in the ResourceLoaderOptions
> constructor make me think we should be calling superclass constructors
> instead of manually setting member variables in constructors.  That might be
> best done in another patch, though.

Constructing a ResourceLoaderOptions with all options is not very readable.
It would be easier to have some meaningful options profile, overriding 1 or 2 values locally when needed.
Comment 4 youenn fablet 2016-07-20 05:54:29 PDT
Created attachment 284098 [details]
Updating change log
Comment 5 WebKit Commit Bot 2016-07-20 23:13:11 PDT
Comment on attachment 284098 [details]
Updating change log

Clearing flags on attachment: 284098

Committed r203490: <http://trac.webkit.org/changeset/203490>
Comment 6 WebKit Commit Bot 2016-07-20 23:13:15 PDT
All reviewed patches have been landed.  Closing bug.