Re: mshtml: Added IHTMLTable::width property implementation.

[Jacek Caban]

Hi Zhenbo,

On 03/31/14 16:26, Zhenbo Li wrote:
> As my test case shows, windows would truncate float numbers.
> I considered VarInt() in oleaut32.dll, but I think to declare
> var2str_length() is more clear.

+static HRESULT var2str_length(nsAString *nsstr, const VARIANT *p)
+{
+    static WCHAR buf[64];
+    static const WCHAR formatW[] = {'%','d',0};
+    LONG width;
+
+    switch(V_VT(p)) {
+    case VT_BSTR:
+        return nsAString_Init(nsstr, V_BSTR(p));
+    case VT_R8:
+        width = (LONG)(V_R8(p));
+        break;
+    case VT_R4:
+        width = (LONG)(V_R4(p));
+        break;
+    case VT_I4:
+        width = V_I4(p);
+        break;
+    default:
+        FIXME("unsupported arg %s\n", debugstr_variant(p));
+        return E_NOTIMPL;
+    }
+    sprintfW(buf, formatW, width);
+    return nsAString_Init(nsstr, buf);
+}

I would like to see this more generic. How about having var_to_nsstr? It seems to me that we don't really have to convert floats here. Gecko and/or getter may do that instead. If we really have to it here, then some sort of flags controlling helper behaviour on floats may be cleaner.

+    nsAString val;
+    HRESULT nsres;

nsresult and HRESULT, although are very similar, are two different things and should not be mixed.

+    DOUBLE width;
+
+    TRACE("(%p)->(%p)\n", This, p);
+    nsAString_Init(&val, NULL);
+    nsres = nsIDOMHTMLTableElement_GetWidth(This->nstable, &val);
+
+    V_VT(p) = VT_BSTR;
+    nsres = return_nsstr(nsres, &val, &V_BSTR(p));

Please don't use return_nsstr this way. It's a strange helper with unusual behaviour in terms of freeing input parameters to make common Gecko wrapping functions cleaner. Using this for string conversion looks bad. Esp in this case, where it adds unneeded BSTR allocation in cases where the string is converted to int later. However, see bellow first.

+    if (nsres != S_OK)
+        return nsres;
+
+    nsres = VarR8FromStr(V_BSTR(p), 0, 0, &width);
+    if (nsres == S_OK){
+        VariantClear(p);
+        V_VT(p) = VT_I4;
+        V_I4(p) = (LONG)width;
+    }
+    return S_OK;

+#define cmp_length(a,b) _cmp_length(__LINE__,a,b)
+static void _cmp_length(unsigned line, VARIANT got, const char * exstr)
+{
+    static char buf[64];
+    ok_(__FILE__,line) (V_VT(&got)==VT_BSTR || V_VT(&got)==VT_I4,
+                        "V_VT is %hu, expected VT_BSTR(%hu) or VT_I4(%hu)\n",
+                        V_VT(&got), VT_BSTR, VT_I4);
+
+    if (V_VT(&got) == VT_BSTR){
+        ok_(__FILE__,line) (!strcmp_wa(V_BSTR(&got), exstr),
+            "Expected %s, got %s\n", exstr, wine_dbgstr_w(V_BSTR(&got)));
+    }
+    else {
+        sprintf(buf, "%d", V_I4(&got));
+        ok_(__FILE__,line) (!strcmp(buf, exstr),
+            "Expected %s, got %d\n", exstr, V_I4(&got));
+    }
+}

This test should be stricter. You decide on the way to compare based on the value you received. This means that if the implementation always returns VT_BSTR, this will success as well. In fact, my quick testing suggests that native IE always returns string in this case. Also, please consider using more targeted helper so that test would look like:
test_table_width(table, "11");

Thanks,
Jacek