Bug 36874 - Cleanup RedirectScheduler
Summary: Cleanup RedirectScheduler
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
Depends on:
Reported: 2010-03-31 00:40 PDT by Adam Barth
Modified: 2010-03-31 16:22 PDT (History)
7 users (show)

See Also:

Patch (22.79 KB, patch)
2010-03-31 00:44 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (22.92 KB, patch)
2010-03-31 01:53 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (22.92 KB, patch)
2010-03-31 02:04 PDT, Adam Barth
no flags Details | Formatted Diff | Diff
Patch for landing (22.93 KB, patch)
2010-03-31 11:08 PDT, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2010-03-31 00:40:15 PDT
Looking at RedirectScheduler last night with othermaciej, it occurred to us that the psuedo-union ScheduleRedirection is really ugly.  Othermaciej asked me to clean up the class, so here it is in a less ugly form.
Comment 1 Adam Barth 2010-03-31 00:44:38 PDT
Created attachment 52138 [details]
Comment 2 Maciej Stachowiak 2010-03-31 01:29:39 PDT
Comment on attachment 52138 [details]

* 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).
Comment 3 Adam Barth 2010-03-31 01:53:53 PDT
Created attachment 52142 [details]
Comment 4 Early Warning System Bot 2010-03-31 02:00:06 PDT
Attachment 52142 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1621116
Comment 5 WebKit Review Bot 2010-03-31 02:00:42 PDT
Attachment 52142 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1572099
Comment 6 Adam Barth 2010-03-31 02:04:32 PDT
Created attachment 52144 [details]
Comment 7 Darin Fisher (:fishd, Google) 2010-03-31 10:01:46 PDT
Comment on attachment 52144 [details]

> 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.
Comment 8 Adam Barth 2010-03-31 11:05:32 PDT
Yeah, the better solution is probably to rename the class.  :)
Comment 9 Adam Barth 2010-03-31 11:08:12 PDT
Created attachment 52188 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2010-03-31 16:22:29 PDT
Comment on attachment 52188 [details]
Patch for landing

Clearing flags on attachment: 52188

Committed r56875: <http://trac.webkit.org/changeset/56875>
Comment 11 WebKit Commit Bot 2010-03-31 16:22:35 PDT
All reviewed patches have been landed.  Closing bug.