[webkit-reviews] review granted: [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 : [Attachment 403425] Patch

bugzilla-daemon at webkit.org bugzilla-daemon at webkit.org
Fri Jul 3 21:58:31 PDT 2020


Darin Adler <darin at apple.com> has granted John Wilander <wilander at apple.com>'s
request for review:
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
https://bugs.webkit.org/show_bug.cgi?id=213910

Attachment 403425: Patch

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




--- Comment #3 from Darin Adler <darin at apple.com> ---
Comment on attachment 403425
  --> https://bugs.webkit.org/attachment.cgi?id=403425
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


More information about the webkit-reviews mailing list