Use Scoped[Bstr,ComPtr,Variant] instead of their ATL equivalents to reduce de...

Use Scoped[Bstr,ComPtr,Variant] instead of their ATL equivalents to reduce dependencies on ATL.

BUG=5027
TEST=none

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=25879

[tommi (sloooow) - chröme, 2009-09-08 21:15:32]

here are some initial thoughts.  patch only partially reviewed.

http://codereview.chromium.org/200045/diff/1016/26
File base/scoped_bstr_win.cc (right):

http://codereview.chromium.org/200045/diff/1016/26#newcode14
Line 14: ScopedBstr::ScopedBstr(const char* non_bstr) {
I would rather not add this constructor.  I did not include it initially on
principle :)

It opens the door to start doing something like

ScopedBstr foo("foo");
instead of
ScopedBstr foo(L"foo");

Plenty of unnecessary code + cpu cycles spent.

http://codereview.chromium.org/200045/diff/1016/27
File base/scoped_variant_win.cc (right):

http://codereview.chromium.org/200045/diff/1016/27#newcode32
Line 32: var_.vt = vt;
add a DCHECK that vt is either VT_DATE or VT_R8?

http://codereview.chromium.org/200045/diff/1016/31
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/200045/diff/1016/31#newcode258
Line 258: *def_action = ScopedBstr(response().output_string.c_str()).Release();
Allocating a ScopedBstr (or CComBSTR previously) that includes
construction+automatic destruction with all the error checks etc seems wasteful
and not really what scoped classes are all about.

What if we instead add a static method to ScopedBstr that allocates a BSTR?

static BSTR AllocateBSTR(const wchar_t* wide) {
  return ::SysAllocateString(wide);
}

static BSTR AllocateBSTRFromASCII(const char* ascii) {
  return AllocateBSTR(ASCIIToWide(ascii).c_str());
}

[James Hawkins, 2009-09-08 22:15:17]

http://codereview.chromium.org/200045/diff/1016/26
File base/scoped_bstr_win.cc (right):

http://codereview.chromium.org/200045/diff/1016/26#newcode14
Line 14: ScopedBstr::ScopedBstr(const char* non_bstr) {
On 2009/09/08 21:15:32, tommi wrote:
> I would rather not add this constructor.  I did not include it initially on
> principle :)
> 
> It opens the door to start doing something like
> 
> ScopedBstr foo("foo");
> instead of
> ScopedBstr foo(L"foo");
> 
> Plenty of unnecessary code + cpu cycles spent.

Ok, that makes sense.  I've removed the constructor.

http://codereview.chromium.org/200045/diff/1016/27
File base/scoped_variant_win.cc (right):

http://codereview.chromium.org/200045/diff/1016/27#newcode32
Line 32: var_.vt = vt;
On 2009/09/08 21:15:32, tommi wrote:
> add a DCHECK that vt is either VT_DATE or VT_R8?

Done.

http://codereview.chromium.org/200045/diff/1016/31
File chrome/browser/browser_accessibility.cc (right):

http://codereview.chromium.org/200045/diff/1016/31#newcode258
Line 258: *def_action = ScopedBstr(response().output_string.c_str()).Release();
On 2009/09/08 21:15:32, tommi wrote:
> Allocating a ScopedBstr (or CComBSTR previously) that includes
> construction+automatic destruction with all the error checks etc seems
wasteful
> and not really what scoped classes are all about.
> 
> What if we instead add a static method to ScopedBstr that allocates a BSTR?
> 
> static BSTR AllocateBSTR(const wchar_t* wide) {
>   return ::SysAllocateString(wide);
> }
> 
> static BSTR AllocateBSTRFromASCII(const char* ascii) {
>   return AllocateBSTR(ASCIIToWide(ascii).c_str());
> }
> 

Why don't we just replace these with direct calls to
SysAllocateString(ASCIIToWide(..))?  ScopedBstr already has a method Allocate()
which takes ownership of the BSTR, so this new method could be confused with the
previous one.  Also, wrapping SysAllocateString seems contrary to the purpose of
the class.

[tommi1, 2009-09-09 05:51:11]

On Tue, Sep 8, 2009 at 6:15 PM, <jhawkins@chromium.org> wrote:

