Geolocation cleanup: corrects uses of content::AccessTokenStore* and net::URLRequestContextGetter*

Geolocation cleanup: corrects uses of content::AccessTokenStore* and net::URLRequestContextGetter*

This CL corrects uses of content::AccessTokenStore* to scoped_refptr<>
versions, since that class is ref-counted.

Same applies toc* .

Also this CL does some cleanups in the touched files:
- s/NULL/nullptr/
- makes const and/or preferred initialize over assignment for
  some member variables, where applicable/available,
- method ::GetAccessTokenStore() is made private.
- NewSystemLocationProvider(); is changed to return std::unique_ptr<>

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="Geolocation*"
./out/gn/content_unittests --gtest_filter=Geolocation*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

BUG=623114
Committed: https://crrev.com/1161c5753adf296ea253d56f1617d3d926152b55
Cr-Commit-Position: refs/heads/master@{#406410}

[mcasas, 2016-07-09 01:27:42]

Description was changed from

==========
Geolocation cleanup 2: make GeolocationDelegate::CreateAccessTokenStore() return
scoped_refptr

WIP

BUG=623114
==========

to

==========
Geolocation cleanup 2: make GeolocationDelegate::CreateAccessTokenStore() return
scoped_refptr

What the title says: 
AccessTokenStore* GeolocationDelegate::CreateAccessTokenStore()

but AccessTokenStore is refcounted [1]

[1]
https://cs.chromium.org/chromium/src/content/public/browser/access_token_stor...


BUG=623114
TEST=relevant content_unittests and 
browser_tests passing.
==========

[mcasas, 2016-07-09 01:28:20]

Description was changed from

==========
Geolocation cleanup 2: make GeolocationDelegate::CreateAccessTokenStore() return
scoped_refptr

What the title says: 
AccessTokenStore* GeolocationDelegate::CreateAccessTokenStore()

but AccessTokenStore is refcounted [1]

[1]
https://cs.chromium.org/chromium/src/content/public/browser/access_token_stor...


BUG=623114
TEST=relevant content_unittests and 
browser_tests passing.
==========

to

==========
Geolocation cleanup 2: make GeolocationDelegate::CreateAccessTokenStore() return
scoped_refptr

What the title says: 
AccessTokenStore* GeolocationDelegate::CreateAccessTokenStore()

but AccessTokenStore is refcounted [1]

[1]
https://cs.chromium.org/chromium/src/content/public/browser/access_token_stor...


BUG=623114

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[mcasas, 2016-07-09 01:38:39]

Patchset #1 (id:1) has been deleted

[mcasas, 2016-07-11 21:08:09]

Patchset #1 (id:20001) has been deleted

[mcasas, 2016-07-11 21:08:49]

mcasas@chromium.org changed reviewers:
+ lethalantidote@chromium.org, wez@chromium.org

[mcasas, 2016-07-11 21:08:50]

wez@/lethalantidote@ PTAL

[mcasas, 2016-07-12 20:50:31]

ping (easy CL).

[CJ, 2016-07-12 21:03:39]

lgtm

[Wez, 2016-07-12 21:38:19]

nit: Can you tidy up the CL description - it's too terse, which is going to be
really confusing for anyone reading it out of context.  e.g. it says "what the
title says" but then has a copy of the old function signature, and "but... is
refcounted".  What you're saying is just that the API contract is that it's
ref-counted, but it was being returned as a bare-pointer, making that
non-obvious.

[mcasas, 2016-07-12 21:44:11]

Description was changed from

==========
Geolocation cleanup 2: make GeolocationDelegate::CreateAccessTokenStore() return
scoped_refptr

What the title says: 
AccessTokenStore* GeolocationDelegate::CreateAccessTokenStore()

but AccessTokenStore is refcounted [1]

[1]
https://cs.chromium.org/chromium/src/content/public/browser/access_token_stor...


BUG=623114

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Geolocation cleanup 2: make GeolocationDelegate::CreateAccessTokenStore() return
scoped_refptr

GeolocationDelegate interface method:
  AccessTokenStore* GeolocationDelegate::CreateAccessTokenStore()
should not return a bare pointer since
AccessTokenStore is refcounted [1]. This 
CL corrects that wrong return type.

[1]
https://cs.chromium.org/chromium/src/content/public/browser/access_token_stor...


BUG=623114

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[mcasas, 2016-07-12 21:44:35]

Description was changed from

==========
Geolocation cleanup 2: make GeolocationDelegate::CreateAccessTokenStore() return
scoped_refptr

GeolocationDelegate interface method:
  AccessTokenStore* GeolocationDelegate::CreateAccessTokenStore()
should not return a bare pointer since
AccessTokenStore is refcounted [1]. This 
CL corrects that wrong return type.

[1]
https://cs.chromium.org/chromium/src/content/public/browser/access_token_stor...


BUG=623114

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Geolocation cleanup: correct GeolocationDelegate::CreateAccessTokenStore() to
return scoped_refptr

GeolocationDelegate interface method:
  AccessTokenStore* GeolocationDelegate::CreateAccessTokenStore()
should not return a bare pointer since
AccessTokenStore is refcounted [1]. This 
CL corrects that wrong return type.

[1]
https://cs.chromium.org/chromium/src/content/public/browser/access_token_stor...


BUG=623114

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[mcasas, 2016-07-12 21:44:51]

On 2016/07/12 21:38:19, Wez wrote:
> nit: Can you tidy up the CL description - it's too terse, which is going to be
> really confusing for anyone reading it out of context.  e.g. it says "what the
> title says" but then has a copy of the old function signature, and "but... is
> refcounted".  What you're saying is just that the API contract is that it's
> ref-counted, but it was being returned as a bare-pointer, making that
> non-obvious.

CL description comments addressed, PTAL.

[Wez, 2016-07-12 21:55:47]

https://codereview.chromium.org/2129313002/diff/40001/content/browser/geoloca...
File content/browser/geolocation/location_arbitrator_impl.cc (right):

https://codereview.chromium.org/2129313002/diff/40001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl.cc:151: return
access_token_store_.get();
Update this to

if (!access_token_store_)
  ...

https://codereview.chromium.org/2129313002/diff/40001/content/browser/geoloca...
File content/browser/geolocation/location_arbitrator_impl.h (right):

https://codereview.chromium.org/2129313002/diff/40001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl.h:63: virtual
scoped_refptr<AccessTokenStore> NewAccessTokenStore();
If you're changing this then I'd suggest also updating:

scoped_refptr<> GetAccessTokenStore()

.. NewNetworkLocationProvider(
  const scoped_refptr<>& access_token_store,
  const scoped_refptr<>& url_request_context_getter,
  ...
  )

Updating the URLRequestContextGetter* instances to scoped_refptr should be
pretty self-contained to this file, though I notice that there is also a bare
reference in the AccessTokenStore callback signature.

https://codereview.chromium.org/2129313002/diff/40001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl.h:123: LocationProvider*
NewSystemLocationProvider();
nit: While we're here... it's weird that this is a bare pointer when the version
in the class is not.

Why do we even need both the static and member function versions?

[Wez, 2016-07-12 21:56:16]

CL description text looks good, but has weird line-wrap!

[mcasas, 2016-07-12 22:05:26]

Description was changed from

==========
Geolocation cleanup: correct GeolocationDelegate::CreateAccessTokenStore() to
return scoped_refptr

GeolocationDelegate interface method:
  AccessTokenStore* GeolocationDelegate::CreateAccessTokenStore()
should not return a bare pointer since
AccessTokenStore is refcounted [1]. This 
CL corrects that wrong return type.

[1]
https://cs.chromium.org/chromium/src/content/public/browser/access_token_stor...


BUG=623114

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Geolocation cleanup: correct GeolocationDelegate::CreateAccessTokenStore() to
return scoped_refptr

GeolocationDelegate interface method:
  AccessTokenStore* GeolocationDelegate::CreateAccessTokenStore()
should not return a bare pointer since AccessTokenStore is refcounted [1]. This 
CL corrects that wrong return type.

[1]
https://cs.chromium.org/chromium/src/content/public/browser/access_token_stor...


BUG=623114

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Michael van Ouwerkerk, 2016-07-13 10:56:34]

