1) Open Epiphany and access e.g. about:plugins; 2) Kill the respective running WebKitWebProcess; 3) Try to access about:plugins again Expected: The about:plugins works as always. Outcome: about:plugins is not found. I am working on a solution and will have a patch soon.
Created attachment 177001 [details] Patch
Comment on attachment 177001 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177001&action=review Thanks for the patch, it looks good, I've just a few comments. > Source/WebKit2/ChangeLog:3 > + Fixes the use of URI schemes when using libsoup This is not accurate, the bug title probably express better the problem so I would just skip this line. > Source/WebKit2/ChangeLog:9 > + When a URI scheme is registered and the WebProcess is killed, > + those schemes would not work anymore after the process is relaunched. > + > + This was observed in Epiphany and possibly affects any port that > + uses libsoup. All this text should go after the Reviewed by line, replacing the Additional information line. > Source/WebKit2/ChangeLog:34 > + Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!). > + > + * Shared/WebProcessCreationParameters.cpp: > + (WebKit::WebProcessCreationParameters::encode): > + (WebKit::WebProcessCreationParameters::decode): > + * Shared/WebProcessCreationParameters.h: > + (WebProcessCreationParameters): > + * UIProcess/WebContext.cpp: > + (WebKit::WebContext::createNewWebProcess): > + * UIProcess/soup/WebSoupRequestManagerProxy.cpp: > + (WebKit::WebSoupRequestManagerProxy::registerURIScheme): > + (WebKit::WebSoupRequestManagerProxy::getRegisteredURISchemes): > + (WebKit): > + * UIProcess/soup/WebSoupRequestManagerProxy.h: > + (WebSoupRequestManagerProxy): > + * WebProcess/WebProcess.cpp: > + (WebKit::WebProcess::initializeWebProcess): > + * WebProcess/soup/WebSoupRequestManager.h: > + (WebSoupRequestManager): As the comment says, please add per-function descriptions briefly explaining the changes. > Source/WebKit2/Shared/WebProcessCreationParameters.h:79 > + Vector<String> urlSchemesRegisteredForSoup; I would remove the ForSoup suffix. > Source/WebKit2/UIProcess/WebContext.cpp:405 > +#if USE(SOUP) > + const Vector<String> ®isteredUriSchemes = m_soupRequestManagerProxy->getRegisteredURISchemes(); > + for (Vector<String>::const_iterator it = registeredUriSchemes.begin(); it != registeredUriSchemes.end(); it++) > + parameters.urlSchemesRegisteredForSoup.append(*it); > +#endif Instead of using an #ifdef move this code to WebContext::platformInitializeWebProcess() in Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp. We should do the same for EFL in Source/WebKit2/UIProcess/efl/WebContextEfl.cpp Can't you do a simple assignment instead of iterating the vector? parameters.urlSchemesRegistered = m_soupRequestManagerProxy->getRegisteredURISchemes(); > Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.cpp:64 > + m_registeredURISchemes.append(scheme); The API layer should make sure the same scheme is not registered twice, now that we have a vector with the list of schemes, we could add an ASSERT to check the scheme is not already in the Vector. > Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.cpp:97 > +Vector<String>& WebSoupRequestManagerProxy::getRegisteredURISchemes() > +{ > + return m_registeredURISchemes; > +} I think it would be better to return const Vector<String>& and the function should be const too. The name of the function should be registeredURISchemes(), see http://www.webkit.org/coding/coding-style.html#names-setter-getter. Also for simple getters like this, we typically implement them in the header. > Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.h:68 > + Remove this extra line > Source/WebKit2/WebProcess/WebProcess.cpp:288 > +#if USE(SOUP) > + for (Vector<String>::const_iterator it = parameters.urlSchemesRegisteredForSoup.begin(); it != parameters.urlSchemesRegisteredForSoup.end(); it++) > + m_soupRequestManager.registerURIScheme(*it); > +#endif Instead of using #ifdef, move the code to WebProcess::platformInitializeWebProcess() in Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp. I think the vector iteration is easier to read as for (size_t i = 0; i < parameters.urlSchemesRegistered.size(); ++i) m_soupRequestManager.registerURIScheme(parameters.urlSchemesRegistered[i]);
Hi, I will upload a new patch addressing your comments. About the vector iteration, I'll leave that as I did because it's common in C++ and *arguably* results in faster code. Thanks for the review.
Forgot to comment on some of your suggestions: >> Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.cpp:64 >> + m_registeredURISchemes.append(scheme); > > The API layer should make sure the same scheme is not registered twice, now that >we have a vector with the list of schemes, we could add an ASSERT to check the >scheme is not already in the Vector. This means that we have to iterate (one way or another) through the vector which, although it might be insignificant, is extra processing. > Instead of using an #ifdef move this code to WebContext::platformInitializeWebProcess() in Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp. I didn't put it in there because I need the soupRequestManagerProxy that the WebContext.cpp already has. Also, if this works for both the Gtk and Efl implementations, isn't it better than touching two files? > Instead of using #ifdef, move the code to WebProcess::platformInitializeWebProcess() in Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp. I didn't put it in there because I need the soupRequestManager that the WebProcess.cpp already has. Let me know if I'm missing some smart way to access these members from the files you mentioned.
(In reply to comment #3) > Hi, > > I will upload a new patch addressing your comments. > About the vector iteration, I'll leave that as I did because it's common in C++ and *arguably* results in faster code. Why faster code? The code is easier to read as a simple assignment, so if possible I think it would be better. > Thanks for the review. np :-)
(In reply to comment #4) > Forgot to comment on some of your suggestions: > > >> Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.cpp:64 > >> + m_registeredURISchemes.append(scheme); > > > > The API layer should make sure the same scheme is not registered twice, now that >we have a vector with the list of schemes, we could add an ASSERT to check the >scheme is not already in the Vector. > > This means that we have to iterate (one way or another) through the vector which, although it might be insignificant, is extra processing. ASSERT is no-op for a release build, so it's not a problem. > > Instead of using an #ifdef move this code to WebContext::platformInitializeWebProcess() in Source/WebKit2/UIProcess/gtk/WebContextGtk.cpp. > > I didn't put it in there because I need the soupRequestManagerProxy that the WebContext.cpp already has. > Also, if this works for both the Gtk and Efl implementations, isn't it better than touching two files? WebContextGtk.cpp and WebContextEfl.cpp are the same class than WebContext, the files simply add implementation for methods specific to the platform, so you can use soupRequestManagerProxy from them. WebContext::platformInitializeWebProcess() was added exactly for that, so I prefer code in platform specific files and #ifdefs in cross platform code, when possible. > > Instead of using #ifdef, move the code to WebProcess::platformInitializeWebProcess() in Source/WebKit2/WebProcess/soup/WebProcessSoup.cpp. > > I didn't put it in there because I need the soupRequestManager that the WebProcess.cpp already has. Same here, you can use soupRequestManager from WebProcessSoup.cpp, it's the same class. > Let me know if I'm missing some smart way to access these members from the files you mentioned. Nothing smart, simply use them :-)
Created attachment 177214 [details] Patch
Comment on attachment 177214 [details] Patch Attachment 177214 [details] did not pass efl-ews (efl): Output: http://queues.webkit.org/results/15120285
Created attachment 177219 [details] Patch
The last patch (2012-12-03 03:17 PST) fixes the issues found the in EFL's bot output.
Comment on attachment 177214 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177214&action=review > Source/WebKit2/ChangeLog:33 > + * WebProcess/soup/WebSoupRequestManager.h: Make registerURIScheme public nit: End all sentences with a period. > Source/WebKit2/UIProcess/efl/WebContextEfl.cpp:43 > - notImplemented(); > + parameters.urlSchemesRegistered = m_soupRequestManagerProxy->registeredURISchemes(); I guess you need to include WebSoupRequestManagerProxy.h in this file too. > Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.cpp:65 > + ASSERT(!m_registeredURISchemes.find(scheme)); find() returns notFound when item is not found, so you should use either m_registeredURISchemes.find(scheme) != notFound, or use Vector::contains() that returns a bool. > Source/WebKit2/UIProcess/soup/WebSoupRequestManagerProxy.h:57 > + const Vector<String>& registeredURISchemes() const > + { > + return m_registeredURISchemes; > + } This could be a single line.
Created attachment 177223 [details] Patch
Comment on attachment 177223 [details] Patch Thanks!
Comment on attachment 177223 [details] Patch Clearing flags on attachment: 177223 Committed r136389: <http://trac.webkit.org/changeset/136389>
All reviewed patches have been landed. Closing bug.