Bug 109177

Summary: Remove unnecessary calls to get() when calling setTarget and setRelatedTarget
Product: WebKit Reporter: Kentaro Hara <haraken>
Component: DOMAssignee: Kentaro Hara <haraken>
Status: RESOLVED INVALID    
Severity: Normal CC: abarth, ap, darin, dglazkov, ojan.autocc, sam, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Kentaro Hara 2013-02-07 04:30:48 PST
This is a follow-up patch for r142072.

Now we are passing raw pointers from EventContext::handleLocalEvents() to Event::setTarget() and Event::setRelatedTarget(). They should be PassRefPtrs.
Comment 1 Kentaro Hara 2013-02-07 04:32:16 PST
Created attachment 187062 [details]
Patch
Comment 2 Alexey Proskuryakov 2013-02-07 09:35:57 PST
Comment on attachment 187062 [details]
Patch

There is no rationale, and I think that this change is incorrect. PassRefPtr is a performance optimization to be used when passing ownership, which is not the case here.
Comment 3 Adam Barth 2013-02-07 11:16:13 PST
Huh?  This change just makes these call sites more readable by removing the unnecessary call to get().
Comment 4 Kentaro Hara 2013-02-07 16:54:33 PST
(In reply to comment #2)
> (From update of attachment 187062 [details])
> There is no rationale, and I think that this change is incorrect. PassRefPtr is a performance optimization to be used when passing ownership, which is not the case here.

ap: r142072 introduced the following code.

   event->setTarget(m_target.get());
   event->setCurrentTarget(m_currentTarget.get());
   if (m_relatedTarget.get() && event->isMouseEvent())
      toMouseEvent(event)->setRelatedTarget(m_relatedTarget.get());
+  else if (m_relatedTarget.get() && event->isFocusEvent())
+    toFocusEvent(event)->setRelatedTarget(m_relatedTarget);

The change made get() usage in EventContext.cpp inconsistent, so I just tried to clean them up.
Comment 5 Alexey Proskuryakov 2013-02-07 17:02:13 PST
Yes, it's possible that there were many changed introducing PassRefPtr unnecessarily. It's not good to use it without a reason, as that makes semantics of the function confusing.
Comment 6 Kentaro Hara 2013-02-07 17:04:38 PST
(In reply to comment #4)
>    event->setTarget(m_target.get());
>    event->setCurrentTarget(m_currentTarget.get());
>    if (m_relatedTarget.get() && event->isMouseEvent())
>       toMouseEvent(event)->setRelatedTarget(m_relatedTarget.get());
> +  else if (m_relatedTarget.get() && event->isFocusEvent())
> +    toFocusEvent(event)->setRelatedTarget(m_relatedTarget);

> Yes, it's possible that there were many changed introducing PassRefPtr unnecessarily. It's not good to use it without a reason, as that makes semantics of the function confusing.

Thanks. Then the above change should have been:

> +  else if (m_relatedTarget.get() && event->isFocusEvent())
> +    toFocusEvent(event)->setRelatedTarget(m_relatedTarget.get());

? I think the current inconsistent situation is not good at least.
Comment 7 Kentaro Hara 2013-02-07 17:27:20 PST
Created attachment 187206 [details]
Patch
Comment 8 Kentaro Hara 2013-04-08 03:51:30 PDT
Looks like this issue is already fixed on trunk.