Bug 42834

Summary: Clients for optional features should be passed to Page constructor via structure of pointers
Product: WebKit Reporter: Steve Block <steveblock>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, commit-queue, dglazkov, dino, eric, fishd, hans, jorlow, satish, steveblock, webkit-ews, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 39589, 42865    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Steve Block 2010-07-22 10:05:52 PDT
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.
Comment 1 Steve Block 2010-07-22 15:01:27 PDT
Created attachment 62346 [details]
Patch
Comment 2 Steve Block 2010-07-22 15:14:39 PDT
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.
Comment 3 Early Warning System Bot 2010-07-22 15:18:18 PDT
Attachment 62346 [details] did not build on qt:
Build output: http://queues.webkit.org/results/3605223
Comment 4 Steve Block 2010-07-22 16:03:40 PDT
Created attachment 62353 [details]
Patch
Comment 5 WebKit Review Bot 2010-07-22 21:08:08 PDT
Attachment 62353 [details] did not build on win:
Build output: http://queues.webkit.org/results/3591378
Comment 6 Hans Wennborg 2010-07-23 02:29:27 PDT
 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&);
Comment 7 Steve Block 2010-07-26 08:18:32 PDT
Created attachment 62576 [details]
Patch
Comment 8 Steve Block 2010-07-26 08:19:38 PDT
Updated patch to reflect new use of Page::setSpeechClient() in Bug 42367

Still having trouble with the Windows build for unknown reasons.
Comment 9 Satish Sampath 2010-07-26 08:41:14 PDT
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?
Comment 10 Steve Block 2010-07-26 09:03:34 PDT
> 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.
Comment 11 WebKit Review Bot 2010-07-26 11:20:27 PDT
Attachment 62576 [details] did not build on win:
Build output: http://queues.webkit.org/results/3585581
Comment 12 Jeremy Orlow 2010-07-27 07:13:16 PDT
Comment on attachment 62576 [details]
Patch

r=me
Comment 13 Steve Block 2010-07-27 07:16:26 PDT
Comment on attachment 62576 [details]
Patch

I'm still having problems with the Windows build.
Comment 14 Steve Block 2010-07-27 09:20:35 PDT
Created attachment 62698 [details]
Patch
Comment 15 Steve Block 2010-07-27 09:21:20 PDT
Updated patch with missing change to WebKit2
Comment 16 Darin Fisher (:fishd, Google) 2010-07-27 23:39:28 PDT
(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).
Comment 17 Steve Block 2010-07-28 00:30:39 PDT
> 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.
Comment 18 Steve Block 2010-07-28 01:37:34 PDT
Created attachment 62802 [details]
Patch
Comment 19 Steve Block 2010-07-28 01:42:16 PDT
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.
Comment 20 Eric Seidel (no email) 2010-07-28 02:28:53 PDT
Attachment 62802 [details] did not build on mac:
Build output: http://queues.webkit.org/results/3628165
Comment 21 Jeremy Orlow 2010-07-28 03:17:02 PDT
Comment on attachment 62802 [details]
Patch

r=me

looks nice!
Comment 22 WebKit Review Bot 2010-07-28 03:30:49 PDT
Attachment 62802 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3597522
Comment 23 Steve Block 2010-07-28 05:35:55 PDT
Created attachment 62816 [details]
Patch
Comment 24 Steve Block 2010-07-28 09:14:32 PDT
Committed r64208: <http://trac.webkit.org/changeset/64208>