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

[Jacek Caban]

On 04/25/14 15:39, Zhenbo Li wrote:
>>> +
>>>  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?

No, it's not initialized in error case.

>>> +        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?

Well, this helper takes care of that. It will leave percent value
untouched, what's what the second loop and if is for. Why do you think
this won't work?

Jacek