Bug 41877 – Notify browser about popup being deleted

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Lucas De Marchi]

Notify browser about popup being deleted

[Lucas De Marchi]

Created attachment 60916 [details]
Patch

[Lucas De Marchi]

Kent, I added you to CC since you reviewed the BREW implementation on bug 40226. It also applies for you, so I fixed your file too.

[Darin Adler]

Comment on attachment 60916 [details]
Patch

Generally speaking it's not good to do any work from the destructor of a reference-counted object. Because the timing of the destructor can be affected by someone taking a reference to the object. We normally avoid that pattern and do work at some well-defined time other than object destruction.

Can we do that here? Code that does anything except for deallocating memory and clearing out back pointers and such in the destructor of a reference counted object causes long term maintainability problems that I think we would be better off without.

[Lucas De Marchi]

(In reply to comment #3)
> (From update of attachment 60916 [details])
> Can we do that here? Code that does anything except for deallocating memory and clearing out back pointers and such in the destructor of a reference counted object causes long term maintainability problems that I think we would be better off without.

Darin, this will in fact deallocate m_popup that would remain alive otherwise. It's more or less what QT and GTK do, too.

[Kent Tamura]

> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 041e2e3c5e2edcf993c89a756f5ff2564a0324d7..c3bb6ad6eb3ccb8bc3a2049334eaf208b42861a0 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,24 @@
> [phone]  Lucas De Marchi  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Notify browser about popup being deleted. In EFL and BREW ports, the
> +        WebCore::Popup object was being deleted leaving the popup in browser
> +        alive. Popups can be deleted in two ways: either from browser to webcore or
> +        from webcore to browser. The first path was ok. The problem was when the
> +        user changed the page with a popup still opened. This would trigger the
> +        second path and would cause WebCore::Popup to be deleted without
> +        notifying browser.

If we need to delete a popup when the browser navigates to another page, we had better call hide() when the page is unloaded, not ~PopupMenu().  If it's hard to do so, the current code is ok.

[Antonio Gomes]

Luiz, how are we dealing with that on Qt?

[Lucas De Marchi]

This patch is only notifying the C counterpart of the API to give it a chance to free the platform data. This API happens to notify the browser, since it doesn't maintain the respective widget, and then disconnects itself.

I thought QT was using a similar approach, but I couldn't find where the popup would be deleted by WebCore. It's the same of GTK, though.

[WebKit Commit Bot]

Comment on attachment 60916 [details]
Patch

Clearing flags on attachment: 60916

Committed r63316: <http://trac.webkit.org/changeset/63316>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.