Every time I look at FrameLoader, it makes me cry. I think I have some time in my schedule to clean it up a bit. I haven't studied the code in detail, but my plan is as follows: 1) Separation of concerns. FrameLoader has its fingers in a bunch of different pies. In this phase, I'll try to break FrameLoader up into a bunch of smaller objects that are in charge of managing different pots of state. 2) Explicit state machines. FrameLoader holds a lot of state as bool members. In this phase, I'll try to convert these flags into an explicit state machine with clear state graph. 3) API surface reduction. FrameLoader has a large number of public entry points. In this phase, I'll reduce the API surface of the core state machines to it's more clear where clients can enter the machine. This is a big task, and I'm going to try to do it incrementally. If you have thoughts or would like input, let me know. Thanks, Adam
On Sep 25, 2009, at 1:46 PM, Adam Barth wrote:
1) Separation of concerns. FrameLoader has its fingers in a bunch of different pies. In this phase, I'll try to break FrameLoader up into a bunch of smaller objects that are in charge of managing different pots of state.
The whole plan sounds great. I’d like to hear more of the details of this step. For what it’s worth, FrameLoader itself was created by breaking out the loading aspects of the Frame class. It’s actually one of the smaller objects than the original mega-Frame! -- Darin
On Fri, Sep 25, 2009 at 1:52 PM, Darin Adler <darin@apple.com> wrote:
On Sep 25, 2009, at 1:46 PM, Adam Barth wrote:
1) Separation of concerns. FrameLoader has its fingers in a bunch of different pies. In this phase, I'll try to break FrameLoader up into a bunch of smaller objects that are in charge of managing different pots of state.
The whole plan sounds great. I’d like to hear more of the details of this step.
I haven't studied the code in enough detail yet to have many specifics, but here as some examples: A) Scheduled redirections. There's a bunch of state associated with these that seems separable from the concerns of actually performing the loads. B) Methods involving PolicyAction. Interacting with the FrameLoadClient seems like a separable issue from advancing the main state machine. C) Form submission. There seem to be a bunch of special cases having to do with loading form submissions. I haven't looked in detail, but this also seems like it should be a client of the core machine. As we get into the details, I'm sure we'll find some obvious wins. Adam
That is a much needed big job... look forward to seeing how the details flesh out. On Fri, Sep 25, 2009 at 2:01 PM, Adam Barth <abarth@webkit.org> wrote:
On Fri, Sep 25, 2009 at 1:52 PM, Darin Adler <darin@apple.com> wrote:
On Sep 25, 2009, at 1:46 PM, Adam Barth wrote:
1) Separation of concerns. FrameLoader has its fingers in a bunch of different pies. In this phase, I'll try to break FrameLoader up into a bunch of smaller objects that are in charge of managing different pots of state.
The whole plan sounds great. I’d like to hear more of the details of this step.
I haven't studied the code in enough detail yet to have many specifics, but here as some examples:
A) Scheduled redirections. There's a bunch of state associated with these that seems separable from the concerns of actually performing the loads.
B) Methods involving PolicyAction. Interacting with the FrameLoadClient seems like a separable issue from advancing the main state machine.
C) Form submission. There seem to be a bunch of special cases having to do with loading form submissions. I haven't looked in detail, but this also seems like it should be a client of the core machine.
As we get into the details, I'm sure we'll find some obvious wins.
Adam _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Every time we've made changes to the loading architecture, be it the original Frame class, the original Mac-only WebKit loaders, or significant changes to FrameLoader itself, we've caused numerous regressions - both subtle and extreme - that take forever to weed out. This is a primary reason that this oft-requested, much-needed undertaking hasn't gotten serious traction. Testing loading behavior was a later addition to DumpRenderTree (which originally only dumped the render tree, as difficult as it might be to imagine), and has languished behind the volume and scope of changes that have occurred in the loaders. I think before any actual behavioral changes are made, someone needs to actually put in the time and effort to give DRT a substantial overhaul in its ability to regression test the many subtle processes the loader undertakes. ~Brady On Sep 25, 2009, at 1:46 PM, Adam Barth wrote:
Every time I look at FrameLoader, it makes me cry. I think I have some time in my schedule to clean it up a bit. I haven't studied the code in detail, but my plan is as follows:
1) Separation of concerns. FrameLoader has its fingers in a bunch of different pies. In this phase, I'll try to break FrameLoader up into a bunch of smaller objects that are in charge of managing different pots of state.
2) Explicit state machines. FrameLoader holds a lot of state as bool members. In this phase, I'll try to convert these flags into an explicit state machine with clear state graph.
3) API surface reduction. FrameLoader has a large number of public entry points. In this phase, I'll reduce the API surface of the core state machines to it's more clear where clients can enter the machine.
This is a big task, and I'm going to try to do it incrementally. If you have thoughts or would like input, let me know.
Thanks, Adam _______________________________________________ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
On Fri, Sep 25, 2009 at 1:46 PM, Adam Barth <abarth@webkit.org> wrote:
Every time I look at FrameLoader, it makes me cry. I think I have some time in my schedule to clean it up a bit. I haven't studied the code in detail, but my plan is as follows:
1) Separation of concerns. FrameLoader has its fingers in a bunch of different pies. In this phase, I'll try to break FrameLoader up into a bunch of smaller objects that are in charge of managing different pots of state.
2) Explicit state machines. FrameLoader holds a lot of state as bool members. In this phase, I'll try to convert these flags into an explicit state machine with clear state graph.
3) API surface reduction. FrameLoader has a large number of public entry points. In this phase, I'll reduce the API surface of the core state machines to it's more clear where clients can enter the machine.
This is a big task, and I'm going to try to do it incrementally. If you have thoughts or would like input, let me know.
Thanks, Adam
This sounds really wonderful. I agree with Brady that more test coverage is essential. I'd also be happy to help review any patches related to this work! -Darin
Just to followup, I've started doing this slowly. The first patch as landed this evening. If you want to keep track of this project, you can CC yourself on the metabug: https://bugs.webkit.org/show_bug.cgi?id=29947 Adam On Fri, Sep 25, 2009 at 4:40 PM, Darin Fisher <darin@chromium.org> wrote:
On Fri, Sep 25, 2009 at 1:46 PM, Adam Barth <abarth@webkit.org> wrote:
Every time I look at FrameLoader, it makes me cry. I think I have some time in my schedule to clean it up a bit. I haven't studied the code in detail, but my plan is as follows:
1) Separation of concerns. FrameLoader has its fingers in a bunch of different pies. In this phase, I'll try to break FrameLoader up into a bunch of smaller objects that are in charge of managing different pots of state.
2) Explicit state machines. FrameLoader holds a lot of state as bool members. In this phase, I'll try to convert these flags into an explicit state machine with clear state graph.
3) API surface reduction. FrameLoader has a large number of public entry points. In this phase, I'll reduce the API surface of the core state machines to it's more clear where clients can enter the machine.
This is a big task, and I'm going to try to do it incrementally. If you have thoughts or would like input, let me know.
Thanks, Adam
This sounds really wonderful. I agree with Brady that more test coverage is essential. I'd also be happy to help review any patches related to this work! -Darin
Adam, something else that imho must be considered ( while refactoring the state machine ) is adding a "load type" that specifically does not touch session and global history, and avoid "abusing" some of the existent load types like below: <abuse> // FIXME: This seems like a dangerous overloading of the meaning of "FrameLoadTypeReload" ... // shouldn't a more explicit type of reload be defined, that means roughly // "load without affecting history" ? if (shouldReloadToHandleUnreachableURL(newDocumentLoader)) { ASSERT(type == FrameLoadTypeStandard); type = FrameLoadTypeReload; } </abuse> great effort so far , btw -- --Antonio Gomes
participants (6)
-
Adam Barth
-
Brady Eidson
-
Darin Adler
-
Darin Fisher
-
Michael Nordman
-
tonikitoo (Antonio Gomes)