Bug 74605 - [GTK] Simplify loader client WebKit2 GTK+ API
: [GTK] Simplify loader client WebKit2 GTK+ API
Status: RESOLVED FIXED
: WebKit
WebKit2
: 528+ (Nightly build)
: PC Linux
: P2 Normal
Assigned To:
:
: Gtk
:
:
  Show dependency treegraph
 
Reported: 2011-12-15 06:37 PST by
Modified: 2012-01-03 08:05 PST (History)


Attachments
Patch (62.06 KB, patch)
2011-12-15 06:53 PST, Carlos Garcia Campos
no flags Review Patch | Details | Formatted Diff | Diff
Updated patch (61.04 KB, patch)
2011-12-26 02:12 PST, Carlos Garcia Campos
no flags Review Patch | Details | Formatted Diff | Diff
I forgot to include Tools/ChangeLog in previous patch (61.59 KB, patch)
2011-12-26 02:15 PST, Carlos Garcia Campos
gns: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-12-15 06:37:24 PST
The initial idea of the loader client object exposed in the API was mainly to reduce the amount of signals of WebView. During the webkitgtk hackfest we were discussing the current API and agreed on moving to simpler API adding just two signals to WebView and making the current loader client private.
------- Comment #1 From 2011-12-15 06:53:08 PST -------
Created an attachment (id=119424) [details]
Patch

The code is simpler and the API easier to use. Now we also have only one condition for load finish. The patch doesn't include previous status in load-status-changed callback as agreed during the hackfest, because Martin has some doubts about it, it's better to add it later if we finally decide it's useful than remve it.
------- Comment #2 From 2011-12-15 06:55:39 PST -------
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
------- Comment #3 From 2011-12-15 08:29:43 PST -------
(From update of attachment 119424 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=119424&action=review

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:316
> +     * @status: the #WebKitLoadStatus

I prefer @event: the #WebKitLoadEvent here. See below.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:71
> +} WebKitLoadStatus;

So I think this should be renamed to  WebKitLoadEvent since WEBKIT_LOAD_REDIRECTED is not really a status, but something that happened to us.
------- Comment #4 From 2011-12-15 10:47:26 PST -------
(In reply to comment #3)
> > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:71
> > +} WebKitLoadStatus;
> 
> So I think this should be renamed to  WebKitLoadEvent since WEBKIT_LOAD_REDIRECTED is not really a status, but something that happened to us.

hmm, good point!
------- Comment #5 From 2011-12-15 11:10:11 PST -------
(In reply to comment #4)
> (In reply to comment #3)
> > > Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:71
> > > +} WebKitLoadStatus;
> > 
> > So I think this should be renamed to  WebKitLoadEvent since WEBKIT_LOAD_REDIRECTED is not really a status, but something that happened to us.
> 
> hmm, good point!

I guess we can go two ways here: try to make the enum represent a status or an events For instance if we used events, they enum values would be something like:

WEBKIT_LOAD_STARTED_PROVISIONAL
WEBKIT_LOAD_REDIRECTED
WEBKIT_LOAD_COMMITTED
WEBKIT_LOAD_FINISHED

and the argument names would be something like previous_load_event instead of previous_load_status.

If we think of them as states we would have:

WEBKIT_LOAD_PROVISIONAL
WEBKIT_LOAD_REDIRECTING
WEBKIT_LOAD_COMMITTED
WEBKIT_LOAD_FINISHED

Personally, I prefer the 'event' structure because that's the interface that the C API gives us and I think it more closely matches what's happening within WebCore.
------- Comment #6 From 2011-12-16 06:22:15 PST -------
That's a great approach, Martin, I agree with you!
------- Comment #7 From 2011-12-26 02:12:18 PST -------
Created an attachment (id=120538) [details]
Updated patch

Renamed WebKitLoadStatus to WebKitLoadEvent. I also renamed load-status-changed signal as load-changed to avoid the word 'status' and for consistency with load-failed.
------- Comment #8 From 2011-12-26 02:15:05 PST -------
Created an attachment (id=120539) [details]
I forgot to include Tools/ChangeLog in previous patch
------- Comment #9 From 2012-01-03 06:55:23 PST -------
(From update of attachment 120539 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=120539&action=review

OK, looks good to me

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:57
> + * fail for transport issues such as not being able to

s/for/due to/ sounds better I think
------- Comment #10 From 2012-01-03 08:05:21 PST -------
Committed r103938: <http://trac.webkit.org/changeset/103938>