Bug 197270 – [WPE][GTK] Add webkit_frame_get_id() API

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Michael Catanzaro]

Epiphany needs webkit_frame_get_id() to keep track of frames across the web/UI process boundary. Serializing frame IDs is a lot nicer than serializing web process pointers and passing them to the UI process and back.

[Michael Catanzaro]

Created attachment 368208 [details]
Patch

[EWS Watchlist]

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

[Adrian Perez]

Comment on attachment 368208 [details]
Patch

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

The patch by itself LGTM; I have doubts about what this is trying to
achieve, please check my comments below.

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitFrame.cpp:65
> + * Gets the unique identifier of this #WebKitFrame

I have always been under the impression that a way to correlate a WebExtension
with its UIProcess-side view was to use webkit_web_view_get_page_id() in the
UIProcess, which would return the same value as webkit_web_page_get_id() in
the WebProcess. Why is that not enough? I would like to understand which kind
of use-case the proposed API is trying to solve.

Also, side question: Would this new webkit_frame_get_id() return the same
value as webkit_web_page_get_id() when querying the identifier for the main
frame? Or, in other words, does the following hold true?:

  webkit_web_page_get_id(page) == webkit_frame_get_id(webkit_page_get_main_frame(page))

It looks to me as if those should be the same 🤔 — it looks to me as if it
would be good for the reference documentation to elaborate a bit on why these
identifiers are useful and how both the UIProcess and WebProcess sides can
use them.

[Michael Catanzaro]

A WebKitWebPage has one main WebKitFrame and zero or many additional WebKitFrames. A bunch of Epiphany code currently doesn't work in subframes because it relies on running JS only in the main frame.

It's for https://gitlab.gnome.org/GNOME/epiphany/merge_requests/288.

I'll improve the docs a bit, and also think about adding some minimally-useful test

[Adrian Perez]

(In reply to Michael Catanzaro from comment #4)
> A WebKitWebPage has one main WebKitFrame and zero or many additional
> WebKitFrames. A bunch of Epiphany code currently doesn't work in subframes
> because it relies on running JS only in the main frame.
> 
> It's for https://gitlab.gnome.org/GNOME/epiphany/merge_requests/288.

Thanks for the links, I see now how the added function can be
useful: to send something pertaining to a certain frame to the
UIProcess, and then get back the same ID later from it to act
on the same frame again.

> I'll improve the docs a bit, and also think about adding some
> minimally-useful test

Please do, that was going to be my next request.

Still, I am scratching my head with whether

  webkit_web_page_get_id(page) == webkit_frame_get_id(webkit_page_get_main_frame(page))
  
holds true or not :-P

[Michael Catanzaro]

No it doesn't.

Also, the same frame ID can be reused across multiple web processes (they are only process-unique), which I should mention in the documentation. Whereas the page IDs are separate and globally-unique (unique within a single WebKitWebContext).

[Michael Catanzaro]

It's actually hard to add a meaningful test for this without the API added in bug #197271, so I'll add a test there instead.

[Michael Catanzaro]

Created attachment 368431 [details]
Patch

[Michael Catanzaro]

Comment on attachment 368431 [details]
Patch

Actually I found a way.

[Michael Catanzaro]

Created attachment 368435 [details]
Patch

[Michael Catanzaro]

Ping for review.

[Carlos Garcia Campos]

Comment on attachment 368435 [details]
Patch

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

> Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitFrame.cpp:74
> +{

g_return_val_if_fail(WEBKIT_IS_FRAME(fraem), 0);

> Tools/TestWebKitAPI/Tests/WebKitGLib/FrameTest.cpp:167
> +        GRefPtr<JSCValue> undefined = adoptGRef(jsc_value_object_invoke_method(contentWindow.get(), "postMessage", G_TYPE_STRING, "submit the form!", G_TYPE_STRING, "*", G_TYPE_NONE));
> +        g_assert_true(JSC_IS_VALUE(undefined.get()));
> +        g_assert_true(jsc_value_is_undefined(undefined.get()));

Why don't you call submit() directly on the form here? Maybe you don't even need to run loop, just connect the signal before

> Tools/TestWebKitAPI/Tests/WebKitGLib/resources/webkitglib-tests.gresource.xml:5
> +    <file alias="form-in-frame.html">Tools/TestWebKitAPI/Tests/WebKitGLib/resources/form-in-frame.html</file>

Dos this need to be a resource?

[Michael Catanzaro]

(In reply to Carlos Garcia Campos from comment #12)
> > Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitFrame.cpp:74
> > +{
> 
> g_return_val_if_fail(WEBKIT_IS_FRAME(fraem), 0);

Fixed.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/FrameTest.cpp:167
> > +        GRefPtr<JSCValue> undefined = adoptGRef(jsc_value_object_invoke_method(contentWindow.get(), "postMessage", G_TYPE_STRING, "submit the form!", G_TYPE_STRING, "*", G_TYPE_NONE));
> > +        g_assert_true(JSC_IS_VALUE(undefined.get()));
> > +        g_assert_true(jsc_value_is_undefined(undefined.get()));
> 
> Why don't you call submit() directly on the form here? Maybe you don't even
> need to run loop, just connect the signal before

Oops, good catch.

> > Tools/TestWebKitAPI/Tests/WebKitGLib/resources/webkitglib-tests.gresource.xml:5
> > +    <file alias="form-in-frame.html">Tools/TestWebKitAPI/Tests/WebKitGLib/resources/form-in-frame.html</file>
> 
> Dos this need to be a resource?

No, but there's no reason for it not to be.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #13)
> No, but there's no reason for it not to be.

Oh, you mean: does it need to be a separate file? Yes, for use as iframe src.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #13) 
> > > Tools/TestWebKitAPI/Tests/WebKitGLib/FrameTest.cpp:167
> > > +        GRefPtr<JSCValue> undefined = adoptGRef(jsc_value_object_invoke_method(contentWindow.get(), "postMessage", G_TYPE_STRING, "submit the form!", G_TYPE_STRING, "*", G_TYPE_NONE));
> > > +        g_assert_true(JSC_IS_VALUE(undefined.get()));
> > > +        g_assert_true(jsc_value_is_undefined(undefined.get()));
> > 
> > Why don't you call submit() directly on the form here? Maybe you don't even
> > need to run loop, just connect the signal before
> 
> Oops, good catch.

Ah no, it's not possible:

/webkit/WebKitFrame/subframe: [native code]: JS ERROR SecurityError: Blocked a frame with origin "webprocess://test" from accessing a cross-origin frame. Protocols, domains, and ports must match.

That's why I wound up using the message handler.

[Michael Catanzaro]

postMessage is the recommended way to communicate from parent frame to subframe, btw.

[Michael Catanzaro]

Committed r245176: <https://trac.webkit.org/changeset/245176>

[Michael Catanzaro]

Now, the GMainLoop could be replaced with a bool, though. That should work. And avoid the problem of this m_mainLoop shadowing the one in the parent class.

[Michael Catanzaro]

No, we need to run the loop, but we can at least use the parent class's loop.

Hopefully this can be the end of my conversation with myself on Bugzilla?

[Michael Catanzaro]

OK, it's actually a WebProcessTest. m_mainLoop is a member of WebViewTest, not WebProcessTest. Nothing left to do.