Bug 69814 – [GTK] [WebKit2] Allow sharing page clients between WebViews

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Martin Robinson]

Currently the WebKitWebLoaderClient is structured in such a way that it can only be attached to one WebView. Since C API page client callbacks carry the page as an argument, this restriction doesn't need to exist. This bug tracks allowing sharing clients between WebViews. This more closely matches the WebKit1 delegates that the Mac port uses and the WebKit2 C API.

[Martin Robinson]

Created attachment 110469 [details]
Patch

[Martin Robinson]

Created attachment 110474 [details]
Patch also disconnecting new signal handler on fixture destruction

[Carlos Garcia Campos]

Comment on attachment 110474 [details]
Patch also disconnecting new signal handler on fixture destruction

View in context: https://bugs.webkit.org/attachment.cgi?id=110474&action=review

Patch looks good to me, maybe you can use current test instead of making this patch depend on uncomitted patches, so that it can land as soon as it's reviewed.

> Source/WebKit2/UIProcess/API/gtk/WebKitDefines.h:38
> +typedef struct _WebKitWebView WebKitWebView;
> +typedef struct _WebKitWebViewClass WebKitWebViewClass;
> +
> +typedef struct _WebKitWebLoaderClient        WebKitWebLoaderClient;
> +typedef struct _WebKitWebLoaderClientClass   WebKitWebLoaderClientClass;
> +

This change is unrelated, I prefer typedefs of every instance/class in their own header, or is there a reason to hae all of them here?

[Martin Robinson]

Comment on attachment 110474 [details]
Patch also disconnecting new signal handler on fixture destruction

View in context: https://bugs.webkit.org/attachment.cgi?id=110474&action=review

>> Source/WebKit2/UIProcess/API/gtk/WebKitDefines.h:38
>> +
> 
> This change is unrelated, I prefer typedefs of every instance/class in their own header, or is there a reason to hae all of them here?

I was unhappy with this change as well, but WebKitWebLoaderClient.h refers to WebKitWebView* and WebKitWebView.h refers to WebKitWebLoaderClient*. If you can think of another way to solve the issue, I'll give it a shot.

[Philippe Normand]

Comment on attachment 110474 [details]
Patch also disconnecting new signal handler on fixture destruction

The patch looks good to me, one suggestion maybe, have a test with a single loader client handling multiple webViews?

[Xan Lopez]

Comment on attachment 110474 [details]
Patch also disconnecting new signal handler on fixture destruction

Seems to be basically just moving code around, so looks good to me (without having much context about this stuff yet!). A couple of small comments:

- Seems the estimated progress property could have been moved to the view in a previous patch. Would have made things easier to review?

- I agree with Phil that some code testing the new possibility of multiple views per loader client makes sense.

[Martin Robinson]

(In reply to comment #6)
> (From update of attachment 110474 [details])
> Seems to be basically just moving code around, so looks good to me (without having much context about this stuff yet!). A couple of small comments:
> - Seems the estimated progress property could have been moved to the view in a previous patch. Would have made things easier to review?
> - I agree with Phil that some code testing the new possibility of multiple views per loader client makes sense.

Thanks for the review. I landed this patch with a new unit test as described.

[Martin Robinson]

Committed r97920: <http://trac.webkit.org/changeset/97920>