Bug 42834

Created attachment 62346 [details] Patch

This patch moves the following clients from Page constructor arguments to the new OptionalClients structure. - ContextMenuClient - DragClient - InspectorClient - GeolocationControllerClient - DeviceOrientationClient 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] Patch

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] > Patch 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] Patch

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. WebCore/page/Page.h:93 + // 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] Patch r=me

Comment on attachment 62576 [details] Patch I'm still having problems with the Windows build.

Created attachment 62698 [details] Patch

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 > BackForwardControllerClient). 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] Patch

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] Patch r=me looks nice!

Attachment 62802 [details] did not build on chromium: Build output: http://queues.webkit.org/results/3597522

Created attachment 62816 [details] Patch

Committed r64208: <http://trac.webkit.org/changeset/64208>