drive-by!

https://codereview.chromium.org/2129313002/diff/40001/content/browser/geoloca...
File content/browser/geolocation/location_arbitrator_impl.h (right):

https://codereview.chromium.org/2129313002/diff/40001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl.h:123: LocationProvider*
NewSystemLocationProvider();
On 2016/07/12 21:55:47, Wez wrote:
> nit: While we're here... it's weird that this is a bare pointer when the
version
> in the class is not.
> 
> Why do we even need both the static and member function versions?

The virtual member function NewSystemLocationProvider is overridden in
location_arbitrator_impl_unittest.cc. It seems like an unusual pattern, nowadays
we would use something like setFooForTesting instead, if possible.

[mcasas, 2016-07-15 21:34:47]

Patchset #2 (id:60001) has been deleted

[mcasas, 2016-07-15 21:58:33]

Patchset #2 (id:80001) has been deleted

[mcasas, 2016-07-15 22:38:50]

Patchset #2 (id:100001) has been deleted

[mcasas, 2016-07-15 22:38:59]

Patchset #2 (id:120001) has been deleted

[mcasas, 2016-07-15 22:44:02]

Description was changed from

==========
Geolocation cleanup: correct GeolocationDelegate::CreateAccessTokenStore() to
return scoped_refptr

GeolocationDelegate interface method:
  AccessTokenStore* GeolocationDelegate::CreateAccessTokenStore()
