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

• Component: WebKitGTK
• Status: RESOLVED INVALID

[Milan Crha]

While working on the Evolution port to the WebKit2, I realized that there is a problem when to attach DOM bindings, especially when the web view contains iframe-s (and these iframe-s can be formatted mail messages, which also contain iframe-s). The only current way of noticing when the loading was finished is to listen to WebKitWebView::load-event and add the bindings when the load_event == WEBKIT_LOAD_FINISHED, but it's a matter of luck whether the internal iframe-s will be loaded or not at that time.

Due to this, I propose a new signal to be added to the WebKitWebView with a name "frame-loaded", with a prototype:

    void
    frame_loaded (WebKitWebView *web_view,
                  const gchar *frame_src, /* 'src' attribute content of the loaded (i)frame */
                  const gchar *frame_id,  /* 'id' attribute content of the loaded (i)frame */
                  gpointer user_data)

which will be called whenever any (i)frame will be in a state equivalent of the load_event == WEBKIT_LOAD_FINISHED of the main WebKitWebView.
That way, the subscriber to this signal will know that everything is ready and the DOM bindings can be safely done on that particular frame.

I was told that you have a similar request from someone else already, but I didn't find it in the bugzilla (maybe I used wrong search term), thus I filled it myself.

[Milan Crha]

Created attachment 279474 [details]
proposed patch

This adds three new signals to the WebkitWebView:
   "frame-load-changed"
   "frame-load-failed"
   "frame-load-failed-with-tls-errors"
similar to its counterparts:
   "load-changed"
   "load-failed"
   "load-failed-with-tls-errors"
except they have added one more argument, frame URI whose load status changed. These are not called for the main frame, only for the subframe-s.

My initial request wanted also the 'id' attribute of the iframe, but I realized that this information is not available, thus only the URI is passed to the signals. It's perfectly fine, the 'id' was just about a "nice to have" thing.

As the size of _WebKitWebViewClass structure changed, I did bump the soname version too.

[Michael Catanzaro]

Comment on attachment 279474 [details]
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.

[Milan Crha]

Created attachment 279568 [details]
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.

[WebKit Commit Bot]

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

[Michael Catanzaro]

Thanks, looks good.

