device_orientation::ProviderImpl to stop polling thread asynchronously

device_orientation::ProviderImpl to stop polling thread asynchronously
and avoid doing that on the thread disallowing IO.

Encapsulate all tasks that need to be done on polling thread into
a different class and allow it to be deleted asynchronously. Also
update unit tests as MockOrientationFactory is no longer guaranteed
to be deleted after one test is done and before next test is starting.


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

[pivanof, 2012-07-30 03:29:12]

hans: Please review. Statistics for provider_impl.cc shows big numbers but it
mostly consists of shuffling code around to give it the new structure.

[hans, 2012-07-30 09:17:52]

+aousterh, who is currently doing some work on ProviderImpl

[hans, 2012-07-30 12:56:40]

Thanks very much for working on this! Just a couple of nits.

https://chromiumcodereview.appspot.com/10835030/diff/2001/content/browser/dev...
File content/browser/device_orientation/provider_impl.cc (right):

https://chromiumcodereview.appspot.com/10835030/diff/2001/content/browser/dev...
content/browser/device_orientation/provider_impl.cc:43: explicit
PollingThread(const char* name,
nit: no need for explicit here, I think

https://chromiumcodereview.appspot.com/10835030/diff/2001/content/browser/dev...
content/browser/device_orientation/provider_impl.cc:49: void
InitializePollingThread(
since it's a member of PollingThread, I think just calling this "Initialize"
would be fine?

https://chromiumcodereview.appspot.com/10835030/diff/2001/content/browser/dev...
content/browser/device_orientation/provider_impl.cc:81: : base::Thread(name),
nit indentation looks off here: there should be four spaces before the :

https://chromiumcodereview.appspot.com/10835030/diff/2001/content/browser/dev...
content/browser/device_orientation/provider_impl.cc:94: for (Iterator i =
factories.begin(), e = factories.end(); i != e; ++i) {
nit: since you're touching this line anyway, feel free to rewrite as "for
(Iterator i = factories.begin(); i != factories.end(); ++i) {"

https://chromiumcodereview.appspot.com/10835030/diff/2001/content/browser/dev...
content/browser/device_orientation/provider_impl.cc:264: for (Iterator i =
observers_.begin(), e = observers_.end(); i != e; ++i)
nit: since you're touching this line anyway, feel free to rewrite as "for
(Iterator i = observers_.begin(); i != observers_.end(); ++i)"

https://chromiumcodereview.appspot.com/10835030/diff/2001/content/browser/dev...
File content/browser/device_orientation/provider_impl.h (right):

https://chromiumcodereview.appspot.com/10835030/diff/2001/content/browser/dev...
content/browser/device_orientation/provider_impl.h:37: friend class
PollingThread;
nit: it feels like only one of these should be necessary, but if your compiler
insists, then I won't argue about it :)

https://chromiumcodereview.appspot.com/10835030/diff/2001/content/browser/dev...
File content/browser/device_orientation/provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/10835030/diff/2001/content/browser/dev...
content/browser/device_orientation/provider_unittest.cc:328:
MockOrientationFactory::SetCurInstance(NULL);
maybe insert a blank line before this, so it doesn't look like the comment above
is connected to the call to SetCurInstance.

[pivanof, 2012-07-30 16:43:42]

Agreed with everything and updated.

On 2012/07/30 12:56:40, hans wrote:
>
https://chromiumcodereview.appspot.com/10835030/diff/2001/content/browser/dev...
> File content/browser/device_orientation/provider_impl.h (right):
> 
>
https://chromiumcodereview.appspot.com/10835030/diff/2001/content/browser/dev...
> content/browser/device_orientation/provider_impl.h:37: friend class
> PollingThread;
> nit: it feels like only one of these should be necessary, but if your compiler
> insists, then I won't argue about it :)

It turns out clang doesn't need "friend" here which is a little bit of a
surprise to me (I would expect that other line was unnecessary at least in some
compilers). So I removed it, let's see what other compilers think about that.

[hans, 2012-07-30 16:59:35]

On 2012/07/30 16:43:42, pivanof wrote:
> Agreed with everything and updated.
> 
> On 2012/07/30 12:56:40, hans wrote:
> >
>
https://chromiumcodereview.appspot.com/10835030/diff/2001/content/browser/dev...
> > File content/browser/device_orientation/provider_impl.h (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/10835030/diff/2001/content/browser/dev...
> > content/browser/device_orientation/provider_impl.h:37: friend class
> > PollingThread;
> > nit: it feels like only one of these should be necessary, but if your
compiler
> > insists, then I won't argue about it :)
> 
> It turns out clang doesn't need "friend" here which is a little bit of a
> surprise to me (I would expect that other line was unnecessary at least in
some
> compilers). So I removed it, let's see what other compilers think about that.

Thanks! I've started try jobs, we'll see what they say.

In the meantime, I got this error from the windows bot: 
"provider_impl.cc(46) :error C3665:
'device_orientation::ProviderImpl::PollingThread::~PollingThread' : override
specifier 'override' not allowed on a destructor"

I guess there shouldn't be an override on that destructor. It's interesting that
Clang didn't complain about this..

[pivanof, 2012-07-30 17:59:50]

hans: I've updated to fix compilation failure in Win and test failure on Mac (I
wonder why it didn't fail like that on Linux?). Could you please submit a try
for that?

[hans, 2012-07-30 18:40:47]

On 2012/07/30 17:59:50, pivanof wrote:
> hans: I've updated to fix compilation failure in Win and test failure on Mac
(I
> wonder why it didn't fail like that on Linux?). Could you please submit a try
> for that?

Thanks! I've started new try jobs.

[hans, 2012-07-30 18:41:16]

On 2012/07/30 18:40:47, hans wrote:
> Thanks! I've started new try jobs.

oh, and the patch is LGTM assuming the try bots think so too :)

[commit-bot: I haz the power, 2012-07-30 20:16:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/paivanof@gmail.com/10835030/6002

[commit-bot: I haz the power, 2012-07-30 23:56:07]

Try job failure for 10835030-6002 (retry) (retry) on win_rel for step
"browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[pivanof, 2012-07-31 00:10:32]

hans: Do I understand correctly that if try job failure looks completely
unrelated and earlier the same (?) try job was successful I should just
re-commit?

[commit-bot: I haz the power, 2012-07-31 00:48:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/paivanof@gmail.com/10835030/6002

[commit-bot: I haz the power, 2012-07-31 02:15:59]

Change committed as 149117

[hans, 2012-07-31 10:31:19]

On 2012/07/31 00:10:32, pivanof wrote:
> hans: Do I understand correctly that if try job failure looks completely
> unrelated and earlier the same (?) try job was successful I should just
> re-commit?

That's correct. Looks like it landed successfully :)