[vlc-devel] [PATCH] ActiveX/Mozilla FullScreen support

[Sergey Radionov]

23.05.2011 15:20, Jean-Paul Saman пишет:
> Thank you for submitting a patch for fullscreen window for VLC ActiveX
> and mozilla plugin.
>
> On Sun, May 22, 2011 at 4:10 PM, Sergey Radionov<rsatom at gmail.com>  wrote:
>> based on git://git.videolan.org/vlc/vlc-1.1.git
>>
>> P.S. This is my first patch, so, may be some mistakes...
>
> Some review comments and questions:
>
> 1) New files should NOT be in the parent directory of
> projects/activex, but in project/activex itself:
>
>   create mode 100644 projects/axVLCFullScreenWnd.h
>   create mode 100644 projects/defullscreen.bmp
>   create mode 100644 projects/new_message.bmp
>   create mode 100644 projects/npVLCFullScreenWnd.h
>   create mode 100644 projects/pause.bmp
>   create mode 100644 projects/play.bmp
>   create mode 100644 projects/volume-muted.bmp
>   create mode 100644 projects/volume.bmp
>
> These files are going to be part of VLC activex webplugin and should
> therefor be part of project/activex directory:
>
> projects/activex/axVLCFullScreenWnd.h
> projects/activex/defullscreen.bmp
> projects/activex/new_message.bmp
> projects/activex/npVLCFullScreenWnd.h
> projects/activex/pause.bmp
> projects/activex/play.bmp
> projects/activex/volume-muted.bmp
> projects/activex/volume.bmp
>
> It means that the BMP need to be added to the mozilla directory too.
> Although it is duplication of these files, it is necessary since
> activex and mozilla directories are split of in separate repositories
> for vlc-1.2.0-git.
>
Accepted. I will move it.

> ---
> 2) What is the purpose of:
>
> +    STDMETHODIMP get_NewMessageFlag(VARIANT_BOOL* vYes);
> +    STDMETHODIMP put_NewMessageFlag(VARIANT_BOOL vYes);
>
> (I have the impression it is debug code.)
>
> The methods extent the JavaScript interface and its usage is not clear
> nor its purpose.
> Please explain.
>
This methods need for possibility of inform user (in fullscreen mode) about some events wich need 
user attention (but I forget add changed idl file for it). It needed by project for which I made 
fullscreen support in VLC. I can remove this, if no need in this function.

> ---
> 3) The methods (on the fullscreen window for activex AND mozilla):
>
> +void VLCFullScreenWnd::RegisterEvents()
> +void VLCFullScreenWnd::UnRegisterEvents()
> +void VLCFullScreenWnd::handle_input_state_event(const libvlc_event_t*
> event, void *param)
>
no, it not duplication. FullScreen window is exactly the same client to vlc lib, as VLCPlugin. I 
made this to minimize changes in main code.

> are duplication of methods in file plugin.cpp (for mozilla the file is
> vlcplugin.cpp):
>
> void VLCPlugin::player_register_events()
> void VLCPlugin::player_unregister_events()
> static void handle_input_state_event(const libvlc_event_t* event, void *param)
>
> The fullscreen window should refer to the object VLCPlugin (which
> wraps libvlc access) instead of duplicating its
> functionality. In case of fullscreen a messsage should also be sent to
> the objects in the fullscreen window (if appropriate).
I think, this is not nessesary complication, which have no any advantages, but can bring some new 
bugs. But, may be, I don't understand something :)

>
> ---
> 4) Please split your patch in:
>
> - fullscreen for activex
> - fullscreen for mozilla
> - test page
>
ok, accepted.

> ---
> 5) File: projects/mozilla/vlcplugin.cpp. Please revert this part of
> the patch the compiler warning is deliberate:
>
> @@ -217,7 +249,7 @@ void VlcPlugin::event_callback(const
> libvlc_event_t* event, void *param)
>       plugin->events.callback(event);
>       NPN_PluginThreadAsyncCall(plugin->getBrowser(), eventAsync, plugin);
>   #else
> -#warning NPN_PluginThreadAsyncCall not implemented yet.
> +//#warning NPN_PluginThreadAsyncCall not implemented yet.
>       printf("No NPN_PluginThreadAsyncCall(), doing nothing.");
>   #endif
>   }
>
Ok, accepted.

> ---
> 6) File: projects/mozilla/vlcplugin.h
>
> Printing an error in this case is necessary IMHO, so please revert this part:
>
> @@ -200,7 +204,7 @@ public:
>       {
>           if( !libvlc_media_player )
>           {
> -             libvlc_printerr("no mediaplayer");
> +             //libvlc_printerr("no mediaplayer");
>           }
>           return libvlc_media_player;
>       }
>
I use this function too often in fullscreen window, and if no comment, it will be too mach warnings.

> ---
> 7)  File: projects/mozilla/vlcplugin.h
>
> NO functional change here, so revert this part:
>
> @@ -209,8 +213,8 @@ public:
>       char*               getAbsoluteURL(const char *url);
>       NPWindow&            getWindow()
>                               { return npwindow; };
> -    void                setWindow(const NPWindow&window)
> -                            { npwindow = window; };
> +    void                setWindow(const NPWindow&window);
> +                            //{ npwindow = window; };
>
>       NPClass*            getScriptClass()
>                               { return p_scriptClass; };
>
>
I move body of this function to cpp. It become too big for .h file.

>
> Kind Regards,
> Jean-Paul Saman
> _______________________________________________
> vlc-devel mailing list
> To unsubscribe or modify your subscription options:
> http://mailman.videolan.org/listinfo/vlc-devel
>

-- 
With best wishes,
Sergey Radionov