Re: [webkit-dev] Cleaning up Document.* (was DeviceOrientation/Motion on Document rather than Page)
My theory on re-organizing document is we do the same thing we've been doing to FrameLoader. Just start lopping off chunks. My understanding is Adam was attacking FrameLoader by just grabbing a set of seemingly related member variables, throwing them in a separate class (in the same file) and hitting compile. :) And then letting the compiler explain to you what functions you should be moving off of the big class and onto your new smaller class. Possible classes which could split of from Document: - A DOM API object (to handle all the actual api) - An event handling object (see all the DEFINE_ATTRIBUTE_EVENT_LISTENER? Maybe this is part of the DOM API object) - All the style computation junk "uses*Rules, as well as the ownership of the styleselector, etc. - Form element tracking - Management of the Rendering Tree? (RenderArena, RenderView should be owned by some renderingTree() object instead it seems) - Style and "color" management - Marker management (wow, that's a huge section of needlessly Document code!) - The JSC Wrapper cache - Dashboard support - URL management (base, cookies, etc.) - Stylesheet management (maybe part of style/color management above) That list was just from scanning Document.h There is clearly a huge amount of low-hanging fruit. When Adam was cleaning up FrameLoader, and when we more recently re-wrote the DocumentParser infrastructure, we found and fixed *tons* of bugs when the code was split down into bite-sized chunks. I suspect we'd find a bunch of dead code and bugs if we started ripping Document.cpp apart. -eric On Tue, Aug 17, 2010 at 11:20 AM, Dean Jackson <dino@apple.com> wrote:
On 17/08/2010, at 7:21 AM, Eric Seidel wrote:
My apologies for derailing your earlier discussion. I just was in Document.cpp again yesterday and my mind was blown by the madness that is that god-class. Then you posted about adding to Document (which sounds like the right corse of action here!) and I took advantage of your post for my stop-pooping-on-Document PSA.
I too have 100% confidence in Dean here. :) As Maciej says, it's just a controller on Document.
Congratulations on being the first person to ever have confidence in me :)
I too would like to know how to reorganise Document better. It is HUGE. Seeing as you are discussing similar topics on IRC with EricC, maybe now is the time to bring it up.
Dean
Carry on.
-eric
On Tue, Aug 17, 2010 at 8:50 AM, Dean Jackson <dino@apple.com> wrote:
On 17/08/2010, at 12:22 AM, Maciej Stachowiak wrote:
On Aug 16, 2010, at 10:52 PM, Eric Seidel wrote:
Where-ever it goes, please don't put it on Document directly. :) (Feel free to tie it to Document's lifetime, just don't add to Document's 300+ methods.)
The madness must stop... Document is long overdue for some weightloss...
I assume Dean is suggesting that Document would gain a new data member (DeviceMotionController?) which strikes me as a reasonable approach.
Right - either one or two (+DeviceOrientationController). I do think the controllers of both events could be a single object - but that is another discussion.
If it wasn't on Document, what do you suggest otherwise? DOMWindow? Putting it on Frame but tying it to Document seems less than perfect.
Dean
-eric
On Mon, Aug 16, 2010 at 11:43 PM, Dean Jackson <dino@apple.com> wrote:
Hi,
I've been looking into implementing the clients for DeviceOrientation/Motion Events. Currently, the controllers for these events are members of Page. I think they are better suited on Document.
Here are a few reasons:
- Page isn't tied to any actual web page or document. Would we want to share the same controller and client across multiple web pages? I don't think so. - Document is already the place that is listening for these events - It's easy to suspend and resume the client from the Document-level when the user navigates to another page, or the document enters the cache, or a platform needs to temporarily suspend events for some reason. - When the API is on Page, it is hidden in the WebView, from where it is difficult to access. - it would allow the client to live in WebCore. This avoids tweaking the WebView implementations for all platforms to pass messages back and forth https://bugs.webkit.org/show_bug.cgi?id=41616
I assume one of the advantages of having them on Page is that it allows a Mock Controller to be easily created for testing from Dump Render Tree. Am I right? Is this that important?
FWIW, Geolocation seems to take both approaches. One implementation is down in Navigator/Document/DOMWindow, but the mock controller is on Page. I've found the low-level approach much easier to implement.
Thoughts?
For this sort of thing, it seems reasonable to me that there is a layer of the implementation that is a per-document controller, and then below that a singleton object in case we don't need things to happen multiple times per document. It doesn't seem especially helpful to have something happen at the Page level, in normal operation.
The reason it seems a singleton would be useful is that you only want to register with the OS service once, and then multicast the relevant notifications to all clients that are listening.
For purposes of substituting a mock controller:
(1) If there is a singleton beneath the per-document controllers, perhaps the mock object can insert itself at that level. (2) Even if the mock controller is at the per-document level, it is likely still sufficient for most tests; it might mean a little extra work if we need to specifically test geolocation or device motion/orientation from a subframe.
Regards, Maciej
On Aug 17, 2010, at 9:35 AM, Eric Seidel wrote:
My theory on re-organizing document is we do the same thing we've been doing to FrameLoader. Just start lopping off chunks.
My understanding is Adam was attacking FrameLoader by just grabbing a set of seemingly related member variables, throwing them in a separate class (in the same file) and hitting compile. :) And then letting the compiler explain to you what functions you should be moving off of the big class and onto your new smaller class.
Possible classes which could split of from Document:
- A DOM API object (to handle all the actual api)
This one should just *be* Document. Our DOM objects are the DOM API. Some of the other pieces you mention might be reasonable chunks to break off. Regards, Maciej
- An event handling object (see all the DEFINE_ATTRIBUTE_EVENT_LISTENER? Maybe this is part of the DOM API object) - All the style computation junk "uses*Rules, as well as the ownership of the styleselector, etc. - Form element tracking - Management of the Rendering Tree? (RenderArena, RenderView should be owned by some renderingTree() object instead it seems) - Style and "color" management - Marker management (wow, that's a huge section of needlessly Document code!) - The JSC Wrapper cache - Dashboard support - URL management (base, cookies, etc.) - Stylesheet management (maybe part of style/color management above)
That list was just from scanning Document.h
There is clearly a huge amount of low-hanging fruit. When Adam was cleaning up FrameLoader, and when we more recently re-wrote the DocumentParser infrastructure, we found and fixed *tons* of bugs when the code was split down into bite-sized chunks. I suspect we'd find a bunch of dead code and bugs if we started ripping Document.cpp apart.
-eric
On Tue, Aug 17, 2010 at 11:20 AM, Dean Jackson <dino@apple.com> wrote:
On 17/08/2010, at 7:21 AM, Eric Seidel wrote:
My apologies for derailing your earlier discussion. I just was in Document.cpp again yesterday and my mind was blown by the madness that is that god-class. Then you posted about adding to Document (which sounds like the right corse of action here!) and I took advantage of your post for my stop-pooping-on-Document PSA.
I too have 100% confidence in Dean here. :) As Maciej says, it's just a controller on Document.
Congratulations on being the first person to ever have confidence in me :)
I too would like to know how to reorganise Document better. It is HUGE. Seeing as you are discussing similar topics on IRC with EricC, maybe now is the time to bring it up.
Dean
Carry on.
-eric
On Tue, Aug 17, 2010 at 8:50 AM, Dean Jackson <dino@apple.com> wrote:
On 17/08/2010, at 12:22 AM, Maciej Stachowiak wrote:
On Aug 16, 2010, at 10:52 PM, Eric Seidel wrote:
Where-ever it goes, please don't put it on Document directly. :) (Feel free to tie it to Document's lifetime, just don't add to Document's 300+ methods.)
The madness must stop... Document is long overdue for some weightloss...
I assume Dean is suggesting that Document would gain a new data member (DeviceMotionController?) which strikes me as a reasonable approach.
Right - either one or two (+DeviceOrientationController). I do think the controllers of both events could be a single object - but that is another discussion.
If it wasn't on Document, what do you suggest otherwise? DOMWindow? Putting it on Frame but tying it to Document seems less than perfect.
Dean
-eric
On Mon, Aug 16, 2010 at 11:43 PM, Dean Jackson <dino@apple.com> wrote: > Hi, > > I've been looking into implementing the clients for DeviceOrientation/Motion Events. Currently, the controllers for these events are members of Page. I think they are better suited on Document. > > Here are a few reasons: > > - Page isn't tied to any actual web page or document. Would we want to share the same controller and client across multiple web pages? I don't think so. > - Document is already the place that is listening for these events > - It's easy to suspend and resume the client from the Document-level when the user navigates to another page, or the document enters the cache, or a platform needs to temporarily suspend events for some reason. > - When the API is on Page, it is hidden in the WebView, from where it is difficult to access. > - it would allow the client to live in WebCore. This avoids tweaking the WebView implementations for all platforms to pass messages back and forth > https://bugs.webkit.org/show_bug.cgi?id=41616 > > I assume one of the advantages of having them on Page is that it allows a Mock Controller to be easily created for testing from Dump Render Tree. Am I right? Is this that important? > > FWIW, Geolocation seems to take both approaches. One implementation is down in Navigator/Document/DOMWindow, but the mock controller is on Page. I've found the low-level approach much easier to implement. > > Thoughts?
For this sort of thing, it seems reasonable to me that there is a layer of the implementation that is a per-document controller, and then below that a singleton object in case we don't need things to happen multiple times per document. It doesn't seem especially helpful to have something happen at the Page level, in normal operation.
The reason it seems a singleton would be useful is that you only want to register with the OS service once, and then multicast the relevant notifications to all clients that are listening.
For purposes of substituting a mock controller:
(1) If there is a singleton beneath the per-document controllers, perhaps the mock object can insert itself at that level. (2) Even if the mock controller is at the per-document level, it is likely still sufficient for most tests; it might mean a little extra work if we need to specifically test geolocation or device motion/orientation from a subframe.
Regards, Maciej
That's not always true, is it? Window at least used to have its API spread across many classes (including frame, and at least 2 window classes). Document's API is a beast. http://trac.webkit.org/browser/trunk/WebCore/dom/Document.idl We can call the thing which has all the Document DOM API document, but the thing which owns it would need a new name. In any case, whatever does the document DOM API, should *only* do the Document DOM API in my opinion. But there are also many lower-hanging fruit than splitting off the API into its own object (alternately said: ripping all the rest of the non-DOM-API out of the object called Document). -eric On Tue, Aug 17, 2010 at 2:01 PM, Maciej Stachowiak <mjs@apple.com> wrote:
On Aug 17, 2010, at 9:35 AM, Eric Seidel wrote:
My theory on re-organizing document is we do the same thing we've been doing to FrameLoader. Just start lopping off chunks.
My understanding is Adam was attacking FrameLoader by just grabbing a set of seemingly related member variables, throwing them in a separate class (in the same file) and hitting compile. :) And then letting the compiler explain to you what functions you should be moving off of the big class and onto your new smaller class.
Possible classes which could split of from Document:
- A DOM API object (to handle all the actual api)
This one should just *be* Document. Our DOM objects are the DOM API. Some of the other pieces you mention might be reasonable chunks to break off.
Regards, Maciej
- An event handling object (see all the DEFINE_ATTRIBUTE_EVENT_LISTENER? Maybe this is part of the DOM API object) - All the style computation junk "uses*Rules, as well as the ownership of the styleselector, etc. - Form element tracking - Management of the Rendering Tree? (RenderArena, RenderView should be owned by some renderingTree() object instead it seems) - Style and "color" management - Marker management (wow, that's a huge section of needlessly Document code!) - The JSC Wrapper cache - Dashboard support - URL management (base, cookies, etc.) - Stylesheet management (maybe part of style/color management above)
That list was just from scanning Document.h
There is clearly a huge amount of low-hanging fruit. When Adam was cleaning up FrameLoader, and when we more recently re-wrote the DocumentParser infrastructure, we found and fixed *tons* of bugs when the code was split down into bite-sized chunks. I suspect we'd find a bunch of dead code and bugs if we started ripping Document.cpp apart.
-eric
On Tue, Aug 17, 2010 at 11:20 AM, Dean Jackson <dino@apple.com> wrote:
On 17/08/2010, at 7:21 AM, Eric Seidel wrote:
My apologies for derailing your earlier discussion. I just was in Document.cpp again yesterday and my mind was blown by the madness that is that god-class. Then you posted about adding to Document (which sounds like the right corse of action here!) and I took advantage of your post for my stop-pooping-on-Document PSA.
I too have 100% confidence in Dean here. :) As Maciej says, it's just a controller on Document.
Congratulations on being the first person to ever have confidence in me :)
I too would like to know how to reorganise Document better. It is HUGE. Seeing as you are discussing similar topics on IRC with EricC, maybe now is the time to bring it up.
Dean
Carry on.
-eric
On Tue, Aug 17, 2010 at 8:50 AM, Dean Jackson <dino@apple.com> wrote:
On 17/08/2010, at 12:22 AM, Maciej Stachowiak wrote:
On Aug 16, 2010, at 10:52 PM, Eric Seidel wrote:
> Where-ever it goes, please don't put it on Document directly. :) > (Feel free to tie it to Document's lifetime, just don't add to > Document's 300+ methods.) > > The madness must stop... Document is long overdue for some weightloss...
I assume Dean is suggesting that Document would gain a new data member (DeviceMotionController?) which strikes me as a reasonable approach.
Right - either one or two (+DeviceOrientationController). I do think the controllers of both events could be a single object - but that is another discussion.
If it wasn't on Document, what do you suggest otherwise? DOMWindow? Putting it on Frame but tying it to Document seems less than perfect.
Dean
> > -eric > > On Mon, Aug 16, 2010 at 11:43 PM, Dean Jackson <dino@apple.com> wrote: >> Hi, >> >> I've been looking into implementing the clients for DeviceOrientation/Motion Events. Currently, the controllers for these events are members of Page. I think they are better suited on Document. >> >> Here are a few reasons: >> >> - Page isn't tied to any actual web page or document. Would we want to share the same controller and client across multiple web pages? I don't think so. >> - Document is already the place that is listening for these events >> - It's easy to suspend and resume the client from the Document-level when the user navigates to another page, or the document enters the cache, or a platform needs to temporarily suspend events for some reason. >> - When the API is on Page, it is hidden in the WebView, from where it is difficult to access. >> - it would allow the client to live in WebCore. This avoids tweaking the WebView implementations for all platforms to pass messages back and forth >> https://bugs.webkit.org/show_bug.cgi?id=41616 >> >> I assume one of the advantages of having them on Page is that it allows a Mock Controller to be easily created for testing from Dump Render Tree. Am I right? Is this that important? >> >> FWIW, Geolocation seems to take both approaches. One implementation is down in Navigator/Document/DOMWindow, but the mock controller is on Page. I've found the low-level approach much easier to implement. >> >> Thoughts?
For this sort of thing, it seems reasonable to me that there is a layer of the implementation that is a per-document controller, and then below that a singleton object in case we don't need things to happen multiple times per document. It doesn't seem especially helpful to have something happen at the Page level, in normal operation.
The reason it seems a singleton would be useful is that you only want to register with the OS service once, and then multicast the relevant notifications to all clients that are listening.
For purposes of substituting a mock controller:
(1) If there is a singleton beneath the per-document controllers, perhaps the mock object can insert itself at that level. (2) Even if the mock controller is at the per-document level, it is likely still sufficient for most tests; it might mean a little extra work if we need to specifically test geolocation or device motion/orientation from a subframe.
Regards, Maciej
On Aug 17, 2010, at 12:11 PM, Eric Seidel wrote:
Window at least used to have its API spread across many classes (including frame, and at least 2 window classes).
That is a historical anomaly. Some classes like Window used to be implemented in the bindings layer. That was a bad idea, mixing language bindings with the DOM itself. We now have a far cleaner architecture where the bindings layer connects to an underlying object in a mechanical way and the bindings are generated. We definitely want a single class for the entire DOM API.
In any case, whatever does the document DOM API, should *only* do the Document DOM API in my opinion.
That’s one way to do it. But I think not the best way. I suggest the Document class be the Document DOM API plus the “hub” functions that get you to the various sub-objects. The rest of the DOM all does this, including Node and all classes derived from it, including all the HTML elements. And a Document is a Node. I do support breaking this class up, but I think it’s more like when we broke up the Frame class than like our ongoing work to break up FrameLoader. -- Darin
participants (3)
-
Darin Adler
-
Eric Seidel
-
Maciej Stachowiak