[Webkit-unassigned] [Bug 157899] [GTK] Provide frame-related load signals in WebKitWebView

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=157899

Michael Catanzaro <mcatanzaro at igalia.com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #279474|review?, commit-queue?      |review-, commit-queue-
              Flags|                            |

--- Comment #2 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 279474
  --> https://bugs.webkit.org/attachment.cgi?id=279474
proposed patch

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

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.

> Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:43
> +    static const char* getFrameURL(const WebFrameProxy& frame)

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.)

> Source/WebKit2/UIProcess/API/gtk/WebKitLoaderClient.cpp:49
>  private:

Leave a blank line above private.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1137
> +     * for any of the frames in the @web_view. See #WebKitWebView::load-changed

except it is invoked for any of the frames in the @web_view except the main frame.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1167
> +     * WebKitWebView::frame-load-changed will always be emitted with

with the

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1221
> +            g_signal_accumulator_true_handled, 0 /* accumulator data */,

No need for this copypasted comment

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.cpp:1940
> +void webkitWebViewFrameLoadFailed(WebKitWebView* webView, const char* uri, WebKitLoadEvent loadEvent, const char* failingURI, GError *error)

GError* error

> Source/WebKit2/UIProcess/API/gtk/WebKitWebView.h:268
>      void (*_webkit_reserved1) (void);

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.

> Source/WebKit2/UIProcess/API/gtk/WebKitWebViewPrivate.h:41
> +void webkitWebViewFrameLoadFailed(WebKitWebView*, const char* uri, WebKitLoadEvent, const char* failingURI, GError*);

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.

> Source/cmake/OptionsGTK.cmake:18
> +CALCULATE_LIBRARY_VERSIONS_FROM_LIBTOOL_TRIPLE(WEBKIT2 52 0 14)

Nope, ABI compatibility is an important feature as WebKitGTK+ is used outside of Linux distros.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.webkit.org/pipermail/webkit-unassigned/attachments/20160520/b8defa04/attachment.html>