[prev in list] [next in list] [prev in thread] [next in thread]
List: wine-devel
Subject: Re: mshtml: Added IHTMLTable::width property implementation.
From: Jacek Caban <jacek () codeweavers ! com>
Date: 2014-03-31 15:23:26
Message-ID: 5339886E.6060709 () codeweavers ! com
[Download RAW message or body]
[Attachment #2 (multipart/alternative)]
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
[Attachment #5 (text/html)]
<html>
<head>
<meta content="text/html; charset=ISO-8859-1"
http-equiv="Content-Type">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi Zhenbo<small>,</small><br>
<br>
On 03/31/14 16:26, Zhenbo Li wrote:<br>
</div>
<blockquote cite="mid:53397B19.5020403@gmail.com" type="cite">As my
test case shows, windows would truncate float numbers.
<br>
I considered VarInt() in oleaut32.dll, but I think to declare
<br>
var2str_length() is more clear.
<br>
</blockquote>
<br>
<pre wrap="">+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;
</pre>
<br>
<pre wrap="">+#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
</pre>
</body>
</html>
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic