August 2010 - webkit-dev - lists.webkit.org

Re: [webkit-dev] Cleaning up Document.* (was DeviceOrientation/Motion on Document rather than Page)
by Eric Seidel 17 Aug '10

17 Aug '10

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(a)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(a)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(a)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 >>>> >>> >>> > >

3 3

Getting rid of WebCore.xcodeproj developmentRegion diff
by Dimitri Glazkov 17 Aug '10

17 Aug '10

Hi all, After a mini-b*tchfest on #webkit, Eric suggested I take it to the mailing lists :) It seems that we have this annoying little diff thrashing around in WebCore/WebCore.xcodeproj/project.pbxproj: http://trac.webkit.org/changeset/65520/trunk/WebCore/WebCore.xcodeproj/proj… http://trac.webkit.org/changeset/65468/trunk/WebCore/WebCore.xcodeproj/proj… http://trac.webkit.org/changeset/65349/trunk/WebCore/WebCore.xcodeproj/proj… http://trac.webkit.org/changeset/65344/trunk/WebCore/WebCore.xcodeproj/proj… ... Do you guys know of the cause of this and how we can make it go away? :DG<

7 8

ANGLE
by Zoltan Herczeg 17 Aug '10

17 Aug '10

Hi, ANGLE looks like a graphics helper library. Why it is placed in the root WebKit directory? Perhaps WebCore/platform/graphics or some kind of /3rparty directory would be better, wouldn't it? Regards, Zoltan

6 9

A proposal for "Platform Mechanisms"
by Anders Carlsson 17 Aug '10

17 Aug '10

Hi everyone, We've now reached the point in WebKit2 development where we need to be able to override some global calls in WebCore so that we can funnel them through to another process, in a similar way to what Chromium does. We also need to be able to override the calls at run-time, so that we can use the same WebCore framework with both current WebKit and WebKit2. Here's a proposal for something I call "Platform mechanisms" that I hope can be used by other ports as well as replacing the Chromium bridge long term: The design pattern we use in WebCore for when a port wants to override functionality is the "abstract client class" pattern. We have a FrameLoaderClient per frame, a ChromeClient per Page etc. Some functionality is global, and doesn't really belong to a specific object, for example: * Clipboard handling * File access * Plug-ins. I propose that we create an abstract class, "PlatformMechanism" which acts as the starting point for accessing such functionality, something like: class PlatformMechanism { virtual ClipboardMechanism* clipboardMechanism() = 0; virtual FileAccessMechanism* fileAccessMechanism() = 0; virtual PluginMechanism* pluginMechanism() = 0; }; class PluginMechanism { virtual void refreshPlugins() = 0; virtual void getPluginInfo(Vector<PluginInfo>&) = 0; }; The various ports would subclass PlatformMechanism, implement the various mechanism classes and then call into WebCore to set the PlatformMechanism. This approach gives a natural separation of the functionality. (There's of course nothing stopping you from having a single class inherit from all of the mechanism classes). We could also consider adding some functions to PlatformMechanism directly, for example if a mechanism class would end up with just a single function. The advantage of having a single "PlatformMechanism" aggregator class is that we don't need lots of setFooMechanism calls that ports would need, and if someone adds a new mechanism class, ports will fail to build instead of mysteriously crash when it turns out someone has forgotten to add a call to set the mechanism. We would also provide WebCore implementations of the various mechanisms, so that ports that don't want to override anything would just return the WebCore mechanisms. We could even have a WebCorePlatformMechanism class that you could set as the default class. This would enable ports to pick where WebCore should be used. I would very much appreciate any comments on this, and if I don't hear any major objections I will start landing parts of this, conditionally compiled by a WTF_USE_PLATFORM_MECHANISM define that's turned on for Mac WebKit. Thanks, Anders

12 20

null clients of Page
by Dean Jackson 17 Aug '10

17 Aug '10

It was suggested I bring this to webkit-dev. There was some recent work to move the many parameters passed to the Page constructor into a single PageClients structure. The current assumption is that if a feature is enabled (at runtime or compile time) then Page will always get a non-null client for that feature. This allows the Page constructor to have something like this: , m_deviceMotionController(RuntimeEnabledFeatures::deviceMotionEnabled() ? n ew DeviceMotionController(pageClients.deviceMotionClient) : 0) and DeviceMotionController's constructor has an ASSERT(m_client). The problem is that not every Page needs every client. SVGImage, for example, creates a Page without all clients. Some platform ports branch around this code. The current solution is to create EmptyClient implementations. https://bugs.webkit.org/show_bug.cgi?id=43848 https://bugs.webkit.org/show_bug.cgi?id=43533 Related discussion: https://lists.webkit.org/pipermail/webkit-dev/2010-July/013597.html Wouldn't it be easier to allow null clients than to have these EmptyClient implementations? Dean

