[webkit-reviews] review denied: [Bug 27713] WINCE port: make plugin work for wince : [Attachment 33545] the patch

[bugzilla-daemon at webkit.org]

George Staikos <staikos at kde.org> has denied Yong Li <yong.li at torchmobile.com>'s
request for review:
Bug 27713: WINCE port: make plugin work for wince
https://bugs.webkit.org/show_bug.cgi?id=27713

Attachment 33545: the patch
https://bugs.webkit.org/attachment.cgi?id=33545&action=review

------- Additional Comments from George Staikos <staikos at kde.org>

> +DWORD SHGetValue(HKEY hkey, LPCWSTR pszSubKey, LPCWSTR pszValue, LPDWORD
pdwType, LPVOID pvData, LPDWORD pcbData)
> +{
> +    HKEY key;
> +	if (RegOpenKeyEx(hkey, pszSubKey, 0, 0, &key) == ERROR_SUCCESS) {
> +	   DWORD result = RegQueryValueEx(key, pszValue, 0, pdwType,
(LPBYTE)pvData, pcbData);
> +	   RegCloseKey(key);
> +	   return result;
> +	}
> +    return ERROR_INVALID_NAME;
> +}

  Tabs should be removed.


>      HKEY key;
> +#if PLATFORM(WINCE)
> +    HRESULT result = RegOpenKeyExW(rootKey, L"Software\\MozillaPlugins", 0,
0, &key);
> +#else
>      HRESULT result = RegOpenKeyExW(rootKey, L"Software\\MozillaPlugins", 0,
KEY_ENUMERATE_SUB_KEYS, &key);
> +#endif

   Does it make more sense to #define KEY_ENUMERATE_SUB_KEYS if it's not
available?

>  static inline void addWindowsMediaPlayerPluginDirectory(Vector<String>&
directories)
>  {
> +#if PLATFORM(WINCE)
> +#else
   Change to #if !PLATFORM(WINCE)

>  static inline void addMacromediaPluginDirectories(Vector<String>&
directories)
>  {
> +#if PLATFORM(WINCE)
> +    directories.append("\\Windows\\System\\macromed\\Flash");
> +#else

   Is this valid?

> -    NP_Initialize = (NP_InitializeFuncPtr)GetProcAddress(m_module,
"NP_Initialize");
> -    NP_GetEntryPoints = (NP_GetEntryPointsFuncPtr)GetProcAddress(m_module,
"NP_GetEntryPoints");
> -    m_NPP_Shutdown = (NPP_ShutdownProcPtr)GetProcAddress(m_module,
"NP_Shutdown");
> +    NP_Initialize = (NP_InitializeFuncPtr)GetProcAddress(m_module,
_T("NP_Initialize"));
> +    NP_GetEntryPoints = (NP_GetEntryPointsFuncPtr)GetProcAddress(m_module,
_T("NP_GetEntryPoints"));
> +    m_NPP_Shutdown = (NPP_ShutdownProcPtr)GetProcAddress(m_module,
_T("NP_Shutdown"));

   Will this not affect the Win32 build?
 
> @@ -264,6 +284,10 @@ static bool isWindowsMessageUserGesture(UINT message)
>  LRESULT
>  PluginView::wndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
>  {
> +#if PLATFORM(WINCE)
> +    if (!m_pluginWndProc)
> +	   return 1;
> +#endif

   Can this be unconditional on WINCE?

> +#if PLATFORM(WINCE)
> +    m_windowRect = frameView->contentsToWindow(frameRect());
> +#else
>      m_windowRect =
IntRect(frameView->contentsToWindow(frameRect().location()),
frameRect().size());
> +#endif

   The difference here concerns me.

> @@ -491,15 +525,24 @@ void PluginView::paint(GraphicsContext* context, const
IntRect& rect)
>      m_npWindow.type = NPWindowTypeDrawable;
>      m_npWindow.window = hdc;
>  
> -    IntPoint p =
static_cast<FrameView*>(parent())->contentsToWindow(frameRect().location());
> -    
>      WINDOWPOS windowpos;
>      memset(&windowpos, 0, sizeof(windowpos));
>  
> +#if PLATFORM(WINCE)
> +    IntRect r =
static_cast<FrameView*>(parent())->contentsToWindow(frameRect());
> +    
> +    windowpos.x = r.x();
> +    windowpos.y = r.y();
> +    windowpos.cx = r.width();
> +    windowpos.cy = r.height();
> +#else
> +    IntPoint p =
static_cast<FrameView*>(parent())->contentsToWindow(frameRect().location());
> +    
>      windowpos.x = p.x();
>      windowpos.y = p.y();
>      windowpos.cx = frameRect().width();
>      windowpos.cy = frameRect().height();
> +#endif

  Likewise.  Why are the coordinates behaving so differently?
> @@ -697,14 +770,24 @@ void PluginView::stop()
>  
>      ASSERT(m_streams.isEmpty());
>  
> +    if (m_manualStream)
> +	   m_manualStream->stop();
> +
>      m_isStarted = false;

   This looks like it will affect the Win32 case.  Should probably be checked
in separately if it's valid there too.

>  #if PLATFORM(WIN_OS) && PLATFORM(QT)
>	   m_window = window;
>  #else
>	   setPlatformWidget(window);
> +	   m_isWindowed = true;
>  #endif

   Same here.