41877 2010-07-08 11:17:45 -0700 Notify browser about popup being deleted 2010-07-14 07:10:43 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) Other OS X 10.5 RESOLVED FIXED P2 Normal --- 1 lucas.de.marchi lucas.de.marchi barbieri commit-queue luiz tkent tonikitoo oldest_to_newest 248355 0 lucas.de.marchi 2010-07-08 11:17:45 -0700 Notify browser about popup being deleted 248366 1 60916 lucas.de.marchi 2010-07-08 11:29:08 -0700 Created attachment 60916 Patch 248369 2 lucas.de.marchi 2010-07-08 11:32:45 -0700 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. 248376 3 60916 darin 2010-07-08 11:36:05 -0700 Comment on attachment 60916 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. 248386 4 lucas.de.marchi 2010-07-08 11:46:27 -0700 (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. 249731 5 tkent 2010-07-12 02:50:20 -0700 > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > index 041e2e3c5e2edcf993c89a756f5ff2564a0324d7..c3bb6ad6eb3ccb8bc3a2049334eaf208b42861a0 100644 > --- a/WebCore/ChangeLog > +++ b/WebCore/ChangeLog > @@ -1,3 +1,24 @@ > +2010-07-08 Lucas De Marchi <lucas.demarchi@profusion.mobi> > + > + 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. 249805 6 tonikitoo 2010-07-12 06:54:06 -0700 Luiz, how are we dealing with that on Qt? 250822 7 lucas.de.marchi 2010-07-13 16:57:40 -0700 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. 251168 8 60916 commit-queue 2010-07-14 07:10:39 -0700 Comment on attachment 60916 Patch Clearing flags on attachment: 60916 Committed r63316: <http://trac.webkit.org/changeset/63316> 251169 9 commit-queue 2010-07-14 07:10:43 -0700 All reviewed patches have been landed. Closing bug. 60916 2010-07-08 11:29:08 -0700 2010-07-14 07:10:38 -0700 Patch bug-41877-20100708153000.patch text/plain 2363 lucas.de.marchi ZGlmZiAtLWdpdCBhL1dlYkNvcmUvQ2hhbmdlTG9nIGIvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXgg MDQxZTJlM2M1ZTJlZGNmOTkzYzg5YTc1NmY1ZmYyNTY0YTAzMjRkNy4uYzNiYjZhZDZlYjNjY2I4 YmMzYTIwNDkzMzRlYWYyMDhiNDI4NjFhMCAxMDA2NDQKLS0tIGEvV2ViQ29yZS9DaGFuZ2VMb2cK KysrIGIvV2ViQ29yZS9DaGFuZ2VMb2cKQEAgLTEsMyArMSwyNCBAQAorMjAxMC0wNy0wOCAgTHVj YXMgRGUgTWFyY2hpICA8bHVjYXMuZGVtYXJjaGlAcHJvZnVzaW9uLm1vYmk+CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTm90aWZ5IGJyb3dzZXIgYWJv dXQgcG9wdXAgYmVpbmcgZGVsZXRlZC4gSW4gRUZMIGFuZCBCUkVXIHBvcnRzLCB0aGUKKyAgICAg ICAgV2ViQ29yZTo6UG9wdXAgb2JqZWN0IHdhcyBiZWluZyBkZWxldGVkIGxlYXZpbmcgdGhlIHBv cHVwIGluIGJyb3dzZXIKKyAgICAgICAgYWxpdmUuIFBvcHVwcyBjYW4gYmUgZGVsZXRlZCBpbiB0 d28gd2F5czogZWl0aGVyIGZyb20gYnJvd3NlciB0byB3ZWJjb3JlIG9yCisgICAgICAgIGZyb20g d2ViY29yZSB0byBicm93c2VyLiBUaGUgZmlyc3QgcGF0aCB3YXMgb2suIFRoZSBwcm9ibGVtIHdh cyB3aGVuIHRoZQorICAgICAgICB1c2VyIGNoYW5nZWQgdGhlIHBhZ2Ugd2l0aCBhIHBvcHVwIHN0 aWxsIG9wZW5lZC4gVGhpcyB3b3VsZCB0cmlnZ2VyIHRoZQorICAgICAgICBzZWNvbmQgcGF0aCBh bmQgd291bGQgY2F1c2UgV2ViQ29yZTo6UG9wdXAgdG8gYmUgZGVsZXRlZCB3aXRob3V0CisgICAg ICAgIG5vdGlmeWluZyBicm93c2VyLgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9NDE4NzcKKworICAgICAgICBObyBuZXcgZnVuY3Rpb25hbGl0eSBzbyBu byBuZXcgdGVzdHMuCisKKyAgICAgICAgKiBwbGF0Zm9ybS9icmV3L1BvcHVwTWVudUJyZXcuY3Bw OgorICAgICAgICAoV2ViQ29yZTo6UG9wdXBNZW51Ojp+UG9wdXBNZW51KTogY2FsbCBoaWRlKCkg d2hlbiBvYmplY3QgaXMKKyAgICAgICAgZGVzdHJveWVkLgorICAgICAgICAqIHBsYXRmb3JtL2Vm bC9Qb3B1cE1lbnVFZmwuY3BwOgorICAgICAgICAoV2ViQ29yZTo6UG9wdXBNZW51Ojp+UG9wdXBN ZW51KTogZGl0dG8uCisKIDIwMTAtMDctMDggIFhhbiBMb3BleiAgPHhsb3BlekBpZ2FsaWEuY29t PgogCiAgICAgICAgIFJldmlld2VkIGJ5IEd1c3Rhdm8gTm9yb25oYS4KZGlmZiAtLWdpdCBhL1dl YkNvcmUvcGxhdGZvcm0vYnJldy9Qb3B1cE1lbnVCcmV3LmNwcCBiL1dlYkNvcmUvcGxhdGZvcm0v YnJldy9Qb3B1cE1lbnVCcmV3LmNwcAppbmRleCA4OWYzZmExNjQzMDIwODk4NmExM2M3YjkxMDYw ZTc1MTRiODM4NWU5Li5lYjAzYzY2MjJjNGFlNDNiZTA0MWNhMjZmNWNjOGM4ZjcwZWNiNjNkIDEw MDY0NAotLS0gYS9XZWJDb3JlL3BsYXRmb3JtL2JyZXcvUG9wdXBNZW51QnJldy5jcHAKKysrIGIv V2ViQ29yZS9wbGF0Zm9ybS9icmV3L1BvcHVwTWVudUJyZXcuY3BwCkBAIC00MCw2ICs0MCw5IEBA IFBvcHVwTWVudTo6UG9wdXBNZW51KFBvcHVwTWVudUNsaWVudCogbWVudUxpc3QpCiAKIFBvcHVw TWVudTo6flBvcHVwTWVudSgpCiB7CisgICAgLy8gVGVsbCBjbGllbnQgdG8gZGVzdHJveSBkYXRh IHJlbGF0ZWQgdG8gdGhpcyBwb3B1cCBzaW5jZSB0aGlzIG9iamVjdCBpcworICAgIC8vIGdvaW5n IGF3YXkuCisgICAgaGlkZSgpOwogfQogCiB2b2lkIFBvcHVwTWVudTo6c2hvdyhjb25zdCBJbnRS ZWN0JiByZWN0LCBGcmFtZVZpZXcqIHZpZXcsIGludCBpbmRleCkKZGlmZiAtLWdpdCBhL1dlYkNv cmUvcGxhdGZvcm0vZWZsL1BvcHVwTWVudUVmbC5jcHAgYi9XZWJDb3JlL3BsYXRmb3JtL2VmbC9Q b3B1cE1lbnVFZmwuY3BwCmluZGV4IDRkOTY3N2I1ZDVjM2EyNzk2NjRmMmE1ZGQ4OWMwMGM0NDU2 NzRiYWQuLjlkYzY5ODY5NmE5ZTEzZDlmZWMxODcwODgzZmJjMTRlNjJlOWU2NTcgMTAwNjQ0Ci0t LSBhL1dlYkNvcmUvcGxhdGZvcm0vZWZsL1BvcHVwTWVudUVmbC5jcHAKKysrIGIvV2ViQ29yZS9w bGF0Zm9ybS9lZmwvUG9wdXBNZW51RWZsLmNwcApAQCAtMzksNiArMzksOSBAQCBQb3B1cE1lbnU6 OlBvcHVwTWVudShQb3B1cE1lbnVDbGllbnQqIGNsaWVudCkKIAogUG9wdXBNZW51Ojp+UG9wdXBN ZW51KCkKIHsKKyAgICAvLyBUZWxsIGNsaWVudCB0byBkZXN0cm95IGRhdGEgcmVsYXRlZCB0byB0 aGlzIHBvcHVwIHNpbmNlIHRoaXMgb2JqZWN0IGlzCisgICAgLy8gZ29pbmcgYXdheS4KKyAgICBo aWRlKCk7CiB9CiAKIHZvaWQgUG9wdXBNZW51OjpzaG93KGNvbnN0IEludFJlY3QmIHJlY3QsIEZy YW1lVmlldyogdmlldywgaW50IGluZGV4KQo=