should not return a bare pointer since AccessTokenStore is refcounted [1]. This 
CL corrects that wrong return type.

[1]
https://cs.chromium.org/chromium/src/content/public/browser/access_token_stor...


BUG=623114

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="GeolocationBrowserTest.*"
./out/gn/content_unittests --gtest_filter="GeolocationLocationArbitratorTest.*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Geolocation cleanup: correct GeolocationDelegate::CreateAccessTokenStore() to
return scoped_refptr


This CL corrects uses of content::AccessTokenStore* to scoped_refptr<> 
versions, since that class is ref-counted. 

Same applies to net::URLRequestContextGetter .

Also this CL does some cleanups in the touched files:
- s/NULL/nullptr/
- makes const and/or preferred initialize over assignment for
  some member variables, where applicable/available,
- method ::GetAccessTokenStore() is made private.
- NewSystemLocationProvider(); is changed to return std::unique_ptr<>

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="Geolocation*"
./out/gn/content_unittests --gtest_filter=Geolocation*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[mcasas, 2016-07-15 22:45:54]

wez@ PTAL

There was a rebase between PS1 and PS2, so I'd recommend
not comparing both. However, most changes are mechanical.

https://codereview.chromium.org/2129313002/diff/40001/content/browser/geoloca...
File content/browser/geolocation/location_arbitrator_impl.cc (right):

https://codereview.chromium.org/2129313002/diff/40001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl.cc:151: return
access_token_store_.get();
On 2016/07/12 21:55:47, Wez wrote:
> Update this to
> 
> if (!access_token_store_)
>   ...

Done.

https://codereview.chromium.org/2129313002/diff/40001/content/browser/geoloca...
File content/browser/geolocation/location_arbitrator_impl.h (right):

https://codereview.chromium.org/2129313002/diff/40001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl.h:63: virtual
scoped_refptr<AccessTokenStore> NewAccessTokenStore();
On 2016/07/12 21:55:47, Wez wrote:
> If you're changing this then I'd suggest also updating:
> 
> scoped_refptr<> GetAccessTokenStore()
> 
> .. NewNetworkLocationProvider(
>   const scoped_refptr<>& access_token_store,
>   const scoped_refptr<>& url_request_context_getter,
>   ...
>   )
> 
> Updating the URLRequestContextGetter* instances to scoped_refptr should be
> pretty self-contained to this file, though I notice that there is also a bare
> reference in the AccessTokenStore callback signature.

Done.

https://codereview.chromium.org/2129313002/diff/40001/content/browser/geoloca...
content/browser/geolocation/location_arbitrator_impl.h:123: LocationProvider*
NewSystemLocationProvider();
On 2016/07/12 21:55:47, Wez wrote:
> nit: While we're here... it's weird that this is a bare pointer when the
version
> in the class is not.
> 
> Why do we even need both the static and member function versions?

Done.

[mcasas, 2016-07-15 22:46:40]

Description was changed from

==========
Geolocation cleanup: correct GeolocationDelegate::CreateAccessTokenStore() to
return scoped_refptr


This CL corrects uses of content::AccessTokenStore* to scoped_refptr<> 
versions, since that class is ref-counted. 

Same applies to net::URLRequestContextGetter .

Also this CL does some cleanups in the touched files:
- s/NULL/nullptr/
- makes const and/or preferred initialize over assignment for
  some member variables, where applicable/available,
