mshtml: Added IHTMLTable::width property implementation.

[Zhenbo Li]

hank you very much.

As MSDN says[1], "VARIANT of type VT_I4 or VT_BSTR that
specifies one of the values listed in Possible Values." VT_I4 is easy
to solve, but VT_BSTR is too flexible. All these usages are legal:

  document.getElementById('table1').width=20.6; //Win IE treats as 20
  document.getElementById('table1').width="20.6"; //Win IE treats as 20
  document.getElementById('table1').width="50.9%"; //Win IE treats as "50.9%"
(You may check my sample HTML code[2])

[1]: http://msdn.microsoft.com/en-us/library/aa768621%28v=vs.85%29.aspx
[2]: http://paste.opensuse.org/87682041

2014-03-31 23:23 GMT+08:00 Jacek Caban <jacek at codeweavers.com>:
> 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)
>
> 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.
I think to make it more generic is a good idea, and it is necessary to define
*how generic* it is. How about this solution? Or to make it more generic, that
what to return is decided by parameters?
(To make this mail shorter, I removed some traces)

static HRESULT var_to_nsstr(nsAString *nsstr, const VARIANT *p, BOOL truncate)
{
    static WCHAR buf[64];
    static const WCHAR formatW[] = {'%','d',0};
    LONG width;
    DOUBLE width_r8;
    VARIANT var;
    HRESULT hres;

    switch(V_VT(p)) {
    case VT_BSTR:
        hres = VarR8FromStr(V_BSTR(p), 0, 0, &width_r8);
        if (hres == DISP_E_TYPEMISMATCH) /* Symbols are in this string */
            return nsAString_Init(nsstr, V_BSTR(p));
        V_VT(&var) = VT_R8;
        V_R8(&var) = width_r8;
        break;
    case VT_I4: case VT_R8:
        var = *p;
        break;
    case VT_R4:
        V_VT(&var) = VT_R8;
        V_R8(&var) = V_R4(&var);
        break;
    default:
        FIXME("unsupported arg %s\n", debugstr_variant(p));
        return E_NOTIMPL;
    }
    if (V_VT(&var) == VT_R8) {
        if (truncate)  width = (LONG)V_R8(&var);
        else            VarI4FromR8(V_R8(&var), &width);
    }
    else width = V_I4(&var);
    VariantClear(&var);
    sprintfW(buf, formatW, width);
    return nsAString_Init(nsstr, buf);
}


>
> +    nsAString val;
> +    HRESULT nsres;
>
> nsresult and HRESULT, although are very similar, are two different things
> and should not be mixed.
Sorry, this is my fault.

>
> +    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.
Thank you for mentioning it. Could my new solution be checked?
(I'll add code to check if the pointers are legal before sending my new patch to
wine-patches)

static HRESULT WINAPI HTMLTable_get_width(IHTMLTable *iface, VARIANT *p)
{
    HTMLTable *This = impl_from_IHTMLTable(iface);
    nsAString val;
    nsresult nsres;
    DOUBLE width;
    const PRUnichar *str;

    TRACE("(%p)->(%p)\n", This, p);
    nsAString_Init(&val, NULL);
    nsres = nsIDOMHTMLTableElement_GetWidth(This->nstable, &val);
    nsAString_GetData(&val, &str);
    nsres = VarR8FromStr(str, 0, 0, &width);
    if (nsres == S_OK){
        V_VT(p) = VT_I4;
        V_I4(p) = (LONG)width;
    } else {
        V_VT(p) = VT_BSTR;
        V_BSTR(p) = SysAllocString(str);
    }
    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.
My test results are the same. But they are not guaranteed in MSDN.
Should I depend
on them?

> Also, please consider using
> more targeted helper so that test would look like:
> test_table_width(table, "11");
>
I was not sure about the helper's name. I guess(not tested yet) height
property would
use the same helper. So to call it test_table_width is a good idea?
>
> Thanks,
> Jacek

I really appreciate your suggestion. Thank you very much.


-- 
Have a nice day!
Zhenbo Li