Bug 69610 - [GTK] Add webkit_web_view_stop_loading() to WebKit2 GTK+ API
Summary: [GTK] Add webkit_web_view_stop_loading() to WebKit2 GTK+ API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2011-10-07 02:20 PDT by Carlos Garcia Campos
Modified: 2011-10-21 01:08 PDT (History)
3 users (show)

See Also:


Attachments
Patch (5.05 KB, patch)
2011-10-07 02:29 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch (5.10 KB, patch)
2011-10-07 02:40 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch updated to current git master (5.76 KB, patch)
2011-10-14 05:03 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Updated patch according to review comments (6.95 KB, patch)
2011-10-18 03:48 PDT, Carlos Garcia Campos
mrobinson: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2011-10-07 02:20:33 PDT
It's currently missing.
Comment 1 Carlos Garcia Campos 2011-10-07 02:29:51 PDT
Created attachment 110111 [details]
Patch
Comment 2 Carlos Garcia Campos 2011-10-07 02:40:01 PDT
Created attachment 110112 [details]
Updated patch

I forgot to add g_return_if_fail macro.
Comment 3 Carlos Garcia Campos 2011-10-14 05:03:21 PDT
Created attachment 110995 [details]
Patch updated to current git master
Comment 4 Martin Robinson 2011-10-14 09:13:48 PDT
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?
Comment 5 Carlos Garcia Campos 2011-10-18 03:44:53 PDT
(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.
Comment 6 Carlos Garcia Campos 2011-10-18 03:48:44 PDT
Created attachment 111418 [details]
Updated patch according to review comments
Comment 7 Martin Robinson 2011-10-20 23:00:00 PDT
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.
Comment 8 Carlos Garcia Campos 2011-10-21 01:08:18 PDT
Committed r98080: <http://trac.webkit.org/changeset/98080>