Bug 57835 – [GTK] Initial loader client implementation in WebKitWebView

• Component: WebKit2
• Status: RESOLVED DUPLICATE

[Carlos Garcia Campos]

I would allow the minibrowser to update the window title and location entry.

[Carlos Garcia Campos]

Created attachment 88211 [details]
Patch

[Carlos Garcia Campos]

The patch applies on top of patch attached to bug #57820

[Martin Robinson]

Comment on attachment 88211 [details]
Patch

We should allow applications to use either the WebKit1 or WebKit2 style API. Installing a Client on the widget seems to add a lot of extra overhead. Perhaps we can make them separate widgets.

[Carlos Garcia Campos]

Created attachment 88441 [details]
Updated patch

Updated patch on top of patch attached to bug #57820. It also updates GtkLauncher instead of MiniBrowser

[Alejandro G. Castro]

Comment on attachment 88441 [details]
Updated patch

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:120
> +
> +    g_object_class_install_property(gobjectClass,
> +                                    PROP_TITLE,
> +                                    g_param_spec_string("title",
> +                                                        "Title",
> +                                                        "Returns the @web_view's document title",
> +                                                        0,
> +                                                        G_PARAM_READABLE));
> +    g_object_class_install_property(gobjectClass,
> +                                    PROP_URI,
> +                                    g_param_spec_string("uri",
> +                                                        "URI",
> +                                                        "Returns the current URI of the contents displayed by the @web_view",
> +                                                        0,
> +                                                        G_PARAM_READABLE));
> +    g_object_class_install_property(gobjectClass,
> +                                    PROP_PROGRESS,
> +                                    g_param_spec_double("progress",
> +                                                        "Progress",
> +                                                        "Determines the current progress of the load",
> +                                                        0.0, 1.0, 1.0,
> +                                                        G_PARAM_READABLE));

Maybe stating to add the doc of the API it is a good idea.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:139
> +    // TODO: update load-status property

In this cases we are using NotImplemented without the TODO comment, I would stick to that and add the comment just in case it adds any other information.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:287
> +    WKPageLoaderClient loadClient = {
> +        0,       /* version */
> +        webView, /* clientInfo */
> +        didStartProvisionalLoadForFrame,
> +        didReceiveServerRedirectForProvisionalLoadForFrame,
> +        didFailProvisionalLoadWithErrorForFrame,
> +        didCommitLoadForFrame,
> +        didFinishDocumentLoadForFrame,
> +        didFinishLoadForFrame,
> +        didFailLoadWithErrorForFrame,
> +        0, /* didSameDocumentNavigationForFrame */
> +        didReceiveTitleForFrame,
> +        didFirstLayoutForFrame,
> +        didFirstVisuallyNonEmptyLayoutForFrame,
> +        didRemoveFrameFromHierarchy,
> +        0, /* didDisplayInsecureContentForFrame */
> +        0, /* didRunInsecureContentForFrame */
> +        0, /* canAuthenticateAgainstProtectionSpaceInFrame */
> +        0, /* didReceiveAuthenticationChallengeInFrame */
> +        didStartProgress,
> +        didChangeProgress,
> +        didFinishProgress,
> +        didBecomeUnresponsive,
> +        didBecomeResponsive,
> +        0,  /* processDidCrash */
> +        0,  /* didChangeBackForwardList */
> +        0   /* shouldGoToBackForwardListItem */
> +    };

Shall we add all these functions to a different file? Maybe PageLoaderClient.

[Carlos Garcia Campos]

(In reply to comment #5)
> (From update of attachment 88441 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=88441&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:120
> > +
> > +    g_object_class_install_property(gobjectClass,
> > +                                    PROP_TITLE,
> > +                                    g_param_spec_string("title",
> > +                                                        "Title",
> > +                                                        "Returns the @web_view's document title",
> > +                                                        0,
> > +                                                        G_PARAM_READABLE));
> > +    g_object_class_install_property(gobjectClass,
> > +                                    PROP_URI,
> > +                                    g_param_spec_string("uri",
> > +                                                        "URI",
> > +                                                        "Returns the current URI of the contents displayed by the @web_view",
> > +                                                        0,
> > +                                                        G_PARAM_READABLE));
> > +    g_object_class_install_property(gobjectClass,
> > +                                    PROP_PROGRESS,
> > +                                    g_param_spec_double("progress",
> > +                                                        "Progress",
> > +                                                        "Determines the current progress of the load",
> > +                                                        0.0, 1.0, 1.0,
> > +                                                        G_PARAM_READABLE));
> 
> Maybe stating to add the doc of the API it is a good idea.

Yes, it's a matter of copy and paste from webkit1, I haven't included the docs in this first patches to make them simple. 

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:139
> > +    // TODO: update load-status property
> 
> In this cases we are using NotImplemented without the TODO comment, I would stick to that and add the comment just in case it adds any other information.

Ok, I'll add it, the TDO comment was to remember that load-status property is what should be updated there, and not emit the load-* signals that are deprecated. Hopefully this won't be unimplemented for long time :-)

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:287
> > +    WKPageLoaderClient loadClient = {
> > +        0,       /* version */
> > +        webView, /* clientInfo */
> > +        didStartProvisionalLoadForFrame,
> > +        didReceiveServerRedirectForProvisionalLoadForFrame,
> > +        didFailProvisionalLoadWithErrorForFrame,
> > +        didCommitLoadForFrame,
> > +        didFinishDocumentLoadForFrame,
> > +        didFinishLoadForFrame,
> > +        didFailLoadWithErrorForFrame,
> > +        0, /* didSameDocumentNavigationForFrame */
> > +        didReceiveTitleForFrame,
> > +        didFirstLayoutForFrame,
> > +        didFirstVisuallyNonEmptyLayoutForFrame,
> > +        didRemoveFrameFromHierarchy,
> > +        0, /* didDisplayInsecureContentForFrame */
> > +        0, /* didRunInsecureContentForFrame */
> > +        0, /* canAuthenticateAgainstProtectionSpaceInFrame */
> > +        0, /* didReceiveAuthenticationChallengeInFrame */
> > +        didStartProgress,
> > +        didChangeProgress,
> > +        didFinishProgress,
> > +        didBecomeUnresponsive,
> > +        didBecomeResponsive,
> > +        0,  /* processDidCrash */
> > +        0,  /* didChangeBackForwardList */
> > +        0   /* shouldGoToBackForwardListItem */
> > +    };
> 
> Shall we add all these functions to a different file? Maybe PageLoaderClient.

Yes, there's a FIXME with the same question indeed.

Thanks

[Martin Robinson]

Comment on attachment 88441 [details]
Updated patch

Going to remove the r? flag here until we can resolve API discussions.

[Carlos Garcia Campos]

I had forgotten this, marking as dupl.

*** This bug has been marked as a duplicate of bug 68085 ***