[webkit-reviews] review denied: [Bug 30754] Drags and Dragover events not in sync : [Attachment 42443] Patch with test case

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Daniel Bates <dbates at webkit.org>'s
request for review:
Bug 30754: Drags and Dragover events not in sync
https://bugs.webkit.org/show_bug.cgi?id=30754

Attachment 42443: Patch with test case
https://bugs.webkit.org/attachment.cgi?id=42443&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>

>  
>	   if (m_dragTarget) {
> -	       Frame* frame = (m_dragTarget->hasTagName(frameTag) ||
m_dragTarget->hasTagName(iframeTag)) ?
static_cast<HTMLFrameElementBase*>(m_dragTarget.get())->contentFrame() : 0;
> +	       Frame* frame = dragTargetUseToBeAFrameElement?
static_cast<HTMLFrameElementBase*>(m_dragTarget.get())->contentFrame() : 0;

Space before ?

> +
> +	   if (newTarget) {
> +	       // Q: Why do we do this?
> +	       //
> +	       // A: As per section 7.9.4 of the HTML 5 spec., we must fire the
dragover event on the new target (if it exists).
> +	       //    Notice, there are three drag events that could preceed a
dragover event (in order of firing): drag, dragenter,
> +	       //    and dragleave. Lets look at the interesting case when we
enter a targer area. Then, in the first call to this
> +	       //    function, we may see the following events fired: drag,
dragenter, (dragleave)?. In a subsequent call to this
> +	       //    function, we may see the following events fired (drag)?,
dragover.
> +	       //
> +	       // Q: Why do we set a field here as opposed to explicitly
calling dispatchDragEvent to fire the dragover event?
> +	       //
> +	       // A: This function is called during the processing of various
OS-generated mouse events, including mouse up,
> +	       //    mouse down, and mouse move. Notice, it is always the case
that this function is called at least twice
> +	       //    (one on mouse up and one on mouse down) (*). Moreover, on
either a mouse up or mouse down event, it is always
> +	       //    the case that m_dragTarget == newTarget (by (**)). So, a
dragover event is always fired (if we are over a target, i.e. 
> +	       //    newTarget is not null). Therefore, we do not explicitly
call dispatchDragEvent here because it could ultimately
> +	       //    result in the appearance that two dragover events fired.
> +	       //
> +	       // Q: Isn't setting m_shouldOnlyFireDragOverEvent inane? How can
we be guranteed that we will be called again? How 
> +	       //    can we be guranteed that we will execute the else clause
of the ambient if-else statement?
> +	       //
> +	       // A: By (*) we are guranteed to be called at least twice, and
by (**) at least one of those calls executes the "else"
> +	       //    clause of the ambient if-else statement. So, we can set a
flag here to fire the dragover event and we are
> +	       //    guaranteed that the dragover event will be fired on some
subsequent call to this function.
> +	       //
> +	       // (**) Let the smallest unit of measurement be a pixel. Let A
and B be two distinct target areas (i.e. there is at least one 
> +	       //     pixel in each target area that does not overlap some
pixel in the other target area - so that it is possible to mouse
> +	       //     up/down in both target areas). We say that a mouse move
event is fired whenever the coordinates of the mouse change by
> +	       //     one pixel in some direction.
> +	       //
> +	       // Claim: On a mouse up, mouse down, or mouse move event,
m_dragTarget == newTarget.
> +	       // Proof: Proof by contradiction. Suppose on a mouse up, mouse
down, or mouse move event that m_dragTarget != newTarget.
> +	       //	 Without loss of generality, lets consider the case of
a mouse up event.
> +	       //	 Notice, we must have a corresponding mouse down event
that preceeded the mouse up event.
> +	       //	 Assume the mouse down event occurs in target area
m_dragTarget := A, and that on a mouse move event we set m_dragTarget ==
newTarget.
> +	       //	 Suppose a mouse up event is fired and m_dragTarget !=
newTarget.
> +	       //	 Then the cursor must be positioned over B (i.e.
newTarget == B).
> +	       //	 Since A != B (by distinctness), the mouse must have
moved at least one pixel in some direction.
> +	       //	 But then m_dragTarget == newTarget (since a mouse move
event fired).
> +	       //	 Contradiction, by assumption, m_dragTarget !=
newTarget.
> +	       //	 Therefore, m_dragTarget == newTarget.

Your supposition is entirely correct -- this comment is _far_ too long, it
shouldn't need to be more than 3 or 4 lines, and we have far more complex logic
that has less, as reading the code gives you a reliably up to date reference
for the behaviour, and regression tests should prevent unintentional changes.

r- because of this.

> +	       else {
> +		   // Normally we would fire a drag then a dragover event here.
However when dealing with sub-frames, we may
> +		   // need to fire only a dragover event. This may be because
we have to notify the old target, which is in
> +		   // a different frame from that of the new target, to fire a
dragleave event. Hence, we check the field
> +		   // m_shouldOnlyFireDragOverEvent to determine if we need to
fire only a dragover event.

This comment also seems overly verbose
>      
> -void EventHandler::dragSourceMovedTo(const PlatformMouseEvent& event)
> +void EventHandler::dragSourceMovedTo(const PlatformMouseEvent&)
>  {
> -    if (dragState().m_dragSrc && dragState().m_dragSrcMayBeDHTML)
> -	   // for now we don't care if event handler cancels default behavior,
since there is none
> -	   dispatchDragSrcEvent(eventNames().dragEvent, event);
>  }

This function is now empty so should be removed, which of course will require
updating various webkit clients.