Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
10532099:
Switching to IDispatch::Invoke when calling Omaha interfaces. (Closed)
|
Created:
8 years, 6 months ago by alexeypa (please no reviews) Modified:
8 years, 6 months ago Reviewers:
Wez CC:
chromium-reviews, jamiewalch+watch_chromium.org, dcaiafa+watch_chromium.org, simonmorris+watch_chromium.org, hclam+watch_chromium.org, wez+watch_chromium.org, amit, sanjeevr, garykac+watch_chromium.org, lambroslambrou+watch_chromium.org, alexeypa+watch_chromium.org, sergeyu+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Chromoting] Using IDispatch::Invoke() to call Omaha inetrfaces.
Omaha uses non-standard scheme for versioning its COM interfaces. Instead of letting the callers to know a fixed UUID and prototype of an interface, Omaha relies on the callers to use late binding via IDispatch::Invoke. This way it can extend an interface after it has been released by adding new members and changing its UUID. The limitation is that vtable can no longer be used.
BUG=131498
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141960
Patch Set 1 #
Total comments: 25
Patch Set 2 : CR feedback. #
Total comments: 19
Patch Set 3 : More CR feedback. #Patch Set 4 : rebased. #
Messages
Total messages: 10 (0 generated)
PTAL.
On 2012/06/12 00:03:16, alexeypa wrote: > PTAL. Ping!
Mostly looks good, but the CL description needs a summary of why we want to switch to using un-typed Invoke() rather than the typed interfaces! https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... File remoting/base/dispatch_win.h (right): https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h:3: // DO NOT EDIT BY HAND!!! I'm going to assume you haven't edited by hand, and not review by eye. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... File remoting/base/dispatch_win.h.pump (right): https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:8: $$ It is choosend to match the number of arguments base::Bind() supports. choosend -> chosen https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:31: // |VARIANT| structures (output parameters). Why do you need that? If Marshal() were simply an overloaded method then would compilation not fail for unsupported types simply because of the missing overload? https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:39: // |ScopedVariantArg| to a pointer to a vector of |VARIANTARG| strcutures. strcutures -> structures https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:39: // |ScopedVariantArg| to a pointer to a vector of |VARIANTARG| strcutures. For this to make sense the reader needs two bits of context that may not be obvious; that IDispatch provides an un-typed method-invokation interface for COM objects, and that the parameters are passed using VARIANTs and may be in or out parameters - hence our need to manage resources allocated to hold the parameters, and the fact that this helper class has slightly odd-looking marshal/unmarshal interfaces. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:40: class ScopedVariantArg : public VARIANTARG { Would it make sense to derived from base::win::ScopedVariant here instead? https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:53: HRESULT Marshall(const T& param) { Marshall -> Marshal https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:65: const base::win::ScopedVariant& param) { Do you need this? ScopedVariant has an implicit conversion to const VARIANT* defined. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:70: HRESULT Marshall<VARIANT*>(VARIANT* const & param) { Add a comment to clarify that this is the out-parameter specialization, so we don't copy the |param| value into the argument in this case. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:77: void Unmarshall(const T& param_out) { Unmarshall -> Unmarshal https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:133: COMPILE_ASSERT(internal::is_param<P$(ARG)>::value, parameters_must_be_variants); nit: Line too long? https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:164: DISPPARAMS disp_params = { $if ARITY > 0 [[disp_args]] $else [[NULL]], NULL, $(ARITY), 0 }; nit: Line too long?
FYI. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... File remoting/base/dispatch_win.h.pump (right): https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:8: $$ It is choosend to match the number of arguments base::Bind() supports. On 2012/06/13 00:34:36, Wez wrote: > choosend -> chosen Done. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:31: // |VARIANT| structures (output parameters). On 2012/06/13 00:34:36, Wez wrote: > Why do you need that? It wasn't really needed. Overloaded methods work much better in this case. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:39: // |ScopedVariantArg| to a pointer to a vector of |VARIANTARG| strcutures. On 2012/06/13 00:34:36, Wez wrote: > strcutures -> structures Done. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:39: // |ScopedVariantArg| to a pointer to a vector of |VARIANTARG| strcutures. On 2012/06/13 00:34:36, Wez wrote: > For this to make sense the reader needs two bits of context that may not be > obvious; that IDispatch provides an un-typed method-invokation interface for COM > objects, and that the parameters are passed using VARIANTs and may be in or out > parameters - hence our need to manage resources allocated to hold the > parameters, and the fact that this helper class has slightly odd-looking > marshal/unmarshal interfaces. Done. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:40: class ScopedVariantArg : public VARIANTARG { On 2012/06/13 00:34:36, Wez wrote: > Would it make sense to derived from base::win::ScopedVariant here instead? I don't think so. base::win::ScopedVariant seems to be prone to adding new data members/virtual methods in the future. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:53: HRESULT Marshall(const T& param) { On 2012/06/13 00:34:36, Wez wrote: > Marshall -> Marshal Done. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:65: const base::win::ScopedVariant& param) { On 2012/06/13 00:34:36, Wez wrote: > Do you need this? ScopedVariant has an implicit conversion to const VARIANT* > defined. The implicit conversion works if it is an overloaded method (it does not work for template specialization). Done. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:70: HRESULT Marshall<VARIANT*>(VARIANT* const & param) { On 2012/06/13 00:34:36, Wez wrote: > Add a comment to clarify that this is the out-parameter specialization, so we > don't copy the |param| value into the argument in this case. Done. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:77: void Unmarshall(const T& param_out) { On 2012/06/13 00:34:36, Wez wrote: > Unmarshall -> Unmarshal Done. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:133: COMPILE_ASSERT(internal::is_param<P$(ARG)>::value, parameters_must_be_variants); On 2012/06/13 00:34:36, Wez wrote: > nit: Line too long? This line was removed. https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:164: DISPPARAMS disp_params = { $if ARITY > 0 [[disp_args]] $else [[NULL]], NULL, $(ARITY), 0 }; On 2012/06/13 00:34:36, Wez wrote: > nit: Line too long? Only the one in .pump. I changed it so that the line is short enough in both .h and .pump.
LGTM w/ a couple of typos & nits, but the CL still needs a proper description of why we want to use an un-typed API rather than a typed one! https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... File remoting/base/dispatch_win.h.pump (right): https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:40: class ScopedVariantArg : public VARIANTARG { On 2012/06/13 16:39:47, alexeypa wrote: > On 2012/06/13 00:34:36, Wez wrote: > > Would it make sense to derived from base::win::ScopedVariant here instead? > > I don't think so. base::win::ScopedVariant seems to be prone to adding new data > members/virtual methods in the future. Actually I'd missed that this derives from VARIANTARG while ScopedVariant contains a VARIANT, so that won't work anyway! https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... File remoting/base/dispatch_win.h (right): https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:28: // - [out] parameters are initialized by IDispatch::Invoke(). It if up to typo: if -> is https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:29: // the caller to free leakable variants (such as VT_DISPATCH). It's also the case that any value in the VARIANT out-param that the caller supplies is freed by this wrapper when it is replaced with the value from Invoke, I think? https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:36: // provides marshaling methods that convert between C++ types and VARIANTs. nit: This implies that you could pass e.g. an int to the call and it would be wrapped, but I think you mean specific C++ types like ScopedVariant? https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:42: // It should be possible to cast a pointer to an array of |ScopedVariantArg| to nit: should -> must? https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:56: HRESULT Marshal(const VARIANT& param) { nit: I would recommend CopyFrom rather than Marshal, which implies some form of serialization taking place. https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:67: void Unmarshal(const VARIANT& param_out) { nit: Similarly, I'd recommend MoveTo rather then Unmarshal. https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:72: *param_out = *this; Do you need to VariantClear() param_out before replacing it (see comment above re out-param semantics)?
FYI https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... File remoting/base/dispatch_win.h.pump (right): https://chromiumcodereview.appspot.com/10532099/diff/1/remoting/base/dispatch... remoting/base/dispatch_win.h.pump:40: class ScopedVariantArg : public VARIANTARG { VARIANT and VARIANTARG is the same thing. https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... File remoting/base/dispatch_win.h (right): https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:28: // - [out] parameters are initialized by IDispatch::Invoke(). It if up to On 2012/06/13 17:55:16, Wez wrote: > typo: if -> is Done. https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:29: // the caller to free leakable variants (such as VT_DISPATCH). On 2012/06/13 17:55:16, Wez wrote: > It's also the case that any value in the VARIANT out-param that the caller > supplies is freed by this wrapper when it is replaced with the value from > Invoke, I think? No, it is not. One should be able to pass a pointer to uninitialized VARIANT as an [out] parameter. https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:36: // provides marshaling methods that convert between C++ types and VARIANTs. On 2012/06/13 17:55:16, Wez wrote: > nit: This implies that you could pass e.g. an int to the call and it would be > wrapped, but I think you mean specific C++ types like ScopedVariant? With proper support (i.e. Marshal/Unmarshal methods taking int) it will work. https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:42: // It should be possible to cast a pointer to an array of |ScopedVariantArg| to On 2012/06/13 17:55:16, Wez wrote: > nit: should -> must? Done. https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:56: HRESULT Marshal(const VARIANT& param) { On 2012/06/13 17:55:16, Wez wrote: > nit: I would recommend CopyFrom rather than Marshal, which implies some form of > serialization taking place. CopyFrom sounds confusing to me. It also does not imply direct relation to MoveTo. How about Wrap/Unwrap? https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:72: *param_out = *this; On 2012/06/13 17:55:16, Wez wrote: > Do you need to VariantClear() param_out before replacing it (see comment above > re out-param semantics)? No, *param_out is not initialized at this point.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/alexeypa@chromium.org/10532099/1010
Change committed as 141960
https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... File remoting/base/dispatch_win.h (right): https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:29: // the caller to free leakable variants (such as VT_DISPATCH). On 2012/06/13 18:45:11, alexeypa wrote: > On 2012/06/13 17:55:16, Wez wrote: > > It's also the case that any value in the VARIANT out-param that the caller > > supplies is freed by this wrapper when it is replaced with the value from > > Invoke, I think? > > No, it is not. One should be able to pass a pointer to uninitialized VARIANT as > an [out] parameter. In which case it's important that the supplied VARIANTARG not have been initialized with leakable content, otherwise it will be leaked; that should be clarified in the comments. https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:36: // provides marshaling methods that convert between C++ types and VARIANTs. On 2012/06/13 18:45:11, alexeypa wrote: > On 2012/06/13 17:55:16, Wez wrote: > > nit: This implies that you could pass e.g. an int to the call and it would be > > wrapped, but I think you mean specific C++ types like ScopedVariant? > > With proper support (i.e. Marshal/Unmarshal methods taking int) it will work. OK; consider adding a line to clarify that currently only the ScopedVariant wrapper C++ class is supported. https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:56: HRESULT Marshal(const VARIANT& param) { On 2012/06/13 18:45:11, alexeypa wrote: > On 2012/06/13 17:55:16, Wez wrote: > > nit: I would recommend CopyFrom rather than Marshal, which implies some form > of > > serialization taking place. > > CopyFrom sounds confusing to me. It also does not imply direct relation to > MoveTo. > > How about Wrap/Unwrap? CopyFrom() describes the actual action that the call performs, though. Wrap doesn't - you don't wrap in-parameters, you create a copy to pass to the API. Actually, why do you need to take a copy, rather than passing the caller-managed VARIANT in directly? Isn't VARIANT resource management only required for out-params, really?
FYI https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... File remoting/base/dispatch_win.h (right): https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:29: // the caller to free leakable variants (such as VT_DISPATCH). On 2012/06/13 21:02:16, Wez wrote: > In which case it's important that the supplied VARIANTARG not have been > initialized with leakable content, otherwise it will be leaked; that should be > clarified in the comments. The same rule applies to any other C++ function taking out parameters as pointers. I'll add a note to the comments. https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:36: // provides marshaling methods that convert between C++ types and VARIANTs. On 2012/06/13 21:02:16, Wez wrote: > OK; consider adding a line to clarify that currently only the ScopedVariant > wrapper C++ class is supported. Will do. https://chromiumcodereview.appspot.com/10532099/diff/8002/remoting/base/dispa... remoting/base/dispatch_win.h:56: HRESULT Marshal(const VARIANT& param) { On 2012/06/13 21:02:16, Wez wrote: > CopyFrom() describes the actual action that the call performs, though. Wrap > doesn't - you don't wrap in-parameters, you create a copy to pass to the API. I still think CopyFrom() is one level too verbose than needed. The actions performed is "prepare the parameter for passing to API" and "pass the parameter back ro the original caller". The fact that we copy it is just an implementation detail. > Actually, why do you need to take a copy, rather than passing the caller-managed > VARIANT in directly? Isn't VARIANT resource management only required for > out-params, really? When it is time to clean up I don't know if the parameter is [in] or [out]. It has to be cleaned in either case. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
