Bug 175746 – [Beacon] Content extensions should be able to intercept Beacon / Ping redirects

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

Content extensions should be able to intercept Beacon / Ping redirects.

[Chris Dumez]

<rdar://problem/33946050>

[Chris Dumez]

Created attachment 318650 [details]
WIP Patch

[Chris Dumez]

Created attachment 318658 [details]
WIP Patch

[Chris Dumez]

Created attachment 318662 [details]
WIP Patch

[Chris Dumez]

Created attachment 318672 [details]
WIP Patch

[Chris Dumez]

Created attachment 318673 [details]
WIP Patch

[Chris Dumez]

Created attachment 318676 [details]
WIP Patch

[Chris Dumez]

Created attachment 318678 [details]
Patch

[Alex Christensen]

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.

[Chris Dumez]

(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;

[WebKit Commit Bot]

Comment on attachment 318678 [details]
Patch

Clearing flags on attachment: 318678

Committed r220996: <http://trac.webkit.org/changeset/220996>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Geoffrey Garen]

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?

[Chris Dumez]

(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.

[Chris Dumez]

(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.