- method ::GetAccessTokenStore() is made private.
- NewSystemLocationProvider(); is changed to return std::unique_ptr<>

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="Geolocation*"
./out/gn/content_unittests --gtest_filter=Geolocation*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Geolocation cleanup: corrects uses of content::AccessTokenStore* and
net::URLRequestContextGetter*


This CL corrects uses of content::AccessTokenStore* to scoped_refptr<> 
versions, since that class is ref-counted. 

Same applies toc* .

Also this CL does some cleanups in the touched files:
- s/NULL/nullptr/
- makes const and/or preferred initialize over assignment for
  some member variables, where applicable/available,
- method ::GetAccessTokenStore() is made private.
- NewSystemLocationProvider(); is changed to return std::unique_ptr<>

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="Geolocation*"
./out/gn/content_unittests --gtest_filter=Geolocation*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[mcasas, 2016-07-15 23:17:54]

Description was changed from

==========
Geolocation cleanup: corrects uses of content::AccessTokenStore* and
net::URLRequestContextGetter*


This CL corrects uses of content::AccessTokenStore* to scoped_refptr<> 
versions, since that class is ref-counted. 

Same applies toc* .

Also this CL does some cleanups in the touched files:
- s/NULL/nullptr/
- makes const and/or preferred initialize over assignment for
  some member variables, where applicable/available,
- method ::GetAccessTokenStore() is made private.
- NewSystemLocationProvider(); is changed to return std::unique_ptr<>

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="Geolocation*"
./out/gn/content_unittests --gtest_filter=Geolocation*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Geolocation cleanup: corrects uses of content::AccessTokenStore* and
net::URLRequestContextGetter*


This CL corrects uses of content::AccessTokenStore* to scoped_refptr<> 
versions, since that class is ref-counted. 

Same applies toc* .

Also this CL does some cleanups in the touched files:
- s/NULL/nullptr/
- makes const and/or preferred initialize over assignment for
  some member variables, where applicable/available,
- method ::GetAccessTokenStore() is made private.
- NewSystemLocationProvider(); is changed to return std::unique_ptr<>

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="Geolocation*"
./out/gn/content_unittests --gtest_filter=Geolocation*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

BUG=623114
==========

[Wez, 2016-07-16 00:35:10]

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right):

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:114: const
scoped_refptr<AccessTokenStore>& /* access_token_store */,
nit: Why make these into comments?

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:185: const
std::unique_ptr<MockLocationObserver> observer_;
Why do all these need/want to be const?

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
File content/browser/geolocation/network_location_provider.cc (right):

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/network_location_provider.cc:169: if
(position.Validate())
nit: Keep the { } for consistency with the rest of this class.

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/network_location_provider.cc:186:
DCHECK(wifi_data_provider_manager_ == nullptr);
nit: !wifi_data_provider_manager_

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
File content/browser/geolocation/network_location_provider_unittest.cc (right):

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/network_location_provider_unittest.cc:270:
ASSERT_TRUE(parsed_json.get() != nullptr);
nit: ASSERT_FALSE(parsed_json)

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/network_location_provider_unittest.cc:319: const
scoped_refptr<MockWifiDataProvider> wifi_data_provider_;
Is it useful to const-ify all these test members?

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/network_location_provider_unittest.cc:326:
EXPECT_TRUE(nullptr != provider.get());
EXPECT_TRUE(provider)

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/network_location_provider_unittest.cc:335:
ASSERT_TRUE(fetcher != nullptr);
ASSERT_TRUE(fetcher) here and below.

[mcasas, 2016-07-16 01:17:46]

PTAL

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
File content/browser/geolocation/location_arbitrator_impl_unittest.cc (right):

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:114: const
scoped_refptr<AccessTokenStore>& /* access_token_store */,
On 2016/07/16 00:35:10, Wez wrote:
> nit: Why make these into comments?

There was this Chromium style or Google Style that
recommended commenting out unused parameters,
but I can't find it anymore and, since I should do all
or none, I'll revert these ones.

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/location_arbitrator_impl_unittest.cc:185: const
std::unique_ptr<MockLocationObserver> observer_;
On 2016/07/16 00:35:10, Wez wrote:
> Why do all these need/want to be const?

