FrameLoaderClient::dispatchDidFailLoad(const ResourceError& error) has a number of naked pointers that should be refactored to use smart pointers.
Created attachment 206155 [details] Patch
This fix was motivated after looking for other areas in the codebase that may have memory leaks. There are no leaks in this function, however with this refactoring any changes in the future are less likely to introduce unexpected errors. I have run the gtk tests and can confirm that they pass after this change.
Comment on attachment 206155 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206155&action=review Good patch. I've just added a couple of minor comments as an informal review. > Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1075 > + if (isHandled || !shouldFallBack(error)) { You don't need that extra { now you got a single line if > Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1084 > + GRefPtr<GFile> errorFile(adoptGRef(g_file_new_for_uri(errorURI.get()))); We normally use the = assignment for this kind of statments: GRefPtr<GFile> errorFile = adoptGRef(g_file_new_for_uri(errorURI.get()));
Created attachment 206157 [details] Patch
Comment on attachment 206157 [details] Patch Looks fine. r=me.
Comment on attachment 206157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206157&action=review > Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1083 > + GOwnPtr<gchar> errorURI(g_filename_to_uri(errorPath.get(), 0, 0)); > + GRefPtr<GFile> errorFile = adoptGRef(g_file_new_for_uri(errorURI.get())); I know the code was already that way, but I wondering why we need to convert the path to uri instead of using g_file_new_for_path() directly, if it's possible we could take advantage that you are changing this code to improve this too. > Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1091 > + gboolean loaded = g_file_load_contents(errorFile.get(), 0, &fileContent.outPtr(), 0, 0, 0); > if (!loaded) > content = makeString("<html><body>", webError->message, "</body></html>"); You can probably do if (!g_file_load_contents(errorFile.get(), 0, &fileContent.outPtr(), 0, 0, 0)) and you don't need the local variable that is only used for this.
Oops, I didn't mean to change the flag, please feel free to r+ it again, sorry.
(In reply to comment #7) > Oops, I didn't mean to change the flag, please feel free to r+ it again, sorry. It's fine. Since you have comments, I'll let you handle this one :)
Comment on attachment 206157 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=206157&action=review >> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1083 >> + GRefPtr<GFile> errorFile = adoptGRef(g_file_new_for_uri(errorURI.get())); > > I know the code was already that way, but I wondering why we need to convert the path to uri instead of using g_file_new_for_path() directly, if it's possible we could take advantage that you are changing this code to improve this too. Good point, I wasn't really focusing on simplifying the logic but it makes sense to do so now. >> Source/WebKit/gtk/WebCoreSupport/FrameLoaderClientGtk.cpp:1091 >> content = makeString("<html><body>", webError->message, "</body></html>"); > > You can probably do if (!g_file_load_contents(errorFile.get(), 0, &fileContent.outPtr(), 0, 0, 0)) and you don't need the local variable that is only used for this. ok
Created attachment 206223 [details] Revised patch
Comment on attachment 206223 [details] Revised patch Thanks!
Comment on attachment 206223 [details] Revised patch Would you mind setting cg+ ?
Comment on attachment 206223 [details] Revised patch Clearing flags on attachment: 206223 Committed r152446: <http://trac.webkit.org/changeset/152446>
All reviewed patches have been landed. Closing bug.