Bug 160404

Summary: [Fetch API] Add support to ReferrerPolicy
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebCore Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, commit-queue, japhet
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=170061
Bug Depends on:    
Bug Blocks: 151937    
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing none

Description youenn fablet 2016-08-01 07:48:20 PDT
[Fetch API] Add support to ReferrerPolicy
Comment 1 youenn fablet 2016-08-01 08:13:42 PDT
Created attachment 285012 [details]
Patch
Comment 2 WebKit Commit Bot 2016-08-01 08:15:31 PDT
Attachment 285012 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/ThreadableLoaderClientWrapper.h:98:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Alex Christensen 2016-08-01 19:18:18 PDT
Comment on attachment 285012 [details]
Patch

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

> Source/WebCore/Modules/fetch/FetchRequest.cpp:298
> +    if (m_internalRequest.referrer != "no-referrer" && m_internalRequest.referrer != "client")

It seems like a case-sensitive check is correct in this case.  Could you add a test that tests No-Referrer and Client?
Could we use enum values instead of doing several string comparisons here?

> Source/WebCore/workers/WorkerScriptLoader.cpp:84
>      ASSERT(client);
> +    ASSERT(scriptExecutionContext);

We should come back later and make all these references instead of pointers at the callers.
Comment 4 youenn fablet 2016-08-01 23:49:19 PDT
Thanks for the review.

(In reply to comment #3)
> Comment on attachment 285012 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=285012&action=review
> 
> > Source/WebCore/Modules/fetch/FetchRequest.cpp:298
> > +    if (m_internalRequest.referrer != "no-referrer" && m_internalRequest.referrer != "client")
> 
> It seems like a case-sensitive check is correct in this case.  Could you add
> a test that tests No-Referrer and Client?
> Could we use enum values instead of doing several string comparisons here?

I will add a FIXME.
I do not like the current handling of referrer.
Although convenient, it is odd to pass it in the headers as it only serves to compute the actual value in CachedResource.
I would much prefer to pass it as a field in a structure like CachedResourceRequest for instance. The same applies to Origin.


> > Source/WebCore/workers/WorkerScriptLoader.cpp:84
> >      ASSERT(client);
> > +    ASSERT(scriptExecutionContext);
> 
> We should come back later and make all these references instead of pointers
> at the callers.

Right.
Comment 5 youenn fablet 2016-08-01 23:51:45 PDT
Created attachment 285078 [details]
Patch for landing
Comment 6 WebKit Commit Bot 2016-08-01 23:54:34 PDT
Attachment 285078 [details] did not pass style-queue:


ERROR: Source/WebCore/loader/ThreadableLoaderClientWrapper.h:98:  Should be indented on a separate line, with the colon or comma first on that line.  [whitespace/indent] [4]
Total errors found: 1 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 7 WebKit Commit Bot 2016-08-02 00:21:42 PDT
Comment on attachment 285078 [details]
Patch for landing

Clearing flags on attachment: 285078

Committed r204019: <http://trac.webkit.org/changeset/204019>
Comment 8 WebKit Commit Bot 2016-08-02 00:21:47 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 youenn fablet 2016-08-02 00:25:34 PDT
Reopening to attach new patch.
Comment 10 youenn fablet 2016-08-02 00:25:41 PDT
Created attachment 285080 [details]
Patch for landing