[webkit-reviews] review granted: [Bug 136837] Videos with controls enabled never receive 'dragstart' events. : [Attachment 238143] Patch

[bugzilla-daemon at webkit.org]

Simon Fraser (smfr) <simon.fraser at apple.com> has granted Jer Noble
<jer.noble at apple.com>'s request for review:
Bug 136837: Videos with controls enabled never receive 'dragstart' events.
https://bugs.webkit.org/show_bug.cgi?id=136837

Attachment 238143: Patch
https://bugs.webkit.org/attachment.cgi?id=238143&action=review

------- Additional Comments from Simon Fraser (smfr) <simon.fraser at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=238143&action=review


> Source/WebCore/page/DragController.cpp:732
> +    if (!state.source->isMediaElement() &&
!state.source->contains(hitTestResult.innerNode()))
>	   // The original node being dragged isn't under the drag origin
anymore... maybe it was
>	   // hidden or moved out from under the cursor. Regardless, we don't
want to start a drag on
>	   // something that's not actually under the drag origin.
> +	   // FIXME(136836): Investigate whether all elements should use the
containsIncludingShadowDOM() path here.
>	   return false;
> +
> +    if (state.source->isMediaElement() &&
!state.source->containsIncludingShadowDOM(hitTestResult.innerNode()))
> +	   return false;

I think this would be cleaner as:

if (!state.source->isMediaElement()) {
  if (!state.source->contains(hitTestResult.innerNode()))
     ...
} else {
  if (!state.source->containsIncludingShadowDOM(hitTestResult.innerNode()))
}

Or better:
bool includeShadowDOM = state.source->isMediaElement();
bool sourceContainsHitNode = false;
if (!includeShadowDOM)
  sourceContainsHitNode = state.source->contains(hitTestResult.innerNode());
else
  sourceContainsHitNode =
state.source->containsIncludingShadowDOM(hitTestResult.innerNode());

if (! sourceContainsHitNode)
  return false