[webkit-reviews] review denied: [Bug 219949] Cannot see 'Add to my stations' button in BBC World service Radio : [Attachment 416350] Patch

[bugzilla-daemon at webkit.org]

Alex Christensen <achristensen at apple.com> has denied
katherine_cheney at apple.com's request for review:
Bug 219949: Cannot see 'Add to my stations' button in BBC World service Radio
https://bugs.webkit.org/show_bug.cgi?id=219949

Attachment 416350: Patch

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




--- Comment #3 from Alex Christensen <achristensen at apple.com> ---
Comment on attachment 416350
  --> https://bugs.webkit.org/attachment.cgi?id=416350
Patch

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

> Source/WebCore/loader/FrameLoader.cpp:2724
> +    if (m_frame.document()) {

if (auto* document = m_frame.document())
That'll allow you to use it in the next two lines.

> Source/WebCore/loader/FrameLoader.cpp:2726
> +	   if (m_frame.document()->settings().needsSiteSpecificQuirks()) {
> +	       if (m_frame.document()->url().string() ==
Quirks::staticRadioPlayerURLString() && m_client->BBCRadioPlayerPopup()) {

This could be another && instead of two if statements.

> Source/WebCore/loader/FrameLoaderClient.h:385
> +    virtual bool BBCRadioPlayerPopup() const { return false; };

This probably shouldn't be here, but definitely needs a better name indicating
its use.

> Source/WebCore/page/DOMWindow.cpp:2550
> +	   frame->setBBCRadioPlayerPopup(true);

We should probably add a null check for frame after it's declared and return
nullptr if it's null instead of crashing.  There was a code path that used it
unchecked, but we can fix that one too.

> Source/WebCore/page/Frame.h:385
> +    bool m_BBCRadioPlayerPopup { false };

I don't think we should introduce a boolean for this because that will likely
lead to cases where it's not reset properly.  If we do have to add one, it
should have a prefix like isShowing or something, but I don't think it should
be necessary here or on WebPage, and there certainly shouldn't be two places
that remember that we are doing the bbc radio quirk.

> Source/WebCore/page/Quirks.cpp:1116
> +			       auto domWindow = document->domWindow();

document is a weak pointer.  It should be checked before dereferencing.

> Source/WebCore/page/Quirks.cpp:1118
> +				   ExceptionOr<RefPtr<WindowProxy>>
proxyOrException =  domWindow->open(*domWindow, *domWindow,
staticRadioPlayerURLString(), emptyString(),
BBCRadioPlayerPopUpWindowFeatureString);

auto
extra space.
Should we do anything with proxyOrException?

> Source/WebKit/UIProcess/WebPageProxy.cpp:5582
> +    if (request.url().string() == "https://static.radioplayer.co.uk/")

Quirks::staticRadioPlayerURLString