Re: [PATCH v2 3/5] ieframe: Add stub implementation of IInternetExplorerManager.

[Zebediah Figura]

On 08/09/2017 06:07 AM, Jacek Caban wrote:
> Hi Zebediah,
>
> On 08.08.2017 20:04, Zebediah Figura wrote:
> > HRESULT register_class_object(BOOL do_reg)
> >   {
> >       HRESULT hres;
> >
> > -    static DWORD cookie;
> > +    static DWORD ie_cookie, iem_cookie;
> >
> > if(do_reg) {
> >           hres = CoRegisterClassObject(&CLSID_InternetExplorer,
> >                   (IUnknown*)&InternetExplorerFactory, CLSCTX_SERVER,
> > -                REGCLS_MULTIPLEUSE|REGCLS_SUSPENDED, &cookie);
> > -        if (FAILED(hres)) {
> > +                REGCLS_MULTIPLEUSE, &ie_cookie);
> > +        if (FAILED(hres))
> > +            ERR("failed to register object %08x\n", hres);
> > +        hres = CoRegisterClassObject(&CLSID_InternetExplorerManager,
> > +                (IUnknown*)&InternetExplorerManagerFactory, CLSCTX_SERVER,
> > +                REGCLS_MULTIPLEUSE, &iem_cookie);
> > +        if (FAILED(hres))
> >               ERR("failed to register object %08x\n", hres);
> > -            return hres;
> > -        }
> > -
> > -        hres = CoResumeClassObjects();
> > -        if(SUCCEEDED(hres))
> > -            return hres;
> > -
> > -        ERR("failed to resume object %08x\n", hres);
> >       }
> > -
> > -    return CoRevokeClassObject(cookie);
> > +    else
> > +    {
> > +        hres = CoRevokeClassObject(ie_cookie);
> > +        if (FAILED(hres))
> > +            ERR("failed to register object %08x\n", hres);
> > +        hres = CoRevokeClassObject(iem_cookie);
> > +        if (FAILED(hres))
> > +            ERR("failed to register object %08x\n", hres);
> > +    }
> > +    return hres;
> >   }
>
> Are you sure that's how it's supposed to work? I didn't test it myself, 
> but given how it's registered and documented, I'd expect that 
> CLSID_InternetExplorerManager should live in a different process from 
> CLSID_InternetExplorer. It should be easy to distinguish those processes 
> with -startmanager command line argument.

I'm sorry, I don't understand what you mean. Isn't CoRegisterClassObject 
process-agnostic?
> > +static HRESULT WINAPI InternetExplorerManager_CreateObject(IInternetExplorerManager *iface, DWORD config, LPCWSTR url, REFIID riid, void **ppv)
> > +{
> > +    FIXME("(%p)->(0x%x, %s, %s, %p) semi-stub\n", iface, config, debugstr_w(url), debugstr_guid(riid), ppv);
> > +
> > +    return CoCreateInstance(&CLSID_InternetExplorer, NULL, CLSCTX_LOCAL_SERVER, riid, ppv);
> > +}
>
> Using CoCreateInstance is very questionable here (eg. due to the comment 
> above). Also, a patch adding a stub should just return E_NOTIMPL. 
> Implementation deserves a separated patch. I'd expect that it should 
> just use internal ieframe functions to create a new IE instance.
>
> Also, I'd suggest to reorder patches to something like: 2, 4, 5, 1 
> (likely replaced by support for -startmanager command line argument), 3 
> and then a new patch implementing CreateObject.
>
> Thanks,
> Jacek

Thanks for the review. I'll reorder the existing patches, but leave out 
an implementation of CreateObject() until I understand what it should be 
doing.