Bug 175746 - [Beacon] Content extensions should be able to intercept Beacon / Ping redirects
Summary: [Beacon] Content extensions should be able to intercept Beacon / Ping redirects
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on: 175756
Blocks: 147885
  Show dependency treegraph
 
Reported: 2017-08-18 17:01 PDT by Chris Dumez
Modified: 2017-08-22 12:14 PDT (History)
9 users (show)

See Also:


Attachments
WIP Patch (9.50 KB, patch)
2017-08-21 11:34 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (13.77 KB, patch)
2017-08-21 12:20 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (13.81 KB, patch)
2017-08-21 12:59 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (19.40 KB, patch)
2017-08-21 14:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (19.88 KB, patch)
2017-08-21 14:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
WIP Patch (20.97 KB, patch)
2017-08-21 14:48 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (26.71 KB, patch)
2017-08-21 15:28 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2017-08-18 17:01:36 PDT
Content extensions should be able to intercept Beacon / Ping redirects.
Comment 1 Chris Dumez 2017-08-18 17:01:51 PDT
<rdar://problem/33946050>
Comment 2 Chris Dumez 2017-08-21 11:34:12 PDT
Created attachment 318650 [details]
WIP Patch
Comment 3 Chris Dumez 2017-08-21 12:20:18 PDT
Created attachment 318658 [details]
WIP Patch
Comment 4 Chris Dumez 2017-08-21 12:59:50 PDT
Created attachment 318662 [details]
WIP Patch
Comment 5 Chris Dumez 2017-08-21 14:24:57 PDT
Created attachment 318672 [details]
WIP Patch
Comment 6 Chris Dumez 2017-08-21 14:26:34 PDT
Created attachment 318673 [details]
WIP Patch
Comment 7 Chris Dumez 2017-08-21 14:48:49 PDT
Created attachment 318676 [details]
WIP Patch
Comment 8 Chris Dumez 2017-08-21 15:28:32 PDT
Created attachment 318678 [details]
Patch
Comment 9 Alex Christensen 2017-08-21 17:38:01 PDT
Comment on attachment 318678 [details]
Patch

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

> Source/WebKit/NetworkProcess/PingLoad.cpp:129
>              completionHandler({ });
> +            didFinish(ResourceError { String(), 0, currentRequest().url(), ASCIILiteral("Blocked by Content Security Policy"), ResourceError::Type::AccessControl });

I think it would be nicer if the didFinish calls were before the completionHandler calls.
Comment 10 Chris Dumez 2017-08-21 18:03:41 PDT
(In reply to Alex Christensen from comment #9)
> Comment on attachment 318678 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318678&action=review
> 
> > Source/WebKit/NetworkProcess/PingLoad.cpp:129
> >              completionHandler({ });
> > +            didFinish(ResourceError { String(), 0, currentRequest().url(), ASCIILiteral("Blocked by Content Security Policy"), ResourceError::Type::AccessControl });
> 
> I think it would be nicer if the didFinish calls were before the
> completionHandler calls.

This is not possible because didFinish() calls delete this;
Comment 11 WebKit Commit Bot 2017-08-21 18:32:29 PDT
Comment on attachment 318678 [details]
Patch

Clearing flags on attachment: 318678

Committed r220996: <http://trac.webkit.org/changeset/220996>
Comment 12 WebKit Commit Bot 2017-08-21 18:32:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Geoffrey Garen 2017-08-22 10:19:15 PDT
Comment on attachment 318678 [details]
Patch

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

> Source/WebKit/NetworkProcess/PingLoad.cpp:324
> +// Returns true if we should block the load.

This would be nicer as an enum class return value. Maybe just return status, and then the caller can check status.blockedLoad?
Comment 14 Chris Dumez 2017-08-22 10:26:37 PDT
(In reply to Geoffrey Garen from comment #13)
> Comment on attachment 318678 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=318678&action=review
> 
> > Source/WebKit/NetworkProcess/PingLoad.cpp:324
> > +// Returns true if we should block the load.
> 
> This would be nicer as an enum class return value. Maybe just return status,
> and then the caller can check status.blockedLoad?

I copied the existing pattern but I agree. I'll clean this up in a follow-up.
Comment 15 Chris Dumez 2017-08-22 12:14:23 PDT
(In reply to Chris Dumez from comment #14)
> (In reply to Geoffrey Garen from comment #13)
> > Comment on attachment 318678 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=318678&action=review
> > 
> > > Source/WebKit/NetworkProcess/PingLoad.cpp:324
> > > +// Returns true if we should block the load.
> > 
> > This would be nicer as an enum class return value. Maybe just return status,
> > and then the caller can check status.blockedLoad?
> 
> I copied the existing pattern but I agree. I'll clean this up in a follow-up.

Done via Bug 175834.