[webkit-reviews] review denied: [Bug 173598] [GTK][WAYLAND] Create WaylandCompositorDisplay unconditionally when initializing the WebProcess : [Attachment 313397] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied Miguel Gomez
<magomez at igalia.com>'s request for review:
Bug 173598: [GTK][WAYLAND] Create WaylandCompositorDisplay unconditionally when
initializing the WebProcess
https://bugs.webkit.org/show_bug.cgi?id=173598

Attachment 313397: Patch

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




--- Comment #3 from Carlos Garcia Campos <cgarcia at igalia.com> ---
Comment on attachment 313397
  --> https://bugs.webkit.org/attachment.cgi?id=313397
Patch

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

> Source/WebKit2/ChangeLog:8
> +	   Move WaylandCompositorDisplay code to its own files to it can be
used from other clases. Then, instead of

to it -> so it
clases -> classes

>> Source/WebKit2/WebProcess/WebProcess.cpp:318
>>  #endif
> 
> Can this be done in platformInitializeWebProcess()? Currently the GTK+ and
WPE ports are building WebProcessSoup.cpp, where that method is defined, and
it's soup-specific. But IMO the implementation file should be GLib-specific,
with m_waylandCompositorDisplay initialization guarded with PLATFORM(WAYLAND).

I guess PLATFORM(WAYLAND) is not enabled in WPE

>> Source/WebKit2/WebProcess/WebProcess.h:35
>> +#include "WaylandCompositorDisplay.h"
> 
> This needs to be guarded with PLATFORM(WAYLAND). Furthermore, it should be
possible to just forward-declare the WaylandCompositorDisplay class, and
include the header only in the implementation file.

Yes, better forward declare it.

> Source/WebKit2/WebProcess/gtk/WaylandCompositorDisplay.cpp:37
> +std::unique_ptr<WaylandCompositorDisplay>
WaylandCompositorDisplay::create(String displayName)

const String&

> Source/WebKit2/WebProcess/gtk/WaylandCompositorDisplay.h:30
> +#include "WebPage.h"

You can probably forward declare this.

> Source/WebKit2/WebProcess/gtk/WaylandCompositorDisplay.h:39
> +    static std::unique_ptr<WaylandCompositorDisplay> create(String);

I'm not sure if you also have to add a destructor for the smart pointers, even
if it's just = default.