3 3

Report errors from Qt/Win EWS bots
by Daniel Cheng 17 Aug '10

17 Aug '10

Is it possible to make the Qt/Windows EWS bots report what the build errors actually were? Qt doesn't seem to provide any stdio output. MSVC does, but it logs the output into BuildLog.htm files--would it be possible to scrape the text information out of these files and make them available? I ask because one of my patches didn't build on the Windows EWS bot, and so far, I still haven't managed to get the Windows port of WebKit to successfully compile. Daniel

2 1

Why does isLocationChange() return false for redirection?
by Eric Zhou 17 Aug '10

17 Aug '10

Hi all, following is the related code. My question is why it returns false when ScheduledRedirection.type is redirection. >From the name of the function, I think if the url changes, it should return true. Thus, for most of redirection, it should return true. //FrameLoader.cpp bool FrameLoader::isLocationChange(const ScheduledRedirection& redirection) { switch (redirection.type) { case ScheduledRedirection::redirection: return false; case ScheduledRedirection::historyNavigation: case ScheduledRedirection::locationChange: case ScheduledRedirection::formSubmission: return true; } ASSERT_NOT_REACHED(); return false; }

2 1

Tests that separate JS and HTML
by Alexey Proskuryakov 17 Aug '10

17 Aug '10

On multiple occasions, I've been noticing that separating JavaScript for tests into its own file has been causing me pain. There are several disadvantages that I know of: 1. One doesn't see test content when a tool (such as run-webkit-tests) sends them to a failing test. If I want to see it in browser, I need to copy/paste script src, or manually edit the address in address bar. 2. The proliferation of script-tests prompts some contributors to unnecessarily complicate their tests to fit the format. Sometimes, the DOM that could just be in the HTML content is dynamically created. Other times, complicated schemes for async tests are used, instead of directly calling notifyDone(). 3. These tests cannot be attached to Bugzilla bugs in a working form (e.g. if one wants to file a bug against another browser). 4. The test does not live next to its expected results, so one has to keep two folders open when working on the test. On the other hand, I can't think of any clear advantages. Some potential advantages could be: 1. Ease of boilerplate modification. I don't see why we would want to modify boilerplate in existing tests - it's more likely that such changes would break them than improve them. 2. Ease of test writing. Maybe people who use script tests often would object, but for me, a step that I must do any time I start working on a test - at a very specific moment - is a problem. Usually, I start with a simple test that I keep on desktop (or in Bugzilla), and only move it to LayoutTests later. 3. Higher quality of boilerplate. The boilerplate is quite straightforward, all the logic is in external files. 4. Tests become more concise. This is true, but I don't think it has practical benefits - it's quite easy to focus on test code even if everything is in one file. Of course, there is the special case of fast/js tests, which we (I think) still hope to run without a WebView one day. For that, keeping JS separate is obviously desirable. We have lots of imported tests in WebKit regression test suite, as well as tests employing various custom runners developed by WebKit contributors. So to a degree, the manner in which a test is written will always remain a matter of personal preference, we can't and shouldn't standardize this. However, as I was personally seeing more script-tests not just being added, but living their active lives, I started feeling frustration towards them. When reviewing patches, I recommend against using tests that have their main logic in files other than the main HTML. - WBR, Alexey Proskuryakov

5 4

Re: [webkit-dev] cr-win bot broken -- GYP Ninjas, please help.
by James Robinson 14 Aug '10

14 Aug '10

Indeed! Dimitri or I will fix the python by Monday. - James On Aug 14, 2010 11:31 AM, "Adam Barth" <abarth(a)webkit.org> wrote: The Chromium bots were supposed to have their Python upgraded ages ago. If we ever want to run the LayoutTests on them, they'll need their Python upgraded anyway. I think the correct solution is to fix the bots. Adam On Sat, Aug 14, 2010 at 12:00 AM, Eric Seidel <eric(a)webkit.org> wrote: > Fixed that one, but error...

1 0

cr-win bot broken -- GYP Ninjas, please help.
by Eric Seidel 14 Aug '10

14 Aug '10

http://build.webkit.org/builders/Chromium%20Win%20Release/builds/10478/step… It seems that cr-win for some reason isn't generating the HTMLEntityTable.cpp file like all the others are. Is it missing python? (Or python from its path?) Is there a gyp dependency problem? Any thoughts would be most appreciated. -eric

2 4