Re: [2/6] shdocvw: Implement InterneExplorer::ExecWB

[Jacek Caban]

On 7/21/10 2:08 PM, Alexander Nicolaysen Sørnes wrote:
> Implement InterneExplorer::ExecWB

+    if(IUnknown_QueryInterface(This->doc_host.document,&IID_IOleCommandTarget, (LPVOID*)&Target) != S_OK)
+    {
+        return E_NOTIMPL;

It doesn't seem like a right error code.

+    } else
+    {

You don't really need else block if you return in if anyway.

+        HRESULT ret = IOleCommandTarget_Exec(Target,&CGID_MSHTML, cmdID, cmdexecopt, pvaIn, pvaOut);

Did you test that it should use CGID_MSHTML? I'm not saying that it's wrong and writing a test for winetest would be tricky, but I'd like to know that it's somehow confirmed, esp. because you depend on it in next patches.

Jacek

[2/6] shdocvw: Implement InterneExplorer::ExecWB

[Alexander Nicolaysen Sørnes]

Onsdag 21. juli 2010 14.43.59 skrev Jacek Caban :
> On 7/21/10 2:08 PM, Alexander Nicolaysen Sørnes wrote:
> > Implement InterneExplorer::ExecWB
>
> +   
>  if(IUnknown_QueryInterface(This->doc_host.document,&IID_IOleCommandTarget,
>  (LPVOID*)&Target) != S_OK) +    {
> +        return E_NOTIMPL;
>
> It doesn't seem like a right error code.
>
> +    } else
> +    {
>
> You don't really need else block if you return in if anyway.
>
> +        HRESULT ret = IOleCommandTarget_Exec(Target,&CGID_MSHTML, cmdID,
>  cmdexecopt, pvaIn, pvaOut);
>
> Did you test that it should use CGID_MSHTML? I'm not saying that it's wrong
>  and writing a test for winetest would be tricky, but I'd like to know that
>  it's somehow confirmed, esp. because you depend on it in next patches.
>
> Jacek

No, I didn't test it. My only reference is MSDN, which linked to a list of 
MSHTML command identifiers for a set of valid commands.

I can remove it from the patchset for now. It's only used for printing, which 
crashes anyway.

Oh, and thanks for the review! :) 


Alexander

[2/6] shdocvw: Implement InterneExplorer::ExecWB

[Alexander Nicolaysen Sørnes]

Onsdag 21. juli 2010 15.39.56 skrev Alexander Nicolaysen Sørnes :
> Onsdag 21. juli 2010 14.43.59 skrev Jacek Caban :
> > On 7/21/10 2:08 PM, Alexander Nicolaysen Sørnes wrote:
> > > Implement InterneExplorer::ExecWB
> >
> > +
> >
> > if(IUnknown_QueryInterface(This->doc_host.document,&IID_IOleCommandTarget
> > , (LPVOID*)&Target) != S_OK) +    {
> > +        return E_NOTIMPL;
> >
> > It doesn't seem like a right error code.
> >
> > +    } else
> > +    {
> >
> > You don't really need else block if you return in if anyway.
> >
> > +        HRESULT ret = IOleCommandTarget_Exec(Target,&CGID_MSHTML, cmdID,
> >  cmdexecopt, pvaIn, pvaOut);
> >
> > Did you test that it should use CGID_MSHTML? I'm not saying that it's
> > wrong and writing a test for winetest would be tricky, but I'd like to
> > know that it's somehow confirmed, esp. because you depend on it in next
> > patches.
> >
> > Jacek
>
> No, I didn't test it. My only reference is MSDN, which linked to a list of
> MSHTML command identifiers for a set of valid commands.
>
> I can remove it from the patchset for now. It's only used for printing,
>  which crashes anyway.
>
> Oh, and thanks for the review! :)
>
> Alexander

Just to clarify: When writing the patch, I did some research into old articles 
to confirm that doing execWB() with IDM_PRINT is supposed to work.

I was unable to test it, however, because the command had been disabled in 
newer IE versions due to security concerns.



Alexander N. Sørnes