[webkit-reviews] review denied: [Bug 55515] Implement proper handling of mouseover/mouseout events in regard to shadow DOM boundaries. : [Attachment 88854] Adjusted indent

[bugzilla-daemon at webkit.org]

Ojan Vafai <ojan at chromium.org> has denied Dimitri Glazkov (Google)
<dglazkov at chromium.org>'s request for review:
Bug 55515: Implement proper handling of mouseover/mouseout events in regard to
shadow DOM boundaries.
https://bugs.webkit.org/show_bug.cgi?id=55515

Attachment 88854: Adjusted indent
https://bugs.webkit.org/attachment.cgi?id=88854&action=review

------- Additional Comments from Ojan Vafai <ojan at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=88854&action=review

> LayoutTests/fast/events/shadow-boundary-crossing.html:100
> +	   var counter = function()

Nit: functions are usually verbs, no?

> LayoutTests/fast/events/shadow-boundary-crossing.html:112
> +	   var count = 0;
> +	   var fileInput =
document.body.appendChild(document.createElement('input'));
> +	   fileInput.setAttribute('type', 'file');
> +	   var counter = function()
> +	   {
> +	       count++;
> +	   }
> +	   moveOverLeftQuarterOf(fileInput);
> +	   document.body.addEventListener('mouseover', counter, false);
> +	   document.body.addEventListener('mouseout', counter, false);
> +	   moveOverRightQuarterOf(fileInput);
> +	   log("The mouseover/mouseout event between two elements inside the
same shadow subtree should not propagate out of the shadow DOM", count == 0);
> +	   document.body.removeEventListener('mouseover', counter, false);
> +	   document.body.removeEventListener('mouseout', counter, false);
> +	   fileInput.parentNode.removeChild(fileInput);
> +    },

This would be easier to read with some more line-breaks.

> Source/WebCore/dom/EventDispatcher.cpp:193
> +PassRefPtr<EventTarget> EventDispatcher::adjustRelatedTarget(Event* event,
PassRefPtr<EventTarget> relatedTargetArg)

Not sure this name is accurate anymore since this is also adjusting m_ancestors
now.

> Source/WebCore/dom/EventDispatcher.cpp:228
> +    BoundaryAdjustments adjustments = adjustToShadowBoundaries(target,
m_ancestors, relatedTargetAncestors);
> +    m_ancestors.shrink(adjustments.ancestorSize);
> +    return adjustments.relatedTarget ? adjustments.relatedTarget :
relatedTarget;

Can you make adjustToShadowBoundaries a private method? Then, it can adjust
m_ancestors directly instead of returning a BoundayAdjustments object. Also, it
can then just return the related target or null as approriate.

I think the BoundaryAdjustments struct makes this code a little harder to make
sense of.