<html>
<head>
<base href="https://bugs.webkit.org/" />
</head>
<body><span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span> changed
<a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Provide frame-related load signals in WebKitWebView"
href="https://bugs.webkit.org/show_bug.cgi?id=157899">bug 157899</a>
<br>
<table border="1" cellspacing="0" cellpadding="8">
<tr>
<th>What</th>
<th>Removed</th>
<th>Added</th>
</tr>
<tr>
<td style="text-align:right;">Attachment #279474 Flags</td>
<td>review?, commit-queue?
</td>
<td>review-, commit-queue-
</td>
</tr></table>
<p>
<div>
<b><a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Provide frame-related load signals in WebKitWebView"
href="https://bugs.webkit.org/show_bug.cgi?id=157899#c2">Comment # 2</a>
on <a class="bz_bug_link
bz_status_NEW "
title="NEW - [GTK] Provide frame-related load signals in WebKitWebView"
href="https://bugs.webkit.org/show_bug.cgi?id=157899">bug 157899</a>
from <span class="vcard"><a class="email" href="mailto:mcatanzaro@igalia.com" title="Michael Catanzaro <mcatanzaro@igalia.com>"> <span class="fn">Michael Catanzaro</span></a>
</span></b>
<pre>Comment on <span class=""><a href="attachment.cgi?id=279474&action=diff" name="attach_279474" title="proposed patch">attachment 279474</a> <a href="attachment.cgi?id=279474&action=edit" title="proposed patch">[details]</a></span>
proposed patch
View in context: <a href="https://bugs.webkit.org/attachment.cgi?id=279474&action=review">https://bugs.webkit.org/attachment.cgi?id=279474&action=review</a>
I like it, thanks for working on this. It needs to be approved by a second GTK+ reviewer and it should be announced on the mailing list first. Could you propose this on the mailing list, please?
Importantly, we need to add API tests in Tools/TestWebKitAPI/Tests/WebKit2Gtk. Probably TestLoaderClient.cpp would be the right place for most of these, plus TestSSL.cpp for the TLS errors signal. r- because of this.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:43
> + static const char* getFrameURL(const WebFrameProxy& frame)</span >
This doesn't make sense as a public function. I would keep the static keyword but declare it above rather than inside the class; you'll notice it doesn't need to be a class member at all. (Alternatively, it could be a private static function.)
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:49
> private:</span >
Leave a blank line above private.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1137
> + * for any of the frames in the @web_view. See #WebKitWebView::load-changed</span >
except it is invoked for any of the frames in the @web_view except the main frame.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1167
> + * WebKitWebView::frame-load-changed will always be emitted with</span >
with the
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1221
> + g_signal_accumulator_true_handled, 0 /* accumulator data */,</span >
No need for this copypasted comment
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1940
> +void webkitWebViewFrameLoadFailed(WebKitWebView* webView, const char* uri, WebKitLoadEvent loadEvent, const char* failingURI, GError *error)</span >
GError* error
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:268
> void (*_webkit_reserved1) (void);</span >
We have sufficient padding here, so just remove three of the padding pointers: then we don't need the soname bump, and you can still have your new API. It's unfortunate that we're running low on padding, but it exists to be used after all.
<span class="quote">> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h:41
> +void webkitWebViewFrameLoadFailed(WebKitWebView*, const char* uri, WebKitLoadEvent, const char* failingURI, GError*);</span >
Another option, which wouldn't require adding three new functions, would be to add the uri parameter to the existing functions up above, rename it to frameURI, and use nullptr to indicate that it corresponds to the main frame.
But thinking about this some more, it's probably simpler and easier to read the way you have it now.
<span class="quote">> Source/cmake/OptionsGTK.cmake:18
> +CALCULATE_LIBRARY_VERSIONS_FROM_LIBTOOL_TRIPLE(WEBKIT2 52 0 14)</span >
Nope, ABI compatibility is an important feature as WebKitGTK+ is used outside of Linux distros.</pre>
</div>
</p>
<hr>
<span>You are receiving this mail because:</span>
<ul>
<li>You are the assignee for the bug.</li>
</ul>
</body>
</html>