[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

Milan Crha <mcrha at redhat.com> changed:

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

--- Comment #3 from Milan Crha <mcrha at redhat.com> ---
Created attachment 279568
  --> https://bugs.webkit.org/attachment.cgi?id=279568&action=review
proposed patch ][

(In reply to comment #2)
> ...and it should be announced on the mailing list first

What is that mailing list, please? It's a new thing for me, since the last time I made/proposed any changes into the WebKitGTK+.

I addressed all the points you asked for (unless I missed something). Writing the tests was a good idea, it uncovered few bugs in the previous patch. Due to found behaviour I changed the failed signals signatures too, because they did not have set the frameURI at all, only the failingURI was available. It wasn't the only bug in the previous patch, tough.

I do not know what you use for the string concatenation, and I didn't want to allocate the memory in the tests using GLib, thus I chose std::string. If that's wrong, I can change it.

See my other minor notes below.

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

It's a private class, thus I didn't care that much about making the function "even more private". I moved it to the private section, rather than out of the class, because it should be used by that class only.

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

Okay, it's up to you. I change the soname versions when I make changes which require it (and I do not forget to bump it) during the development phase and keep the padding/reserved members for the stable releases, where the API/ABI freezes are in the effect. You do it differently, then I'll follow what the WebKit is used to. I changed the _webkit_reserved definition, for a better one, which will generate smaller diff, when changed. The same/similar approach is used both in the libsoup and in the evolution-data-server (and possibly many other projects, these two are those I saw it in recently).

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

Yes, I thought of it too, but then decided to add new signals, because it's easier to catch on an ABI change and new signals, than hunting for changed signal signatures. Such change would also require the soname version bump and a big release announcement.

-- 
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/20160523/342b3607/attachment-0001.html>