[webkit-reviews] review granted: [Bug 185366] CSP status-code incorrect for document blocked due to violation of its frame-ancestors directive : [Attachment 339692] Patch

[bugzilla-daemon at webkit.org]

Brent Fulgham <bfulgham at webkit.org> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 185366: CSP status-code incorrect for document blocked due to violation of
its frame-ancestors directive
https://bugs.webkit.org/show_bug.cgi?id=185366

Attachment 339692: Patch

https://bugs.webkit.org/attachment.cgi?id=339692&action=review




--- Comment #7 from Brent Fulgham <bfulgham at webkit.org> ---
Comment on attachment 339692
  --> https://bugs.webkit.org/attachment.cgi?id=339692
Patch

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

Looks good! I had a few minor suggestions, but nothing you need to change if
you disagree. r=me

> Source/WebCore/ChangeLog:4
> +	   https://bugs.webkit.org/show_bug.cgi?id=185366

Is there a radar tracking this? If not please import to radar. Either way,
please copy the radar # here.

> Source/WebCore/dom/Document.cpp:3320
> +    auto* documentLoader = frame ? frame->loader().documentLoader() :
nullptr;

Is the only reason we need documentLoader is for its response status code?
Maybe we should just capture that here rather than embedding it twice below?

> Source/WebCore/dom/Document.cpp:5515
> +    auto* loader = m_frame->loader().documentLoader();

Should this be called 'documentLoader' instead? Should we just capture its
httpStatusCode here instead of doing a ternary operator on line 5525?