Current WebSoupCustomProtocolRequestManager::registerSchemeForCustomProtocol generates a crash when duplicated scheme is registered. However application can register duplicate scheme by mistake or on purpose. Thus IMO it would be good if we don't register it instead of registering it or generating a crash on debug mode when duplicated scheme is registered.
Created attachment 255340 [details] Patch
Comment on attachment 255340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255340&action=review > Source/WebKit2/UIProcess/Network/CustomProtocols/soup/WebSoupCustomProtocolRequestManager.cpp:85 > + ASSERT(!scheme.isNull()); > + if (m_registeredSchemes.contains(scheme)) > + return; I don't think this is the right place to check this. I think APIs should check it earlier. Registering the same scheme twice is a programmer error, at least in GTK+ API, but with this patch, there could be a change in behaviour, I mean, imagine you do: ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback1, nullptr); ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback2, nullptr); With this patch, the scheme will not be registered again, but the handler will be replaced with the second callback. If that's the desired behaviour for EFL, then the patch is correct. I'll add a g_return_if_fail protector to GTK+ API anyway.
I'm thinking that maybe replacing the callback is indeed the best approach. We would just need to document it, though.
Comment on attachment 255340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255340&action=review >> Source/WebKit2/UIProcess/Network/CustomProtocols/soup/WebSoupCustomProtocolRequestManager.cpp:85 >> + return; > > I don't think this is the right place to check this. I think APIs should check it earlier. Registering the same scheme twice is a programmer error, at least in GTK+ API, but with this patch, there could be a change in behaviour, I mean, imagine you do: > > ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback1, nullptr); > ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback2, nullptr); > > With this patch, the scheme will not be registered again, but the handler will be replaced with the second callback. If that's the desired behaviour for EFL, then the patch is correct. I'll add a g_return_if_fail protector to GTK+ API anyway. I would like to suggest a feature that user can change registered callback for same scheme. If GTK port don't mind to check same scheme in this place, I upload a patch which modifies callback as you mentioned.
(In reply to comment #4) > Comment on attachment 255340 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255340&action=review > > >> Source/WebKit2/UIProcess/Network/CustomProtocols/soup/WebSoupCustomProtocolRequestManager.cpp:85 > >> + return; > > > > I don't think this is the right place to check this. I think APIs should check it earlier. Registering the same scheme twice is a programmer error, at least in GTK+ API, but with this patch, there could be a change in behaviour, I mean, imagine you do: > > > > ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback1, nullptr); > > ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback2, nullptr); > > > > With this patch, the scheme will not be registered again, but the handler will be replaced with the second callback. If that's the desired behaviour for EFL, then the patch is correct. I'll add a g_return_if_fail protector to GTK+ API anyway. > > I would like to suggest a feature that user can change registered callback > for same scheme. If GTK port don't mind to check same scheme in this place, > I upload a patch which modifies callback as you mentioned. I'm not sure I get what you mean, this patch already modifies the callback if I understood it correctly. If that was your intention and the desired behaviour for EFL API, I suggest you to update the unit test to not only check you can register the same scheme twice without crashing, but also that the second callback is the one called after the second register.
Comment on attachment 255340 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255340&action=review >>>> Source/WebKit2/UIProcess/Network/CustomProtocols/soup/WebSoupCustomProtocolRequestManager.cpp:85 >>>> + return; >>> >>> I don't think this is the right place to check this. I think APIs should check it earlier. Registering the same scheme twice is a programmer error, at least in GTK+ API, but with this patch, there could be a change in behaviour, I mean, imagine you do: >>> >>> ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback1, nullptr); >>> ewk_context_url_scheme_register(ewk_view_context_get(webView()), "fooscheme", schemeRequestCallback2, nullptr); >>> >>> With this patch, the scheme will not be registered again, but the handler will be replaced with the second callback. If that's the desired behaviour for EFL, then the patch is correct. I'll add a g_return_if_fail protector to GTK+ API anyway. >> >> I would like to suggest a feature that user can change registered callback for same scheme. If GTK port don't mind to check same scheme in this place, I upload a patch which modifies callback as you mentioned. > > I'm not sure I get what you mean, this patch already modifies the callback if I understood it correctly. If that was your intention and the desired behaviour for EFL API, I suggest you to update the unit test to not only check you can register the same scheme twice without crashing, but also that the second callback is the one called after the second register. yes, you understand what I meant correctly. I'm modifying this test now.
Created attachment 255352 [details] Patch
Comment on attachment 255352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255352&action=review > Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:155 > ASSERT_TRUE(waitUntilTrue(finishTest, testTimeoutSeconds)); How do you know it was the second callback the one finishing the test and not the first one if both change the same variable?
Comment on attachment 255352 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=255352&action=review >> Source/WebKit2/UIProcess/API/efl/tests/test_ewk2_context.cpp:155 >> ASSERT_TRUE(waitUntilTrue(finishTest, testTimeoutSeconds)); > > How do you know it was the second callback the one finishing the test and not the first one if both change the same variable? To check if second callback is called correctly, I change custom scheme's url with *MyNewPath* in 154 line. Then the second callback(:= schemeRequestCallback2()) checks the changed url as below, static void schemeRequestCallback2(Ewk_Url_Scheme_Request* request, void* userData) { ... ASSERT_STREQ("fooscheme:MyNewPath", url); const char* path = ewk_url_scheme_request_path_get(request); ASSERT_STREQ("MyNewPath", path); } Besides *finishTest* flag is reset to test second callback.
Comment on attachment 255352 [details] Patch Ah, ok, I didn't notice the paths detail, sorry.
(In reply to comment #10) > Comment on attachment 255352 [details] > Patch > > Ah, ok, I didn't notice the paths detail, sorry. No problem. Your review is nice !
Comment on attachment 255352 [details] Patch Clearing flags on attachment: 255352 Committed r185866: <http://trac.webkit.org/changeset/185866>
All reviewed patches have been landed. Closing bug.