First, for const-correctness :)  (ask miu@)
Second, I like this syntax very much: at a glance
I can tell the member variables that are initialized
in constructor and never modified any time later.

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
File content/browser/geolocation/network_location_provider.cc (right):

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/network_location_provider.cc:169: if
(position.Validate())
On 2016/07/16 00:35:10, Wez wrote:
> nit: Keep the { } for consistency with the rest of this class.

Done.

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/network_location_provider.cc:186:
DCHECK(wifi_data_provider_manager_ == nullptr);
On 2016/07/16 00:35:10, Wez wrote:
> nit: !wifi_data_provider_manager_

Done.

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
File content/browser/geolocation/network_location_provider_unittest.cc (right):

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/network_location_provider_unittest.cc:270:
ASSERT_TRUE(parsed_json.get() != nullptr);
On 2016/07/16 00:35:10, Wez wrote:
> nit: ASSERT_FALSE(parsed_json)

You mean ASSERT_TRUE(parsed_json);
Done

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/network_location_provider_unittest.cc:319: const
scoped_refptr<MockWifiDataProvider> wifi_data_provider_;
On 2016/07/16 00:35:10, Wez wrote:
> Is it useful to const-ify all these test members?

Absolutely, please see my reply before.

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/network_location_provider_unittest.cc:326:
EXPECT_TRUE(nullptr != provider.get());
On 2016/07/16 00:35:10, Wez wrote:
> EXPECT_TRUE(provider)

Done.

https://codereview.chromium.org/2129313002/diff/140001/content/browser/geoloc...
content/browser/geolocation/network_location_provider_unittest.cc:335:
ASSERT_TRUE(fetcher != nullptr);
On 2016/07/16 00:35:10, Wez wrote:
> ASSERT_TRUE(fetcher) here and below.

Done here and elsewhere.

[mcasas, 2016-07-16 03:21:21]

Patchset #3 (id:150001) has been deleted

[Wez, 2016-07-18 17:50:53]

https://codereview.chromium.org/2129313002/diff/170001/chrome/browser/geoloca...
File chrome/browser/geolocation/access_token_store_browsertest.cc (right):

https://codereview.chromium.org/2129313002/diff/170001/chrome/browser/geoloca...
chrome/browser/geolocation/access_token_store_browsertest.cc:48:
scoped_refptr<AccessTokenStore> store,
Now that this isn't a ptr or reference, you're not actually writing it back into
|token_store_|.  Are you sure the test still works? :P

https://codereview.chromium.org/2129313002/diff/170001/content/browser/geoloc...
File content/browser/geolocation/network_location_provider.cc (right):

https://codereview.chromium.org/2129313002/diff/170001/content/browser/geoloc...
content/browser/geolocation/network_location_provider.cc:119:
position_cache_(new PositionCache()),
nit: No () if no parameters to pass to ctor.

