Summary: | Cleanup RedirectScheduler | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Barth <abarth> | ||||||||||
Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | ap, commit-queue, darin, dglazkov, mjs, webkit-ews, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Adam Barth
2010-03-31 00:40:15 PDT
Created attachment 52138 [details]
Patch
Comment on attachment 52138 [details]
Patch
* Suggested renames:
- AbstractRedirect --> ScheduledNavigation
- SimpleRedirect --> ScheduledURLNavigation
* It may or may not be helpful to also add "Scheduled" back to the concrete classes too.
* Odd that locationChangePending is true for classes other than LocationChange. Also: even if the API on the scheduler is locationChangePending(), shouldn't the boolean method on an individual AbstractRedirect (or ScheduledNavigation) object be isLocationChange or countsAsLocationChange or something? It's only telling you about what it is, not what is pending.
That's my style comments. r- to consider those. I will have to do another pass to check behavior-preservingness of the refactoring (since the lines got moved around a lot, was hard to check that with a linear scan).
Created attachment 52142 [details]
Patch
Attachment 52142 [details] did not build on qt: Build output: http://webkit-commit-queue.appspot.com/results/1621116 Attachment 52142 [details] did not build on chromium: Build output: http://webkit-commit-queue.appspot.com/results/1572099 Created attachment 52144 [details]
Patch
Comment on attachment 52144 [details] Patch > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > + Cleanup RedirectScheduler > + https://bugs.webkit.org/show_bug.cgi?id=36874 > + > + Removed the nutty ScheduledRedirection struct in favor of a hierarchy ^^^ bad indentation otherwise, this all looks good to me. given the naming of the file, it seems like it would be nice if the base interface were named ScheduledRedirect, but I realize why you chose not to do that. Yeah, the better solution is probably to rename the class. :) Created attachment 52188 [details]
Patch for landing
Comment on attachment 52188 [details] Patch for landing Clearing flags on attachment: 52188 Committed r56875: <http://trac.webkit.org/changeset/56875> All reviewed patches have been landed. Closing bug. |