Several of the client arguments to the Page constructor are for optional features, for which most platforms pass 0. The use of a structure of pointers would reduce the number of arguments required. The members of the structure would default to 0, so adding a new optional feature would not require updates to the call sites for platforms not using the feature.
Note that this approach is preferred to a setter for each client, as it avoids the time window where no client is provided.
Created attachment 62346 [details]
This patch moves the following clients from Page constructor arguments to the new OptionalClients structure.
It also changes the SpeechInputClient from being set via a setter method to being passed as part of the structure, to avoid there being a period of time where the client is not set.
It seems that some platforms pass null for the PluginHalterClient and BackForwardControllerClient, but these features do not seem to be controlled by an ENABLE flag, so I chose not to count them as optional.
Attachment 62346 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3605223
Created attachment 62353 [details]
Attachment 62353 [details] did not build on win:
Build output: http://queues.webkit.org/results/3591378
Bug 42635(In reply to comment #4)
> Created an attachment (id=62353) [details]
I think this seems like a good approach.
This certainly feels like it makes things simpler:
> - Page(ChromeClient*, ContextMenuClient*, EditorClient*, DragClient*, InspectorClient*, PluginHalterClient*, GeolocationControllerClient*, DeviceOrientationClient*, BackForwardControllerClient*);
> + Page(ChromeClient*, EditorClient*, PluginHalterClient*, BackForwardControllerClient*, const OptionalClients&);
Created attachment 62576 [details]
Updated patch to reflect new use of Page::setSpeechClient() in Bug 42367
Still having trouble with the Windows build for unknown reasons.
I like the idea.
+ // Clients for optional features. If the feature is enabled, a non-null client is required.
IIRC in the webkit-dev thread Darin and Eric suggested not to zero initialize the pointers, and that we should perhaps make sure all the clients are valid when passed in. That is perhaps going to be a much larger change. Are you consciously deciding not to go that route and support null client pointers as it is now?
> IIRC in the webkit-dev thread Darin and Eric suggested not to zero initialize
> the pointers,
It's true that Eric suggested this, but to do so would erase come of the benefits of this approach. Without zero initialisation, for every new feature, all ports would have to explicitly set the client, even when setting the client to null when the feature is not enabled.
> and that we should perhaps make sure all the clients are valid
> when passed in.
Sure, that seems like a good idea, but I think it's independent of this change. Currently, platforms pass many null clients for features that are not enabled. We only require clients to be non-null if the feature is enabled, as mentioned in the comment in Page.h. This patch doesn't change anything with regard to the need to check that clients for enabled features are non-null.
Attachment 62576 [details] did not build on win:
Build output: http://queues.webkit.org/results/3585581
Comment on attachment 62576 [details]
Comment on attachment 62576 [details]
I'm still having problems with the Windows build.
Created attachment 62698 [details]
Updated patch with missing change to WebKit2
(In reply to comment #2)
> This patch moves the following clients from Page constructor arguments to the
> new OptionalClients structure.
Why not just create a PageClients structure and place all clients in there? Some of the clients are not needed by Chromium, yet you have not included them in the optional set (e.g., PluginHalterClient and BackForwardControllerClient).
> Why not just create a PageClients structure and place all clients in there? Some of the clients are not
> needed by Chromium, yet you have not included them in the optional set (e.g., PluginHalterClient and
My reasoning was to use the new OptionalClients structure only for clients for optional features, ie those guarded by an ENABLE flag. In this case, the client must be non-null when the feature is enabled, otherwise it can be null. This keeps the meaning of 'optional' very clear. If a platform does not enable the feature, it can use the default null value from the structure.
In the case of PluginHalterClient and BackForwardControllerClient, these features are not guarded by ENABLE flags and I'm not sure of the precise conditions under which it's safe to pass a null client. Keeping them out of the optional set means that platforms must make an explicit decision to pass null, which seems safer.
Created attachment 62802 [details]
Updated patch in response to fishd's comments. We now pass all clients through the structure for simplicity. All clients default to null, so attempts to use any uninitialized clients will result in a crash.
Attachment 62802 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3628165
Comment on attachment 62802 [details]
Attachment 62802 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3597522
Created attachment 62816 [details]
Committed r64208: <http://trac.webkit.org/changeset/64208>