Miscellaneous tiny cleanups done while converting files to use ScopedCOMInitializer, pulled out sep…

Miscellaneous tiny cleanups done while converting files to use ScopedCOMInitializer, pulled out separately to make review easier.

BUG=none
TEST=none
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=159526

[dmazzoni, 2012-09-27 06:22:38]

https://codereview.chromium.org/10991052/diff/1/chrome/browser/ui/views/acces...
File
chrome/browser/ui/views/accessibility/browser_views_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/10991052/diff/1/chrome/browser/ui/views/acces...
chrome/browser/ui/views/accessibility/browser_views_accessibility_browsertest.cc:62:
::CoInitialize(NULL);
Should this be using ScopedComInitializer as long as you're refactoring?

https://codereview.chromium.org/10991052/diff/1/chrome/browser/ui/views/acces...
chrome/browser/ui/views/accessibility/browser_views_accessibility_browsertest.cc:65:
BrowserViewsAccessibilityTest~BrowserViewsAccessibilityTest() {
Typo here?

[Peter Kasting, 2012-09-27 06:25:15]

https://codereview.chromium.org/10991052/diff/1/chrome/browser/ui/views/acces...
File
chrome/browser/ui/views/accessibility/browser_views_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/10991052/diff/1/chrome/browser/ui/views/acces...
chrome/browser/ui/views/accessibility/browser_views_accessibility_browsertest.cc:62:
::CoInitialize(NULL);
On 2012/09/27 06:22:39, Dominic Mazzoni wrote:
> Should this be using ScopedComInitializer as long as you're refactoring?

This is precisely why I said: "...done while converting files to use
ScopedCOMInitializer, pulled out separately to make review easier." in my CL
description.

Because I'll be making that change separately.

https://codereview.chromium.org/10991052/diff/1/chrome/browser/ui/views/acces...
chrome/browser/ui/views/accessibility/browser_views_accessibility_browsertest.cc:65:
BrowserViewsAccessibilityTest~BrowserViewsAccessibilityTest() {
On 2012/09/27 06:22:39, Dominic Mazzoni wrote:
> Typo here?

Indeed.  Good catch.

[grt (UTC plus 2), 2012-09-27 12:59:27]

chrome_frame lgtm

[cpu_(ooo_6.6-7.5), 2012-09-27 19:14:36]

lgtm

No need to act on the comments, they are just rants. I was expecting you to
change code to use more scopedcominitializer but I frankly I am fine to keep as
is.

https://codereview.chromium.org/10991052/diff/11001/chrome/browser/history/hi...
File chrome/browser/history/history_publisher_win.cc (right):

https://codereview.chromium.org/10991052/diff/11001/chrome/browser/history/hi...
chrome/browser/history/history_publisher_win.cc:29: for
(base::win::RegistryKeyIterator r_iter(root, path); r_iter.Valid();
I wonder if >> still trips vs2010. I assume it trips vs2008. Just rambling here.

https://codereview.chromium.org/10991052/diff/11001/chrome/browser/ui/views/a...
File
chrome/browser/ui/views/accessibility/browser_views_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/10991052/diff/11001/chrome/browser/ui/views/a...
chrome/browser/ui/views/accessibility/browser_views_accessibility_browsertest.cc:110:
::VariantInit(&acc_role);
Note that we have wrappers for variants as well.

https://codereview.chromium.org/10991052/diff/11001/printing/backend/win_help...
File printing/backend/win_helper.cc (right):

https://codereview.chromium.org/10991052/diff/11001/printing/backend/win_help...
printing/backend/win_helper.cc:260: // printer drivers will use).
and this is the kind of crap that make one hate com.

[Peter Kasting, 2012-09-27 19:50:31]

https://codereview.chromium.org/10991052/diff/11001/chrome/browser/ui/views/a...
File
chrome/browser/ui/views/accessibility/browser_views_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/10991052/diff/11001/chrome/browser/ui/views/a...
chrome/browser/ui/views/accessibility/browser_views_accessibility_browsertest.cc:110:
::VariantInit(&acc_role);
On 2012/09/27 19:14:36, cpu wrote:
> Note that we have wrappers for variants as well.

Awesome.  I have an upcoming change to use ScopedBSTR in this function, I'll add
in something for the VARIANT as well.

[Peter Kasting, 2012-09-29 21:46:17]

More reviewers for OWNERS:

sky - ui/, chrome/browser/history/
scottbyer - printing/
rsleevi - net/

[Scott Byer, 2012-09-29 22:25:32]

Printing LGTM. We'll want to watch the slight change in logic. It looks more
correct, but, well, because printer drivers.

On 2012/09/29 21:46:17, Peter Kasting wrote:
> More reviewers for OWNERS:
> 
> sky - ui/, chrome/browser/history/
> scottbyer - printing/
> rsleevi - net/

[Peter Kasting, 2012-09-30 04:44:04]

On 2012/09/29 22:25:32, Scott Byer wrote:
> Printing LGTM. We'll want to watch the slight change in logic. It looks more
> correct, but, well, because printer drivers.

MSDN documents CoInitializeEx() as returning three possible values, of which
RPC_E_CHANGED_MODE is the only non-SUCCEEDED.  We also rely on this elsewhere in
Chrome.  MSDN could be wrong, but since this is a kernel function and not one
that printer drivers have anything to do with, I feel pretty safe.

[Ryan Sleevi, 2012-10-01 14:21:00]

The change in net/ needs work, but the work is so trivial that there's no need
for review, so LGTM.

https://codereview.chromium.org/10991052/diff/6005/net/url_request/url_reques...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/10991052/diff/6005/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:705: CoInitialize(NULL);
According to the e-mail that went out, there shouldn't be explicit calls to
CoInitialize, and that functions should rely on the thread being configured
appropriately.

It seems like this should either be ScopedCOMInitializer (in order to ensure
that decrement of the COM initialization refcount on test failure) or, and this
seems entirely more appropriate, that net_unittests should be calling
CoInitialize in the overall test setup ( net/base/run_all_unittests.cc )

[sky, 2012-10-01 14:56:35]

LGTM

https://codereview.chromium.org/10991052/diff/6005/chrome/browser/ui/views/ac...
File
chrome/browser/ui/views/accessibility/browser_views_accessibility_browsertest.cc
(right):

https://codereview.chromium.org/10991052/diff/6005/chrome/browser/ui/views/ac...
chrome/browser/ui/views/accessibility/browser_views_accessibility_browsertest.cc:35:
~BrowserViewsAccessibilityTest();
virtual

[Peter Kasting, 2012-10-01 18:18:54]

https://codereview.chromium.org/10991052/diff/6005/net/url_request/url_reques...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/10991052/diff/6005/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:705: CoInitialize(NULL);
On 2012/10/01 14:21:01, Ryan Sleevi wrote:
> It seems like this should either be ScopedCOMInitializer (in order to ensure
> that decrement of the COM initialization refcount on test failure) or, and
this
> seems entirely more appropriate, that net_unittests should be calling
> CoInitialize in the overall test setup ( net/base/run_all_unittests.cc )

I am in fact going to change this to ScopedCOMInitializer -- that's why I was
touching this code in the first place.  I've just split the changes into pieces
to make them more reviewable.

I'm not opposed to the idea of moving the initialization to happening in
net/base, but I don't know if it's necessary.  This seems to be the only place
in all of net/ that wants COM initialized.  Do you still want to move the
initialization?

[Ryan Sleevi, 2012-10-01 20:54:42]

https://codereview.chromium.org/10991052/diff/6005/net/url_request/url_reques...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/10991052/diff/6005/net/url_request/url_reques...
net/url_request/url_request_unittest.cc:705: CoInitialize(NULL);
On 2012/10/01 18:18:55, Peter Kasting wrote:
> On 2012/10/01 14:21:01, Ryan Sleevi wrote:
> > It seems like this should either be ScopedCOMInitializer (in order to ensure
> > that decrement of the COM initialization refcount on test failure) or, and
> this
> > seems entirely more appropriate, that net_unittests should be calling
> > CoInitialize in the overall test setup ( net/base/run_all_unittests.cc )
> 
> I am in fact going to change this to ScopedCOMInitializer -- that's why I was
> touching this code in the first place.  I've just split the changes into
pieces
> to make them more reviewable.
> 
> I'm not opposed to the idea of moving the initialization to happening in
> net/base, but I don't know if it's necessary.  This seems to be the only place
> in all of net/ that wants COM initialized.  Do you still want to move the
> initialization?

For some reason, I thought there were more places in net/ where we were doing
crap like this with shortcuts.

I think it's fine to just change this call, and in a future CL, so LGTM with no
nits :)