Re: Patch feedback requested for OleCreatePropertyFrame()

[Geoffrey Hausheer]

Ok, I have reimplemented OleCreatePropertyFrame() from scratch.  There
is no code from Hidenori's patch in mine, nor did I use it as a
reference (though of course I can't claim that I didn't learn anything
from his patch during my previous cleanup)

Would someone mind reviewing this for style/content?  I still have no
idea how to write a test for a modal dialog, so if a test is required
for submission, I'd really appreciate someone giving me pointers on
how to go about it.

There are some features I didn't implement because I didn't really
understand MDSN sufficiently (specifcially the IPropertyPageSite
stuff), but it is mostly complete.

.Geoff

Patch feedback requested for OleCreatePropertyFrame()

[Wolfram Sang]

Geoffrey Hausheer wrote:
> Would someone mind reviewing this for style/content?  I still have no

Can't say much about the content, but stylewise the pointer handling
could be improved. For example,

+    if ( (iface==0) || (ppvObject==0) )

should at least use NULL instead of 0 as both are pointers, but !ptr is
even better here. Accordingly

+        *ppvObject = 0;

should assign NULL.

Also pointer declaration is not consistent, note the asterisk:

+  OLEPropertyPageSite *this = (OLEPropertyPageSite *)iface;

[...]

+  IPropertyPageSite*  iface,

(and there are two spaces here)

I don't know which way is preferred, but for readability it should be
the same throughout the file.

+    if (IsEqualGUID(&IID_IUnknown, riid))
+        *ppvObject = iface;
+    else if (IsEqualGUID(&IID_IPropertyPageSite, riid))
+        *ppvObject = iface;

if (a || b) ?

+static ULONG WINAPI PPS_Release(IPropertyPageSite* iface)
+{
+  OLEPropertyPageSite *this = (OLEPropertyPageSite *)iface;
+  ULONG ret;
+  TRACE("(%p)->(ref=%d)\n", this, this->ref);
+
+  /* Decrease the reference count for current interface */
+  ret = InterlockedDecrement(&this->ref);
+
+  return ret;
+}

ret is not needed here.

+    /* This was expected to work but didn't:
+    LONG baseunits = GetDialogBaseUnits();
+    basex = LOWORD(baseunits);
+    basey = HIWORD(baseunits);
+    */

Don't know the common wine-sense here, but without syntax-highlighting,
I think it is easy to miss that this code is disabled. I'd suggest

/*
  *
  */

commenting or something similar which is immediately visible.

Those are my 2 cents, thanks for the work!

Regards,

Wolfram

Patch feedback requested for OleCreatePropertyFrame()

[Rob Shearman]

2010/1/8 Wolfram Sang :
> Geoffrey Hausheer wrote:
> > Would someone mind reviewing this for style/content?  I still have no
>
> Can't say much about the content, but stylewise the pointer handling
> could be improved. For example,
> - if ( (iface==0) || (ppvObject==0) )
>
> should at least use NULL instead of 0 as both are pointers, but !ptr is
> even better here. Accordingly

iface shouldn't generally be checked for NULL unless a program depends
on it and a testcase shows this behaviour in native.