>
> http://codereview.chromium.org/200045/diff/1016/26
> File base/scoped_bstr_win.cc (right):
>
> http://codereview.chromium.org/200045/diff/1016/26#newcode14
> Line 14: ScopedBstr::ScopedBstr(const char* non_bstr) {
> On 2009/09/08 21:15:32, tommi wrote:
>
>> I would rather not add this constructor.  I did not include it
>>
> initially on
>
>> principle :)
>>
>
>  It opens the door to start doing something like
>>
>
>  ScopedBstr foo("foo");
>> instead of
>> ScopedBstr foo(L"foo");
>>
>
>  Plenty of unnecessary code + cpu cycles spent.
>>
>
> Ok, that makes sense.  I've removed the constructor.
>
> http://codereview.chromium.org/200045/diff/1016/27
> File base/scoped_variant_win.cc (right):
>
> http://codereview.chromium.org/200045/diff/1016/27#newcode32
> Line 32: var_.vt = vt;
> On 2009/09/08 21:15:32, tommi wrote:
>
>> add a DCHECK that vt is either VT_DATE or VT_R8?
>>
>
> Done.
>
> http://codereview.chromium.org/200045/diff/1016/31
> File chrome/browser/browser_accessibility.cc (right):
>
> http://codereview.chromium.org/200045/diff/1016/31#newcode258
> Line 258: *def_action =
> ScopedBstr(response().output_string.c_str()).Release();
> On 2009/09/08 21:15:32, tommi wrote:
>
>> Allocating a ScopedBstr (or CComBSTR previously) that includes
>> construction+automatic destruction with all the error checks etc seems
>>
> wasteful
>
>> and not really what scoped classes are all about.
>>
>
>  What if we instead add a static method to ScopedBstr that allocates a
>>
> BSTR?
>
>  static BSTR AllocateBSTR(const wchar_t* wide) {
>>   return ::SysAllocateString(wide);
>> }
>>
>
>  static BSTR AllocateBSTRFromASCII(const char* ascii) {
>>   return AllocateBSTR(ASCIIToWide(ascii).c_str());
>> }
>>
>
>
> Why don't we just replace these with direct calls to
> SysAllocateString(ASCIIToWide(..))?  ScopedBstr already has a method
> Allocate() which takes ownership of the BSTR, so this new method could
> be confused with the previous one.  Also, wrapping SysAllocateString
> seems contrary to the purpose of the class.

sgtm :)

>
>
> http://codereview.chromium.org/200045
>

[James Hawkins, 2009-09-09 21:48:13]

Oops, don't waste your time reviewing yet; there are more changes I need to
make.

[James Hawkins, 2009-09-09 22:00:01]

Patch Set 6 is ready for review.  I changed the CComBSTR(...).Release() to
SysAllocString().

[tommi (sloooow) - chröme, 2009-09-10 00:38:52]

lgtm.

http://codereview.chromium.org/200045/diff/5036/5049
File chrome/browser/history/history_publisher_win.cc (right):

http://codereview.chromium.org/200045/diff/5036/5049#newcode120
Line 120: ScopedVariant(var_time, VT_DATE),
I know this isn't your code, but ... unless indexers_.size() is always 1 or 0,
all of these scoped variables should be allocated outside the loop (unless
indexers_.size() is 0 of course... then nothing should be done).

I'll leave it up to you whether you want to do something about it in this patch.

http://codereview.chromium.org/200045/diff/5036/5049#newcode136
Line 136: ScopedVariant(var_begin_time, VT_DATE),
same here

[James Hawkins, 2009-09-10 18:08:27]

On 2009/09/10 00:38:52, tommi wrote:
> lgtm.
> 
> http://codereview.chromium.org/200045/diff/5036/5049
> File chrome/browser/history/history_publisher_win.cc (right):
> 
> http://codereview.chromium.org/200045/diff/5036/5049#newcode120
> Line 120: ScopedVariant(var_time, VT_DATE),
> I know this isn't your code, but ... unless indexers_.size() is always 1 or 0,
> all of these scoped variables should be allocated outside the loop (unless
> indexers_.size() is 0 of course... then nothing should be done).
> 
> I'll leave it up to you whether you want to do something about it in this
patch.
> 
> http://codereview.chromium.org/200045/diff/5036/5049#newcode136
> Line 136: ScopedVariant(var_begin_time, VT_DATE),
> same here

Done.

[tommi (sloooow) - chröme, 2009-09-10 20:22:41]

lgtm

http://codereview.chromium.org/200045/diff/6043/5063
File chrome/browser/history/history_publisher_win.cc (right):

http://codereview.chromium.org/200045/diff/6043/5063#newcode118
Line 118: ScopedVariant time(var_time, VT_DATE);
one more absolute nit...
you might want to surround this entire block and the one in
DeleteUserHistoryBetween() with an

if (indexers_.size()) {
  ...
}

that way we're close to zero cycles for the most common case :)

thanks again for doing this.

[tommi (sloooow) - chröme, 2009-09-10 20:25:35]

hey james - I see you've already committed the patch.. nevermind the last
bit then (unless you need to touch that file again)

On Thu, Sep 10, 2009 at 4:22 PM, <tommi@chromium.org> wrote:

> lgtm
>
>
> http://codereview.chromium.org/200045/diff/6043/5063
> File chrome/browser/history/history_publisher_win.cc (right):
>
> http://codereview.chromium.org/200045/diff/6043/5063#newcode118
> Line 118: ScopedVariant time(var_time, VT_DATE);
> one more absolute nit...
> you might want to surround this entire block and the one in
> DeleteUserHistoryBetween() with an
>
> if (indexers_.size()) {
>  ...
> }
>
> that way we're close to zero cycles for the most common case :)
>
> thanks again for doing this.
>
>
> http://codereview.chromium.org/200045
>