[webkit-reviews] review denied: [Bug 76181] [GTK] FullScreen signals : [Attachment 123118] Patch

[bugzilla-daemon at webkit.org]

Xan Lopez <xan.lopez at gmail.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 76181: [GTK] FullScreen signals
https://bugs.webkit.org/show_bug.cgi?id=76181

Attachment 123118: Patch
https://bugs.webkit.org/attachment.cgi?id=123118&action=review

------- Additional Comments from Xan Lopez <xan.lopez at gmail.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=123118&action=review


r- at least for the accumulators used.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:932
> +	   return;

If the handler handled the signal it will return TRUE, so I still think if you
need to flip the boolean here. Perhaps this is not working correctly because
you are using the wrong accumulator.

> Source/WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:951
> +    g_signal_emit_by_name(m_webView, "leaving-fullscreen", &returnValue);

You need to check the return value and do nothing if it's TRUE, per what the
documentation says.

> Source/WebKit/gtk/tests/testwebview.c:478
> +    g_test_add_data_func("/webkit/webview/fullscreen", (gconstpointer) TRUE,
test_webkit_web_view_fullscreen);

No spaces before the cast?

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2638
> +	*

FALSE to block the event? Usually TRUE = hanlded ("block the event"), FALSE =
continue emission. So I think you are saying the same thing twice basically.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2646
> +			    g_signal_accumulator_first_wins, 0,

I think you want true_handled, not first_wins.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:2670
> +			    g_signal_accumulator_first_wins, 0,

Same.