Re: [webkit-dev] Clients and the Page constructor
https://bugs.webkit.org/show_bug.cgi?id=42834 has now been r+'ed. I'll submit in the morning if there are no further comments. Thanks, Steve ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Clients and the Page constructor
On Wed, Jul 21, 2010 at 8:57 PM, Steve Block stevebl...@google.com wrote: Another argument for the setter is that it makes it easier to inject a mock for testing in response to a LayoutTestController method call, by simply calling the setter again to replace the real client with a mock client. Personally, however, I think that this is somewhat abusing the idea of setting a client. If the real implementation needs to be switched for a mock at runtime in DRT, this should probably be handled by the client itself - so the same client is used throughout the lifetime of the Page. Furthermore, none of the cases that I am familiar with (Geolocation, Device Orientation, speech) require to start in DRT with a real client implementation and then, later, switch to a mock in response to some DRT method call. They all can quite happily pass the mock client pointer in the Page ctor. So the mock injection argument you mention above is for a problem that does not exist in reality. Thanks, Andrei ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Clients and the Page constructor
I've filed https://bugs.webkit.org/show_bug.cgi?id=42834 to track the addition of a structure to pass pointers to the clients for optional features. Feedback welcome. Steve -- Google UK Limited Registered Office: Belgrave House, 76 Buckingham Palace Road, London SW1W 9TQ Registered in England Number: 3977902 ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
[webkit-dev] Clients and the Page constructor
Currently, nine clients are passed to the Page constructor and the number is growing. Recently, clients have been added for Geolocation, DeviceOrientation and BackForwardController. This approach doesn't seem scalable. Instead, I'd like to suggest that clients, at least those for optional features, are not passed to the Page constructor. Instead, the client should default to null and can be set with an explicit method call, eg page-setFooClient(client) or page-getFooController()-setClient(client). This is the approach that was taken for SpeechInput in http://trac.webkit.org/changeset/63230. If there are no objections, I'll send patches to make this change for Geolocation and DeviceOrientation. For reference, see the discussion in https://bugs.webkit.org/show_bug.cgi?id=39589. Thanks, Steve ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Clients and the Page constructor
On Jul 21, 2010, at 8:47 AM, Steve Block wrote: Currently, nine clients are passed to the Page constructor and the number is growing. Recently, clients have been added for Geolocation, DeviceOrientation and BackForwardController. This approach doesn't seem scalable. What exactly does not scalable mean? Passing arguments to the constructor rather than setting them up later is often good because there is no time window where the object is not set up. Generally speaking we don’t want to have to write code to handle a client of 0 unless it’s absolutely necessary. Instead, the client should default to null and can be set with an explicit method call Another option, if the objection is a function with a lot of arguments, is to pass in a structure with all the client pointers to the constructor. What concrete problem are we trying to solve here? The words “doesn’t seem scalable” alone don’t make that clear to me. -- Darin ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Clients and the Page constructor
What exactly does not scalable mean? Passing arguments to the constructor rather than setting them up later is often good because there is no time window where the object is not set up. Generally speaking we don’t want to have to write code to handle a client of 0 unless it’s absolutely necessary. Agreed. My motivation was to avoid the long, growing, list of arguments and to avoid the need to update the call sites for all platforms with an extra '0' each time a new optional feature is added. Another option, if the objection is a function with a lot of arguments, is to pass in a structure with all the client pointers to the constructor. That sounds reasonable. A structure of the client pointers for all optional features, with each entry defaulting to 0, would solve this problem. Another argument for the setter is that it makes it easier to inject a mock for testing in response to a LayoutTestController method call, by simply calling the setter again to replace the real client with a mock client. Personally, however, I think that this is somewhat abusing the idea of setting a client. If the real implementation needs to be switched for a mock at runtime in DRT, this should probably be handled by the client itself - so the same client is used throughout the lifetime of the Page. Steve ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
Re: [webkit-dev] Clients and the Page constructor
That still makes it required to add null checks for some (or all) of the clients. Something we've avoided in the past. This wouldn't add a need for null checks. The structure would only be for clients for optional features. If the feature is enabled, the client must be non-null. This is no different from the case with the individual arguments used currently. ___ webkit-dev mailing list webkit-dev@lists.webkit.org http://lists.webkit.org/mailman/listinfo.cgi/webkit-dev
