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

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(a)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

mshtml: Added IHTMLTable::width property implementation.

[Jacek Caban]

On 04/01/14 05:40, Zhenbo Li wrote:
> 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.
> From my (and most Wine developers') experience: don't take MSDN too
> seriously. It may be treated more like guidelines than a real
> documentation (or specification). You've just found another example of
> MSDN being wrong. There are even more obvious mistakes on the page that
> you quoted like percentage being "Not applicable to C++".
> 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

You may change appropriate lines to:

txt += document.getElementById('table1').width + " of type " +
typeof(document.getElementById('table1').width);

in your tests to see how those attributes are truly exposed.
> 2014-03-31 23:23 GMT+08:00 Jacek Caban <jacek(a)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?

Error handling needs more thoughts here. Also, we probably don't want to
convert floats to ints by default at all. In fact, I would suggest to
ignore rounding floats here. Passing float to Gecko will not change much
(I'd expect layout engine to behave exactly the same way as if int was
passed) and it doesn't guarantee that value exposed by getter will
always be int (since there are other, not controlled by us, ways to set
the value). If you really want to guarantee that, getter would be a
better place (but I'd suggest ignoring this detail for now; I don't
expect this to cause problems for real world pages).
> (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)

No, this should returns a strings as shown by tests.
> 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?

Yes, tests are definitely more trustful than MSDN.
> > 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?

Since this will have to be changed to always assume strings, there won't
be much to share with height property (just a check of one string
VARIANT). Both solutions are good then. Still, the nice thing about such
helpers is that it makes obvious what you mean to test here.

Cheers,
Jacek