Bug 69610 – [GTK] Add webkit_web_view_stop_loading() to WebKit2 GTK+ API

• Component: WebKit2
• Status: RESOLVED FIXED

[Carlos Garcia Campos]

It's currently missing.

[Carlos Garcia Campos]

Created attachment 110111 [details]
Patch

[Carlos Garcia Campos]

Created attachment 110112 [details]
Updated patch

I forgot to add g_return_if_fail macro.

[Carlos Garcia Campos]

Created attachment 110995 [details]
Patch updated to current git master

[Martin Robinson]

Comment on attachment 110995 [details]
Patch updated to current git master

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

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:287
> + * Does nothing if no content is being loaded.

Does nothing... -> This method does nothing...

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:290
> + * #WebKitWebLoaderClient::load-failed will be emitted on current

current -> the current

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:103
> +    WebKitWebLoaderClient* client = webkit_web_view_get_loader_client(test->m_webView);
> +
> +    g_signal_connect(client, "load-committed", G_CALLBACK(loadCancelledLoadCommitted), test);
> +    g_signal_connect(client, "load-failed", G_CALLBACK(loadCancelledLoadFailed), test);
> +    g_signal_connect(client, "load-finished", G_CALLBACK(loadCancelledLoadFinished), test);
> +
> +    webkit_web_view_load_uri(test->m_webView, kServer->getURIForPath("/cancelled").data());

I think this can be simplified by using a load tracking test. Here you can just make a simple class:

class LoadStopTrackingTest : public LoadTrackingTest {
    MAKE_GLIB_TEST_FIXTURE(LoadTrackingTest);
public:
    virtual void loadCommitted(WebKitWebLoaderClient* client) {
        LoadTrackingTest::loadCommitted(client);
        webkit_web_view_stop_loading(test->m_webView);
    }
}

and then later

LoadStopTrackingTest::add("WebKitWebView", "stop-loading", testLoadCancelled);

You could also store the load error as a GOwnPtr member to assert that it's the right one. This would be a useful feature for LoadTrackingTest.

> Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:158
> +    } else if (g_str_equal(path, "/cancelled")) {
> +        soup_message_headers_set_encoding(message->response_headers, SOUP_ENCODING_CHUNKED);
> +        soup_message_body_append(message->response_body, SOUP_MEMORY_STATIC, responseString, strlen(responseString));
> +        soup_server_unpause_message(server, message);
> +        return;

Why is it important to have a cancelled message here? Wouldn't the existing default endpoint be fine?

[Carlos Garcia Campos]

(In reply to comment #4)
> (From update of attachment 110995 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=110995&action=review
> 
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:287
> > + * Does nothing if no content is being loaded.
> 
> Does nothing... -> This method does nothing...

Ok.

> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:290
> > + * #WebKitWebLoaderClient::load-failed will be emitted on current
> 
> current -> the current

Ok.

> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:103
> > +    WebKitWebLoaderClient* client = webkit_web_view_get_loader_client(test->m_webView);
> > +
> > +    g_signal_connect(client, "load-committed", G_CALLBACK(loadCancelledLoadCommitted), test);
> > +    g_signal_connect(client, "load-failed", G_CALLBACK(loadCancelledLoadFailed), test);
> > +    g_signal_connect(client, "load-finished", G_CALLBACK(loadCancelledLoadFinished), test);
> > +
> > +    webkit_web_view_load_uri(test->m_webView, kServer->getURIForPath("/cancelled").data());
> 
> I think this can be simplified by using a load tracking test. Here you can just make a simple class:
> 
> class LoadStopTrackingTest : public LoadTrackingTest {
>     MAKE_GLIB_TEST_FIXTURE(LoadTrackingTest);
> public:
>     virtual void loadCommitted(WebKitWebLoaderClient* client) {
>         LoadTrackingTest::loadCommitted(client);
>         webkit_web_view_stop_loading(test->m_webView);
>     }
> }
> 
> and then later
> 
> LoadStopTrackingTest::add("WebKitWebView", "stop-loading", testLoadCancelled);

Ok, it looks much better indeed.

> You could also store the load error as a GOwnPtr member to assert that it's the right one. This would be a useful feature for LoadTrackingTest.
> 
> > Source/WebKit2/UIProcess/API/gtk/tests/TestWebKitWebLoaderClient.cpp:158
> > +    } else if (g_str_equal(path, "/cancelled")) {
> > +        soup_message_headers_set_encoding(message->response_headers, SOUP_ENCODING_CHUNKED);
> > +        soup_message_body_append(message->response_body, SOUP_MEMORY_STATIC, responseString, strlen(responseString));
> > +        soup_server_unpause_message(server, message);
> > +        return;
> 
> Why is it important to have a cancelled message here? Wouldn't the existing default endpoint be fine?

stop_loading() is aync too, so this is to make sure the load is cancelled before it's finished.

[Carlos Garcia Campos]

Created attachment 111418 [details]
Updated patch according to review comments

[Martin Robinson]

Comment on attachment 111418 [details]
Updated patch according to review comments

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

Great.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:293
> + * If there is a loading operation in progress, it will be cancelled and
> + * #WebKitWebLoaderClient::provisional-load-failed or
> + * #WebKitWebLoaderClient::load-failed will be emitted on the current
> + * #WebKitWebLoaderClient with %WEBKIT_NETWORK_ERROR_CANCELLED error.
> + * See also webkit_web_view_get_loader_client().

This doc is awesome.

[Carlos Garcia Campos]

Committed r98080: <http://trac.webkit.org/changeset/98080>