(In reply to comment #3)
> 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+.

[e-mail]

It's just a formality, to give folks a chance to notice and propose changes.
 
> 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.

That was a good guess, but we actually use WTF::String in Source/WTF/wtf/text/WTFString.h.

See https://trac.webkit.org/wiki/EfficientStrings for string concatenation.

[Milan Crha]

Created attachment 279635 [details]
proposed patch ]I[

I replaced std::string usage with WTF::StringBuilder. The code looks less efficient, but it's in tests only, thus might not matter that much, right? I tried to use WTF::String, but the compilation was failing and I didn't want to spend whole day on hunting what was wrong (surely something what I did and how I did it).

[Carlos Garcia Campos]

(In reply to comment #0)
> While working on the Evolution port to the WebKit2, I realized that there is
> a problem when to attach DOM bindings, especially when the web view contains
> iframe-s (and these iframe-s can be formatted mail messages, which also
> contain iframe-s).

What do you mean by attach DOM bindings? DOM bindings live in the web process, so I don't understand how new API in the UI process can help with that.

> The only current way of noticing when the loading was
> finished is to listen to WebKitWebView::load-event and add the bindings when
> the load_event == WEBKIT_LOAD_FINISHED, but it's a matter of luck whether
> the internal iframe-s will be loaded or not at that time.
> 
> Due to this, I propose a new signal to be added to the WebKitWebView with a
> name "frame-loaded", with a prototype:
> 
>     void
>     frame_loaded (WebKitWebView *web_view,
>                   const gchar *frame_src, /* 'src' attribute content of the
> loaded (i)frame */
>                   const gchar *frame_id,  /* 'id' attribute content of the
> loaded (i)frame */
>                   gpointer user_data)
> 
> which will be called whenever any (i)frame will be in a state equivalent of
> the load_event == WEBKIT_LOAD_FINISHED of the main WebKitWebView.
> That way, the subscriber to this signal will know that everything is ready
> and the DOM bindings can be safely done on that particular frame.

The right place to use DOM bindings is WebKitWebPage::document-loaded in the web extensions API. It's true that is only available for the main frame, but if we need to grow the API to non main frames it has to be in the web process, probably adding WebKitFrame::document-loaded and webkit_frame_get_dom_document().

> I was told that you have a similar request from someone else already, but I
> didn't find it in the bugzilla (maybe I used wrong search term), thus I
> filled it myself.

We have tried hard not to expose frames in the UI process API, since it was very confusing in webkit1 and often misused, so we need a use case here to expose it.

[Milan Crha]

(In reply to comment #7)
> (In reply to comment #0)
> > While working on the Evolution port to the WebKit2, I realized that there is
> > a problem when to attach DOM bindings, especially when the web view contains
> > iframe-s (and these iframe-s can be formatted mail messages, which also
> > contain iframe-s).
> 
> What do you mean by attach DOM bindings? DOM bindings live in the web
> process, so I don't understand how new API in the UI process can help with
> that.

The evolution plays a ping-pong over D-Bus between the UI part (evolution) and the WebProcess part (webkit extension, to which the D-Bus is connected). Currently, in the webkit2 port branch of the evolution, the UI part listens for WebKitWebView::load-changed signal. Once it receives WEBKIT_LOAD_FINISHED event it asks the WebProcess part to attach DOM bindings. This goes through D-Bus and there is no guarantee when exactly it'll be delivered, thus, when the parts being meant already available in the DOM structure, are sometimes available and sometimes not, depending whether the WebKit had enough time to load also sub-frames of the main frame.

> The right place to use DOM bindings is WebKitWebPage::document-loaded in the
> web extensions API.

I understood it as an equivalent of the WebKitWebView::load-changed signal. The thing is that the DOM bindings are of a nature "button clicked", which the UI part responds to and does some UI magics for the user, when "the button" is clicked. That means that the UI part generates some HTML, then it asks the web extension to attach "click" event listener on the elements, and when they are clicked the D-Bus signal lets the UI part know. Interprocess communication is sort of nightmare.

> We have tried hard not to expose frames in the UI process API, since it was
> very confusing in webkit1 and often misused, so we need a use case here to
> expose it.

Okay, I would try to provide a test application, but as it involves the WebProcess extension, then it could be pretty complex, thus I hesitate to do it, unless really necessary.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #0)
> > > While working on the Evolution port to the WebKit2, I realized that there is
> > > a problem when to attach DOM bindings, especially when the web view contains
> > > iframe-s (and these iframe-s can be formatted mail messages, which also
> > > contain iframe-s).
> > 
> > What do you mean by attach DOM bindings? DOM bindings live in the web
> > process, so I don't understand how new API in the UI process can help with
> > that.
> 
> The evolution plays a ping-pong over D-Bus between the UI part (evolution)
> and the WebProcess part (webkit extension, to which the D-Bus is connected).
> Currently, in the webkit2 port branch of the evolution, the UI part listens
> for WebKitWebView::load-changed signal. Once it receives
> WEBKIT_LOAD_FINISHED event it asks the WebProcess part to attach DOM
> bindings. This goes through D-Bus and there is no guarantee when exactly
> it'll be delivered, thus, when the parts being meant already available in
> the DOM structure, are sometimes available and sometimes not, depending
> whether the WebKit had enough time to load also sub-frames of the main frame.

That's indeed not the right way to do it. The web extension should emit a signal when document loaded happens instead of relying on web view load events that have nothing to do with dom bindings.

> > The right place to use DOM bindings is WebKitWebPage::document-loaded in the
> > web extensions API.
> 
> I understood it as an equivalent of the WebKitWebView::load-changed signal.

It's not.

> The thing is that the DOM bindings are of a nature "button clicked", which
> the UI part responds to and does some UI magics for the user, when "the
> button" is clicked. That means that the UI part generates some HTML, then it
> asks the web extension to attach "click" event listener on the elements, and
> when they are clicked the D-Bus signal lets the UI part know. Interprocess
> communication is sort of nightmare.

The UI process should wait for the document-loaded signal to allow the user click on that button, or send a message to the extension, and the extension should queue it until document-loaded happens.

> > We have tried hard not to expose frames in the UI process API, since it was
> > very confusing in webkit1 and often misused, so we need a use case here to
> > expose it.
> 
> Okay, I would try to provide a test application, but as it involves the
> WebProcess extension, then it could be pretty complex, thus I hesitate to do
> it, unless really necessary.

This doesn't look like a valid use case for exposing frames in the UI process to me yet, but maybe I didn't understand it correctly.

[Milan Crha]

(In reply to comment #9)
> That's indeed not the right way to do it. The web extension should emit a
> signal when document loaded happens instead of relying on web view load
> events that have nothing to do with dom bindings.

I am currently at commit 33e641672d5a84a0 of a webkit git master checkout, the `git log` says: "REGRESSION (r200638): -[DOMHTMLVideoElement play] disappeared from ObjC bindings" and at the end "git-svn-id: http://svn.webkit.org/repository/webkit/trunk@201212 268f45cc-cd09-0410-ab3c-d52691b4dbfc".

I modified evolution's code to check these things in a way that there is loaded a page with one iframe, (and the frame is a plain html), thus at the end the structure looks roughly like this:
  <html>
  <body>
     <iframe src="..."></iframe>
  </body>
  </html>

and the load of the iframe content is done asynchronously, where I added a 5 seconds delay to make things easier. The result of my debug prints is:

web_view_load_changed_cb: 42305544881 started (pid:19619)
   web_page_document_loaded_cb: 42305558657 (pid:19653)
     > mail_request_process_mail_sync: 42305561698 waiting for 5 seconds (pid:19619)
     > mail_request_process_mail_sync: 42310561790 done wait for 5 seconds (pid:19619)
web_view_load_changed_cb: 42310582854 finished (pid:19619)

where the very long number is a result of g_get_monotonic_time () (in microseconds) and the pid 19653 is my web-extension (WebProcess), while the pid 19619 is the evolution itself (UI process).

It shows that the web page's document-loaded signal (caught by web_page_document_loaded_cb) doesn't count with sub-frames and claims the page being loaded as soon as the main frame is populated.

Your proposal to add WebKitFrame::document-loaded signal doesn't scale, because:
a) I cannot connect to one central place, but should connect to the WebKitFrame instances;
b) How do I know that a frame instance had been created?
c) should I traverse the DOM several times to know whether a new frame had been added or not? That's quite inefficient and requires a complex code to maintain.

> The UI process should wait for the document-loaded signal to allow the user
> click on that button, or send a message to the extension, and the extension
> should queue it until document-loaded happens.

See the debug prints above, it would still not work, because the WebKitPage::document-loaded signal is called when the inner frame content is not ready yet.

[Carlos Garcia Campos]

(In reply to comment #10)
> (In reply to comment #9)
> > That's indeed not the right way to do it. The web extension should emit a
> > signal when document loaded happens instead of relying on web view load
> > events that have nothing to do with dom bindings.
> 
> I am currently at commit 33e641672d5a84a0 of a webkit git master checkout,
> the `git log` says: "REGRESSION (r200638): -[DOMHTMLVideoElement play]
> disappeared from ObjC bindings" and at the end "git-svn-id:
> http://svn.webkit.org/repository/webkit/trunk@201212
> 268f45cc-cd09-0410-ab3c-d52691b4dbfc".
> 
> I modified evolution's code to check these things in a way that there is
> loaded a page with one iframe, (and the frame is a plain html), thus at the
> end the structure looks roughly like this:
>   <html>
>   <body>
>      <iframe src="..."></iframe>
>   </body>
>   </html>
> 
> and the load of the iframe content is done asynchronously, where I added a 5
> seconds delay to make things easier. The result of my debug prints is:
> 
> web_view_load_changed_cb: 42305544881 started (pid:19619)
>    web_page_document_loaded_cb: 42305558657 (pid:19653)
>      > mail_request_process_mail_sync: 42305561698 waiting for 5 seconds
> (pid:19619)
>      > mail_request_process_mail_sync: 42310561790 done wait for 5 seconds
> (pid:19619)
> web_view_load_changed_cb: 42310582854 finished (pid:19619)
> 
> where the very long number is a result of g_get_monotonic_time () (in
> microseconds) and the pid 19653 is my web-extension (WebProcess), while the
> pid 19619 is the evolution itself (UI process).
> 
> It shows that the web page's document-loaded signal (caught by
> web_page_document_loaded_cb) doesn't count with sub-frames and claims the
> page being loaded as soon as the main frame is populated.

Yes, we know it, as I said before we would need new API to cover other frames.

> Your proposal to add WebKitFrame::document-loaded signal doesn't scale,
> because:
> a) I cannot connect to one central place, but should connect to the
> WebKitFrame instances;
> b) How do I know that a frame instance had been created?
> c) should I traverse the DOM several times to know whether a new frame had
> been added or not? That's quite inefficient and requires a complex code to
> maintain.

