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

[Jacek Caban]

On 04/22/14 17:30, Zhenbo Li wrote:
> diff --git a/dlls/mshtml/htmltable.c b/dlls/mshtml/htmltable.c
> index d711ed7..e16aee2 100644
> --- a/dlls/mshtml/htmltable.c
> +++ b/dlls/mshtml/htmltable.c
> @@ -57,6 +57,33 @@ static inline HTMLTable *impl_from_IHTMLTable3(IHTMLTable3 *iface)
>      return CONTAINING_RECORD(iface, HTMLTable, IHTMLTable3_iface);
>  }
>
> +static HRESULT var2str(nsAString *nsstr, const VARIANT *p)
> +{

Nit: please swap arguments order.
> - 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.
> -
>
> 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.
> -
>    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.
> -
>        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;
}

Cheers,
Jacek

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

[Zhenbo Li]

Thank, Jacek

2014-04-25 20:26 GMT+08:00 Jacek Caban :
> 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

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