Re: mshtml: Added IHTMLTable::width property implementation. (try 4)
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.
+ 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; + BSTR bstr; + 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; + } + + hres = nsstr_to_truncated_bstr(&val, &bstr); + if (FAILED(hres)) { + SysFreeString(bstr);
And bstr is not initialized in error case. Also, as Dmitry pointed, you don't check for some errors in var2str. Jacek
Thank you. I'm sorry for the mistakes I've made. As this patch still needs improving, maybe mark 104193, 104110, 103576 as rejected? 2014-04-26 20:44 GMT+08:00 Jacek Caban <jacek(a)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?
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?
+ + 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.
Also, as Dmitry pointed, you don't check for some errors in var2str.
This is my fault, I'll fix it then. Thanks Dmitry, too
Jacek
-- Have a nice day! Zhenbo Li
On 04/26/14 15:20, Zhenbo Li wrote:
Thank you. I'm sorry for the mistakes I've made.
As this patch still needs improving, maybe mark 104193, 104110, 103576 as rejected?
Don't worry too much about them being marked one way or another.
2014-04-26 20:44 GMT+08:00 Jacek Caban <jacek(a)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.
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. Jacek
Hi Jacek, 2014-04-26 21:33 GMT+08:00 Jacek Caban <jacek(a)codeweavers.com>:
On 04/26/14 15:20, Zhenbo Li wrote:
2014-04-26 20:44 GMT+08:00 Jacek Caban <jacek(a)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; +}
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; } static HRESULT WINAPI HTMLTable_get_width(IHTMLTable *iface, VARIANT *p) { //.... hres = nsstr_to_truncated_bstr(&val, &bstr); if (FAILED(hres)) return hres; //... }
Jacek
-- Have a nice day! Zhenbo Li
On 04/26/14 16:38, Zhenbo Li wrote:
Hi Jacek,
2014-04-26 21:33 GMT+08:00 Jacek Caban <jacek(a)codeweavers.com>:
On 04/26/14 15:20, Zhenbo Li wrote:
2014-04-26 20:44 GMT+08:00 Jacek Caban <jacek(a)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
participants (2)
-
Jacek Caban -
Zhenbo Li