It was not actually a proposal, just an idea, the details of the new API is what we should discuss here.

> > The UI process should wait for the document-loaded signal to allow the user
> > click on that button, or send a message to the extension, and the extension
> > should queue it until document-loaded happens.
> 
> See the debug prints above, it would still not work, because the
> WebKitPage::document-loaded signal is called when the inner frame content is
> not ready yet.

I haven't said the current API allows to do what you need, my only point is that if we need to add new API for your use case it has to be added to the web extensions API and not the UI process. What API specifically is something to be defined and your input here would help a lot. The way we usually do this is sending an email to the mailing list explaining the use case in detail, and proposing a possible API.

[Milan Crha]

Created attachment 279764 [details]
proposed patch IV

Okay, then add it for the WebKitWebPage. I can change the evolution bits to move some parts into the web extension, but not everything. This particular thing about signalling element clicks can be moved with not much harm (the code will be slightly more complicated on the evolution side).

[Michael Catanzaro]

Comment on attachment 279764 [details]
proposed patch IV

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

LGTM, Carlos?

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:84
> +    g_assert(frame_uri && *frame_uri);

I would use a stronger assertion here if possible; it should only be called once, and the URI should end with unknown-file.html, right?

[Carlos Garcia Campos]

Comment on attachment 279764 [details]
proposed patch IV

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

