[webkit-reviews] review denied: [Bug 109897] Add setValue and closePopup methods to PagePopupController : [Attachment 188490] Patch

[bugzilla-daemon at webkit.org]

Kent Tamura (ooo until Mar 15) <tkent at chromium.org> has denied Keishi Hattori
<keishi at webkit.org>'s request for review:
Bug 109897: Add setValue and closePopup methods to PagePopupController
https://bugs.webkit.org/show_bug.cgi?id=109897

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

------- Additional Comments from Kent Tamura (ooo until Mar 15)
<tkent at chromium.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=188490&action=review


> Source/WebKit/chromium/src/DateTimeChooserImpl.cpp:173
> -    RefPtr<DateTimeChooserImpl> protector(this);
>      if (numValue >= 0)
> -	   m_client->didChooseValue(stringValue);
> +	   setValue(stringValue);
> +    endChooser();
> +}

The protector is required here.  setValue(), which might dispatch some events,
can delete 'this'.

> Source/WebKit/chromium/src/DateTimeChooserImpl.cpp:178
> +    RefPtr<DateTimeChooserImpl> protector(this);
> +    m_client->didChooseValue(value);

The protector makes no sense. We do nothing after didChooseValue, which might
dispatch some events.
Also, I'm afraid that we have no ways to avoid calendar picker JavaScript code
execution after didChooseValue because event handler might delete the page
popup.