Bug 305040 – [GTK][Regression] Some emoji letters rendered as "missing glyph"

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Milan Crha]

Using `MiniBrowser --editor-mode` from

  webkit2gtk4.1-2.50.4-1.fc43.x86_64
  webkitgtk6.0-2.50.4-1.fc43.x86_64

Paste the below emojis into the address bar, where they draw properly, then paste them into the body, where them both draw as a "missing glyph" instead:

🚀️🙃️

Using "Insert Emoji" context menu option also results into the "missing glyph" character.

[Michael Catanzaro]

I didn't know the editor mode existed. That's amazing.

[Jeff Fortin]

There might be a random component to triggering this: https://gitlab.gnome.org/GNOME/evolution/-/issues/3197#note_2694039

[Milan Crha]

Created attachment 479063 [details]
proposed patch

[Milan Crha]

Created attachment 479064 [details]
cairo part patch, but untested

It seemed to find also a flaw in the cairo part of the code, but it seems the Fedora builds use Skia these days, thus I could not test it. I'm adding it here in case it would be of any interest for you.

[Michael Catanzaro]

Comment on attachment 479063 [details]
proposed patch

Huh. Good job.

Let's take the Skia-only patch. I'm not brave enough to land the untested cairo changes. (Nowadays, cairo is only used on Windows, not on Linux.)

Although we normally commit patches via pull requests nowadays, I think it's still possible to set the r+ and cq+ Bugzilla flags to land your patch. I'll ask Carlos Garcia to review this first, though, since he's familiar with the fonts code. But it sure looks plausible.

[Milan Crha]

It at least helped with my test case. Is it possible to compile with cairo? If there's an easy way to do it on Linux I can give it a try.

[Michael Catanzaro]

The webkitglib/2.50 branch still supports building with -DUSE_CAIRO=ON. You could test it there.

But since 2.52, we only support Skia on Linux. Cairo is still supported, but only for Windows and PlayStation. So some build system hacking will likely be required.

[Carlos Garcia Campos]

Comment on attachment 479063 [details]
proposed patch

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

> Source/WebCore/platform/graphics/skia/SkiaHarfBuzzFont.cpp:129
> +    , m_isColorBitmapFont(FontPlatformData::skiaTypefaceHasAnySupportedColorTable(typeface))

With this we are calling skiaTypefaceHasAnySupportedColorTable twice when the SkiaHarfBuzzFont is created. So, I would leave this here, and then add a bool isColorBitmapFont to SkiaHarfBuzzFont, then in FontPlatformData::platformDataInit we can initialize m_isColorBitmapFont from the m_hbFont without calling skiaTypefaceHasAnySupportedColorTable again.

[Milan Crha]

Thanks for the pointer. The cairo version seems to work fine without modification. By the way, it required both -DUSE_CAIRO=ON -DUSE_SKIA=OFF to take into effect in the cmakeconfig.h

[Milan Crha]

Created attachment 479084 [details]
updated proposed patch

Here you are (it would be quicker, but compiling WebKitGTK takes considerable time here)

[Carlos Garcia Campos]

Comment on attachment 479084 [details]
updated proposed patch

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

This patch is missing the m_isColorBitmapFont initialization on SkiaHarfBuzzFont constructor

> Source/WebCore/platform/graphics/skia/SkiaHarfBuzzFont.h:52
> +    void setIsColorBitmapFont(bool isColorBitmapFont) { m_isColorBitmapFont = isColorBitmapFont; }

make the method const

[Milan Crha]

> This patch is missing the m_isColorBitmapFont initialization on SkiaHarfBuzzFont constructor

Despite that it works, at least the test data I tried; is it weird? ;)

[Milan Crha]

> make the method const

A setter, which changes internal data, being `const`? I do not follow C++ development closely, I'm many years behind, but a const setter still does not look right.

[Milan Crha]

