Bug 224149 - [GTK] Properly use CompletionHandler when USE_OPENGL_OR_ES is set to OFF
Summary: [GTK] Properly use CompletionHandler when USE_OPENGL_OR_ES is set to OFF
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-04-03 06:37 PDT by Charlene Wendling
Modified: 2021-04-10 15:03 PDT (History)
12 users (show)

See Also:


Attachments
Patch (1.83 KB, patch)
2021-04-03 06:38 PDT, Charlene Wendling
Hironori.Fujii: review+
Hironori.Fujii: commit-queue-
Details | Formatted Diff | Diff
Revised patch (1.66 KB, patch)
2021-04-05 14:20 PDT, Charlene Wendling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Charlene Wendling 2021-04-03 06:37:30 PDT
Properly use CompletionHandler when USE_OPENGL_OR_ES is set to OFF
Comment 1 Charlene Wendling 2021-04-03 06:38:36 PDT
Created attachment 425093 [details]
Patch
Comment 2 Charlene Wendling 2021-04-03 06:41:45 PDT
When trying to build WebKit with USE_OPENGL_OR_ES set to off, the build fails with:

LayerTreeHost.h:216:28: error: out-of-line definition of 'forceRepaintAsync'
+does not match any declaration in 'WebKit::LayerTreeHost'
LayerTreeHost.h:82:28: note: type of 1st parameter of member declaration does
not match definition ('CompletionHandler<void ()> &&' vs
'CompletionHandler<void ()> &')

It has been introduced by that change : https://trac.webkit.org/changeset/271171/webkit

The attached patch fixes that issue.
Comment 3 Alex Christensen 2021-04-05 08:56:25 PDT
Comment on attachment 425093 [details]
Patch

You might consider calling it.
Comment 4 Fujii Hironori 2021-04-05 13:32:16 PDT
Comment on attachment 425093 [details]
Patch

style-checker failed. Please fix it. Otherwise, commit bot will complain.
Comment 5 Charlene Wendling 2021-04-05 14:20:50 PDT
Created attachment 425205 [details]
Revised patch

Here is it is. It passes check-webkit-style this time.
Comment 6 Alex Christensen 2021-04-05 18:34:20 PDT
Comment on attachment 425205 [details]
Revised patch

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

> Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/LayerTreeHost.h:216
> +inline void LayerTreeHost::forceRepaintAsync(CompletionHandler<void()>&&) { }

I recommend calling the completion handler.
Comment 7 Fujii Hironori 2021-04-07 13:18:30 PDT
Charlene, what's your opinion on Alex's review point?
I think it's just a stub to avoid linkage errors, not called from anywhere. I think it's no problem. 
Adding ASSERT_NOT_REACHED() might be another idea.
inline void LayerTreeHost::forceRepaintAsync(CompletionHandler<void()>&&) { ASSERT_NOT_REACHED(); }
Comment 8 Charlene Wendling 2021-04-08 00:29:42 PDT
(In reply to Fujii Hironori from comment #7)
> Charlene, what's your opinion on Alex's review point?
> I think it's just a stub to avoid linkage errors, not called from anywhere.
> I think it's no problem. 
> Adding ASSERT_NOT_REACHED() might be another idea.
> inline void LayerTreeHost::forceRepaintAsync(CompletionHandler<void()>&&) {
> ASSERT_NOT_REACHED(); }

In fact, while i reported the build failure and proposed a possible fix, i'm not able to define what would be the proper course of action.
Comment 9 EWS 2021-04-10 15:02:57 PDT
Committed r275802 (236373@main): <https://commits.webkit.org/236373@main>

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