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

[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.

---
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.

---
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)

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).

---
4) Please split your patch in:

- fullscreen for activex
- fullscreen for mozilla
- test page

---
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
 }

---
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;
     }

---
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; };



Kind Regards,
Jean-Paul Saman