Changes ProviderImpl to use a single DataFetcherFactory

Changes ProviderImpl to use a single DataFetcherFactory instead of a
vector of DataFetcherFactories

In the current implementation, any OS can only use at most one DataFetcher, so
there will always be 0 or 1 DataFetcherFactories in the vector. Therefore it
is simpler to use a single DataFetcherFactory instead of an array. Since DataFetcherFactory is a pointer itself, it can be NULL when there are no available DataFetchers.

BUG=none


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

[hans, 2012-08-01 19:08:46]

Thanks! This makes things simpler.

I think we should be able to pass around the factory by value rather than by
pointer though, as it is already a pointer actually.

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
File content/browser/device_orientation/provider.cc (right):

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
content/browser/device_orientation/provider.cc:25: const
ProviderImpl::DataFetcherFactory *default_factory = NULL;
hmm, I thought DataFetcherFactory was of type "pointer to function that returns
a DataFetcher", so I'm surprised that default_factory is becoming a pointer..

but it also seems to work below :/ i'm confused.

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
File content/browser/device_orientation/provider_impl.cc (right):

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
content/browser/device_orientation/provider_impl.cc:93: if (factory != NULL) {
i wonder if it would make the code simpler to check if factory == NULL first,
and bail out early if so

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
content/browser/device_orientation/provider_impl.cc:193: factory_ = new
DataFetcherFactory(*factory);
this looks overly complex; i think it should just boil down to factory_ =
factory

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
File content/browser/device_orientation/provider_impl.h (right):

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
content/browser/device_orientation/provider_impl.h:27: // Create a ProviderImpl
that uses the pointer to a factory to create a
i don't think it should take a pointer to a factory, i think it should just take
a "factory"

because the factory is already a pointer, so it can be NULL for example, i
believe

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
File content/browser/device_orientation/provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
content/browser/device_orientation/provider_unittest.cc:170: provider_ = new
ProviderImpl(&factory);
here too i think it's just the value that should be passed in, not a pointer to
it

[aousterh, 2012-08-02 10:10:37]

Thanks for the comments! I switched it to pass the DataFetcherFactory by value
instead of as a pointer and that makes things a bit simpler.

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
File content/browser/device_orientation/provider.cc (right):

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
content/browser/device_orientation/provider.cc:25: const
ProviderImpl::DataFetcherFactory *default_factory = NULL;
On 2012/08/01 19:08:47, hans wrote:
> hmm, I thought DataFetcherFactory was of type "pointer to function that
returns
> a DataFetcher", so I'm surprised that default_factory is becoming a pointer..
> 
> but it also seems to work below :/ i'm confused.

Hmmm I'm not sure why it was working before. But now default_factory is just a
DataFetcherFactory.

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
File content/browser/device_orientation/provider_impl.cc (right):

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
content/browser/device_orientation/provider_impl.cc:93: if (factory != NULL) {
On 2012/08/01 19:08:47, hans wrote:
> i wonder if it would make the code simpler to check if factory == NULL first,
> and bail out early if so

I don't think it would really be simpler, because we need to call
ScheduleDoNotify(Orientation::Empty()) in two cases - when factory == NULL and
when factory != NULL but the fetcher can't provide Orientation data. So if we
checked for factory == NULL first and then bailed, we would have to duplicate
the call ScheduleDoNotify(Orientation::Empty()).

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
content/browser/device_orientation/provider_impl.cc:193: factory_ = new
DataFetcherFactory(*factory);
On 2012/08/01 19:08:47, hans wrote:
> this looks overly complex; i think it should just boil down to factory_ =
> factory

Done.

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
File content/browser/device_orientation/provider_impl.h (right):

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
content/browser/device_orientation/provider_impl.h:27: // Create a ProviderImpl
that uses the pointer to a factory to create a
On 2012/08/01 19:08:47, hans wrote:
> i don't think it should take a pointer to a factory, i think it should just
take
> a "factory"
> 
> because the factory is already a pointer, so it can be NULL for example, i
> believe

Good point, that does simply things further. Done.

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
File content/browser/device_orientation/provider_unittest.cc (right):

https://chromiumcodereview.appspot.com/10837055/diff/1/content/browser/device...
content/browser/device_orientation/provider_unittest.cc:170: provider_ = new
ProviderImpl(&factory);
On 2012/08/01 19:08:47, hans wrote:
> here too i think it's just the value that should be passed in, not a pointer
to
> it

Done.

[hans, 2012-08-02 10:19:07]

On 2012/08/02 10:10:37, aousterh wrote:
> Thanks for the comments! I switched it to pass the DataFetcherFactory by value
> instead of as a pointer and that makes things a bit simpler.

Great! LGTM

[hans, 2012-08-02 10:19:30]

Oh, and maybe we should update the CL description a little, it still says
"pointer to a ..Factory".

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

https://chromiumcodereview.appspot.com/10837055/diff/7001/content/browser/dev...
content/browser/device_orientation/provider_impl.h:27: // Create a ProviderImpl
that uses the pointer to a factory to create a
we can just say "uses the factory to create a ..."

[commit-bot: I haz the power, 2012-08-02 10:28:06]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/aousterh@chromium.org/10837055/11

[commit-bot: I haz the power, 2012-08-02 11:56:24]

Change committed as 149614