There are some crashes when we clicked some link that opens child window via window.open or a tag with _blank target if process model is multiple secondary. It's because multiple secondary tries to assign new webprocess if related page is null. In order to keep the child window in same process with opener, we should pass related page when we create WebPageProxy.
Created attachment 254796 [details] first_suggestion
Comment on attachment 254796 [details] first_suggestion View in context: https://bugs.webkit.org/attachment.cgi?id=254796&action=review > Source/WebKit2/ChangeLog:13 > + page is null. In order to keep the child window in same process with opener, > "In order to keep the child window in same process with opener," When we run EWebKit with EWK_PROCESS_MODEL_MULTIPLE_SECONDARY though, should we keep new child window in same webprocess ? Does the "same process" mean UIProcess ? > Source/WebKit2/UIProcess/API/efl/ewk_view.h:366 > +EAPI Evas_Object *ewk_view_smart_add_with_opener(Evas *e, Evas_Smart *smart, Evas_Object *opener); I'm not sure if "opener" is clear name for this API.
(In reply to comment #2) > Comment on attachment 254796 [details] > first_suggestion > > View in context: > https://bugs.webkit.org/attachment.cgi?id=254796&action=review > > > Source/WebKit2/ChangeLog:13 > > + page is null. In order to keep the child window in same process with opener, > > > "In order to keep the child window in same process with opener," > > When we run EWebKit with EWK_PROCESS_MODEL_MULTIPLE_SECONDARY though, should > we keep new child window in same webprocess ? Does the "same process" mean > UIProcess ? > Sure, If not, we can't share the information between opener and child. same process mean that same webprocess. > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:366 > > +EAPI Evas_Object *ewk_view_smart_add_with_opener(Evas *e, Evas_Smart *smart, Evas_Object *opener); > > I'm not sure if "opener" is clear name for this API. I agree, however I don't have good name for this. In fact, this API is little bit ugly because we should use it only for window.open and opener means the ewk_view indicating javscript object "window.opener". Anyway, there are too many APIs to create ewk_view.
(In reply to comment #3) > (In reply to comment #2) > > Comment on attachment 254796 [details] > > first_suggestion > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=254796&action=review > > > > > Source/WebKit2/ChangeLog:13 > > > + page is null. In order to keep the child window in same process with opener, > > > > > "In order to keep the child window in same process with opener," > > > > When we run EWebKit with EWK_PROCESS_MODEL_MULTIPLE_SECONDARY though, should > > we keep new child window in same webprocess ? Does the "same process" mean > > UIProcess ? > > > Sure, If not, we can't share the information between opener and child. > same process mean that same webprocess. As far as I know, EWK_PROCESS_MODEL_MULTIPLE_SECONDARY creates a new webprocess for a child window, right ? Under this understanding, I don't know below sentence well. Because EWK_PROCESS_MODEL_MULTIPLE_SECONDARY makes new webprocess for new child window. But I see that child window needs to have a reference to opener window. > "In order to keep the child window in same process with opener," > > > Source/WebKit2/UIProcess/API/efl/ewk_view.h:366 > > > +EAPI Evas_Object *ewk_view_smart_add_with_opener(Evas *e, Evas_Smart *smart, Evas_Object *opener); > > > > I'm not sure if "opener" is clear name for this API. > > I agree, however I don't have good name for this. > In fact, this API is little bit ugly because we should use it only for > window.open and opener means the ewk_view indicating javscript object > "window.opener". > > Anyway, there are too many APIs to create ewk_view. How about ewk_view_smart_add_with_parent ? Or, if we use _opener, I think we have to add more detailed description in API.
Comment on attachment 254796 [details] first_suggestion View in context: https://bugs.webkit.org/attachment.cgi?id=254796&action=review >>>> Source/WebKit2/ChangeLog:13 >>>> + page is null. In order to keep the child window in same process with opener, >>> >>> >> >> Sure, If not, we can't share the information between opener and child. >> same process mean that same webprocess. > > As far as I know, EWK_PROCESS_MODEL_MULTIPLE_SECONDARY creates a new webprocess for a child window, right ? Under this understanding, I don't know below sentence well. Because EWK_PROCESS_MODEL_MULTIPLE_SECONDARY makes new webprocess for new child window. But I see that child window needs to have a reference to opener window. EWK_PROCESS_MODEL_MULTIPLE_SECONDARY basically creates new webprocess for the new webview. But child webview via window.open or A link(with _blank) should be created in webprocess of opener. >>>> Source/WebKit2/UIProcess/API/efl/ewk_view.h:366 >>>> +EAPI Evas_Object *ewk_view_smart_add_with_opener(Evas *e, Evas_Smart *smart, Evas_Object *opener); >>> >>> I'm not sure if "opener" is clear name for this API. >> >> I agree, however I don't have good name for this. >> In fact, this API is little bit ugly because we should use it only for window.open and opener means the ewk_view indicating javscript object "window.opener". >> >> Anyway, there are too many APIs to create ewk_view. > > How about ewk_view_smart_add_with_parent ? Or, if we use _opener, I think we have to add more detailed description in API. ewk_view_smart_add_with_parent looks general. I will consider mode description. BTW, I hope that we have a chance to refactor ewk APIs like MAC port ? Then we may need only two APIs. ewk_view_add(Evas*); ewk_view_smart_add(Evas*, Evas_Smart*, Ewk_View_Configuration*); Then, we don't need Ewk_Context and Ewk_PageGroup.
(In reply to comment #5) > BTW, I hope that we have a chance to refactor ewk APIs like MAC port ? > Then we may need only two APIs. > ewk_view_add(Evas*); > ewk_view_smart_add(Evas*, Evas_Smart*, Ewk_View_Configuration*); > > Then, we don't need Ewk_Context and Ewk_PageGroup. I agree with this change. We also need to use APIConfiguration.
(In reply to comment #6) > (In reply to comment #5) > > BTW, I hope that we have a chance to refactor ewk APIs like MAC port ? > > Then we may need only two APIs. > > ewk_view_add(Evas*); > > ewk_view_smart_add(Evas*, Evas_Smart*, Ewk_View_Configuration*); > > > > Then, we don't need Ewk_Context and Ewk_PageGroup. > > I agree with this change. We also need to use APIConfiguration. OK, then I will check and write an email about it. I will hold this issue until we decide direction.
Created attachment 265111 [details] Patch
Created attachment 265115 [details] Patch
Comment on attachment 265115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265115&action=review LGTM now. Thanks. > Tools/MiniBrowser/efl/main.c:2272 > + Ewk_Context *context = ewk_view_context_get(window->ewk_view); It's extremely rare case ewk_view_context_get() returns null though, it would be good to add ASSERT(context);
(In reply to comment #10) > > Tools/MiniBrowser/efl/main.c:2272 > > + Ewk_Context *context = ewk_view_context_get(window->ewk_view); > > It's extremely rare case ewk_view_context_get() returns null though, it > would be good to add ASSERT(context); I missed to know this file is one of MiniBrowser. Let's land !
Comment on attachment 265115 [details] Patch Clearing flags on attachment: 265115 Committed r192196: <http://trac.webkit.org/changeset/192196>
All reviewed patches have been landed. Closing bug.
Comment on attachment 265115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265115&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:746 > + if (!ewk_view_title_get(s_newWindowObject), "Page1") Did you mean if (!strcmp(ewk_view_title_get(s_newWindowObject), "Page1")) ? > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:758 > + if (!ewk_view_title_get(s_newWindowObject), "Page2") ditto > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:780 > + if (!ewk_view_title_get(s_newWindowObject), "Page1") ditto > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:792 > + if (!ewk_view_title_get(s_newWindowObject), "Page2") ditto
(In reply to comment #14) rs=me for the followup fix.
Comment on attachment 265115 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=265115&action=review >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:746 >> + if (!ewk_view_title_get(s_newWindowObject), "Page1") > > Did you mean if (!strcmp(ewk_view_title_get(s_newWindowObject), "Page1")) ? OMG. My stupid fault. :(
(In reply to comment #16) > Comment on attachment 265115 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=265115&action=review > > >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_view.cpp:746 > >> + if (!ewk_view_title_get(s_newWindowObject), "Page1") > > > > Did you mean if (!strcmp(ewk_view_title_get(s_newWindowObject), "Page1")) ? > > OMG. My stupid fault. :( Sorry, I created th bug 151247 to cover this issue. After fixed these typos, there are some bugs that first test(for window.open()) affect second test(for blank target).