Sorry, I forgot about this new patch. I don't think this is what we want though, see my comments below

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:60
> +    FRAME_LOADED,

I don't think this belongs here. This could makes sense in the UI process where we don't expose frames, but in the web extensions API we have WebKitFrame, so there's no reason for adding frame related signals to the page object. One of the reasons why we didn't want to expose frames was because we had a lot of signals duplicated in web view and frame, making it confusing. The web view signals actually meant main frame events, so we had two ways of getting events for the main frame. I agree we would be making the same mistake again here if we add document-loaded signal to WebKitFrame. Fortunately in the web extensions API we only have one signal related to the frame, document-loaded, so maybe we can deprecate it and add a new one that receives the frame. Or deprecated it, and move it to WebKitFrame (I know we only have API to get the main frame yet, but we could add frame-created signal or something like that).

> Source/WebKit2/WebProcess/InjectedBundle/API/gtk/WebKitWebPage.cpp:186
> +        g_signal_emit(WEBKIT_WEB_PAGE(clientInfo), signals[FRAME_LOADED], 0, toImpl(frame)->coreFrame()->document()->url().string().utf8().data());

This is not frame-loaded, this should be frame-document-loaded or document-loaded-in-frame and it should receive a WebKitFrame, not a document URL.

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestWebExtensions.cpp:81
> +    const char* frame_uri = nullptr;

frame_uri -> frameURI

> Tools/TestWebKitAPI/Tests/WebKit2Gtk/WebExtensionTest.cpp:139
> +static void frameLoadedCallback(WebKitWebPage* webPage, const gchar* frame_uri, WebKitWebExtension* extension)

Ditto.

[Milan Crha]

Hrm, this is embarrassing. It seems I made my initial tests incorrectly and the issue is not on the WebKitGTK+ side. I've still applied the latest patch in my local checkout of the WebKitGTK+ from git, thus I can connect to both "document-loaded" and "frame-loaded" signals (the later is that added in my patch). I added debug prints into both handlers and into the WebkitWebView's "load-event" signal, to see when is delivered what. I also artificially slowed down subframe generations, thus there is a time for the system to receive the signals when needed. The result shows this sequence:

   WebView::load-event WEBKIT_LOAD_STARTED
   WebView::load-event WEBKIT_LOAD_COMMITTED
      WebPage::document-loaded
      WebPage::frame-loaded
      WebPage::frame-loaded
      ....
      WebPage::frame-loaded
   WebView::load-event WEBKIT_LOAD_FINISHED

and that's all.

It shows three things:
1) the WebPage::document-loaded is delivered when the main frame is loaded,
   but without the sub-iframe-s being available
2) the WebPage::frame-loaded is emitted before the UI part knows about
   the load finished
3) the WebView::load-event with WEBKIT_LOAD_FINISHED is received only after
   all the subframes are fully loaded

That means that my statement from comment #0 is false, it's not a "matter of luck", it's more likely that I made something wrong in the Evolution.

Maybe the WebPage::frame-document-loaded could be found useful for someone in the future, but, as you indicated, you are trying not to expose frames in both the UI and the Web processes, then it's not needed to be added as of now, because the Evolution would not use it anyway (it'll use the 3) from the above, which it already does, though it can be that it does so in some odd way and it needs fixing on the Evolution side, not on the WebKitGTK+ side).

I'm closing this due to these new findings. I'm sorry for the false noise. I'd not bother you, if I do my testing correctly at the first place. Thank you for your time.