Bug 36874 – Cleanup RedirectScheduler

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Adam Barth]

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.

[Adam Barth]

Created attachment 52138 [details]
Patch

[Maciej Stachowiak]

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).

[Adam Barth]

Created attachment 52142 [details]
Patch

[Early Warning System Bot]

Attachment 52142 [details] did not build on qt:
Build output: http://webkit-commit-queue.appspot.com/results/1621116

[WebKit Review Bot]

Attachment 52142 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/1572099

[Adam Barth]

Created attachment 52144 [details]
Patch

[Darin Fisher (:fishd, Google)]

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.

[Adam Barth]

Yeah, the better solution is probably to rename the class.  :)

[Adam Barth]

Created attachment 52188 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 52188 [details]
Patch for landing

Clearing flags on attachment: 52188

Committed r56875: <http://trac.webkit.org/changeset/56875>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.