[webkit-reviews] review granted: [Bug 176760] [GTK] Need WebKitContextMenuItemType to open emoji picker : [Attachment 370039] Patch

[bugzilla-daemon at webkit.org]

Michael Catanzaro <mcatanzaro at igalia.com> has granted Carlos Garcia Campos
<cgarcia at igalia.com>'s request for review:
Bug 176760: [GTK] Need WebKitContextMenuItemType to open emoji picker
https://bugs.webkit.org/show_bug.cgi?id=176760

Attachment 370039: Patch

https://bugs.webkit.org/attachment.cgi?id=370039&action=review




--- Comment #5 from Michael Catanzaro <mcatanzaro at igalia.com> ---
Comment on attachment 370039
  --> https://bugs.webkit.org/attachment.cgi?id=370039
Patch

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

I've asked the GTK devs to expose this for GTK 4, so maybe we'll only need to
copy it for GTK 3 in the future.

Matthias suggests that we check the master branch for recent fixes to the emoji
chooser to make it populate incrementally, which are still pending backport to
GTK 3.

> Source/WebKit/ChangeLog:9
> +	   context menu or keyboard shortcuts. GtkEmojiChooser is proivate in
GTK, so we include our own copy, adapted to

private

> Source/WebKit/ChangeLog:11
> +	   content. I'm going to add public API in a follow up patch to be able
to use your own chooser, or event prevent

even

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:232
> +    CompletionHandler<void(String)> emojiChooserCompletionHandler;
> +    RunLoop::Timer<WebKitWebViewBasePrivate> releaseEmojiChooserTimer;

What happens if the WebKitWebViewBase is destroyed before the timer fires? I
assume the callback will not execute. So we will not gtk_widget_destroy() the
emoji chooser in that case. But that should be fine, right? Because it's
pointing relative to the WebKitWebViewBase, it should be disposed when the
parent widget is destroyed?

What happens if the WebKitWebViewBase is destroyed before the
emojiChooserCompletionHandler is called, i.e. when
emojiChooserCompletionHandler is nonnull at time of destruction? I think the
CompletionHandler destructor will assert and crash the UI process, right? We
need to make sure the CompletionHandler is guaranteed to be called.

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:1797
> +    priv->releaseEmojiChooserTimer.startOneShot(2_min);

Irrelevant: I love this timer syntax. You can't screw up the units!