Bug 213418 - Storage Access API: Add the capability to call the Storage Access API as a quirk, on behalf of websites that should be doing it themselves
Summary: Storage Access API: Add the capability to call the Storage Access API as a qu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-06-19 16:44 PDT by John Wilander
Modified: 2020-07-16 14:30 PDT (History)
14 users (show)

See Also:


Attachments
Patch (46.56 KB, patch)
2020-06-19 17:24 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (46.67 KB, patch)
2020-06-22 16:40 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (46.98 KB, patch)
2020-06-22 19:12 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2020-06-19 16:44:56 PDT
There are websites and embedded content that should be calling the Storage Access API but currently are not. We should add the capability to call the API.
Comment 1 Radar WebKit Bug Importer 2020-06-19 16:45:24 PDT
<rdar://problem/64549429>
Comment 2 John Wilander 2020-06-19 16:45:58 PDT
Correction: We should add the capability to call the API *on their behalf*.
Comment 3 John Wilander 2020-06-19 17:24:35 PDT
Created attachment 402357 [details]
Patch
Comment 4 John Wilander 2020-06-22 16:40:20 PDT
Created attachment 402520 [details]
Patch
Comment 5 Alex Christensen 2020-06-22 16:47:37 PDT
Comment on attachment 402520 [details]
Patch

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

This seems ok to me.

> Source/WebCore/dom/DocumentStorageAccess.cpp:111
> +    // The existence of a frame and page has been checked in requestStorageAccessQuickCheck().

This might be true now, but it might not be true with future use of this code.  I think it's a good idea to leave the null checks in place, maybe with assert_not_reached.
Comment 6 John Wilander 2020-06-22 18:55:49 PDT
(In reply to Alex Christensen from comment #5)
> Comment on attachment 402520 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=402520&action=review
> 
> This seems ok to me.

Thanks for the review!

> > Source/WebCore/dom/DocumentStorageAccess.cpp:111
> > +    // The existence of a frame and page has been checked in requestStorageAccessQuickCheck().
> 
> This might be true now, but it might not be true with future use of this
> code.  I think it's a good idea to leave the null checks in place, maybe
> with assert_not_reached.

Right now I RELEASE_ASSERT which should force us to find the problem. Would you prefer an early return with just a debug assert?
Comment 7 John Wilander 2020-06-22 19:12:38 PDT
Created attachment 402526 [details]
Patch
Comment 8 John Wilander 2020-06-22 19:13:25 PDT
Waiting for EWS to make sure I fixed the Windows build issue.
Comment 9 EWS 2020-06-22 21:13:28 PDT
Committed r263383: <https://trac.webkit.org/changeset/263383>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 402526 [details].
Comment 10 Simon Fraser (smfr) 2020-07-16 14:30:43 PDT
Comment on attachment 402526 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:366
> +    if (hasClass() && Quirks::StorageAccessResult::ShouldCancelEvent == document().quirks().triggerOptionalStorageAccessQuirk(eventType, classNames()))
> +        return false;

This is such a weird place to add a quirk.