https://codereview.chromium.org/2129313002/diff/170001/content/browser/geoloc...
content/browser/geolocation/network_location_provider.cc:142: if
(!weak_factory_.HasWeakPtrs() || is_wifi_data_complete_) {
For a follow-up: We should fix this to at least explain why HasWeakPtrs() being
false indicates all data is ready, and preferably just not to use it, since I
suspect it does not have the intended behaviour.

[mcasas, 2016-07-18 21:48:08]

Patchset #4 (id:190001) has been deleted

[mcasas, 2016-07-18 21:48:53]

wez@ PTAL

https://codereview.chromium.org/2129313002/diff/170001/chrome/browser/geoloca...
File chrome/browser/geolocation/access_token_store_browsertest.cc (right):

https://codereview.chromium.org/2129313002/diff/170001/chrome/browser/geoloca...
chrome/browser/geolocation/access_token_store_browsertest.cc:48:
scoped_refptr<AccessTokenStore> store,
On 2016/07/18 17:50:53, Wez wrote:
> Now that this isn't a ptr or reference, you're not actually writing it back
into
> |token_store_|.  Are you sure the test still works? :P

Interestingly, the test still works. Reverted.

https://codereview.chromium.org/2129313002/diff/170001/content/browser/geoloc...
File content/browser/geolocation/network_location_provider.cc (right):

https://codereview.chromium.org/2129313002/diff/170001/content/browser/geoloc...
content/browser/geolocation/network_location_provider.cc:119:
position_cache_(new PositionCache()),
On 2016/07/18 17:50:53, Wez wrote:
> nit: No () if no parameters to pass to ctor.

Done.

https://codereview.chromium.org/2129313002/diff/170001/content/browser/geoloc...
content/browser/geolocation/network_location_provider.cc:142: if
(!weak_factory_.HasWeakPtrs() || is_wifi_data_complete_) {
On 2016/07/18 17:50:53, Wez wrote:
> For a follow-up: We should fix this to at least explain why HasWeakPtrs()
being
> false indicates all data is ready, and preferably just not to use it, since I
> suspect it does not have the intended behaviour.

Done.

[mcasas, 2016-07-18 22:24:55]

Patchset #4 (id:210001) has been deleted

[Wez, 2016-07-18 22:49:18]

https://codereview.chromium.org/2129313002/diff/170001/chrome/browser/geoloca...
File chrome/browser/geolocation/access_token_store_browsertest.cc (right):

https://codereview.chromium.org/2129313002/diff/170001/chrome/browser/geoloca...
chrome/browser/geolocation/access_token_store_browsertest.cc:48:
scoped_refptr<AccessTokenStore> store,
On 2016/07/18 21:48:52, mcasas wrote:
> On 2016/07/18 17:50:53, Wez wrote:
> > Now that this isn't a ptr or reference, you're not actually writing it back
> into
> > |token_store_|.  Are you sure the test still works? :P
> 
> Interestingly, the test still works. Reverted.

It should still say if (!*store), I think.

Looking at what AccessTokenStore does, it seems that it just stores & loads
values from the browser's pref store, which is why breaking this doesn't break
the test - it just makes it [very slightly] less efficient.

I'd suggest just creating the AccessTokenStore on token_store_ on test startup
and getting rid of this on-demand creation stuff. The store isn't thread-bound
(it's ref-counted-thread-safe in fact) so that should be fine - then you just
pass the reference to it into this callback, to actually use.

https://codereview.chromium.org/2129313002/diff/230001/content/browser/geoloc...
File content/browser/geolocation/location_arbitrator_impl.cc (right):

https://codereview.chromium.org/2129313002/diff/230001/content/browser/geoloc...
content/browser/geolocation/location_arbitrator_impl.cc:152: return
access_token_store_;
nit: Why did this move back?

[mcasas, 2016-07-19 01:18:19]

https://codereview.chromium.org/2129313002/diff/170001/chrome/browser/geoloca...
File chrome/browser/geolocation/access_token_store_browsertest.cc (right):

https://codereview.chromium.org/2129313002/diff/170001/chrome/browser/geoloca...
chrome/browser/geolocation/access_token_store_browsertest.cc:48:
scoped_refptr<AccessTokenStore> store,
On 2016/07/18 22:49:18, Wez wrote:
> On 2016/07/18 21:48:52, mcasas wrote:
> > On 2016/07/18 17:50:53, Wez wrote:
> > > Now that this isn't a ptr or reference, you're not actually writing it
back
> > into
> > > |token_store_|.  Are you sure the test still works? :P
> > 
> > Interestingly, the test still works. Reverted.
> 
> It should still say if (!*store), I think.
> 
> Looking at what AccessTokenStore does, it seems that it just stores & loads
> values from the browser's pref store, which is why breaking this doesn't break
> the test - it just makes it [very slightly] less efficient.
> 
> I'd suggest just creating the AccessTokenStore on token_store_ on test startup
> and getting rid of this on-demand creation stuff. The store isn't thread-bound
> (it's ref-counted-thread-safe in fact) so that should be fine - then you just
> pass the reference to it into this callback, to actually use.

Done.

https://codereview.chromium.org/2129313002/diff/230001/content/browser/geoloc...
File content/browser/geolocation/location_arbitrator_impl.cc (right):

https://codereview.chromium.org/2129313002/diff/230001/content/browser/geoloc...
content/browser/geolocation/location_arbitrator_impl.cc:152: return
access_token_store_;
On 2016/07/18 22:49:18, Wez wrote:
> nit: Why did this move back?

I moved it to where it was supposed to go to,
if method definition was to follow method
declaration, and then I recently realized that
they are not following that (Chromium) pattern,
so in the interest of smallest amount of modified
lines, I moved it back to where it was.

[Wez, 2016-07-19 20:24:22]

The CQ bit was checked by wez@chromium.org

[Wez, 2016-07-19 20:24:23]

lgtm

[Wez, 2016-07-19 20:24:23]

The patchset sent to the CQ was uploaded after l-g-t-m from
lethalantidote@chromium.org
Link to the patchset: https://codereview.chromium.org/2129313002/#ps250001
(title: "Removed StartTestStepFromClientThread()")

[commit-bot: I haz the power, 2016-07-19 20:25:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 20:25:17]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[mcasas, 2016-07-19 20:34:27]

mcasas@chromium.org changed reviewers:
+ jam@chromium.org

[mcasas, 2016-07-19 20:34:28]

jam@ Owners RS please

[mcasas, 2016-07-19 20:34:48]

The CQ bit was unchecked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-07-19 20:35:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-07-19 20:35:08]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[jam, 2016-07-19 23:15:53]

lgtm

[mcasas, 2016-07-19 23:16:40]

The CQ bit was checked by mcasas@chromium.org

[commit-bot: I haz the power, 2016-07-19 23:17:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-19 23:17:33]

Your CL relies on deprecated CQ feature(s):
 * Specifying master names in CQ_INCLUDE_TRYBOTS part of description without
"master." prefix is deprecated:
     tryserver.chromium.linux
   For more details, see http://crbug.com/617627.

[commit-bot: I haz the power, 2016-07-19 23:24:28]

Description was changed from

==========
Geolocation cleanup: corrects uses of content::AccessTokenStore* and
net::URLRequestContextGetter*


This CL corrects uses of content::AccessTokenStore* to scoped_refptr<> 
versions, since that class is ref-counted. 

Same applies toc* .

Also this CL does some cleanups in the touched files:
- s/NULL/nullptr/
- makes const and/or preferred initialize over assignment for
  some member variables, where applicable/available,
- method ::GetAccessTokenStore() is made private.
- NewSystemLocationProvider(); is changed to return std::unique_ptr<>

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="Geolocation*"
./out/gn/content_unittests --gtest_filter=Geolocation*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

BUG=623114
==========

to

==========
Geolocation cleanup: corrects uses of content::AccessTokenStore* and
net::URLRequestContextGetter*


This CL corrects uses of content::AccessTokenStore* to scoped_refptr<> 
versions, since that class is ref-counted. 

Same applies toc* .

Also this CL does some cleanups in the touched files:
- s/NULL/nullptr/
- makes const and/or preferred initialize over assignment for
  some member variables, where applicable/available,
- method ::GetAccessTokenStore() is made private.
- NewSystemLocationProvider(); is changed to return std::unique_ptr<>

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="Geolocation*"
./out/gn/content_unittests --gtest_filter=Geolocation*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

BUG=623114
==========

[commit-bot: I haz the power, 2016-07-19 23:24:30]

Committed patchset #5 (id:250001)

[commit-bot: I haz the power, 2016-07-19 23:25:04]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-19 23:26:13]

Description was changed from

==========
Geolocation cleanup: corrects uses of content::AccessTokenStore* and
net::URLRequestContextGetter*


This CL corrects uses of content::AccessTokenStore* to scoped_refptr<> 
versions, since that class is ref-counted. 

Same applies toc* .

Also this CL does some cleanups in the touched files:
- s/NULL/nullptr/
- makes const and/or preferred initialize over assignment for
  some member variables, where applicable/available,
- method ::GetAccessTokenStore() is made private.
- NewSystemLocationProvider(); is changed to return std::unique_ptr<>

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="Geolocation*"
./out/gn/content_unittests --gtest_filter=Geolocation*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

BUG=623114
==========

to

==========
Geolocation cleanup: corrects uses of content::AccessTokenStore* and
net::URLRequestContextGetter*

This CL corrects uses of content::AccessTokenStore* to scoped_refptr<>
versions, since that class is ref-counted.

Same applies toc* .

Also this CL does some cleanups in the touched files:
- s/NULL/nullptr/
- makes const and/or preferred initialize over assignment for
  some member variables, where applicable/available,
- method ::GetAccessTokenStore() is made private.
- NewSystemLocationProvider(); is changed to return std::unique_ptr<>

TEST=all relevant unittests and browser_tests working, in
particular
./out/gn/browser_tests --gtest_filter="Geolocation*"
./out/gn/content_unittests --gtest_filter=Geolocation*
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

BUG=623114

Committed: https://crrev.com/1161c5753adf296ea253d56f1617d3d926152b55
Cr-Commit-Position: refs/heads/master@{#406410}
==========

[commit-bot: I haz the power, 2016-07-19 23:26:14]

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