[webkit-reviews] review denied: [Bug 117220] Allow for toggling fullscreen on <video> elements : [Attachment 203989] Patch

[bugzilla-daemon at webkit.org]

Carlos Garcia Campos <cgarcia at igalia.com> has denied  review:
Bug 117220: Allow for toggling fullscreen on <video> elements
https://bugs.webkit.org/show_bug.cgi?id=117220

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

------- Additional Comments from Carlos Garcia Campos <cgarcia at igalia.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=203989&action=review


Thanks for fixing the GTK+ bits, but unfortunately it breaks WebKit1 GTK+ API
and adds new API to WebKit2 GTK+. This should be reviewed by at least 2 GTK+
reviewers. r- because of the API break.

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:72
> + * @WEBKIT_CONTEXT_MENU_ACTION_EXIT_VIDEO_FULLSCREEN: Exit current video
element in fullscreen mode.

This is new API, could you please move this to the end of the enum and append
Since 2.2 before landing?

@WEBKIT_CONTEXT_MENU_ACTION_EXIT_VIDEO_FULLSCREEN: Exit current video element
in fullscreen mode. Since 2.2

> Source/WebKit2/UIProcess/API/gtk/WebKitContextMenuActions.h:122
> +    WEBKIT_CONTEXT_MENU_ACTION_EXIT_VIDEO_FULLSCREEN,

Move this to the end please.

> Source/WebKit/gtk/webkit/webkitglobals.h:95
> - * @WEBKIT_CONTEXT_MENU_ACTION_ENTER_VIDEO_FULLSCREEN: Show current video
element in fullscreen mode.
> + * @WEBKIT_CONTEXT_MENU_ACTION_TOGGLE_VIDEO_FULLSCREEN: Show current video
element in fullscreen mode.

This is an API break.

> Source/WebKit/gtk/webkit/webkitglobals.h:140
> -    WEBKIT_CONTEXT_MENU_ACTION_ENTER_VIDEO_FULLSCREEN,
> +    WEBKIT_CONTEXT_MENU_ACTION_TOGGLE_VIDEO_FULLSCREEN,

Isn't it possible to do the same than in wk2, instead of renaming? Otherwise we
need to deprecate the ENTER one and add TOGGLE at the end.