Bug 109177 – Remove unnecessary calls to get() when calling setTarget and setRelatedTarget

• Component: DOM
• Status: RESOLVED INVALID

[Kentaro Hara]

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.

[Kentaro Hara]

Created attachment 187062 [details]
Patch

[Alexey Proskuryakov]

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.

[Adam Barth]

Huh?  This change just makes these call sites more readable by removing the unnecessary call to get().

[Kentaro Hara]

(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.

[Alexey Proskuryakov]

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.

[Kentaro Hara]

(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.

[Kentaro Hara]

Created attachment 187206 [details]
Patch

[Kentaro Hara]

Looks like this issue is already fixed on trunk.