Re: [1/2] shdocvw: Added support for IAdviseSink (resend)

[Jacek Caban]

Hi Alistair,

Alistair Leslie-Hughes wrote:
> Hi,
>
> Could I get some feedback on this patch please?

+    HRESULT hr = E_INVALIDARG;
+
+    TRACE("(%p)->(%p, %p)\n", This, pAdvSink, pdwConnection);
+
+    if(!pdwConnection || !pAdvSink)
+        return hr;
+
+    if(!This->holder)
+    {
+        hr = CreateOleAdviseHolder(&This->holder);
+        if(FAILED(hr))
+            ERR("CreateOleAdviseHolder failed\n");
+    }
+
+    if(hr == S_OK && This->holder)
+    {
+        hr = IOleAdviseHolder_Advise(This->holder, pAdvSink, pdwConnection);
+    }
+
+    return hr;

It's a matter of taste, but you complicate things here. You could 
achieve the same effect easier:

TRACE("(%p)->(%p, %p)\n", This, pAdvSink, pdwConnection);

if(!pdwConnection || !pAdvSink)
        return E_INVALIDARG;

if(!This->holder) {
        HRESULT hr = CreateOleAdviseHolder(&This->holder);
        if(FAILED(hr)) {
            ERR("CreateOleAdviseHolder failed\n");
            return hr;
        }
    }

return IOleAdviseHolder_Advise(This->holder, pAdvSink, pdwConnection);

Also you don't really add support for IAdviseSink. You only store and 
never use it and your test don't document missing functionality. It 
would be nice not to call Unadvise just after Advise, but keep it 
advised and properly test (with todo_wine for now). It's easy to do by 
adding something like
trace(#func "\n"); \
to CHECK_EXPECT2 and running the test on Windows. Then you can easily 
guess when the callback should be called and add corresponding 
SET_EXPECT/CHECK_CALLED macros. Note that, although it's not needed for 
tests to success, these macros are kept in order of calling as much as 
possible for documentation purposes.

If you're not going to extend tests, please leave FIXME messages. 
Otherwise you hide the problem with no evidence.

Thanks,
    Jacek