Re: [PATCH 3/3] msscript: Implement Timeout property

[Jacek Caban]

Hi Nikolay,

On 06/11/2016 12:02 PM, Nikolay Sivov wrote:
> static HRESULT WINAPI ScriptControl_put_Timeout(IScriptControl *iface, LONG milliseconds)
>  {
>      ScriptControl *This = impl_from_IScriptControl(iface);
> - FIXME("(%p)->(%d)\n", This, milliseconds);
> - return E_NOTIMPL;
> -
> - TRACE("(%p)->(%d)\n", This, milliseconds);
> -
> - if (milliseconds < -1)
> -
>    return CTL_E_INVALIDPROPERTYVALUE;
> -
> - This->timeout = milliseconds;
> - return S_OK;
>
> }

Getter/setter implementation seems fine, but it's not full
implementation and I'm pretty sure that at least first versions that
will be able to execute script code won't support that (builtin jscript
nor vbscript supports that, so there is very little point in
implementing it here), so FIXME or WARN here would be more appropriate here.

Thanks,
Jacek

[PATCH 3/3] msscript: Implement Timeout property

[Nikolay Sivov]

On 13.06.2016 15:59, Jacek Caban wrote:
> Hi Nikolay,
>
> On 06/11/2016 12:02 PM, Nikolay Sivov wrote:
> > static HRESULT WINAPI ScriptControl_put_Timeout(IScriptControl *iface, LONG milliseconds)
> >  {
> >      ScriptControl *This = impl_from_IScriptControl(iface);
> > - FIXME("(%p)->(%d)\n", This, milliseconds);
> > - return E_NOTIMPL;
> > -
> > - TRACE("(%p)->(%d)\n", This, milliseconds);
> > -
> > - if (milliseconds < -1)
> > -
> >    return CTL_E_INVALIDPROPERTYVALUE;
> > -
> > - This->timeout = milliseconds;
> > - return S_OK;
> >
> > }
>
> Getter/setter implementation seems fine, but it's not full
> implementation and I'm pretty sure that at least first versions that
> will be able to execute script code won't support that (builtin jscript
> nor vbscript supports that, so there is very little point in
> implementing it here), so FIXME or WARN here would be more appropriate here.

I don't follow. Of course simply stored timeout value won't make
anything time out. I can certainly put some tracing here, but it seems
more appropriate to add a FIXME to methods that trigger execution. By
the way, do you know how it's supposed to work or could be made to work?
Does it have to be separate thread running the engine, and
InterruptScriptThread() when wait fails or there's some standard
ActiveScript way to handle that? (that ours vbscript/jscript don't support).
> Thanks,
> Jacek

[PATCH 3/3] msscript: Implement Timeout property

[Jacek Caban]

On 06/13/2016 01:33 PM, Nikolay Sivov wrote:
> On 13.06.2016 15:59, Jacek Caban wrote:
> > Hi Nikolay,
> >
> > On 06/11/2016 12:02 PM, Nikolay Sivov wrote:
> > > static HRESULT WINAPI ScriptControl_put_Timeout(IScriptControl *iface, LONG milliseconds)
> > >  {
> > >      ScriptControl *This = impl_from_IScriptControl(iface);
> > > - FIXME("(%p)->(%d)\n", This, milliseconds);
> > > - return E_NOTIMPL;
> > > -
> > > - TRACE("(%p)->(%d)\n", This, milliseconds);
> > > -
> > > - if (milliseconds < -1)
> > > -
> > >    return CTL_E_INVALIDPROPERTYVALUE;
> > > -
> > > - This->timeout = milliseconds;
> > > - return S_OK;
> > >
> > > }
> >
> > Getter/setter implementation seems fine, but it's not full
> > implementation and I'm pretty sure that at least first versions that
> > will be able to execute script code won't support that (builtin jscript
> > nor vbscript supports that, so there is very little point in
> > implementing it here), so FIXME or WARN here would be more appropriate here.
>
> I don't follow. Of course simply stored timeout value won't make
> anything time out. I can certainly put some tracing here, but it seems
> more appropriate to add a FIXME to methods that trigger execution. By
> the way, do you know how it's supposed to work or could be made to work?
> Does it have to be separate thread running the engine, and
> InterruptScriptThread() when wait fails or there's some standard
> ActiveScript way to handle that? (that ours vbscript/jscript don't support).

I think it should use IActiveScriptSiteInterruptPoll, which is not yet
supported by script engines. And yeah, putting a FIXME to methods will
work as well.

Thanks,
Jacek