Bug 213910 – Storage Access API: Add the capability to open a popup and get user interaction so we can call the Storage Access API as a quirk, on behalf of websites that should be doing it themselves

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[John Wilander]

If the third-party requesting storage access has not yet received user interaction, we should help them by opening a popup to them.

[Radar WebKit Bug Importer]

<rdar://problem/65058017>

[John Wilander]

Created attachment 403425 [details]
Patch

[Darin Adler]

Comment on attachment 403425 [details]
Patch

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

> Source/WebCore/ChangeLog:11
> +        This patch not only adds the capability but also adds a site-specific quirk for
> +        the family of Kinja sites so that no previous user interaction with kinja.com
> +        results in a login popup for kinja.com.

This is a particular long and elaborate quirk. We should figure out how to make it simpler and less sensitively dependent on the precise details of the current version of kinja.com, or we are likely to have to revise it.

> Source/WebCore/ChangeLog:13
> +        No new tests. This is for site-specific quirks.

Need to create a way to test these kinds of site-specific quirks with our automated regression tests. Not OK to just have them untested, especially when they are complex enough that someone could easily break them.

> Source/WebCore/ChangeLog:17
> +        * dom/Document.cpp:
> +        (WebCore::Document::openForUserInteractionQuirk):
> +            New function for quirk popups. It's basically a wrapper for DOMWindow::open().

We don’t need this wrapper in Document. The code in Quirks.cpp can call DOMWindow::open. Flowing through Document isn’t helpful.

> Source/WebCore/page/Quirks.cpp:892
> +                        // FIXME: Add appropriate logging.

I don’t think this FIXME should be here unless the logging is particularly important. We could add it, but why is it important?

> Source/WebCore/page/Quirks.cpp:908
> +                if (classNames.contains("js_switch-to-burner-login")
> +                    || classNames.contains("js_header-userbutton")
> +                    || classNames.contains("sc-1il3uru-3") || classNames.contains("cIhKfd")
> +                    || classNames.contains("iyvn34-0") || classNames.contains("bYIjtl"))

Formatting here seems a bit peculiar. Might be useful t have some comments explaining how we determined this is the correct list.

> Source/WebCore/page/Quirks.cpp:914
> +                if (element.tagName() == "svg")

There is a more efficient way to do this check inside WebKit than calling tagName. I think it’s is<SVGSVGElement>.

> Source/WebCore/page/Quirks.cpp:916
> +                else if (element.tagName() == "path" && element.parentElement() && element.parentElement()->tagName() == "svg")

is<???PathElement> should work here too.

Another benefit of is<SVGSVGElement> is that it will do a null check for you if you pass it a pointer.

> Source/WebCore/page/Quirks.cpp:929
> +                        // FIXME: Add appropriate logging.

I don’t think this FIXME should be here unless the logging is particularly important. We could add it, but why is it important?

> Source/WebCore/page/Quirks.cpp:936
> +                ExceptionOr<RefPtr<WindowProxy>> proxyOrException = m_document->openForUserInteractionQuirk(kinjaURL, loginPopupWindowFeatureString);
> +                if (proxyOrException.hasException())
> +                    return Quirks::StorageAccessResult::ShouldNotCancelEvent;

Putting the call to DOMWindow::open here would make not make the code appreciably more complex; perhaps simpler.

> Source/WebCore/page/Quirks.cpp:943
> +                        Ref<DOMWrapperWorld> world = ScriptController::createWorld("kinjaComQuirkWorld", ScriptController::WorldType::User);

auto

[Darin Adler]

Comment on attachment 403425 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:902
> +            bool isRightElement = false;

Since the "should apply quirk" code is getting complex, I suggest factoring it into its own separate function. Then we’d use return statements instead of "isRightElement". I think that would help separate concerns and make this easier to read.

[John Wilander]

Thanks, Darin! Your requests are all good enhancements and I will address them (except test which is more of an open question) before landing. I will also have a look at the Windows bot failure.

[John Wilander]

Created attachment 403621 [details]
Patch

[John Wilander]

Comment on attachment 403621 [details]
Patch

No review requested. Just waiting for EWS feedback.

[John Wilander]

The Windows layout test failures look unrelated.

[EWS]

Committed r263992: <https://trac.webkit.org/changeset/263992>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 403621 [details].

[Darin Adler]

Comment on attachment 403621 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:855
> +bool Quirks::isKinjaLoginAvatarElement(const Element& element) const

This should be a free function, not a Quirks member function.

    static bool isKinjaLoginAvatarElement(const Element& element)

[John Wilander]

(In reply to Darin Adler from comment #10)
> Comment on attachment 403621 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=403621&action=review
> 
> > Source/WebCore/page/Quirks.cpp:855
> > +bool Quirks::isKinjaLoginAvatarElement(const Element& element) const
> 
> This should be a free function, not a Quirks member function.
> 
>     static bool isKinjaLoginAvatarElement(const Element& element)

Fixed in https://trac.webkit.org/changeset/264001.

[Alex Christensen]

Comment on attachment 403621 [details]
Patch

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

> Source/WebCore/page/Quirks.cpp:951
> +                    page->addUserScriptAwaitingNotification(world.get(), kinjaLoginUserScript);

I'm curious why you used a user script awaiting notification here and WaitForNotificationBeforeInjecting::Yes.  That is a strange feature introduced for web extensions, and they are only notified in Mac Safari for the possibility that web extensions are installed.  Would using WaitForNotificationBeforeInjecting::No work?