Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
11786005:
Remove uses of PropVariantTo*; removing the need to DelayLoad propsys.dll. (Closed)
|
Created:
7 years, 11 months ago by gab Modified:
7 years, 11 months ago CC:
chromium-reviews, erikwright+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionRemove uses of PropVariantTo*; removing the need to DelayLoad propsys.dll.
Introduces base::win::ScopedPropVariant. A cleanup CL will follow to remove other specialized instances (e.g., string only) of a scoped PROPVARIANT and to fix unscoped PROPVARIANTs (some of which are leaking memory as it is now).
R=grt@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=177951
Patch Set 1 : #
Total comments: 6
Patch Set 2 : ScopedPropVariant #Patch Set 3 : +comment #
Total comments: 8
Patch Set 4 : better ScopedPropVariant #
Total comments: 11
Patch Set 5 : even better! #
Total comments: 11
Patch Set 6 : nits #
Total comments: 4
Patch Set 7 : comments #Patch Set 8 : merge up to r176700 #Patch Set 9 : dont call PropVariantClear if VT_EMPTY #
Messages
Total messages: 26 (0 generated)
Sir, as discussed on https://codereview.chromium.org/11712003/ :).
https://codereview.chromium.org/11786005/diff/12002/base/test/test_shortcut_w... File base/test/test_shortcut_win.cc (right): https://codereview.chromium.org/11786005/diff/12002/base/test/test_shortcut_w... base/test/test_shortcut_win.cc:124: PROPVARIANT pv_app_id; Init+Clear this as in the impl to avoid memory leaks. https://codereview.chromium.org/11786005/diff/12002/chrome/browser/shell_inte... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11786005/diff/12002/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:435: NOTREACHED(); PropVariantClear(&pv_app_id) here to ensure that whatever memory that was allocated by GetValue is released (or follow Robert's suggestion and use a scoped helper). https://codereview.chromium.org/11786005/diff/12002/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:461: NOTREACHED(); same comment here
Sir, created a ScopedPropVariant, PTAL. Wasn't sure how to make it easy to access the inner PROPVARIANT's members' though; overloaded the -> operator to basically do that; looks a little bit weird though since a PROPVARIANT isn't actually a pointer... a non-pointer getter is out of the question though since this would imply copying the struct...; I tried overloading the const& operator, but that didn't work (and it makes sense since that would allow implicit overriding of the . operator). Anyways, eager to hear your C++ master thoughts :)! Cheers! Gab https://codereview.chromium.org/11786005/diff/12002/base/test/test_shortcut_w... File base/test/test_shortcut_win.cc (right): https://codereview.chromium.org/11786005/diff/12002/base/test/test_shortcut_w... base/test/test_shortcut_win.cc:124: PROPVARIANT pv_app_id; On 2013/01/08 04:21:45, grt wrote: > Init+Clear this as in the impl to avoid memory leaks. Done. https://codereview.chromium.org/11786005/diff/12002/chrome/browser/shell_inte... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11786005/diff/12002/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:435: NOTREACHED(); On 2013/01/08 04:21:45, grt wrote: > PropVariantClear(&pv_app_id) here to ensure that whatever memory that was > allocated by GetValue is released (or follow Robert's suggestion and use a > scoped helper). Added a ScopedPropVariant to base/win. https://codereview.chromium.org/11786005/diff/12002/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:461: NOTREACHED(); On 2013/01/08 04:21:45, grt wrote: > same comment here Done.
https://codereview.chromium.org/11786005/diff/20007/base/win/scoped_propvaria... File base/win/scoped_propvariant.h (right): https://codereview.chromium.org/11786005/diff/20007/base/win/scoped_propvaria... base/win/scoped_propvariant.h:15: class ScopedPropVariant { it'd be nice if this scoper was safe to use. as it is, one could use operator& multiple times and leak. i suggest replacing operator& with a Receive() that has a DCHECK_EQ(pv_.vt, VT_EMPTY); see, for example, base/win/scoped_comptr.h. https://codereview.chromium.org/11786005/diff/20007/base/win/scoped_propvaria... base/win/scoped_propvariant.h:29: const PROPVARIANT* operator->() const { a conversion function would be a nice fit: operator const PROPVARIANT&() const { return pv_; } this allows consumers of ScopedPropVariant to use it exactly as a PROPVARIANT. you may also want: const PROPVARIANT* operator&() const { return &pv_; } so that it can be passed to funcs that require a pointer.
https://chromiumcodereview.appspot.com/11786005/diff/20007/base/win/scoped_pr... File base/win/scoped_propvariant.h (right): https://chromiumcodereview.appspot.com/11786005/diff/20007/base/win/scoped_pr... base/win/scoped_propvariant.h:15: class ScopedPropVariant { On 2013/01/09 14:30:10, grt wrote: > it'd be nice if this scoper was safe to use. as it is, one could use operator& > multiple times and leak. i suggest replacing operator& with a Receive() that has > a DCHECK_EQ(pv_.vt, VT_EMPTY); see, for example, base/win/scoped_comptr.h. A Reset() method that calls PropVariantClear would be useful, too.
Thanks, PTAL. https://codereview.chromium.org/11786005/diff/20007/base/win/scoped_propvaria... File base/win/scoped_propvariant.h (right): https://codereview.chromium.org/11786005/diff/20007/base/win/scoped_propvaria... base/win/scoped_propvariant.h:15: class ScopedPropVariant { On 2013/01/09 14:30:10, grt wrote: > it'd be nice if this scoper was safe to use. as it is, one could use operator& > multiple times and leak. i suggest replacing operator& with a Receive() that has > a DCHECK_EQ(pv_.vt, VT_EMPTY); see, for example, base/win/scoped_comptr.h. Ah good point, done. https://codereview.chromium.org/11786005/diff/20007/base/win/scoped_propvaria... base/win/scoped_propvariant.h:15: class ScopedPropVariant { On 2013/01/09 16:06:16, grt wrote: > On 2013/01/09 14:30:10, grt wrote: > > it'd be nice if this scoper was safe to use. as it is, one could use operator& > > multiple times and leak. i suggest replacing operator& with a Receive() that > has > > a DCHECK_EQ(pv_.vt, VT_EMPTY); see, for example, base/win/scoped_comptr.h. > > A Reset() method that calls PropVariantClear would be useful, too. Done. https://codereview.chromium.org/11786005/diff/20007/base/win/scoped_propvaria... base/win/scoped_propvariant.h:29: const PROPVARIANT* operator->() const { On 2013/01/09 14:30:10, grt wrote: > a conversion function would be a nice fit: > > operator const PROPVARIANT&() const { return pv_; } This doesn't work directly: i.e. pv_app_it.vt still won't compile. I can either use the -> operator instead (as I'm currently doing, but that feels hacky) or I can add a const PROPVARIANT& get() const { return pv_; } method and use pv_app_id.get().vt (which I also find ugly...). Any other clever C++ ideas? > > this allows consumers of ScopedPropVariant to use it exactly as a PROPVARIANT. > > you may also want: > > const PROPVARIANT* operator&() const { return &pv_; } > > so that it can be passed to funcs that require a pointer. Done. https://codereview.chromium.org/11786005/diff/29003/base/win/scoped_propvaria... File base/win/scoped_propvariant.h (right): https://codereview.chromium.org/11786005/diff/29003/base/win/scoped_propvaria... base/win/scoped_propvariant.h:22: DCHECK_EQ(result, S_OK); I could simply call Reset() here, but I felt it might be safer that the destructor doesn't depend on the implementation of Reset()... what do you think?
https://codereview.chromium.org/11786005/diff/20007/base/win/scoped_propvaria... File base/win/scoped_propvariant.h (right): https://codereview.chromium.org/11786005/diff/20007/base/win/scoped_propvaria... base/win/scoped_propvariant.h:29: const PROPVARIANT* operator->() const { On 2013/01/09 20:43:39, gab wrote: > On 2013/01/09 14:30:10, grt wrote: > > a conversion function would be a nice fit: > > > > operator const PROPVARIANT&() const { return pv_; } > > This doesn't work directly: i.e. pv_app_it.vt still won't compile. Doh! > I can either > use the -> operator instead (as I'm currently doing, but that feels hacky) or I > can add a > const PROPVARIANT& get() const { return pv_; } > method and use pv_app_id.get().vt (which I also find ugly...). Of these options, I prefer the get() method over operator->. > Any other clever C++ ideas? Inheritance rather than encapsulation would do it, but that would open the door to direct manipulation of the underlying PROPVARIANT so it's probably a bad idea. > > this allows consumers of ScopedPropVariant to use it exactly as a PROPVARIANT. > > > > you may also want: > > > > const PROPVARIANT* operator&() const { return &pv_; } > > > > so that it can be passed to funcs that require a pointer. > > Done. https://codereview.chromium.org/11786005/diff/29003/base/win/scoped_propvaria... File base/win/scoped_propvariant.h (right): https://codereview.chromium.org/11786005/diff/29003/base/win/scoped_propvaria... base/win/scoped_propvariant.h:9: #include "base/base_export.h" (for BASE_EXPORT) #include "base/basictypes.h" (for DISALLOW_COPY_AND_ASSIGN) #include "base/logging.h" (for DCHECK) https://codereview.chromium.org/11786005/diff/29003/base/win/scoped_propvaria... base/win/scoped_propvariant.h:15: class ScopedPropVariant { class BASE_EXPORT ScopedPropVariant { https://codereview.chromium.org/11786005/diff/29003/base/win/scoped_propvaria... base/win/scoped_propvariant.h:22: DCHECK_EQ(result, S_OK); On 2013/01/09 20:43:40, gab wrote: > I could simply call Reset() here, but I felt it might be safer that the > destructor doesn't depend on the implementation of Reset()... what do you think? My opinion: call Reset(). https://codereview.chromium.org/11786005/diff/29003/base/win/scoped_propvaria... base/win/scoped_propvariant.h:42: }; DISALLOW_COPY_AND_ASSIGN(ScopedPropVariant); also probably good to do the same for operator== and operator!= https://codereview.chromium.org/11786005/diff/29003/chrome/browser/shell_inte... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11786005/diff/29003/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:442: base::win::ScopedPropVariant pv_dual_mode; you could do away with this second instance by renaming pv_app_id to something more generic and explicitly calling its Reset method here.
Addressed comments and more, see below. Thanks! Gab https://codereview.chromium.org/11786005/diff/20007/base/win/scoped_propvaria... File base/win/scoped_propvariant.h (right): https://codereview.chromium.org/11786005/diff/20007/base/win/scoped_propvaria... base/win/scoped_propvariant.h:29: const PROPVARIANT* operator->() const { On 2013/01/10 03:24:05, grt wrote: > On 2013/01/09 20:43:39, gab wrote: > > On 2013/01/09 14:30:10, grt wrote: > > > a conversion function would be a nice fit: > > > > > > operator const PROPVARIANT&() const { return pv_; } > > > > This doesn't work directly: i.e. pv_app_it.vt still won't compile. > > Doh! > > > I can either > > use the -> operator instead (as I'm currently doing, but that feels hacky) or > I > > can add a > > const PROPVARIANT& get() const { return pv_; } > > method and use pv_app_id.get().vt (which I also find ugly...). > > Of these options, I prefer the get() method over operator->. Ok change removed operator->, added get() method. > > > Any other clever C++ ideas? > > Inheritance rather than encapsulation would do it, but that would open the door > to direct manipulation of the underlying PROPVARIANT so it's probably a bad > idea. > > > > this allows consumers of ScopedPropVariant to use it exactly as a > PROPVARIANT. > > > > > > you may also want: > > > > > > const PROPVARIANT* operator&() const { return &pv_; } > > > > > > so that it can be passed to funcs that require a pointer. > > > > Done. https://codereview.chromium.org/11786005/diff/29003/base/win/scoped_propvaria... File base/win/scoped_propvariant.h (right): https://codereview.chromium.org/11786005/diff/29003/base/win/scoped_propvaria... base/win/scoped_propvariant.h:9: On 2013/01/10 03:24:05, grt wrote: > #include "base/base_export.h" > (for BASE_EXPORT) > #include "base/basictypes.h" > (for DISALLOW_COPY_AND_ASSIGN) > #include "base/logging.h" > (for DCHECK) Oh oops, good catch! https://codereview.chromium.org/11786005/diff/29003/base/win/scoped_propvaria... base/win/scoped_propvariant.h:15: class ScopedPropVariant { On 2013/01/10 03:24:05, grt wrote: > class BASE_EXPORT ScopedPropVariant { This won't link, as discussed this is probably because nothing in base actually uses any of these methods and thus it doesn't get linked in base.dll (and the BASE_EXPORT) prevents the inlining of those methods where the header is included. FAILED: D:\depot_tools\python_bin\python.exe gyp-win-tool link-wrapper environment.x86 link.exe /nologo /OUT:base_unittests.exe /PDB:base_unittests.exe.pdb @base_unittests.exe.rsp && D:\depot_tools\python_bin\python.exe gyp-win-tool manifest-wrapper environment.x86 mt.exe -nologo -manifest obj\base\base_unittests.base_unittests.exe.intermediate.manifest -out:base_unittests.exe.manifest test_support_base.lib(test_support_base.test_shortcut_win.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __thiscall base::win::ScopedPropVariant::~ScopedPropVariant(void)" (__imp_??1ScopedPropVariant@win@base@@QAE@XZ) referenced in function "void __cdecl base::win::ValidateShortcut(class FilePath const &,struct base::win::ShortcutProperties const &)" (?ValidateShortcut@win@base@@YAXABVFilePath@@ABUShortcutProperties@12@@Z) test_support_base.lib(test_support_base.test_shortcut_win.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) public: struct tagPROPVARIANT const & __thiscall base::win::ScopedPropVariant::get(void)const " (__imp_?get@ScopedPropVariant@win@base@@QBEABUtagPROPVARIANT@@XZ) referenced in function "void __cdecl base::win::ValidateShortcut(class FilePath const &,struct base::win::ShortcutProperties const &)" (?ValidateShortcut@win@base@@YAXABVFilePath@@ABUShortcutProperties@12@@Z) test_support_base.lib(test_support_base.test_shortcut_win.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) public: struct tagPROPVARIANT * __thiscall base::win::ScopedPropVariant::Receive(void)" (__imp_?Receive@ScopedPropVariant@win@base@@QAEPAUtagPROPVARIANT@@XZ) referenced in function "void __cdecl base::win::ValidateShortcut(class FilePath const &,struct base::win::ShortcutProperties const &)" (?ValidateShortcut@win@base@@YAXABVFilePath@@ABUShortcutProperties@12@@Z) test_support_base.lib(test_support_base.test_shortcut_win.obj) : error LNK2019: unresolved external symbol "__declspec(dllimport) public: __thiscall base::win::ScopedPropVariant::ScopedPropVariant(void)" (__imp_??0ScopedPropVariant@win@base@@QAE@XZ) referenced in function "void __cdecl base::win::ValidateShortcut(class FilePath const &,struct base::win::ShortcutProperties const &)" (?ValidateShortcut@win@base@@YAXABVFilePath@@ABUShortcutProperties@12@@Z) base_unittests.exe : fatal error LNK1120: 4 unresolved externals https://codereview.chromium.org/11786005/diff/29003/base/win/scoped_propvaria... base/win/scoped_propvariant.h:22: DCHECK_EQ(result, S_OK); On 2013/01/10 03:24:05, grt wrote: > On 2013/01/09 20:43:40, gab wrote: > > I could simply call Reset() here, but I felt it might be safer that the > > destructor doesn't depend on the implementation of Reset()... what do you > think? > > My opinion: call Reset(). Done. https://codereview.chromium.org/11786005/diff/29003/base/win/scoped_propvaria... base/win/scoped_propvariant.h:42: }; On 2013/01/10 03:24:05, grt wrote: > DISALLOW_COPY_AND_ASSIGN(ScopedPropVariant); > also probably good to do the same for operator== and operator!= Done. https://codereview.chromium.org/11786005/diff/29003/chrome/browser/shell_inte... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11786005/diff/29003/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:442: base::win::ScopedPropVariant pv_dual_mode; On 2013/01/10 03:24:05, grt wrote: > you could do away with this second instance by renaming pv_app_id to something > more generic and explicitly calling its Reset method here. Even better, put the ScopedPropVariant outside the for loop so that the same memory can be re-used to check all shortcuts :). https://codereview.chromium.org/11786005/diff/22002/chrome/browser/shell_inte... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11786005/diff/22002/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:421: pv_buffer.Receive()) != S_OK) { I was thinking it could be nice if we replaced Receive() by ResetAndReceive() which would also guarantee we never leak and would prevent having to remember to call Reset() early above. This doesn't really go hand-in-hand with typical Scoped* classes in Chromium though perhaps... What do you think? Something like: PROPVARIANT* ResetAndReceive() { if (pv_.vt != VT_EMPTY) Reset(); return &pv_; }
lg https://codereview.chromium.org/11786005/diff/22002/base/win/scoped_propvaria... File base/win/scoped_propvariant.h (right): https://codereview.chromium.org/11786005/diff/22002/base/win/scoped_propvaria... base/win/scoped_propvariant.h:23: ~ScopedPropVariant() { nit: empty line before this https://codereview.chromium.org/11786005/diff/22002/base/win/scoped_propvaria... base/win/scoped_propvariant.h:27: PROPVARIANT* Receive() { i can haz doc comment? https://codereview.chromium.org/11786005/diff/22002/base/win/scoped_propvaria... base/win/scoped_propvariant.h:32: void Reset() { here, too? https://codereview.chromium.org/11786005/diff/22002/chrome/browser/shell_inte... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11786005/diff/22002/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:383: base::win::ScopedPropVariant pv_buffer; why buffer? https://codereview.chromium.org/11786005/diff/22002/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:421: pv_buffer.Receive()) != S_OK) { On 2013/01/10 15:30:42, gab wrote: > I was thinking it could be nice if we replaced Receive() by ResetAndReceive() > which would also guarantee we never leak and would prevent having to remember to > call Reset() early above. > > This doesn't really go hand-in-hand with typical Scoped* classes in Chromium > though perhaps... > > What do you think? > > Something like: > PROPVARIANT* ResetAndReceive() { > if (pv_.vt != VT_EMPTY) > Reset(); > return &pv_; > } I think code consistency is more important so that readers don't need to know/remember that this one is special for some reason.
Done, PTAL. Thanks! Gab https://codereview.chromium.org/11786005/diff/22002/base/win/scoped_propvaria... File base/win/scoped_propvariant.h (right): https://codereview.chromium.org/11786005/diff/22002/base/win/scoped_propvaria... base/win/scoped_propvariant.h:23: ~ScopedPropVariant() { On 2013/01/10 16:05:20, grt wrote: > nit: empty line before this Done. https://codereview.chromium.org/11786005/diff/22002/base/win/scoped_propvaria... base/win/scoped_propvariant.h:27: PROPVARIANT* Receive() { On 2013/01/10 16:05:20, grt wrote: > i can haz doc comment? Done. https://codereview.chromium.org/11786005/diff/22002/base/win/scoped_propvaria... base/win/scoped_propvariant.h:32: void Reset() { On 2013/01/10 16:05:20, grt wrote: > here, too? Done. https://codereview.chromium.org/11786005/diff/22002/chrome/browser/shell_inte... File chrome/browser/shell_integration_win.cc (right): https://codereview.chromium.org/11786005/diff/22002/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:383: base::win::ScopedPropVariant pv_buffer; On 2013/01/10 16:05:20, grt wrote: > why buffer? Idk, random name choice? |propvariant| better? https://codereview.chromium.org/11786005/diff/22002/chrome/browser/shell_inte... chrome/browser/shell_integration_win.cc:421: pv_buffer.Receive()) != S_OK) { On 2013/01/10 16:05:20, grt wrote: > On 2013/01/10 15:30:42, gab wrote: > > I was thinking it could be nice if we replaced Receive() by ResetAndReceive() > > which would also guarantee we never leak and would prevent having to remember > to > > call Reset() early above. > > > > This doesn't really go hand-in-hand with typical Scoped* classes in Chromium > > though perhaps... > > > > What do you think? > > > > Something like: > > PROPVARIANT* ResetAndReceive() { > > if (pv_.vt != VT_EMPTY) > > Reset(); > > return &pv_; > > } > > I think code consistency is more important so that readers don't need to > know/remember that this one is special for some reason. Ok that's what I thought too, but just checking. Thanks!
https://codereview.chromium.org/11786005/diff/29004/base/win/scoped_propvaria... File base/win/scoped_propvariant.h (right): https://codereview.chromium.org/11786005/diff/29004/base/win/scoped_propvaria... base/win/scoped_propvariant.h:28: // Leaks the underlying PROPVARIANT. This should only be called on a brand new i don't understand the comment. this doesn't leak anything. i was hoping for something more like: // Returns a pointer to the underlying PROPVARIANT for use as an out param in a function call. https://codereview.chromium.org/11786005/diff/29004/base/win/scoped_propvaria... base/win/scoped_propvariant.h:35: // Calls PropVariantClear() on the underlying PROPVARIANT which frees any how about something like this to document the method's use rather than its implementation: // Clears the instance to prepare it for re-use (e.g., via Receive).
Thanks for the better comments. https://codereview.chromium.org/11786005/diff/29004/base/win/scoped_propvaria... File base/win/scoped_propvariant.h (right): https://codereview.chromium.org/11786005/diff/29004/base/win/scoped_propvaria... base/win/scoped_propvariant.h:28: // Leaks the underlying PROPVARIANT. This should only be called on a brand new On 2013/01/10 20:58:08, grt wrote: > i don't understand the comment. this doesn't leak anything. i was hoping for > something more like: > // Returns a pointer to the underlying PROPVARIANT for use as an out param in > a function call. Done. https://codereview.chromium.org/11786005/diff/29004/base/win/scoped_propvaria... base/win/scoped_propvariant.h:35: // Calls PropVariantClear() on the underlying PROPVARIANT which frees any On 2013/01/10 20:58:08, grt wrote: > how about something like this to document the method's use rather than its > implementation: > // Clears the instance to prepare it for re-use (e.g., via Receive). Done.
lgtm. before committing, would you make a pass through the code to find other users of PROPVARIANT just to be sure that your new scoper meets their needs as well? it'd be nice to have a follow-on CL that updates them.
On 2013/01/11 14:57:16, grt wrote: > lgtm. before committing, would you make a pass through the code to find other > users of PROPVARIANT just to be sure that your new scoper meets their needs as > well? it'd be nice to have a follow-on CL that updates them. See comments below, let me know if this still LGTY with proposal of follow-up CL. Interestingly I found a similar class in an unnamed namespace, took the idea of only calling PropVariantClear() if vt != VT_EMPTY from it; does that make sense to you as well? https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/jumplist_w... I have otherwise already identified existing leaks that will be fixed by a the follow-up CL, e.g., https://code.google.com/searchframe#OAMlx_jo-ck/src/base/win/win_util.cc&exac... And an attempt at yet another ScopedPropVariant at https://code.google.com/searchframe#OAMlx_jo-ck/src/media/audio/win/core_audi... this one exposes type() and as_wide_string() instead of a generic get() method; I don't feel this scales well to the generic propvariant case though (although it looks like only string and boolean propvariants are used around the codebase). Although the getters are "nice" because they hide away the DCHECK they also prevent fallback in a case where we have an unexpected type and DCHECKs are turned off. I will do a follow-up clean up CL to get rid of those other instances and make sure all users of PROPVARIANT use my ScopedPropVariant instead. Cheers, Gab
On 2013/01/14 22:38:41, gab wrote: > On 2013/01/11 14:57:16, grt wrote: > > lgtm. before committing, would you make a pass through the code to find other > > users of PROPVARIANT just to be sure that your new scoper meets their needs as > > well? it'd be nice to have a follow-on CL that updates them. > > See comments below, let me know if this still LGTY with proposal of follow-up > CL. still lgtm. > Interestingly I found a similar class in an unnamed namespace, took the idea of > only calling PropVariantClear() if vt != VT_EMPTY from it; does that make sense > to you as well? Sure. There's nothing to be cleared if it's already empty. > https://code.google.com/searchframe#OAMlx_jo-ck/src/chrome/browser/jumplist_w... > > I have otherwise already identified existing leaks that will be fixed by a the > follow-up CL, e.g., > https://code.google.com/searchframe#OAMlx_jo-ck/src/base/win/win_util.cc&exac... > > And an attempt at yet another ScopedPropVariant at > https://code.google.com/searchframe#OAMlx_jo-ck/src/media/audio/win/core_audi... > this one exposes type() and as_wide_string() instead of a generic get() method; > I don't feel this scales well to the generic propvariant case though (although > it looks like only string and boolean propvariants are used around the > codebase). Although the getters are "nice" because they hide away the DCHECK > they also prevent fallback in a case where we have an unexpected type and > DCHECKs are turned off. > > I will do a follow-up clean up CL to get rid of those other instances and make > sure all users of PROPVARIANT use my ScopedPropVariant instead. > > Cheers, > Gab Sounds good. Feel free to add things like type() to your class in the cleanup CL if it'll make the code cleaner.
Perfect, thanks. @brettw for OWNER approval. Cheers, Gab
On 2013/01/15 15:45:33, gab wrote: > Perfect, thanks. > > @brettw for OWNER approval. Seems like the codereview tool hadn't picked up Brett's email correctly, just making sure you got this :)! > > Cheers, > Gab
@brettw: ping Thanks! Gab
How much does this need to be in base? It doesn't look like there are a lot of users of PROPVARIANT and they might all be in chrome except this one test case which I'm not very concerned about. What changes are you planning on making in your followup CL?
On 2013/01/18 20:56:55, brettw wrote: > How much does this need to be in base? It doesn't look like there are a lot of > users of PROPVARIANT and they might all be in chrome except this one test case > which I'm not very concerned about. What changes are you planning on making in > your followup CL? base/win/win_util.cc and media/audio/win/device_enumeration_win.cc use PROPVARIANT for example.
I'd like to get cpu's opinion on whether we should include this in base.
On 2013/01/18 22:10:10, brettw wrote: > I'd like to get cpu's opinion on whether we should include this in base. We already have base/win/scoped_variant.h FWIW.
I guess the issue is why the shortcut stuff is in base. We need another layer or move some visual stuff to src\ui But yes this can go in base imho. Brett, I and the other base owners should powwow soon and figure out a way to create a more layered base aka GTFO stuff.
ok, owners lgtm, didn't read the details.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/gab@chromium.org/11786005/37001
Message was sent while issue was closed.
Change committed as 177951 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
