Bug 20121 - Cleanup of FrameLoader load() in a move towards sanity
Summary: Cleanup of FrameLoader load() in a move towards sanity
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Mark Rowe (bdash)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-07-20 21:29 PDT by Daniel Jalkut
Modified: 2008-07-26 20:50 PDT (History)
2 users (show)

See Also:


Attachments
Proposed changes to FrameLoader's load() methods, and changes to WebCore and WebKit clients of those methods. (36.40 KB, patch)
2008-07-20 21:30 PDT, Daniel Jalkut
no flags Details | Formatted Diff | Diff
Updated patch to eliminate some stray tab characters, fix a typo. (36.56 KB, patch)
2008-07-20 21:55 PDT, Daniel Jalkut
ggaren: review+
Details | Formatted Diff | Diff
Changes for windows (1.39 KB, text/plain)
2008-07-21 07:35 PDT, Matt Lilek
ggaren: review+
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Jalkut 2008-07-20 21:29:12 PDT
I came upon the problem of trying to understand FrameLoader's variety of load() methods, and made an exercise of annotating/debugging my way to roughly understanding which methods are used, and under what circumstances.

I renamed many of methods in an effort to increase mental graspability, and hopefully as a setup for further cleanup iterations. I don't think that the vast number of overloaded "load()" methods was serving the intended value of such overloading.  Hopefully the new naming will give a better context for searching on how the methods are used, and for possibly folding the functionality further into more logical subroutines.

This patch also includes a couple smaller cleanups which are detailed in the changelog notes.
Comment 1 Daniel Jalkut 2008-07-20 21:30:46 PDT
Created attachment 22398 [details]
Proposed changes to FrameLoader's load() methods, and changes to WebCore and WebKit clients of those methods.
Comment 2 Daniel Jalkut 2008-07-20 21:32:24 PDT
I realized my comments above make it sounds like the changes are mainly in renaming, but there is also some degree of changing the signatures of the affected functions as well. In particular, where I spotted some opportunities to move towards a more clearly parameterized functionality.
Comment 3 Daniel Jalkut 2008-07-20 21:55:14 PDT
Created attachment 22399 [details]
Updated patch to eliminate some stray tab characters, fix a typo.
Comment 4 Matt Lilek 2008-07-21 07:34:44 PDT
I'm not a reviewer, but you use "nil" once in FrameLoader.h and again in the .cpp.  I'll also attach a patch that keeps windows building.
Comment 5 Matt Lilek 2008-07-21 07:35:47 PDT
Created attachment 22402 [details]
Changes for windows
Comment 6 Daniel Jalkut 2008-07-21 07:41:27 PDT
Thanks, Matt!
Comment 7 Geoffrey Garen 2008-07-21 14:12:30 PDT
Comment on attachment 22399 [details]
Updated patch to eliminate some stray tab characters, fix a typo.

Just to give you some background, all these "load" methods came about when we merged the loader code, which had been sprinkled throughout WebCore and WebKit, into a single WebCore class. The merge illuminated a spaghetti-like structure (perhaps an image of the holy flying spaghetti monster itself?!) of similarly named and purposed functions. At the time, we named all such functions "load" as a way of saying, "We should probably refactor this code, removing and/or merging redundant noodley appendages, when we get a chance."

Anyway, I think your renames improve clarity, so I'll say "r+", but an even better solution would refactor the loader and eliminate some of these functions altogether.
Comment 8 Geoffrey Garen 2008-07-21 14:12:44 PDT
Comment on attachment 22402 [details]
Changes for windows

r=me
Comment 9 Daniel Jalkut 2008-07-21 14:29:22 PDT
Hi Geoff, appreciate the review and thanks for the clarification on the load methods. I can see how it's funny to have many of them change names again. But I do hope that it will help facilitate further refactoring. Maybe even by me!

Comment 10 Mark Rowe (bdash) 2008-07-26 20:50:26 PDT
Landed in r35377.