[webkit-reviews] review denied: [Bug 199699] Cannot bring up custom media controls at all on v.youku.com : [Attachment 373902] Patch

Thu Jul 11 11:02:14 PDT 2019

Simon Fraser (smfr) <simon.fraser at apple.com> has denied zalan
<zalan at apple.com>'s request for review:
Bug 199699: Cannot bring up custom media controls at all on v.youku.com
https://bugs.webkit.org/show_bug.cgi?id=199699

Attachment 373902: Patch

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

--- Comment #3 from Simon Fraser (smfr) <simon.fraser at apple.com> ---
Comment on attachment 373902
  --> https://bugs.webkit.org/attachment.cgi?id=373902
Patch

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

> Source/WebCore/ChangeLog:9
> +	   The "find the node under the finger" heuristic should only find
nodes that are visible to hittesting.

hit-testing or hit testing.

> Source/WebCore/ChangeLog:13
> +	   This fixes the case when "visibility: hidden" iFrame covers the
target node and it might divert the synthetic click (no hijacking). 

Please re-write to improve readability. I think you could expand this to
explain the two types of hit-testing.

> Source/WebCore/page/EventHandler.cpp:1182
> +    // hitTestResultAtPointIncludingSubframes is specifically used to
hitTest into all frames, thus it always allows child frame content.

hitTest -> hit-test

> Source/WebCore/page/EventHandler.cpp:1216
> -    // hitTestResultAtPoint is specifically used to hitTest into all frames,
thus it always allows child frame content.
> -    HitTestRequest request(hitType |
HitTestRequest::AllowChildFrameContent);
> +    HitTestRequest request(hitType);

Should this mask out the AllowChildFrameContent, or assert that it's not set?

> Source/WebCore/page/PointerCaptureController.cpp:395
> +	   target =
m_page.mainFrame().eventHandler().hitTestResultAtPointIncludingSubframes(docume
ntPoint, HitTestRequest::ReadOnly | HitTestRequest::Active |
HitTestRequest::DisallowUserAgentShadowContent |
HitTestRequest::AllowChildFrameContent).innerNonSharedElement();

Isn't the HitTestRequest::AllowChildFrameContent redundant here?

> Source/WebCore/page/ios/FrameIOS.mm:278
> +    auto adjustHitTestResultIfNeeded = [&](auto& hitTestResult) {
> +	   // Retry hittest with the visible-to-hittest restriction on, if the
initial hittest finds something inside a non-visible iFrame.
> +	   auto* frame = hitTestResult.innerNodeFrame();
> +	   if (!frame || frame->isMainFrame())
> +	       return;
> +	   auto* ownerElement = frame->ownerElement();
> +	   if (!ownerElement || !ownerElement->renderer())
> +	       return;
> +	   auto& renderer = *ownerElement->renderer();
> +	   if (renderer.visibleToHitTesting())
> +	       return;
> +	   hitTestResult = eventHandler().hitTestResultAtPoint(center);
> +    };

This seems wrong. You'd have to walk up the entire frame tree looking for any
frame ancestor whose renderer is not renderer.visibleToHitTesting(). Also,
calling hitTestResultAtPoint(), which doesn't cross frame boundaries, will only
check the main frame, but maybe you found something in a nested frame; you'd
have to call hitTestResultAtPoint() on that inner frame.

I think you need to do the renderer.visibleToHitTesting() check inside of the
initial hitTestResultAtPointIncludingSubframes() pass.

> Source/WebKit/WebProcess/WebPage/ios/WebPageIOS.mm:555
> +    HitTestResult result =
m_page->mainFrame().eventHandler().hitTestResultAtPointIncludingSubframes(m_las
tInteractionLocation, HitTestRequest::ReadOnly | HitTestRequest::Active |
HitTestRequest::AllowChildFrameContent);