Bug 118417

Summary: [GTK] FrameLoaderClient: Refactor naked pointers to use smart pointers
Product: WebKit Reporter: Brian Holt <brian.holt>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cdumez, cgarcia, commit-queue, gustavo, menard, mrobinson, pnormand
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Revised patch none

Brian Holt
Reported 2013-07-05 08:33:26 PDT
FrameLoaderClient::dispatchDidFailLoad(const ResourceError& error) has a number of naked pointers that should be refactored to use smart pointers.
Attachments
Patch (3.80 KB, patch)
2013-07-05 08:38 PDT, Brian Holt 		no flags
DetailsFormatted DiffDiff
Patch (3.80 KB, patch)
2013-07-05 08:57 PDT, Brian Holt 		no flags
DetailsFormatted DiffDiff
Revised patch (5.03 KB, patch)
2013-07-08 01:36 PDT, Brian Holt 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Brian Holt
Comment 1 2013-07-05 08:38:42 PDT
Created attachment 206155 [details] Patch
Brian Holt
Comment 2 2013-07-05 08:45:29 PDT
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.
Mario Sanchez Prada
Comment 3 2013-07-05 08:49:01 PDT
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()));
Brian Holt
Comment 4 2013-07-05 08:57:03 PDT
Created attachment 206157 [details] Patch
Chris Dumez
Comment 5 2013-07-05 09:07:17 PDT
Comment on attachment 206157 [details] Patch Looks fine. r=me.
Carlos Garcia Campos
Comment 6 2013-07-05 09:10:07 PDT
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.
Carlos Garcia Campos
Comment 7 2013-07-05 09:10:56 PDT
Oops, I didn't mean to change the flag, please feel free to r+ it again, sorry.
Chris Dumez
Comment 8 2013-07-05 09:15:38 PDT
(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 :)
Brian Holt
Comment 9 2013-07-08 01:25:43 PDT
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
Brian Holt
Comment 10 2013-07-08 01:36:19 PDT
Created attachment 206223 [details] Revised patch
Carlos Garcia Campos
Comment 11 2013-07-08 02:08:53 PDT
Comment on attachment 206223 [details] Revised patch Thanks!
Brian Holt
Comment 12 2013-07-08 02:14:59 PDT
Comment on attachment 206223 [details] Revised patch Would you mind setting cg+ ?
WebKit Commit Bot
Comment 13 2013-07-08 03:13:58 PDT
Comment on attachment 206223 [details] Revised patch Clearing flags on attachment: 206223 Committed r152446: <http://trac.webkit.org/changeset/152446>
WebKit Commit Bot
Comment 14 2013-07-08 03:14:03 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.