Index: /trunk/src/VBox/Main/MachineImpl.cpp =================================================================== --- /trunk/src/VBox/Main/MachineImpl.cpp (revision 29933) +++ /trunk/src/VBox/Main/MachineImpl.cpp (revision 29934) @@ -9841,5 +9841,5 @@ if (!mData->mSession.mProgress.isNull()) - mData->mSession.mProgress->setOtherProgressObject(aProgress); + mData->mSession.mProgress->setOtherProgressObject(aProgress, 7); return S_OK; @@ -9863,4 +9863,5 @@ if (mData->mSession.mProgress) { + mData->mSession.mProgress->clearOtherProgressObject(tr("Finalizing"), 1); mData->mSession.mProgress->notifyComplete((HRESULT)iResult); mData->mSession.mProgress.setNull(); Index: /trunk/src/VBox/Main/ProgressProxyImpl.cpp =================================================================== --- /trunk/src/VBox/Main/ProgressProxyImpl.cpp (revision 29933) +++ /trunk/src/VBox/Main/ProgressProxyImpl.cpp (revision 29934) @@ -46,8 +46,6 @@ HRESULT ProgressProxy::FinalConstruct() { - mfMultiOperation = false; - muOtherProgressStartWeight = 0; - muOtherProgressWeight = 0; - muOtherProgressStartOperation = 0; + mcOtherProgressObjects = 1; + miCurOtherProgressObject = 0; HRESULT rc = Progress::FinalConstruct(); @@ -68,8 +66,6 @@ BOOL fCancelable) { - mfMultiOperation = false; - muOtherProgressStartWeight = 1; - muOtherProgressWeight = 1; - muOtherProgressStartOperation = 1; + miCurOtherProgressObject = 0; + mcOtherProgressObjects = 0; return Progress::init( @@ -88,12 +84,9 @@ /** - * Initialize for proxying one other progress object. - * - * This is tailored explicitly for the openRemoteSession code, so we start out - * with one operation where we don't have any remote object (powerUp). Then a - * remote object is added and stays with us till the end. - * - * The user must do normal completion notification or risk leave the threads - * waiting forever! + * Initialize for proxying one or more other objects, assuming we start out and + * end without proxying anyone. + * + * The user must call clearOtherProgressObject when there are no more progress + * objects to be proxied or we'll leave threads waiting forever. */ HRESULT ProgressProxy::init( @@ -104,13 +97,12 @@ CBSTR bstrDescription, BOOL fCancelable, + ULONG cOtherProgressObjects, ULONG uTotalOperationsWeight, CBSTR bstrFirstOperationDescription, ULONG uFirstOperationWeight, - ULONG cOtherProgressObjectOperations) -{ - mfMultiOperation = false; - muOtherProgressStartWeight = uFirstOperationWeight; - muOtherProgressWeight = uTotalOperationsWeight - uFirstOperationWeight; - muOtherProgressStartOperation = 1; + OUT_GUID pId) +{ + miCurOtherProgressObject = 0; + mcOtherProgressObjects = cOtherProgressObjects; return Progress::init( @@ -121,9 +113,9 @@ bstrDescription, fCancelable, - 1 + cOtherProgressObjectOperations /* cOperations */, + 1 + cOtherProgressObjects + 1 /* cOperations */, uTotalOperationsWeight, bstrFirstOperationDescription, uFirstOperationWeight, - NULL); + pId); } @@ -131,8 +123,5 @@ { uninit(); - mfMultiOperation = false; - muOtherProgressStartWeight = 0; - muOtherProgressWeight = 0; - muOtherProgressStartOperation = 0; + miCurOtherProgressObject = 0; } @@ -186,5 +175,5 @@ /** Just a wrapper so we can automatically do the handover before setting * the result locally. */ -bool ProgressProxy::notifyPointOfNoReturn(void) +bool ProgressProxy::notifyPointOfNoReturn(void) { AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); @@ -199,51 +188,32 @@ * @returns false if failed/canceled, true if not. * @param pOtherProgress The other progress object. Must not be NULL. - */ -bool ProgressProxy::setOtherProgressObject(IProgress *pOtherProgress) -{ - LogFlowThisFunc(("setOtherProgressObject: %p\n", pOtherProgress)); + * @param uOperationWeight The weight of this operation. (The description + * is taken from the other progress object.) + */ +bool ProgressProxy::setOtherProgressObject(IProgress *pOtherProgress, ULONG uOperationWeight) +{ + LogFlowThisFunc(("setOtherProgressObject: %p %u\n", pOtherProgress, uOperationWeight)); ComPtr ptrOtherProgress = pOtherProgress; - /* - * Query information from the other progress object before we grab the - * lock. - */ - ULONG cOperations; - HRESULT hrc = pOtherProgress->COMGETTER(OperationCount)(&cOperations); - if (FAILED(hrc)) - cOperations = 1; - - Bstr bstrOperationDescription; - hrc = pOtherProgress->COMGETTER(Description)(bstrOperationDescription.asOutParam()); + /* Get the description first. */ + Bstr bstrOperationDescription; + HRESULT hrc = pOtherProgress->COMGETTER(Description)(bstrOperationDescription.asOutParam()); if (FAILED(hrc)) bstrOperationDescription = "oops"; - - /* - * Take the lock and check for cancelation, cancel the other object if - * we've been canceled already. - */ AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); + /* Do the hand over from any previous progress object. */ + clearOtherProgressObjectInternal(false /*fEarly*/); BOOL fCompletedOrCanceled = mCompleted || mCanceled; if (!fCompletedOrCanceled) { - /* - * Advance to the next object and operation. If the other object has - * more operations than anticipated, adjust our internal count. - */ + /* Advance to the next object and operation, checking for cancelation + and completion right away to be on the safe side. */ + Assert(miCurOtherProgressObject < mcOtherProgressObjects); mptrOtherProgress = ptrOtherProgress; - mfMultiOperation = cOperations > 1; - - muOtherProgressStartWeight = m_ulOperationsCompletedWeight + m_ulCurrentOperationWeight; - muOtherProgressWeight = m_ulTotalOperationsWeight - muOtherProgressStartWeight; - Progress::SetNextOperation(bstrOperationDescription, muOtherProgressWeight); - - muOtherProgressStartOperation = m_ulCurrentOperation; - m_cOperations = cOperations + m_ulCurrentOperation; - - /* - * Check for cancelation and completion. - */ + + Progress::SetNextOperation(bstrOperationDescription, uOperationWeight); + BOOL f; hrc = ptrOtherProgress->COMGETTER(Completed)(&f); @@ -263,8 +233,5 @@ else { - /* - * Finally, mirror the cancelable property. - * Note! Note necessary if we do passthru! - */ + /* Mirror the cancelable property. */ if (mCancelable) { @@ -290,4 +257,27 @@ } +/** + * Clears the last other progress objects. + * + * @returns false if failed/canceled, true if not. + * @param pszLastOperationDescription The description of the final bit. + * @param uLastOperationWeight The weight of the final bit. + */ +bool ProgressProxy::clearOtherProgressObject(const char *pszLastOperationDescription, ULONG uLastOperationWeight) +{ + LogFlowThisFunc(("%p %u\n", pszLastOperationDescription, uLastOperationWeight)); + AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); + + clearOtherProgressObjectInternal(false /* fEarly */); + + /* Advance to the next operation if applicable. */ + bool fCompletedOrCanceled = mCompleted || mCanceled; + if (!fCompletedOrCanceled) + Progress::SetNextOperation(Bstr(pszLastOperationDescription), uLastOperationWeight); + + LogFlowThisFunc(("Returns %RTbool\n", !fCompletedOrCanceled)); + return !fCompletedOrCanceled; +} + // Internal methods. //////////////////////////////////////////////////////////////////////////////// @@ -295,7 +285,9 @@ /** - * Clear the other progress object reference, first copying over its state. - * - * This is used internally when completion is signalled one way or another. + * Internal version of clearOtherProgressObject that doesn't advance to the next + * operation. + * + * This is used both by clearOtherProgressObject as well as a number of places + * where we automatically do the hand over because of failure/completion. * * @param fEarly Early clearing or not. @@ -303,9 +295,12 @@ void ProgressProxy::clearOtherProgressObjectInternal(bool fEarly) { - if (mptrOtherProgress.isNotNull()) + if (!mptrOtherProgress.isNull()) { ComPtr ptrOtherProgress = mptrOtherProgress; mptrOtherProgress.setNull(); copyProgressInfo(ptrOtherProgress, fEarly); + + miCurOtherProgressObject++; + Assert(miCurOtherProgressObject <= mcOtherProgressObjects); } } @@ -418,82 +413,33 @@ //////////////////////////////////////////////////////////////////////////////// -STDMETHODIMP ProgressProxy::COMGETTER(Cancelable)(BOOL *aCancelable) -{ - CheckComArgOutPointerValid(aCancelable); +STDMETHODIMP ProgressProxy::COMGETTER(Percent)(ULONG *aPercent) +{ +#if 0 + CheckComArgOutPointerValid(aPercent); AutoCaller autoCaller(this); HRESULT hrc = autoCaller.rc(); - if (SUCCEEDED(hrc)) + if (SUCCEEDED(rc)) { AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - /* ASSUME: The cancelable property can only change to FALSE. */ - if (!mCancelable || mptrOtherProgress.isNull()) - *aCancelable = mCancelable; - else - { - hrc = mptrOtherProgress->COMGETTER(Cancelable)(aCancelable); - if (SUCCEEDED(hrc) && !*aCancelable) - { - LogFlowThisFunc(("point-of-no-return reached\n")); - mCancelable = FALSE; - } - } - } - return hrc; -} - -STDMETHODIMP ProgressProxy::COMGETTER(Percent)(ULONG *aPercent) -{ - CheckComArgOutPointerValid(aPercent); - - AutoCaller autoCaller(this); - HRESULT hrc = autoCaller.rc(); - if (SUCCEEDED(hrc)) - { - AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - - if (mptrOtherProgress.isNotNull()) + if (mptrOtherProgress.isNull()) hrc = Progress::COMGETTER(Percent)(aPercent); else { - /* - * Get the overall percent of the other object and adjust it with - * the weighting given to the period before proxying started. - */ ULONG uPct; hrc = mptrOtherProgress->COMGETTER(Percent)(&uPct); - if (SUCCEEDED(hrc)) - { - double rdPercent = ((double)uPct / 100 * muOtherProgressWeight + muOtherProgressStartWeight) - / m_ulTotalOperationsWeight * 100; - *aPercent = RT_MIN((ULONG)rdPercent, 99); /* mptrOtherProgress is cleared when its completed, so we can never return 100%. */ - } + .... } } return hrc; -} - -STDMETHODIMP ProgressProxy::COMGETTER(TimeRemaining)(LONG *aTimeRemaining) -{ - CheckComArgOutPointerValid(aTimeRemaining); - - AutoCaller autoCaller(this); - HRESULT hrc = autoCaller.rc(); - if (SUCCEEDED(hrc)) - { - AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - - if (mptrOtherProgress.isNotNull()) - hrc = Progress::COMGETTER(TimeRemaining)(aTimeRemaining); - else - hrc = mptrOtherProgress->COMGETTER(TimeRemaining)(aTimeRemaining); - } - return hrc; +#else + return Progress::COMGETTER(Percent)(aPercent); +#endif } STDMETHODIMP ProgressProxy::COMGETTER(Completed)(BOOL *aCompleted) { - /* Not proxied since we EXPECT a normal completion notification call. */ + /* Not proxied since we EXPECT a hand back call. */ return Progress::COMGETTER(Completed)(aCompleted); } @@ -512,11 +458,9 @@ if ( SUCCEEDED(hrc) && !*aCanceled - && mptrOtherProgress.isNotNull() - && mCancelable) + && !mptrOtherProgress.isNull()) { hrc = mptrOtherProgress->COMGETTER(Canceled)(aCanceled); if (SUCCEEDED(hrc) && *aCanceled) - /* This will not complete the object, only mark it as canceled. */ - clearOtherProgressObjectInternal(false /*fEarly*/); + clearOtherProgressObjectInternal(true /*fEarly*/); } } @@ -526,5 +470,5 @@ STDMETHODIMP ProgressProxy::COMGETTER(ResultCode)(LONG *aResultCode) { - /* Not proxied since we EXPECT a normal completion notification call. */ + /* Not proxied yet since we EXPECT a hand back call. */ return Progress::COMGETTER(ResultCode)(aResultCode); } @@ -532,62 +476,12 @@ STDMETHODIMP ProgressProxy::COMGETTER(ErrorInfo)(IVirtualBoxErrorInfo **aErrorInfo) { - /* Not proxied since we EXPECT a normal completion notification call. */ + /* Not proxied yet since we EXPECT a hand back call. */ return Progress::COMGETTER(ErrorInfo)(aErrorInfo); } -STDMETHODIMP ProgressProxy::COMGETTER(Operation)(ULONG *aOperation) -{ - CheckComArgOutPointerValid(aOperation); - - AutoCaller autoCaller(this); - HRESULT hrc = autoCaller.rc(); - if (SUCCEEDED(hrc)) - { - AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - if (mptrOtherProgress.isNull()) - hrc = Progress::COMGETTER(Operation)(aOperation); - else - { - ULONG uCurOtherOperation; - hrc = mptrOtherProgress->COMGETTER(Operation)(&uCurOtherOperation); - if (SUCCEEDED(hrc)) - *aOperation = uCurOtherOperation + muOtherProgressStartOperation; - } - } - return hrc; -} - -STDMETHODIMP ProgressProxy::COMGETTER(OperationDescription)(BSTR *aOperationDescription) -{ - CheckComArgOutPointerValid(aOperationDescription); - - AutoCaller autoCaller(this); - HRESULT hrc = autoCaller.rc(); - if (SUCCEEDED(hrc)) - { - AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - if (mptrOtherProgress.isNull() || !mfMultiOperation) - hrc = Progress::COMGETTER(OperationDescription)(aOperationDescription); - else - hrc = mptrOtherProgress->COMGETTER(OperationDescription)(aOperationDescription); - } - return hrc; -} - STDMETHODIMP ProgressProxy::COMGETTER(OperationPercent)(ULONG *aOperationPercent) { - CheckComArgOutPointerValid(aOperationPercent); - - AutoCaller autoCaller(this); - HRESULT hrc = autoCaller.rc(); - if (SUCCEEDED(hrc)) - { - AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - if (mptrOtherProgress.isNull() || !mfMultiOperation) - hrc = Progress::COMGETTER(OperationPercent)(aOperationPercent); - else - hrc = mptrOtherProgress->COMGETTER(OperationPercent)(aOperationPercent); - } - return hrc; + /* Not proxied, should be proxied later on. */ + return Progress::COMGETTER(OperationPercent)(aOperationPercent); } @@ -618,6 +512,5 @@ LogFlowThisFunc(("aTimeout=%d\n", aTimeout)); - /* No need to wait on the proxied object for these since we'll get the - normal completion notifications. */ + /* For now we'll always block locally. */ hrc = Progress::WaitForCompletion(aTimeout); @@ -628,34 +521,11 @@ STDMETHODIMP ProgressProxy::WaitForOperationCompletion(ULONG aOperation, LONG aTimeout) { + HRESULT hrc; LogFlowThisFuncEnter(); LogFlowThisFunc(("aOperation=%d aTimeout=%d\n", aOperation, aTimeout)); - AutoCaller autoCaller(this); - HRESULT hrc = autoCaller.rc(); - if (SUCCEEDED(hrc)) - { - AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - - CheckComArgExpr(aOperation, aOperation < m_cOperations); - - /* - * Check if we can wait locally. - */ - if ( aOperation + 1 == m_cOperations /* final operation */ - || mptrOtherProgress.isNull()) - { - /* ASSUMES that Progress::WaitForOperationCompletion is using - AutoWriteLock::leave() as it saves us from duplicating the code! */ - hrc = Progress::WaitForOperationCompletion(aOperation, aTimeout); - } - else - { - LogFlowThisFunc(("calling the other object...\n")); - ComPtr ptrOtherProgress = mptrOtherProgress; - alock.release(); - - hrc = ptrOtherProgress->WaitForOperationCompletion(aOperation, aTimeout); - } - } + /* For now we'll always block locally. Later though, we could consider + blocking remotely when we can. */ + hrc = Progress::WaitForOperationCompletion(aOperation, aTimeout); LogFlowThisFuncLeave(); @@ -671,12 +541,21 @@ { AutoWriteLock alock(this COMMA_LOCKVAL_SRC_POS); - if (mptrOtherProgress.isNull() || !mCancelable) - hrc = Progress::Cancel(); + if (mCancelable) + { + if (!mptrOtherProgress.isNull()) + { + hrc = mptrOtherProgress->Cancel(); + if (SUCCEEDED(hrc)) + { + if (m_pfnCancelCallback) + m_pfnCancelCallback(m_pvCancelUserArg); + clearOtherProgressObjectInternal(true /*fEarly*/); + } + } + else + hrc = Progress::Cancel(); + } else - { - hrc = mptrOtherProgress->Cancel(); - if (SUCCEEDED(hrc)) - clearOtherProgressObjectInternal(false /*fEarly*/); - } + hrc = setError(E_FAIL, tr("Operation cannot be canceled")); } Index: /trunk/src/VBox/Main/VirtualBoxImpl.cpp =================================================================== --- /trunk/src/VBox/Main/VirtualBoxImpl.cpp (revision 29933) +++ /trunk/src/VBox/Main/VirtualBoxImpl.cpp (revision 29934) @@ -1995,21 +1995,16 @@ E_INVALIDARG); - /* get the teleporter enable state for the progress object init. */ - BOOL fTeleporterEnabled; - rc = machine->COMGETTER(TeleporterEnabled)(&fTeleporterEnabled); - if (FAILED(rc)) - return rc; - /* create a progress object */ ComObjPtr progress; progress.createObject(); rc = progress->init(this, - static_cast(machine), + static_cast (machine), Bstr(tr("Spawning session")), TRUE /* aCancelable */, - fTeleporterEnabled ? 20 : 10 /* uTotalOperationsWeight */, + 1 /* cOtherProgressObjects */, + 10 /* uTotalOperationsWeight */, Bstr(tr("Spawning session")), - 2 /* uFirstOperationWeight */, - fTeleporterEnabled ? 3 : 1 /* cOtherProgressObjectOperations */); + 3 /* uFirstOperationWeight */, + NULL /* pId */); if (SUCCEEDED(rc)) { @@ -4526,15 +4521,15 @@ ComPtr cbI; ComPtr cbD; - + cbI = sp; cbD = sp; /** - * Would be like this in ideal world, unfortunately our consumers want to be invoked via IDispatch, + * Would be like this in ideal world, unfortunately our consumers want to be invoked via IDispatch, * thus going the hard way. */ -#if 0 +#if 0 if (cbI != NULL) - { + { HRESULT hrc = handleCallback(cbI); if (hrc == VBOX_E_DONT_CALL_AGAIN) @@ -4549,14 +4544,14 @@ ::VariantClear(&varResult); - ::VariantClear(&arg1); + ::VariantClear(&arg1); ::VariantClear(&arg2); - + VARIANTARG args[] = {arg1, arg2}; DISPPARAMS disp = { args, NULL, sizeof(args)/sizeof(args[0]), 0}; cbD->Invoke(dispid, IID_NULL, - LOCALE_USER_DEFAULT, - DISPATCH_METHOD, - &disp, &varResult, + LOCALE_USER_DEFAULT, + DISPATCH_METHOD, + &disp, &varResult, NULL, NULL); } Index: /trunk/src/VBox/Main/include/ProgressProxyImpl.h =================================================================== --- /trunk/src/VBox/Main/include/ProgressProxyImpl.h (revision 29933) +++ /trunk/src/VBox/Main/include/ProgressProxyImpl.h (revision 29934) @@ -59,21 +59,17 @@ CBSTR bstrDescription, BOOL fCancelable, + ULONG cOtherProgressObjects, ULONG uTotalOperationsWeight, CBSTR bstrFirstOperationDescription, ULONG uFirstOperationWeight, - ULONG cOtherProgressObjectOperations); + OUT_GUID pId = NULL); void uninit(); // IProgress properties - STDMETHOD(COMGETTER(Cancelable))(BOOL *aCancelable); STDMETHOD(COMGETTER(Percent))(ULONG *aPercent); - STDMETHOD(COMGETTER(TimeRemaining))(LONG *aTimeRemaining); STDMETHOD(COMGETTER(Completed))(BOOL *aCompleted); STDMETHOD(COMGETTER(Canceled))(BOOL *aCanceled); STDMETHOD(COMGETTER(ResultCode))(LONG *aResultCode); STDMETHOD(COMGETTER(ErrorInfo))(IVirtualBoxErrorInfo **aErrorInfo); - //STDMETHOD(COMGETTER(OperationCount))(ULONG *aOperationCount); - not necessary - STDMETHOD(COMGETTER(Operation))(ULONG *aOperation); - STDMETHOD(COMGETTER(OperationDescription))(BSTR *aOperationDescription); STDMETHOD(COMGETTER(OperationPercent))(ULONG *aOperationPercent); STDMETHOD(COMSETTER(Timeout))(ULONG aTimeout); @@ -96,5 +92,6 @@ const char *aText, ...); bool notifyPointOfNoReturn(void); - bool setOtherProgressObject(IProgress *pOtherProgress); + bool setOtherProgressObject(IProgress *pOtherProgress, ULONG uOperationWeight); + bool clearOtherProgressObject(const char *pszLastOperationDescription, ULONG uLastOperationWeight); /** For com::SupportErrorInfoImpl. */ @@ -108,12 +105,8 @@ /** The other progress object. This can be NULL. */ ComPtr mptrOtherProgress; - /** Set if the other progress object has multiple operations. */ - bool mfMultiOperation; - /** The weight the other progress object started at. */ - ULONG muOtherProgressStartWeight; - /** The weight of other progress object. */ - ULONG muOtherProgressWeight; - /** The operation number the other progress object started at. */ - ULONG muOtherProgressStartOperation; + /** The number of other progress objects expected. */ + ULONG mcOtherProgressObjects; + /** The current other progress object. */ + ULONG miCurOtherProgressObject; };