Re: [2/2] mshtml: Add VT_INT support in IHTMLElementCollection_item

[Jacek Caban]

HI Konstantin,

Konstantin Kondratyuk wrote:
> VT_INT type isn't documented in MSDN, but it is supported in Windows

You're duplicating the code that handles VT_I4 case. Please avoid it.


Jacek

[2/2] mshtml: Add VT_INT support in IHTMLElementCollection_item

[Konstantin Kondratyuk]

Hi, Jacek!

On Friday 19 December 2008 18:04:45 Jacek Caban wrote:
> You're duplicating the code that handles VT_I4 case. Please avoid it.

Do you think, this code will be better? And can I truncate "VT_I4" from trace?

    if(V_VT(&name) == VT_I4 || V_VT(&name) == VT_INT) {
        int i;

        if (V_VT(&name) == VT_I4) {
            i = V_I4(&name);
            TRACE("name is VT_I4: %d\n", i);
        }
        else {
            i = V_INT(&name);
            TRACE("name is VT_INT: %d\n", i);
        }

        if(i < 0)
            return E_INVALIDARG;
        if(i >= This->len)
            return S_OK;

        *pdisp = (IDispatch*)This->elems[i];
        IDispatch_AddRef(*pdisp);
        TRACE("Returning pdisp=%p\n", pdisp);
        return S_OK;
    }


And about my test for this problem - it's OK?

-- 
Best regards,
Konstantin Kondratyuk.

[2/2] mshtml: Add VT_INT support in IHTMLElementCollection_item

[Jacek Caban]

Hi Konstantin,

Konstantin Kondratyuk wrote:
> Hi, Jacek!
>
> On Friday 19 December 2008 18:04:45 Jacek Caban wrote:
> > You're duplicating the code that handles VT_I4 case. Please avoid it.
>
> Do you think, this code will be better? And can I truncate "VT_I4" from trace?

Yes, it looks better, but I'd suggest to move this code to separated 
function.
> And about my test for this problem - it's OK?

Yes, it's OK. There is one improvement that would be nice to have. You 
could test returned disp better than comparing to NULL. It could look 
like an attached patch (compile tested only).
Also tests must pass after each patch, so you should change order of 
patches or mark them as todo_wine and remove todo_wine in second patch 
or send both changes in one patch. I'd suggest sending in one patch in 
this case.



Thanks,
    Jacek

diff --git a/dlls/mshtml/tests/dom.c b/dlls/mshtml/tests/dom.c
index aa8dbb8..7d821e6 100644
--- a/dlls/mshtml/tests/dom.c
+++ b/dlls/mshtml/tests/dom.c
@@ -1005,7 +1005,7 @@ static void _test_elem_collection(unsigned line, IUnknown *unk,
     long len;
     DWORD i;
     VARIANT name, index;
-    IDispatch *disp;
+    IDispatch *disp, *disp2;
     HRESULT hres;
 
     hres = IUnknown_QueryInterface(unk, &IID_IHTMLElementCollection, (void**)&col);
@@ -1021,9 +1021,9 @@ static void _test_elem_collection(unsigned line, IUnknown *unk,
         len = exlen;
 
     V_VT(&index) = VT_EMPTY;
-    V_VT(&name) = VT_I4;
 
     for(i=0; i<len; i++) {
+        V_VT(&name) = VT_I4;
         V_I4(&name) = i;
         disp = (void*)0xdeadbeef;
         hres = IHTMLElementCollection_item(col, name, index, &disp);
@@ -1032,10 +1032,21 @@ static void _test_elem_collection(unsigned line, IUnknown *unk,
         if(FAILED(hres) || !disp)
             continue;
 
+        if(!i) {
+            V_VT(&name) = VT_INT;
+            V_INT(&name) = i;
+            disp2 = (void*)0xdeadbeef;
+            hres = IHTMLElementCollection_item(col, name, index, &disp2);
+            ok_(__FILE__,line) (hres == S_OK, "item(%d) failed: %08x\n", i, hres);
+            ok_(__FILE__,line) (disp == disp2, "disp != disp2\n");
+            IDispatch_Release(disp2);
+        }
+
         _test_elem_type(line, (IUnknown*)disp, elem_types[i]);
         IDispatch_Release(disp);
     }
 
+    V_VT(&name) = VT_I4;
     V_I4(&name) = len;
     disp = (void*)0xdeadbeef;
     hres = IHTMLElementCollection_item(col, name, index, &disp);