103229 2012-11-26 01:41:37 -0800 [EFL][WK2] Duplicated WebPageGroup initialization 2012-12-11 23:42:14 -0800 1 1 1 Unclassified WebKit WebKit EFL 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 103604 103692 1 jinwoo7.song jinwoo7.song cdumez gyuyoung.kim kangil.han kenneth lucas.de.marchi rakuco ryuan.choi webkit.review.bot yael oldest_to_newest 775252 0 jinwoo7.song 2012-11-26 01:41:37 -0800 WebPageGroup does not need to be created in creating EwkView when the pageGroupRef is 0 because the default WebPageGroup(m_defaultPageGroup) is created in WebContext constructor. As a side effect on the duplicated initialization, pageGroupID increases even when the EwkView is created with default context. The issue can be easily detected with the following test case. (Even the window name is same, the new window is created.) http://www.w3schools.com/js/tryit.asp?filename=try_win_name 775260 1 175942 jinwoo7.song 2012-11-26 01:49:50 -0800 Created attachment 175942 Patch 777463 2 kangil.han 2012-11-27 19:18:15 -0800 LGTM, thanks. :) 777471 3 ryuan.choi 2012-11-27 19:50:27 -0800 LGTM too 777509 4 175942 webkit.review.bot 2012-11-27 21:29:14 -0800 Comment on attachment 175942 Patch Clearing flags on attachment: 175942 Committed r135973: <http://trac.webkit.org/changeset/135973> 777510 5 webkit.review.bot 2012-11-27 21:29:18 -0800 All reviewed patches have been landed. Closing bug. 778711 6 webkit.review.bot 2012-11-28 22:48:58 -0800 Re-opened since this is blocked by bug 103604 779660 7 jinwoo7.song 2012-11-29 17:39:28 -0800 ewk_setting API test cases should be fixed to land this patch. (bug 103692) 779889 8 cdumez 2012-11-30 00:31:04 -0800 Kenneth, could you please take a look at this? This patch changes the behavior a bit. All the views from the default context will be in the same PageGroup, meaning that they will share the settings. Previously, we were creating a new PageGroup for each view and therefore, each view had its own settings. 779919 9 175942 kenneth 2012-11-30 01:07:59 -0800 Comment on attachment 175942 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175942&action=review > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:512 > ASSERT(!smartData->priv); > - RefPtr<WebPageGroup> pageGroup = pageGroupRef ? toImpl(pageGroupRef) : WebPageGroup::create(); > - smartData->priv = new EwkViewImpl(ewkView, context, pageGroup, behavior); > + smartData->priv = new EwkViewImpl(ewkView, context, toImpl(pageGroupRef), behavior); > return ewkView; I wonder whether we should add a comment? 780034 10 yael 2012-11-30 05:23:30 -0800 (In reply to comment #8) > Kenneth, could you please take a look at this? > > This patch changes the behavior a bit. All the views from the default context will be in the same PageGroup, meaning that they will share the settings. > Previously, we were creating a new PageGroup for each view and therefore, each view had its own settings. That behavior was wrong. If a page creates a named popup, and then tries to create it again with the same name, we were creating a new popup instead of replacing the existing one. 780658 11 jinwoo7.song 2012-11-30 22:23:23 -0800 (In reply to comment #9) > (From update of attachment 175942 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=175942&action=review > > > Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:512 > > ASSERT(!smartData->priv); > > - RefPtr<WebPageGroup> pageGroup = pageGroupRef ? toImpl(pageGroupRef) : WebPageGroup::create(); > > - smartData->priv = new EwkViewImpl(ewkView, context, pageGroup, behavior); > > + smartData->priv = new EwkViewImpl(ewkView, context, toImpl(pageGroupRef), behavior); > > return ewkView; > > I wonder whether we should add a comment? I added the comment here. 780659 12 177086 jinwoo7.song 2012-11-30 22:26:29 -0800 Created attachment 177086 patch Patch with more comments. 789252 13 177086 webkit.review.bot 2012-12-11 23:42:09 -0800 Comment on attachment 177086 patch Clearing flags on attachment: 177086 Committed r137423: <http://trac.webkit.org/changeset/137423> 789253 14 webkit.review.bot 2012-12-11 23:42:14 -0800 All reviewed patches have been landed. Closing bug. 175942 2012-11-26 01:49:50 -0800 2012-11-30 22:26:29 -0800 Patch bug-103229-20121126184725.patch text/plain 1772 jinwoo7.song U3VidmVyc2lvbiBSZXZpc2lvbjogMTM1Njc4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0Mi9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViS2l0Mi9DaGFuZ2VMb2cKaW5kZXggNzFhMjViYmZlODViNWI0 YjQ0ZWU5YzE4ZTcxNGJkZTAxYTM1NmQyMC4uMjJkN2FlZjM5Njg0YzQ3ZjBmY2Q4YTFhMWY3OGQ5 Y2Q5NGExZTM1ZCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktpdDIvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5IEBACisyMDEyLTExLTI2ICBKaW53 b28gU29uZyAgPGppbndvbzcuc29uZ0BzYW1zdW5nLmNvbT4KKworICAgICAgICBbRUZMXVtXSzJd IER1cGxpY2F0ZWQgV2ViUGFnZUdyb3VwIGluaXRpYWxpemF0aW9uCisgICAgICAgIGh0dHBzOi8v YnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDMyMjkKKworICAgICAgICBSZXZpZXdl ZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXZWJQYWdlR3JvdXAgZG9lcyBub3QgbmVl ZCB0byBiZSBjcmVhdGVkIGluIGNyZWF0aW5nIEV3a1ZpZXcgd2hlbiB0aGUgcGFnZUdyb3VwUmVm IGlzIDAKKyAgICAgICAgYmVjYXVzZSB0aGUgZGVmYXVsdCBXZWJQYWdlR3JvdXAobV9kZWZhdWx0 UGFnZUdyb3VwKSBpcyBjcmVhdGVkIGluIFdlYkNvbnRleHQgY29uc3RydWN0b3IuCisKKyAgICAg ICAgQXMgYSBzaWRlIGVmZmVjdCBvbiB0aGUgZHVwbGljYXRlZCBpbml0aWFsaXphdGlvbiwgcGFn ZUdyb3VwSUQgaW5jcmVhc2VzIGV2ZW4gd2hlbiB0aGUgRXdrVmlldworICAgICAgICBpcyBjcmVh dGVkIHdpdGggZGVmYXVsdCBjb250ZXh0LgorCisgICAgICAgICogVUlQcm9jZXNzL0FQSS9lZmwv ZXdrX3ZpZXcuY3BwOgorICAgICAgICAoY3JlYXRlRXdrVmlldyk6CisKIDIwMTItMTEtMjUgIE1p a2hhaWwgUG96ZG55YWtvdiAgPG1pa2hhaWwucG96ZG55YWtvdkBpbnRlbC5jb20+CiAKICAgICAg ICAgW1dLMl0gVGlsZWRCYWNraW5nU3RvcmU6IHBhZ2UgY29udGVudHMgaXMgc2NhbGVkIHdyb25n bHkKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZsL2V3a192aWV3 LmNwcCBiL1NvdXJjZS9XZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZsL2V3a192aWV3LmNwcAppbmRl eCBkNWQ1MGVkMGFkN2Y3ZTU3MTVkOWUzMDdhZjAwYTUwNTAzODg4ZmEzLi45YzcwYzYyNDY2NmEx MWJiNzk0YTVmOWMxYzI4NmYxODRjMTVhMmM4IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViS2l0Mi9V SVByb2Nlc3MvQVBJL2VmbC9ld2tfdmlldy5jcHAKKysrIGIvU291cmNlL1dlYktpdDIvVUlQcm9j ZXNzL0FQSS9lZmwvZXdrX3ZpZXcuY3BwCkBAIC01MDgsOCArNTA4LDcgQEAgc3RhdGljIGlubGlu ZSBFdmFzX09iamVjdCogY3JlYXRlRXdrVmlldyhFdmFzKiBjYW52YXMsIEV2YXNfU21hcnQqIHNt YXJ0LCBQYXNzUmUKICAgICB9CiAKICAgICBBU1NFUlQoIXNtYXJ0RGF0YS0+cHJpdik7Ci0gICAg UmVmUHRyPFdlYlBhZ2VHcm91cD4gcGFnZUdyb3VwID0gcGFnZUdyb3VwUmVmID8gdG9JbXBsKHBh Z2VHcm91cFJlZikgOiBXZWJQYWdlR3JvdXA6OmNyZWF0ZSgpOwotICAgIHNtYXJ0RGF0YS0+cHJp diA9IG5ldyBFd2tWaWV3SW1wbChld2tWaWV3LCBjb250ZXh0LCBwYWdlR3JvdXAsIGJlaGF2aW9y KTsKKyAgICBzbWFydERhdGEtPnByaXYgPSBuZXcgRXdrVmlld0ltcGwoZXdrVmlldywgY29udGV4 dCwgdG9JbXBsKHBhZ2VHcm91cFJlZiksIGJlaGF2aW9yKTsKICAgICByZXR1cm4gZXdrVmlldzsK IH0KIAo= 177086 2012-11-30 22:26:29 -0800 2012-12-11 23:42:09 -0800 patch context.patch text/plain 1749 jinwoo7.song ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQyL0No YW5nZUxvZwppbmRleCBjMjhkY2ViYi4uNjQ4Y2JmYyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYktp dDIvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQyL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE5 IEBACisyMDEyLTExLTMwICBKaW53b28gU29uZyAgPGppbndvbzcuc29uZ0BzYW1zdW5nLmNvbT4K KworICAgICAgICBbRUZMXVtXSzJdIER1cGxpY2F0ZWQgV2ViUGFnZUdyb3VwIGluaXRpYWxpemF0 aW9uCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDMy MjkKKworICAgICAgICBSZXZpZXdlZCBieSBLZW5uZXRoIFJvaGRlIENocmlzdGlhbnNlbi4KKwor ICAgICAgICBXZWJQYWdlR3JvdXAgZG9lcyBub3QgbmVlZCB0byBiZSBjcmVhdGVkIGluIGNyZWF0 aW5nIEV3a1ZpZXcgd2hlbiB0aGUgcGFnZUdyb3VwUmVmIGlzIDAKKyAgICAgICAgYmVjYXVzZSB0 aGUgZGVmYXVsdCBXZWJQYWdlR3JvdXAobV9kZWZhdWx0UGFnZUdyb3VwKSBpcyBjcmVhdGVkIGlu IFdlYkNvbnRleHQgY29uc3RydWN0b3IuCisKKyAgICAgICAgQXMgYSBzaWRlIGVmZmVjdCBvbiB0 aGUgZHVwbGljYXRlZCBpbml0aWFsaXphdGlvbiwgcGFnZUdyb3VwSUQgaW5jcmVhc2VzIGV2ZW4g d2hlbiB0aGUgRXdrVmlldworICAgICAgICBpcyBjcmVhdGVkIHdpdGggZGVmYXVsdCBjb250ZXh0 LgorCisgICAgICAgICogVUlQcm9jZXNzL0FQSS9lZmwvZXdrX3ZpZXcuY3BwOgorICAgICAgICAo Y3JlYXRlRXdrVmlldyk6CisKIDIwMTItMTEtMzAgIEFuZGVycyBDYXJsc3NvbiAgPGFuZGVyc2Nh QGFwcGxlLmNvbT4KIAogICAgICAgICBBbHdheXMgbGlzdGVuIGZvciB0aGUgRGlkRmlyc3RMYXlv dXQgbGF5b3V0IG1pbGVzdG9uZQpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYktpdDIvVUlQcm9jZXNz L0FQSS9lZmwvZXdrX3ZpZXcuY3BwIGIvU291cmNlL1dlYktpdDIvVUlQcm9jZXNzL0FQSS9lZmwv ZXdrX3ZpZXcuY3BwCmluZGV4IDM0OWIwODYuLjZiMDI5ZmIgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9X ZWJLaXQyL1VJUHJvY2Vzcy9BUEkvZWZsL2V3a192aWV3LmNwcAorKysgYi9Tb3VyY2UvV2ViS2l0 Mi9VSVByb2Nlc3MvQVBJL2VmbC9ld2tfdmlldy5jcHAKQEAgLTUwNyw4ICs1MDcsMTAgQEAgc3Rh dGljIGlubGluZSBFdmFzX09iamVjdCogY3JlYXRlRXdrVmlldyhFdmFzKiBjYW52YXMsIEV2YXNf U21hcnQqIHNtYXJ0LCBQYXNzUmUKICAgICB9CiAKICAgICBBU1NFUlQoIXNtYXJ0RGF0YS0+cHJp dik7Ci0gICAgUmVmUHRyPFdlYlBhZ2VHcm91cD4gcGFnZUdyb3VwID0gcGFnZUdyb3VwUmVmID8g dG9JbXBsKHBhZ2VHcm91cFJlZikgOiBXZWJQYWdlR3JvdXA6OmNyZWF0ZSgpOwotICAgIHNtYXJ0 RGF0YS0+cHJpdiA9IG5ldyBFd2tWaWV3SW1wbChld2tWaWV3LCBjb250ZXh0LCBwYWdlR3JvdXAs IGJlaGF2aW9yKTsKKworICAgIC8vIERlZmF1bHQgV2ViUGFnZUdyb3VwIGlzIGNyZWF0ZWQgaW4g V2ViQ29udGV4dCBjb25zdHJ1Y3RvciBpZiB0aGUgcGFnZUdyb3VwUmVmIGlzIDAsCisgICAgLy8g c28gd2UgZG8gbm90IG5lZWQgdG8gY3JlYXRlIGl0IGhlcmUuCisgICAgc21hcnREYXRhLT5wcml2 ID0gbmV3IEV3a1ZpZXdJbXBsKGV3a1ZpZXcsIGNvbnRleHQsIHRvSW1wbChwYWdlR3JvdXBSZWYp LCBiZWhhdmlvcik7CiAgICAgcmV0dXJuIGV3a1ZpZXc7CiB9CiAK