Summary: | Cleanup of FrameLoader load() in a move towards sanity | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Daniel Jalkut <jalkut> | ||||||||
Component: | WebCore Misc. | Assignee: | Mark Rowe (bdash) <mrowe> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | beidson, dev+webkit | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Mac | ||||||||||
OS: | OS X 10.5 | ||||||||||
Attachments: |
|
Description
Daniel Jalkut
2008-07-20 21:29:12 PDT
Created attachment 22398 [details]
Proposed changes to FrameLoader's load() methods, and changes to WebCore and WebKit clients of those methods.
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. Created attachment 22399 [details]
Updated patch to eliminate some stray tab characters, fix a typo.
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. Created attachment 22402 [details]
Changes for windows
Thanks, Matt! 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 on attachment 22402 [details]
Changes for windows
r=me
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! Landed in r35377. |