Bug 197270 - [WPE][GTK] Add webkit_frame_get_id() API
Summary: [WPE][GTK] Add webkit_frame_get_id() API
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-04-24 21:28 PDT by Michael Catanzaro
Modified: 2019-05-10 09:53 PDT (History)
7 users (show)

See Also:


Attachments
Patch (4.42 KB, patch)
2019-04-24 21:30 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (4.54 KB, patch)
2019-04-28 08:55 PDT, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (11.92 KB, patch)
2019-04-28 14:14 PDT, Michael Catanzaro
cgarcia: review+
cgarcia: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2019-04-24 21:28:51 PDT
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.
Comment 1 Michael Catanzaro 2019-04-24 21:30:07 PDT
Created attachment 368208 [details]
Patch
Comment 2 Build Bot 2019-04-24 21:32:54 PDT
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
Comment 3 Adrian Perez 2019-04-25 04:38:34 PDT
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.
Comment 4 Michael Catanzaro 2019-04-25 06:29:55 PDT
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
Comment 5 Adrian Perez 2019-04-25 13:51:50 PDT
(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
Comment 6 Michael Catanzaro 2019-04-25 14:28:36 PDT
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).
Comment 7 Michael Catanzaro 2019-04-28 08:54:22 PDT
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.
Comment 8 Michael Catanzaro 2019-04-28 08:55:27 PDT
Created attachment 368431 [details]
Patch
Comment 9 Michael Catanzaro 2019-04-28 09:20:29 PDT
Comment on attachment 368431 [details]
Patch

Actually I found a way.
Comment 10 Michael Catanzaro 2019-04-28 14:14:26 PDT
Created attachment 368435 [details]
Patch
Comment 11 Michael Catanzaro 2019-05-06 18:58:57 PDT
Ping for review.
Comment 12 Carlos Garcia Campos 2019-05-10 02:40:54 PDT
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?
Comment 13 Michael Catanzaro 2019-05-10 09:16:06 PDT
(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.
Comment 14 Michael Catanzaro 2019-05-10 09:21:03 PDT
(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.
Comment 15 Michael Catanzaro 2019-05-10 09:35:54 PDT
(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.
Comment 16 Michael Catanzaro 2019-05-10 09:45:42 PDT
postMessage is the recommended way to communicate from parent frame to subframe, btw.
Comment 17 Michael Catanzaro 2019-05-10 09:46:43 PDT
Committed r245176: <https://trac.webkit.org/changeset/245176>
Comment 18 Michael Catanzaro 2019-05-10 09:49:33 PDT
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.
Comment 19 Michael Catanzaro 2019-05-10 09:52:31 PDT
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?
Comment 20 Michael Catanzaro 2019-05-10 09:53:52 PDT
OK, it's actually a WebProcessTest. m_mainLoop is a member of WebViewTest, not WebProcessTest. Nothing left to do.