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

[Jean-Paul Saman]

2011/5/23 Sergey Radionov <rsatom at gmail.com>:
> 23.05.2011 15:20, Jean-Paul Saman пишет:
>> On Sun, May 22, 2011 at 4:10 PM, Sergey Radionov<rsatom at gmail.com>  wrote:

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

I do not see the use case for this from the above description.

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

It looks to me as duplication, so I would like to move the
libvlc_event/handle_event code
to VLCPlugin objects. Since that wraps libVLC code for the webplugins.

In the fullscreen window implementation you need a pointer to the
VLCPlugin instance, hence your defintion of:

+typedef VlcPlugin VLCPlugin;
+
+inline libvlc_media_player_t* getMD(VLCPlugin* p_instance)
+{
+	return p_instance->getMD();
+}

Why not make it integral part of the fullscreen class?

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

Ah, you use the return value of getMD() to decide if the FS button has
to perform an action.
Maybe you should remove this part of the method entirely

         if( !libvlc_media_player )
         {
-             libvlc_printerr("no mediaplayer");
+             //libvlc_printerr("no mediaplayer");
         }

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

ah, ok

Kind regards,
Jean-Paul Saman.