|Summary:||[GTK] Simplify loader client WebKit2 GTK+ API|
|Product:||WebKit||Reporter:||Carlos Garcia Campos <cgarcia>|
|Severity:||Normal||CC:||gustavo, mrobinson, pnormand, webkit.review.bot|
|Version:||528+ (Nightly build)|
Description Carlos Garcia Campos 2011-12-15 06:37:24 PST
Comment 1 Carlos Garcia Campos 2011-12-15 06:53:08 PST
Created attachment 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 WebKit Review Bot 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 Martin Robinson 2011-12-15 08:29:43 PST
Comment on attachment 119424 [details] Patch 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 Carlos Garcia Campos 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 Martin Robinson 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 Gustavo Noronha (kov) 2011-12-16 06:22:15 PST
That's a great approach, Martin, I agree with you!
Comment 7 Carlos Garcia Campos 2011-12-26 02:12:18 PST
Created attachment 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 Carlos Garcia Campos 2011-12-26 02:15:05 PST
Created attachment 120539 [details] I forgot to include Tools/ChangeLog in previous patch
Comment 9 Gustavo Noronha (kov) 2012-01-03 06:55:23 PST
Comment on attachment 120539 [details] I forgot to include Tools/ChangeLog in previous patch 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 Carlos Garcia Campos 2012-01-03 08:05:21 PST
Committed r103938: <http://trac.webkit.org/changeset/103938>