Re: [v2 PATCH] mshtml: Implement IHTMLDocument2 put/get_bgColor

[Jacek Caban]

Hi Alistair,

On 5/31/19 6:29 AM, Alistair Leslie-Hughes wrote:
> Signed-off-by: Alistair Leslie-Hughes
>
> dlls/mshtml/htmldoc.c   | 42 +++++++++++++++++++++++++++++++++++++----
>   dlls/mshtml/tests/dom.c | 30 +++++++++++++++++++++++++++++
>   2 files changed, 68 insertions(+), 4 deletions(-)
>
> diff --git a/dlls/mshtml/htmldoc.c b/dlls/mshtml/htmldoc.c
> index 5b14678a6f..160834cd5b 100644
> --- a/dlls/mshtml/htmldoc.c
> +++ b/dlls/mshtml/htmldoc.c
> @@ -680,15 +680,49 @@ static HRESULT WINAPI HTMLDocument_get_alinkColor(IHTMLDocument2 *iface, VARIANT
>   static HRESULT WINAPI HTMLDocument_put_bgColor(IHTMLDocument2 *iface, VARIANT v)
>   {
>       HTMLDocument *This = impl_from_IHTMLDocument2(iface);
> - FIXME("(%p)->(%s)\n", This, debugstr_variant(&v));
> - return E_NOTIMPL;
> - nsresult nsres;
> - nsAString nsstr;
> -
> - TRACE("(%p)->(%s)\n", This, debugstr_variant(&v));
> -
> - if(V_VT(&v) != VT_BSTR)
> - {
> -
>    FIXME("Unsupported type (%d)\n", V_VT(&v));
> -
>    return E_INVALIDARG;
> - }
> -
> - nsAString_InitDepend(&nsstr, V_BSTR(&v));
> - nsres = nsIDOMHTMLDocument_SetBgColor(This->doc_node->nsdoc, &nsstr);
> - nsAString_Finish(&nsstr);
> - if(NS_FAILED(nsres)) {
> -
>    ERR("SetBgColor failed: %08x\n", nsres);
> -
>    return E_INVALIDARG;
> - }

I think (and this could use testing to be sure) that this value should 
be kept in sync with body.bgColor. Looking at Gecko implementation, it 
just forwards calls to body element. In our body element we have quite a 
few tricks for color handling. If my theory is right, we don't want to 
duplicate that here, so we'd better forward calls to our body element.

As a side note, most of the color handling in body element should be 
probably disabled in standard compliant mode, bur that's a different bug.
> static HRESULT WINAPI HTMLDocument_get_bgColor(IHTMLDocument2 *iface, VARIANT *p)
>   {
>       HTMLDocument *This = impl_from_IHTMLDocument2(iface);
> - FIXME("(%p)->(%p)\n", This, p);
> - return E_NOTIMPL;
> - nsAString nsstr;
> - nsresult nsres;
> - HRESULT hres;
> -
> - TRACE("(%p)->(%p)\n", This, p);
> -
> - nsres = nsIDOMHTMLDocument_GetBgColor(This->doc_node->nsdoc, &nsstr);
> - if(NS_FAILED(nsres))
> -
>    return E_FAIL;

Same as setter, it should probably forward to body element.
> -
> - hres = return_nsstr_variant(nsres, &nsstr, p);
> - if(hres == S_OK)
> - {
> -
>    static const WCHAR defaultcolor[] = {'#','f','f','f','f','f','f',0};
> -
>    if(!V_BSTR(p))

You'd need VT check here.
> +static void test_background_color(IHTMLDocument2 *doc)
> +{
> - static WCHAR newcolor[] = {'#','f','f','0','0','0','0',0};
> - HRESULT hres;
> - IHTMLDocument2 *doc_node;
> - VARIANT color;
> -
> - doc_node = get_doc_node(doc);
> - hres = IHTMLDocument2_get_bgColor(doc_node, &color);
> - ok(hres == S_OK, "failed: %08x\n", hres);
> - ok(V_VT(&color) == VT_BSTR, "type failed: %d\n", V_VT(&color));
> - ok(!strcmp_wa(V_BSTR(&color), "#ffffff"), "str=%s\n", wine_dbgstr_w(V_BSTR(&color)));
> - VariantClear(&color);
> -
> - V_VT(&color) = VT_BSTR;
> - V_BSTR(&color) = SysAllocString(newcolor);

Please use a2bstr() in such cases.
> - hres = IHTMLDocument2_put_bgColor(doc_node, color);
> - ok(hres == S_OK, "failed: %08x\n", hres);
> - VariantClear(&color);
> -
> - hres = IHTMLDocument2_get_bgColor(doc_node, &color);
> - ok(hres == S_OK, "failed: %08x\n", hres);
> - ok(V_VT(&color) == VT_BSTR, "type failed: %d\n", V_VT(&color));
> - ok(!strcmp_wa(V_BSTR(&color), "#ff0000"), "str=%s\n", wine_dbgstr_w(V_BSTR(&color)));
> - VariantClear(&color);
> -
> - IHTMLDocument2_Release(doc_node);
>
> +}
> - static void test_null_write(IHTMLDocument2 *doc)
> {
>     HRESULT hres;
>
> @@ -11032,6 +11061,7 @@ START_TEST(dom)
>       run_domtest(emptydiv_str, test_docfrag);
>       run_domtest(doc_blank, test_replacechild_elems);
>       run_domtest(doctype_str, test_doctype);
> - run_domtest(doctype_str, test_background_color);

We don't need to load entire new document to test that. Please integrate 
it with existing tests (testing it together with body element is 
probably a good idea anyway).

Thanks,

Jacek