mshtml: Added IHTMLTable::width property implementation. (try 4)
Jacek Caban
jacek at codeweavers.com
Sat Apr 26 12:10:34 CDT 2014
On 04/26/14 16:38, Zhenbo Li wrote:
> Hi Jacek,
>
> 2014-04-26 21:33 GMT+08:00 Jacek Caban <jacek at codeweavers.com>:
>> On 04/26/14 15:20, Zhenbo Li wrote:
>>> 2014-04-26 20:44 GMT+08:00 Jacek Caban <jacek at codeweavers.com>:
>>>> Hi Zhenbo,
>>>>
>>>> This is better, but:
>>>>
>>>> On 04/26/14 05:12, Zhenbo Li wrote:
>>>>
>>>> + hres = var2str(&v, &val);
>>>> +
>>>> + if (hres != S_OK){
>>>> + ERR("Set Width(%s) failed when initializing a nsAString!\n",
>>>> + debugstr_variant(&v));
>>>> + nsAString_Finish(&val);
>>>>
>>>>
>>>> Again, this is not initialized in the error case.
>>> As I've added
>>>> + default:
>>>> + nsAString_Init(nsstr, NULL);
>>>> + FIXME("unsupported arg %s\n", debugstr_variant(p));
>>>> + return E_NOTIMPL;
>>>> + }
>>> Isn't it enough?
>> I missed that change, but I think it's wrong. There are other error
>> cases as well. nsAString_Init failure also leaves the string
>> uninitialized (and it's on my TODO list to make sure that we may make
>> nsAString_Init infailable, but that's a different story). It's also
>> generally sane to expect that if a function fails, it's output parameter
>> is not initialized. Please remove this new nsAString_Init call and
>> nsAString_Finish in error case.
>>
> Thank you for your explanation.
> How about this solution:
>
> + return nsAString_Init(nsstr, V_BSTR(p))?
> + S_OK : E_OUTOFMEMORY;
> + case VT_R8:
> + hres = VarBstrFromR8(V_R8(p), 0, 0, &str);
> + break;
> + case VT_R4:
> + hres = VarBstrFromR4(V_R4(p), 0, 0, &str);
> + break;
> + case VT_I4:
> + hres = VarBstrFromI4(V_I4(p), 0, 0, &str);
> + break;
> + default:
> + FIXME("unsupported arg %s\n", debugstr_variant(p));
> + hres = E_NOTIMPL;
> + break;
> + }
> + if (FAILED(hres))
> + return hres;
> +
> + ret = nsAString_Init(nsstr, str);
> + SysFreeString(str);
> + return ret ? S_OK : E_OUTOFMEMORY;
> +}
This looks good.
>>> Or you mean here?
>>>> + ret = nsAString_Init(nsstr, str);
>>>> + SysFreeString(str);
>>>> + return ret ? S_OK : E_OUTOFMEMORY;
>>> Should I free a string which failed when initializing due to out-of-memory?
>> Which one? The way it's done currently is right.
>>
>>>> +
>>>> + hres = nsstr_to_truncated_bstr(&val, &bstr);
>>>> + if (FAILED(hres)) {
>>>> + SysFreeString(bstr);
>>>>
>>>>
>>>> And bstr is not initialized in error case.
>>> The error case comes from
>>>> + ret = end ? SysAllocStringLen(str, end-str) : SysAllocString(str);
>>>> + if(!ret)
>>>> + return E_OUTOFMEMORY;
>>> I have the same question.
>> Please read the code again. bstr is not initialized in this case, so you
>> can't free it.
> Sorry, I made a mistake.
> When SysAllocString failes, should we mark bstr as NULL?
> Like this:
>
> static HRESULT nsstr_to_truncated_bstr(const nsAString *nsstr, BSTR *ret_ptr)
> {
> //....
> ret = end ? SysAllocStringLen(str, end-str) : SysAllocString(str);
>
> *ret_ptr = ret;
> return ret ? S_OK : E_OUTOFMEMORY;
> }
You don't need this...
> static HRESULT WINAPI HTMLTable_get_width(IHTMLTable *iface, VARIANT *p)
> {
> //....
> hres = nsstr_to_truncated_bstr(&val, &bstr);
> if (FAILED(hres))
> return hres;
> //...
> }
...once you change this.
Jacek
More information about the wine-devel
mailing list