Fix the indirect pure virtual calls in DataFetcherSharedMemoryBase destructor.

Fix the indirect pure virtual calls in DataFetcherSharedMemoryBase destructor.

StopFetchingDeviceData() may not be used in the destructor of this class because
it can call the pure virtual Stop() method. This patch fixes this issue by
introducing a StopFetchingAllDeviceData() method and calling it from the derived
DataFetcherSharedMemory class.

BUG=414632
Committed: https://crrev.com/694cd937163f3eb45a2e52dd7da7899c2c9db146
Cr-Commit-Position: refs/heads/master@{#295458}

[timvolodine, 2014-09-17 16:23:00]

timvolodine@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2014-09-17 16:41:01]

https://codereview.chromium.org/578863002/diff/20001/content/browser/device_s...
File content/browser/device_sensors/data_fetcher_shared_memory_base.cc (right):

https://codereview.chromium.org/578863002/diff/20001/content/browser/device_s...
content/browser/device_sensors/data_fetcher_shared_memory_base.cc:107:
DCHECK(started_consumers_ == 0);
If you use DCHECK_EQ() (with the expected value first!), you'll get a nicer
error message on failure.

https://codereview.chromium.org/578863002/diff/20001/content/browser/device_s...
File content/browser/device_sensors/data_fetcher_shared_memory_base.h (right):

https://codereview.chromium.org/578863002/diff/20001/content/browser/device_s...
content/browser/device_sensors/data_fetcher_shared_memory_base.h:80: // Should
be used in deriving classes to make sure all active sensors are
Nit: "derived"

https://codereview.chromium.org/578863002/diff/20001/content/browser/device_s...
content/browser/device_sensors/data_fetcher_shared_memory_base.h:82: void
StopFetchingAllDeviceData();
Instead of requiring each subclass to call this method in its destructor,
couldn't you make this method public and call it before the owner destroys this
object?

[timvolodine, 2014-09-17 18:25:00]

https://codereview.chromium.org/578863002/diff/20001/content/browser/device_s...
File content/browser/device_sensors/data_fetcher_shared_memory_base.cc (right):

https://codereview.chromium.org/578863002/diff/20001/content/browser/device_s...
content/browser/device_sensors/data_fetcher_shared_memory_base.cc:107:
DCHECK(started_consumers_ == 0);
On 2014/09/17 16:41:00, Bernhard Bauer wrote:
> If you use DCHECK_EQ() (with the expected value first!), you'll get a nicer
> error message on failure.

right, done.

https://codereview.chromium.org/578863002/diff/20001/content/browser/device_s...
File content/browser/device_sensors/data_fetcher_shared_memory_base.h (right):

https://codereview.chromium.org/578863002/diff/20001/content/browser/device_s...
content/browser/device_sensors/data_fetcher_shared_memory_base.h:80: // Should
be used in deriving classes to make sure all active sensors are
On 2014/09/17 16:41:00, Bernhard Bauer wrote:
> Nit: "derived"

Done.

https://codereview.chromium.org/578863002/diff/20001/content/browser/device_s...
content/browser/device_sensors/data_fetcher_shared_memory_base.h:82: void
StopFetchingAllDeviceData();
On 2014/09/17 16:41:00, Bernhard Bauer wrote:
> Instead of requiring each subclass to call this method in its destructor,
> couldn't you make this method public and call it before the owner destroys
this
> object?

Done. though not sure it's that much better..

[Bernhard Bauer, 2014-09-17 21:36:37]

LGTM!

https://codereview.chromium.org/578863002/diff/20001/content/browser/device_s...
File content/browser/device_sensors/data_fetcher_shared_memory_base.h (right):

https://codereview.chromium.org/578863002/diff/20001/content/browser/device_s...
content/browser/device_sensors/data_fetcher_shared_memory_base.h:82: void
StopFetchingAllDeviceData();
On 2014/09/17 18:25:00, timvolodine wrote:
> On 2014/09/17 16:41:00, Bernhard Bauer wrote:
> > Instead of requiring each subclass to call this method in its destructor,
> > couldn't you make this method public and call it before the owner destroys
> this
> > object?
> 
> Done. though not sure it's that much better..

Well, it means you only have to care about this in two places in one file, one
of these places being testing code.

(Really, the way around this would be to use delegation instead of
implementation inheritance).

[timvolodine, 2014-09-18 11:36:39]

On 2014/09/17 21:36:37, Bernhard Bauer wrote:
> LGTM!
> 
>
https://codereview.chromium.org/578863002/diff/20001/content/browser/device_s...
> File content/browser/device_sensors/data_fetcher_shared_memory_base.h (right):
> 
>
https://codereview.chromium.org/578863002/diff/20001/content/browser/device_s...
> content/browser/device_sensors/data_fetcher_shared_memory_base.h:82: void
> StopFetchingAllDeviceData();
> On 2014/09/17 18:25:00, timvolodine wrote:
> > On 2014/09/17 16:41:00, Bernhard Bauer wrote:
> > > Instead of requiring each subclass to call this method in its destructor,
> > > couldn't you make this method public and call it before the owner destroys
> > this
> > > object?
> > 
> > Done. though not sure it's that much better..
> 
> Well, it means you only have to care about this in two places in one file, one
> of these places being testing code.
> 
> (Really, the way around this would be to use delegation instead of
> implementation inheritance).

thanks for review!

[timvolodine, 2014-09-18 11:37:01]

The CQ bit was checked by timvolodine@chromium.org

[commit-bot: I haz the power, 2014-09-18 11:37:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/578863002/80001

[commit-bot: I haz the power, 2014-09-18 12:10:01]

Committed patchset #5 (id:80001) as 67fb256774e422810d7963d4fa8aa0848b48a804

[commit-bot: I haz the power, 2014-09-18 12:10:44]

Patchset 5 (id:??) landed as
https://crrev.com/694cd937163f3eb45a2e52dd7da7899c2c9db146
Cr-Commit-Position: refs/heads/master@{#295458}