mshtml: Added IHTMLTable::width property implementation. (try 3)

[Zhenbo Li]

Thank, Jacek

2014-04-25 20:26 GMT+08:00 Jacek Caban <jacek at codeweavers.com>:
> On 04/22/14 17:30, Zhenbo Li wrote:
>>
>> +static HRESULT var2str(nsAString *nsstr, const VARIANT *p)
>> +{
>
> Nit: please swap arguments order.
Thank you, I didn't care that before.
>
>> +    BSTR str;
>> +    BOOL ret;
>> +
>> +    switch(V_VT(p)) {
>> +    case VT_BSTR:
>> +        return nsAString_Init(nsstr, V_BSTR(p))?
>> +            S_OK : E_FAIL;
>
> E_OUTOFMEMORY would be more appropriate here.
>
>> +    case VT_R8:
>> +        VarBstrFromR8(V_R8(p), 0, 0, &str);
>> +        break;
>> +    case VT_R4:
>> +        VarBstrFromR4(V_R4(p), 0, 0, &str);
>> +        break;
>> +    case VT_I4:
>> +        VarBstrFromI4(V_I4(p), 0, 0, &str);
>> +        break;
>> +    default:
>> +        FIXME("unsupported arg %s\n", debugstr_variant(p));
>> +        return E_NOTIMPL;
>> +    }
>> +    ret = nsAString_Init(nsstr, str);
>> +    SysFreeString(str);
>> +    return ret ? S_OK : E_FAIL;
>> +}
>
> Same here.
Got it.
>
>> +
>>  static HRESULT WINAPI HTMLTable_QueryInterface(IHTMLTable *iface,
>>                                                           REFIID riid, void **ppv)
>>  {
>> @@ -395,15 +422,79 @@ static HRESULT WINAPI HTMLTable_get_rows(IHTMLTable *iface, IHTMLElementCollecti
>>  static HRESULT WINAPI HTMLTable_put_width(IHTMLTable *iface, VARIANT v)
>>  {
>>      HTMLTable *This = impl_from_IHTMLTable(iface);
>> -    FIXME("(%p)->(%s)\n", This, debugstr_variant(&v));
>> -    return E_NOTIMPL;
>> +    nsAString val;
>> +    HRESULT hres;
>> +    nsresult nsres;
>> +
>> +    TRACE("(%p)->(%s)\n", This, debugstr_variant(&v));
>> +    hres = var2str(&val, &v);
>> +
>> +    if (hres != S_OK){
>> +        ERR("Set Width(%s) failed when initializing a nsAString!\n",
>> +            debugstr_variant(&v));
>> +        nsAString_Finish(&val);
>
> val is not initialized here, you can't call nsAString_Finish on it.
val is initialized in var2str.
Should I make it more explicit?

>
>> +        return hres;
>> +    }
>> +
>> +    nsres = nsIDOMHTMLTableElement_SetWidth(This->nstable, &val);
>> +    nsAString_Finish(&val);
>> +
>> +    if (NS_FAILED(nsres)){
>> +        ERR("Set Width(%s) failed!\n", debugstr_variant(&v));
>> +        return E_FAIL;
>> +    }
>> +
>> +    return S_OK;
>>  }
>>
>>  static HRESULT WINAPI HTMLTable_get_width(IHTMLTable *iface, VARIANT *p)
>>  {
>>      HTMLTable *This = impl_from_IHTMLTable(iface);
>> -    FIXME("(%p)->(%p)\n", This, p);
>> -    return E_NOTIMPL;
>> +    nsAString val;
>> +    const PRUnichar *str;
>> +    BSTR bstr;
>> +    DOUBLE width;
>> +    nsresult nsres;
>> +    HRESULT hres;
>> +
>> +    TRACE("(%p)->(%p)\n", This, p);
>> +    nsAString_Init(&val, NULL);
>> +    nsres = nsIDOMHTMLTableElement_GetWidth(This->nstable, &val);
>> +    if (NS_FAILED(nsres)){
>> +        ERR("Get Width(%s) failed!\n", debugstr_variant(p));
>> +        nsAString_Finish(&val);
>> +        return E_FAIL;
>> +    }
>> +
>> +    nsAString_GetData(&val, &str);
>> +    hres = VarR8FromStr(str, 0, 0, &width);
>> +
>> +    /* WinAPI would return VT_BSTR, not VT_I4 */
>> +    switch(hres) {
>> +        case S_OK:              /* Truncate that number */
>> +            hres = VarBstrFromI4((LONG)width, 0, 0, &bstr);
>
> There may be overflow here.
Yes, such danger exists. I'll find a proper way to solve it.

>
>> +            if (hres != S_OK) {
>> +                ERR("VarBstrFromI4 failed, err = %d\n", hres);
>> +                return E_FAIL;
>> +            }
>> +            V_VT(p) = VT_BSTR;
>> +            V_BSTR(p) = bstr;
>> +            break;
>> +        case DISP_E_TYPEMISMATCH: /* Has symbols */
>> +            V_VT(p) = VT_BSTR;
>> +            V_BSTR(p) = SysAllocString(str);
>> +            if (!V_BSTR(p)) {
>> +                ERR("SysAllocString(%s) failed!\n", debugstr_w(str));
>> +                return E_FAIL;
>> +            }
>> +            break;
>> +        default:
>> +            ERR("VarR8FromStr(%s) failed, err = %d\n", debugstr_w(str), hres);
>> +            return E_FAIL;
>
> I find it more complicated than it needs to be. You don't really need
> full conversion here, just a simple string parsing. How about using such
> helper:
>
> static HRESULT nsstr_to_rounded_bstr(const nsAString *nsstr, BSTR *ret_ptr)
> {
>     const PRUnichar *str, *ptr, *end = NULL;
>     BSTR ret;
>
>     nsAString_GetData(nsstr, &str);
>
>     for(ptr = str; isdigitW(*ptr); ptr++);
>     if(*ptr == '.') {
>         for(end = ptr++; isdigitW(*ptr); ptr++);
>         if(*ptr)
>             end = NULL;
>     }
>
>     ret = end ? SysAllocStringLen(str, end-str) : SysAllocString(str);
>     if(!ret)
>         return E_OUTOFMEMORY;
>
>     *ret_ptr = ret;
>     return S_OK;
> }
Thank you for mentioning that, but I don't think this helper works.
width property can have two types: a number, or a percentage.
number(like 11.9) would be truncated to 11
percentage("40.1%") won't be truncated. (Test case proved that)

So this helper would cause error.
How about check if the string has the symbol '%', then to decide
whether to call nsstr_to_rounded_bstr?

>
> Cheers,
> Jacek


Thank you

-- 
Have a nice day!
Zhenbo Li