rlz: Remove #include <windows.h>

rlz: Remove #include <windows.h>

To make this possible, replace _beginthread() with BrowserThread::PostTask(). This happens to fix a TODO.

BUG=106213, 46579
TEST=no functionality change

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

[Nico, 2012-02-16 00:59:44]

akalin for the threading thing that you consulted me on earlier today. rogerta
for the rest.

[akalin, 2012-02-16 01:03:36]

http://codereview.chromium.org/9404021/diff/5001/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

http://codereview.chromium.org/9404021/diff/5001/chrome/browser/rlz/rlz.cc#ne...
chrome/browser/rlz/rlz.cc:236: base::Bind(&RLZTracker::PingNowImpl,
base::Unretained(this)));
does PingNowImpl actually depend on instance variables?  If so, you probably
want a weak ptr here.  If not, maybe PingNowImpl should be static

http://codereview.chromium.org/9404021/diff/5001/chrome/browser/rlz/rlz.cc#ne...
chrome/browser/rlz/rlz.cc:240: // This is the entry point of a background
thread, so I/O is allowed.
update this comment, although is the allow_io still needed?

[akalin, 2012-02-16 01:08:24]

http://codereview.chromium.org/9404021/diff/5001/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

http://codereview.chromium.org/9404021/diff/5001/chrome/browser/rlz/rlz.cc#ne...
chrome/browser/rlz/rlz.cc:252: SendFinancialPing(brand, lang, referral)) {
does this do network IO or file IO?  If it's network IO, it probably belongs on
the IO thread instead

[Nico, 2012-02-16 01:11:41]

Thanks!

http://codereview.chromium.org/9404021/diff/5001/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

http://codereview.chromium.org/9404021/diff/5001/chrome/browser/rlz/rlz.cc#ne...
chrome/browser/rlz/rlz.cc:236: base::Bind(&RLZTracker::PingNowImpl,
base::Unretained(this)));
On 2012/02/16 01:03:36, akalin wrote:
> does PingNowImpl actually depend on instance variables?  If so, you probably
> want a weak ptr here.  If not, maybe PingNowImpl should be static

Yes, it seems to look at cache_lock_. The instance lives in a singleton, so it 
shouldn't go away (and the class isn't SupportsWeakPtr<>).

http://codereview.chromium.org/9404021/diff/5001/chrome/browser/rlz/rlz.cc#ne...
chrome/browser/rlz/rlz.cc:240: // This is the entry point of a background
thread, so I/O is allowed.
On 2012/02/16 01:03:36, akalin wrote:
> update this comment, although is the allow_io still needed?

Guess not. Done.

[Nico, 2012-02-16 01:13:30]

http://codereview.chromium.org/9404021/diff/5001/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

http://codereview.chromium.org/9404021/diff/5001/chrome/browser/rlz/rlz.cc#ne...
chrome/browser/rlz/rlz.cc:252: SendFinancialPing(brand, lang, referral)) {
On 2012/02/16 01:08:25, akalin wrote:
> does this do network IO or file IO?  If it's network IO, it probably belongs
on
> the IO thread instead

I believe this accesses the registry behind the scenes which does hit the disk.

[akalin, 2012-02-16 01:22:24]

lgtm

http://codereview.chromium.org/9404021/diff/1007/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

http://codereview.chromium.org/9404021/diff/1007/chrome/browser/rlz/rlz.cc#ne...
chrome/browser/rlz/rlz.cc:231: // TODO(thakis): Once akalin's TaskRunner around
PostDelayedTask() is done,
PostDelayedTask() -> PostTask()?

[Nico, 2012-02-16 01:27:13]

Thanks!

rogerta, I'll wait for your thumbs up too.

http://codereview.chromium.org/9404021/diff/1007/chrome/browser/rlz/rlz.cc
File chrome/browser/rlz/rlz.cc (right):

http://codereview.chromium.org/9404021/diff/1007/chrome/browser/rlz/rlz.cc#ne...
chrome/browser/rlz/rlz.cc:231: // TODO(thakis): Once akalin's TaskRunner around
PostDelayedTask() is done,
On 2012/02/16 01:22:24, akalin wrote:
> PostDelayedTask() -> PostTask()?

Done.

[akalin, 2012-02-16 01:40:54]

On 2012/02/16 01:11:41, Nico wrote:
> Yes, it seems to look at cache_lock_. The instance lives in a singleton, so it

> shouldn't go away (and the class isn't SupportsWeakPtr<>).

Actually, I'd feel better if you added a WeakPtrFactory<RLZTracker> member
variable and get weak ptrs from that.

[Roger Tawa OOO till Jul 10th, 2012-02-16 01:56:17]

lgtm

[Roger Tawa OOO till Jul 10th, 2012-02-16 01:57:15]

Hi Nico,

Can you also resolve http://crbug.com/106213?

Thanks.

[commit-bot: I haz the power, 2012-02-16 02:01:10]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/thakis@chromium.org/9404021/1009

[akalin, 2012-02-16 02:34:07]

On 2012/02/16 02:01:10, I haz the power (commit-bot) wrote:
> CQ is trying da patch. Follow status at
> https://chromium-status.appspot.com/cq/thakis%40chromium.org/9404021/1009

Not gonna address my last comment? :/

[Nico, 2012-02-16 02:40:43]

On 2012/02/16 02:34:07, akalin wrote:
> On 2012/02/16 02:01:10, I haz the power (commit-bot) wrote:
> > CQ is trying da patch. Follow status at
> > https://chromium-status.appspot.com/cq/thakis%2540chromium.org/9404021/1009
> 
> Not gonna address my last comment? :/

The new code matches the current semantics, so I don't think it's urgent. I'll
touch this file another 1231452 times in the next few days, so I figured I'd do
it in one of those patches. (Don't want to switch branches right now, mostly.) I
hope that's ok?

[akalin, 2012-02-16 03:34:51]

On 2012/02/16 02:40:43, Nico wrote:
> On 2012/02/16 02:34:07, akalin wrote:
> > On 2012/02/16 02:01:10, I haz the power (commit-bot) wrote:
> > > CQ is trying da patch. Follow status at
> > >
https://chromium-status.appspot.com/cq/thakis%252540chromium.org/9404021/1009
> > 
> > Not gonna address my last comment? :/
> 
> The new code matches the current semantics, so I don't think it's urgent. I'll
> touch this file another 1231452 times in the next few days, so I figured I'd
do
> it in one of those patches. (Don't want to switch branches right now, mostly.)
I
> hope that's ok?

OK, fair enough.

[commit-bot: I haz the power, 2012-02-16 03:41:52]

Change committed as 122235

[akalin, 2012-03-23 01:27:33]

On 2012/02/16 01:40:54, akalin wrote:

> Actually, I'd feel better if you added a WeakPtrFactory<RLZTracker> member
> variable and get weak ptrs from that.

Actually, I was wrong.  WeakPtrs aren't thread-safe, so you can't use them.