Created attachment 479087 [details]
proposed patch ]I[

[Carlos Garcia Campos]

(In reply to Milan Crha from comment #13)
> > make the method const
> 
> A setter, which changes internal data, being `const`? I do not follow C++
> development closely, I'm many years behind, but a const setter still does
> not look right.

Sorry, I misunderstood the patch, because I proposed the opposite, keep the original patch that initializes the m_isColorBitmapFont in SkiaHarfBuzzFont constructor and add a getter to initialize m_isColorBitmapFont in FontPlatformData::platformDataInit. I read fast and assumed this was the getter that's why I proposed the const.

[Carlos Garcia Campos]

Comment on attachment 479087 [details]
proposed patch ]I[

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

> Source/WebCore/platform/graphics/skia/FontPlatformDataSkia.cpp:139
> +    m_hbFont->setIsColorBitmapFont(m_isColorBitmapFont);

I think my proposal is better, because in this case we are always calling skiaTypefaceHasAnySupportedColorTable even if the font already exists, but moving it to the SkiaHarfBuzzFont constructor it's called only once when the font is created.

[Milan Crha]

Created attachment 479088 [details]
proposed patch IV

Then maybe this?

[Carlos Garcia Campos]

Comment on attachment 479088 [details]
proposed patch IV

Exactly. Thanks!

[EWS]

Committed 311353@main (58d943e440ad): <https://commits.webkit.org/311353@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 479088 [details].

[Carlos Alberto Lopez Perez]

(In reply to Milan Crha from comment #17)
> Created attachment 479088 [details]
> proposed patch IV
> 
> Then maybe this?

The landed patch has an author of "Claude Opus 4.6 (1M context)" <[e-mail]>"  -> https://github.com/WebKit/WebKit/commit/58d943e440ad366a0a117556b5dd75dd8879cec3.patch
Which matches what has been uploaded here: https://bug-305040-attachments.webkit.org/attachment.cgi?id=479088

@Milan Crha: Please next time edit the author of the patch before uploading it here for review.
One thing is to add a "Co-Authored-By: Claude" line and another thing is to have the patch to be completely authored by Claude. I don't think the last is something acceptable.

[Milan Crha]

I would do that, but the truth is that I did not touch the code at all, it did all of the coding on its own, I only passed instructions forth and the code back here. I cannot be credited for other than postman work here ;)

[Michael Catanzaro]

Since WebKit doesn't have any AI contribution policy, we have a free-for-all currently. In the absence of any policy, I think Milan's approach makes sense. If it was an AI-assisted patch, use Co-Authored-By, sure. But when Claude itself writes the entire commit, might as well let it make itself the author.

P.S. Since there is no GitHub pull request to add our backport label to, I am planning to backport this commit manually.

[Michael Catanzaro]

I will note that some developers are now using AI without disclosing it in the commit message at all. Some sort of policy sure would be nice.

[Carlos Alberto Lopez Perez]

(In reply to Milan Crha from comment #21)
> I would do that, but the truth is that I did not touch the code at all, it
> did all of the coding on its own, I only passed instructions forth and the
> code back here. I cannot be credited for other than postman work here ;)

Did you gave the instructions to Claude to write the patch and iterated with it until the solution was acceptable?
Did you reviewed the code changes that Claude made and then did you understood those changes?
Did you tested the patch and verified that it works?

If the answer to any of this is yes then I think you co-authored the patch and you should be on the commit log of it.

[Alberto Garcia]

My opinion about this.

A patch can be produced in a variety of ways:

- A person decides what to change and how, and writes everything by hand.
- A person tells a tool what to do, and the tool generates the code. The person may or may not tweak the generated output. This would be the case for LLMs, but also for other tools like Coccinelle (or even a sophisticated sed command or similar).
- A tool suggests the change, possibly even indicating exactly what to change. This applies to compiler warnings and errors, and also to static analyzers like cppcheck, coverity, rust-clippy, ...

In all cases (at least with the current workflows) there is a human who decides that the patch is correct and uploads it so it can be merged.

I think that the person who does that should take full responsibility for the code, understand it, and be prepared to answer questions and respond to review requests.

If the change was suggested by a different person, the uploader can decide to credit that person. Some projects use a Suggested-By tag, but even in WebKit there are plenty of "this change was suggested by ..." phrases in the commit messages, also for changes suggested by static analyzers.

This is what I would propose cases like this: a patch authored by Milan (in this example) with "Change suggested by Carlos Garcia Campos" in the commit message.

[Milan Crha]

Okay, I did two things here, I've been a postman, and I tested the change. I did tell (Jean) Claude what the problem is and such, but it was solely its work. For me, as a coder, the code I write can be credited to me, not much else, especially when it comes to patches. Do I understand what the patch does? No way.

I understand the argument of "the tool did it, not a human", and I am aware it's just a tool and responsibility comes to the human beings instead, the tool can (and should) be mentioned when it did "significant or non-trivial part of the work", but the tool cannot be responsible for anything. I like the comparison to a Libreoffice Writer, the document it produced by a human pressing the keys is of that human not Writer's. Writer is only a tool, helping the human to achieve its goal.

Trying to credit each and every person involved in the route to the final patch feels like an exhausting process. You can credit each reviewer as Co-Authored, after all the reviewer says what to do, unless the patch is accepted on the first shot, but really, it's unbearable. In case I'd like a reporter to test the change, because something does not trigger on my machine, I should, theoretically, credit also the tester(s). Then I should credit the tools, the text editor I use to edit the code ;) And so on, and so on. It's very complicated, to say the least, and kinda unrealistic, at least for me.

This is kinda philosophic question :)

Nonetheless, I understand what the preference here is, thus in case I'd provide any patches with the cooperation with the/any-such tool I will credit myself as the author with some appropriate note in the commit message (we've been told to use `Assisted-by: ....` lines). I mean, do not worry, I'll do better the next time.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #22)
> P.S. Since there is no GitHub pull request to add our backport label to, I
> am planning to backport this commit manually.

I see Adrian already did so for 2.52.3. Thanks! So Evolution users should benefit immediately.