WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
219949
Cannot see 'Add to my stations' button in BBC World service Radio
https://bugs.webkit.org/show_bug.cgi?id=219949
Summary
Cannot see 'Add to my stations' button in BBC World service Radio
Kate Cheney
Reported
2020-12-16 08:56:52 PST
BBC uses the presence of third party cookies to determine whether to show specific elements in their radio player. These do not appear with ITP and full third party cookie blocking.
Attachments
Patch
(30.29 KB, patch)
2020-12-16 11:31 PST
,
Kate Cheney
no flags
Details
Formatted Diff
Diff
Patch
(25.52 KB, patch)
2020-12-17 15:32 PST
,
Kate Cheney
no flags
Details
Formatted Diff
Diff
Patch
(25.58 KB, patch)
2020-12-18 09:09 PST
,
Kate Cheney
no flags
Details
Formatted Diff
Diff
Patch
(25.61 KB, patch)
2020-12-21 12:33 PST
,
Kate Cheney
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Kate Cheney
Comment 1
2020-12-16 08:59:41 PST
<
rdar://problem/60319532
>
Kate Cheney
Comment 2
2020-12-16 11:31:35 PST
Created
attachment 416350
[details]
Patch
Alex Christensen
Comment 3
2020-12-16 12:32:58 PST
Comment on
attachment 416350
[details]
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
Kate Cheney
Comment 4
2020-12-16 14:54:31 PST
Comment on
attachment 416350
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=416350&action=review
Thanks for the comments Alex
>> 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.
Will change.
>> Source/WebCore/loader/FrameLoader.cpp:2726 >> + if (m_frame.document()->url().string() == Quirks::staticRadioPlayerURLString() && m_client->BBCRadioPlayerPopup()) { > > This could be another && instead of two if statements.
Will change.
>> 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.
Good idea, I will add that.
>> 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.
My intention was to prevent any other page from being automatically redirected if navigating to
https://static.radio.player.com
, which would happen if I didn't include an indication that the pop-up had been triggered. I am not sure how much that should be a concern (
https://static.radio.player.com
is just a blank page). Do you know if there is a way to wait for a page to finish loading from the site in Quirks::triggerOptionalStorageAccessQuirk where I call domWindow->open() to perform the redirect from there? That way we know the redirect only happens for the quirk.
>> Source/WebCore/page/Quirks.cpp:1116 >> + auto domWindow = document->domWindow(); > > document is a weak pointer. It should be checked before dereferencing.
Will add this.
>> 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?
Maybe if we are able to handle the redirect from here instead of in Frame.cpp. Then I can use proxyOrException to perform a redirect.
>> Source/WebKit/UIProcess/WebPageProxy.cpp:5582 >> + if (request.url().string() == "
https://static.radioplayer.co.uk/
") > > Quirks::staticRadioPlayerURLString
Good catch, will change.
Kate Cheney
Comment 5
2020-12-17 15:32:22 PST
Created
attachment 416475
[details]
Patch
Kate Cheney
Comment 6
2020-12-17 15:34:05 PST
Comment on
attachment 416350
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=416350&action=review
>>> 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. > > My intention was to prevent any other page from being automatically redirected if navigating to
https://static.radio.player.com
, which would happen if I didn't include an indication that the pop-up had been triggered. I am not sure how much that should be a concern (
https://static.radio.player.com
is just a blank page). > > Do you know if there is a way to wait for a page to finish loading from the site in Quirks::triggerOptionalStorageAccessQuirk where I call domWindow->open() to perform the redirect from there? That way we know the redirect only happens for the quirk.
In the latest patch I inject javascript on load when I create the popup to solve this.
Kate Cheney
Comment 7
2020-12-18 09:09:05 PST
Created
attachment 416522
[details]
Patch
Alex Christensen
Comment 8
2020-12-18 18:40:37 PST
Comment on
attachment 416522
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=416522&action=review
> Source/WebCore/page/DOMWindow.cpp:2505 > + if (document->settings().needsSiteSpecificQuirks() && urlStringToOpen == Quirks::BBCRadioPlayerURLString()) {
It looks like you'll need to open Quirks.h to fix the windows build.
> Source/WebCore/page/Quirks.cpp:1155 > + if (auto subResourceDomainsInNeedOfStorageAccess = NetworkStorageSession::subResourceDomainsInNeedOfStorageAccessForFirstParty(firstPartyDomain)) {
We tend to avoid deeply nested code like this by adding early returns. auto subResourceDomainsInNeedOfStorageAccess = ; if (!subResourceDomainsInNeedOfStorageAccess) return StorageAccessResult::ShouldNotCancelEvent; if (storageAccessGranted != StorageAccessWasGranted::Yes) return StorageAccessResult::ShouldNotCancelEvent; etc.
> Source/WebCore/page/Quirks.cpp:1168 > + if (abstractFrame && is<Frame>(*abstractFrame)) {
I think there's a version of is that takes a pointer and returns false if it's null.
Kate Cheney
Comment 9
2020-12-21 12:33:08 PST
Created
attachment 416620
[details]
Patch
Kate Cheney
Comment 10
2020-12-21 12:34:10 PST
(In reply to Alex Christensen from
comment #8
)
> Comment on
attachment 416522
[details]
> Patch
Thanks for the review!
> > View in context: >
https://bugs.webkit.org/attachment.cgi?id=416522&action=review
> > > Source/WebCore/page/DOMWindow.cpp:2505 > > + if (document->settings().needsSiteSpecificQuirks() && urlStringToOpen == Quirks::BBCRadioPlayerURLString()) { > > It looks like you'll need to open Quirks.h to fix the windows build. >
I think my latest patch fixed it. I will wait for EWS before landing.
> > Source/WebCore/page/Quirks.cpp:1155 > > + if (auto subResourceDomainsInNeedOfStorageAccess = NetworkStorageSession::subResourceDomainsInNeedOfStorageAccessForFirstParty(firstPartyDomain)) { > > We tend to avoid deeply nested code like this by adding early returns. > auto subResourceDomainsInNeedOfStorageAccess = ; > if (!subResourceDomainsInNeedOfStorageAccess) > return StorageAccessResult::ShouldNotCancelEvent; > if (storageAccessGranted != StorageAccessWasGranted::Yes) > return StorageAccessResult::ShouldNotCancelEvent; > etc.
I adjusted this to use early returns.
> > > Source/WebCore/page/Quirks.cpp:1168 > > + if (abstractFrame && is<Frame>(*abstractFrame)) { > > I think there's a version of is that takes a pointer and returns false if > it's null.
I removed the check for abstractFrame.
EWS
Comment 11
2020-12-22 11:45:34 PST
Committed
r271059
: <
https://trac.webkit.org/changeset/271059
> All reviewed patches have been landed. Closing bug and clearing flags on
attachment 416620
[details]
.
Radar WebKit Bug Importer
Comment 12
2020-12-22 11:46:17 PST
<
rdar://problem/72595090
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug